Bazaar

Merge lp://qastaging/~mbp/bzr/764108-diffoptions into lp://qastaging/bzr

764108-diffoptions
Merge into bzr.dev

Proposed by Martin Pool on 2011-08-01

Status:

Work in progress

Proposed branch:

lp://qastaging/~mbp/bzr/764108-diffoptions

Merge into:

lp://qastaging/bzr

Diff against target:

98 lines (+78/-2) (has conflicts)

2 files modified

bzrlib/diff.py (+12/-2)
bzrlib/tests/test_diff.py (+66/-0)

Text conflict in bzrlib/tests/test_diff.py

To merge this branch:

bzr merge lp://qastaging/~mbp/bzr/764108-diffoptions

Medium

In Progress

Link a bug report

Reviewer	Review Type	Date Requested	Status
John A Meinel		2011-08-01	Approve on 2011-08-05
Review via email: mp+69966@code.qastaging.launchpad.net

Description of the change

This builds on sergei's work in https://bugs.launchpad.net/bzr/+bug/764108 to add an option to show function names within the diff, based on <https://code.launchpad.net/~maria-captains/bzr/diffoptions/+merge/54139>.

I was going to finish it off for him. I haven't looked at it for a while, but it's still assigned to me so I'm putting this up to get it back in circulation. There are a lot of things we possibly could do here, but perhaps we can look for a smaller incremental step.

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-08-01:

This looks good to me.

I wonder what happens if there is no external diff tool that we can use. Does external_diff() fail gracefully?

Similarly, not all external diff tools might display the function name.

We should probably also protect the test with a feature that checks if a functional external diff is available.

Revision history for this message

John A Meinel (jameinel) wrote on 2011-08-05:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/2/2011 1:56 AM, Jelmer Vernooij wrote:
> This looks good to me.
>
> I wonder what happens if there is no external diff tool that we can
> use. Does external_diff() fail gracefully?
>
> Similarly, not all external diff tools might display the function
> name.
>
> We should probably also protect the test with a feature that checks
> if a functional external diff is available.

External diff will fail noisily. However:

merge: approve

There is a text conflict in here that will need to be fixed before it
can land.

Martin, I thought you had created a helper for "iter_search_rules()"
that returned the 1 entry that you were most likely to want. That might
cleanup the check a bit.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk47wLQACgkQJdeBCYSNAAP3NgCffvb9PMZNes2HaT+qhYmd8YTk
Y8cAn0od51oMcxsfx1Wcn4VrGpZb13N2
=N00d
-----END PGP SIGNATURE-----

review: Approve

Revision history for this message

Martin Packman (gz) wrote on 2011-08-05:

As mentioned in the previous merge proposal, I'm concerned the fix is worse than the problem.

It's not very hard to do `bzr alias fdiff=diff --diff-options=-p` or similar. Throwing funny errors from a useful looking option if there isn't a correctly configured external tool that the installers don't bundle is asking for trouble.

Revision history for this message

Martin Pool (mbp) wrote on 2011-08-09:

OK; please leave this unlanded and I'll look into that.

Martin

Unmerged revisions

5731. By Martin Pool on 2011-04-18: merge trunk, including test_diff cleanups
5730. By Martin Pool on 2011-04-18: Add xfail test for showing function name in diff hunk (bug 764108)
5729. By Sergei Golubchik on 2011-03-20: support diff -F in all generated diffs
with per-file configuration rules