Bazaar

Code review comment for lp://staging/~ace17/bzr/fixDiff4

fixDiff4
Merge into bzr.dev

Revision history for this message

Richard Wilbur (richard-wilbur) wrote on 2014-09-25:

Hello Sebastien,

I disagree with your statement that your patch prevents passing the --diff-options to the /usr/bin/diff. With your patch, if you specify --diff-options but not --using, then the --diff-options are passed to /usr/bin/diff, as desired.

I'm looking at what would be involved in making the --using tool (when specified) the first thing tried for all types of situations (including added files). If we must fall back to /usr/bin/diff in the presence of an alternate tool specified through --using, one option would simply be to ignore the --diff-options in that case.

Possibly another angle on this whole discussion occurred to me recently. Witness the following test:
rwilbur@ordinate:~/src/bzr_test$ bzr diff --using='/bin/ls' junk
=== modified file 'junk'
/home/rwilbur/src/bzr_test/junk
/tmp/bzr-diff-ZqM1vD/old/junk
rwilbur@ordinate:~/src/bzr_test$ bzr diff --using='/bin/ls -l' junk
=== modified file 'junk'
-rw-r--r-- 1 rwilbur rwilbur 16 2014-09-25 13:28 /home/rwilbur/src/bzr_test/junk
-r--r--r-- 1 rwilbur rwilbur 5 2014-09-25 13:28 /tmp/bzr-diff-lc5kzb/old/junk

If you create the tool command line with path followed by options, right in the --using declaration, it seems to work just fine (and wouldn't collide with options we specify for /usr/bin/diff in --diff-options provided we merge part of your patch).

In this case we could prioritize as follows:
1. If nothing is specified, we use internal diff.
2. If only --diff-options appears, we pass those options to /usr/bin/diff.
3. If only --using appears, we delegate the diff to the specified tool (options may appear in the --using string). If the specified tool cannot handle a particular diff, we use /usr/bin/diff or internal diff to provide a result.
4. If both --using and --diff-options appear, we delegate the diff to the specified tool as in #3, but if it cannot handle a particular diff we use /usr/bin/diff with the options from --diff-options.

This interpretation would effectively decouple the --using and --diff-options specifications. --diff-options would only ever be passed to /usr/bin/diff. --using would always make the specified tool the first delegate.

What do you think of that landscape?

Hello Sebastien,

I disagree with your statement that your patch prevents passing the --diff-options to the /usr/bin/diff.  With your patch, if you specify --diff-options but not --using, then the --diff-options are passed to /usr/bin/diff, as desired.

I'm looking at what would be involved in making the --using tool (when specified) the first thing tried for all types of situations (including added files).  If we must fall back to /usr/bin/diff in the presence of an alternate tool specified through --using, one option would simply be to ignore the --diff-options in that case.

Possibly another angle on this whole discussion occurred to me recently.  Witness the following test:
rwilbur@ordinate:~/src/bzr_test$ bzr diff --using='/bin/ls' junk
=== modified file 'junk'
/home/rwilbur/src/bzr_test/junk
/tmp/bzr-diff-ZqM1vD/old/junk
rwilbur@ordinate:~/src/bzr_test$ bzr diff --using='/bin/ls -l' junk
=== modified file 'junk'
-rw-r--r-- 1 rwilbur rwilbur 16 2014-09-25 13:28 /home/rwilbur/src/bzr_test/junk
-r--r--r-- 1 rwilbur rwilbur  5 2014-09-25 13:28 /tmp/bzr-diff-lc5kzb/old/junk

In this case we could prioritize as follows:
1.  If nothing is specified, we use internal diff.
2.  If only --diff-options appears, we pass those options to /usr/bin/diff.
3.  If only --using appears, we delegate the diff to the specified tool (options may appear in the --using string).  If the specified tool cannot handle a particular diff, we use /usr/bin/diff or internal diff to provide a result.
4.  If both --using and --diff-options appear, we delegate the diff to the specified tool as in #3, but if it cannot handle a particular diff we use /usr/bin/diff with the options from --diff-options.

This interpretation would effectively decouple the --using and --diff-options specifications.  --diff-options would only ever be passed to /usr/bin/diff.  --using would always make the specified tool the first delegate.

What do you think of that landscape?

« Back to merge proposal