Hi Rene,
Thanks for the review. I'll respond to your comments in this thread,
but will repost my whole patch series separately later on.
On Thu, Mar 11, 2010 at 08:23:16PM +0100, Ren? Scharfe wrote:
> Am 11.03.2010 17:30, schrieb Gary V. Vaughan:
> > Some of the flags used with the first diff found in PATH cause the
> > vendor diff to choke.
> >
> > This patch allows us to replace the problematic diff calls in our
> > build script as follows:
> >
> > if [ "${SB_VAR_DIFFUTILS+set}" = set ]; then
> > ${SB_PATH_SED} -i -e "\
> > s|@DIFF@|${SB_VAR_DIFFUTILS}/bin/gnudiff|g;" \
> > $(find . -type f -exec ${SB_PATH_EGREP} -l '@DIFF@' {} \;)
> > else
> > ${SB_PATH_SED} -i -e "\
> > s|@DIFF@|diff|g;" \
> > $(find . -type f -exec ${SB_PATH_EGREP} -l '@DIFF@' {} \;)
> > fi
> >
> > This is fine for us, but upstream it would be better to search
> > the execution path for a diff that has all the required features at
> > configure time, and to do the substitutions at that time. This is
> > what the latest version of quilt does if an example is useful.
> This patch doesn't seem to add this conversion to git's own build script
> (Makefile). That means the patched scripts now call a command named
> "@DIFF@", which probably doesn't exist on most systems.
Indeed. At TWW the substitution is done by our build recipe as quoted
above (admittedly, not too useful for everyone else).
> > diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh
> > index 2135a8e..329d052 100755
> > --- a/Documentation/install-webdoc.sh
> > +++ b/Documentation/install-webdoc.sh
> > @@ -12,7 +12,7 @@ do
> > then
> > : did not match
> > elif test -f "$T/$h" &&
> > - diff -u -I'Last updated [0-9][0-9]-[A-Z][a-z][a-z]-' "$T/$h" "$h"
> > + @DIFF@ -u -I'Last updated [0-9][0-9]-[A-Z][a-z][a-z]-' "$T/$h" "$h"
> > then
> > :; # up to date
> > else
>
> For build scripts I think it makes sense to do the same for diff as for
> tar, namely to define and export it in Makefile (run "git grep -w TAR"
> to see what I mean).
Agreed, and thanks for the tip, I hadn't noticed the precedent.
> > diff --git a/contrib/hooks/setgitperms.perl b/contrib/hooks/setgitperms.perl
> > index a577ad0..c45ab0d 100644
> > --- a/contrib/hooks/setgitperms.perl
> > +++ b/contrib/hooks/setgitperms.perl
> > @@ -180,7 +180,7 @@ elsif ($read_mode) {
> > rename "$gitmeta.tmp", $gitmeta;
> > }
> > else {
> > - my $diff = `diff -U 0 $gitmeta $gitmeta.tmp`;
> > + my $diff = `@DIFF@ -U 0 $gitmeta $gitmeta.tmp`;
> > if ($diff ne '') {
> > rename "$gitmeta.tmp", $gitmeta;
> > }
>
> I'm not sure the files in contrib/ should be changed at all, as they are
> not touched by Makefile.
Fair enough. However, diff -U is not at all portable unless you have
GNU diff first in your path.
> > --- a/git-merge-one-file.sh
> > +++ b/git-merge-one-file.sh
> > @@ -28,6 +28,8 @@ then
> > exit 1
> > fi
> >
> > +DIFF="@DIFF@"
> > +
> > case "${1:-.}${2:-.}${3:-.}" in
> > #
> > # Deleted in both or deleted in one and unchanged in the other
> > @@ -107,7 +109,7 @@ case "${1:-.}${2:-.}${3:-.}" in
> > # remove lines that are unique to ours.
> > orig=`git-unpack-file $2`
> > sz0=`wc -c <"$orig"`
> > - diff -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
> > + $DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
> > sz1=`wc -c <"$orig"`
> >
> > # If we do not have enough common material, it is not
>
> Why does this one use $DIFF unlike the others?
>
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 1a26843..037bc15 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -1592,7 +1592,7 @@ sub find_file_type_and_diff_status {
> > return ('dir', '') if $path eq '';
> >
> > my $diff_output =
> > - command_oneline(qw(diff --cached --name-status --), $path) || "";
> > + command_oneline(qw(@DIFF@ --cached --name-status --), $path) || "";
> > my $diff_status = (split(' ', $diff_output))[0] || "";
> >
> > my $ls_tree = command_oneline(qw(ls-tree HEAD), $path) || "";
>
> The changed line calls git-diff, not diff; you should keep it as it is.
Okay, thanks.
> > diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
> > index 0241329..0330f13 100755
> > --- a/t/t1002-read-tree-m-u-2way.sh
> > +++ b/t/t1002-read-tree-m-u-2way.sh
> > @@ -215,7 +215,7 @@ test_expect_success \
> > if cmp M.sum actual14a.sum; then false; else :; fi &&
> > check_cache_at nitfol clean &&
> > echo nitfol nitfol >nitfol1 &&
> > - diff nitfol nitfol1 &&
> > + @DIFF@ nitfol nitfol1 &&
> > rm -f nitfol1'
>
> Here and for most of the rest of the test scripts using test_cmp
> (defined in test-lib.sh) instead of diff directly would be better.
>
> test_cmp calls the command in $GIT_TEST_CMP. For your OS, you could set
> it to "cmp" in Makefile instead of the default "diff -u".
>
> (That also means that test_cmp should only be used in places where the
> output is discarded or displayed, not piped into another command which
> might expect a certain diff format.)
Agreed.
> However, if there are no command line switches given to diff but only
> two files, do you need to change anything at all? Every diff
> implementation should be able to handle that, right?
>
> [Lots of test script changes snipped.]
There are a ton of these, so I probably did the substitution with a
sed script, and then tested to make sure the testsuite didn't regress,
rather than manually checking and substituting on a case by case
basis like I did in the other parts of the patch.
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index a0e396a..cd2c886 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -59,7 +59,7 @@ export GIT_MERGE_VERBOSITY
> > export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
> > export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
> > export EDITOR
> > -GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> > +GIT_TEST_CMP=${GIT_TEST_CMP:-@DIFF@}
> >
> > # Protect ourselves from common misconfiguration to export
> > # CDPATH into the environment
>
> Define GIT_TEST_CMP in Makefile..
>
> (I'd split the introduction of DIFF/$DIFF, the diff -> test_cmp
> conversions and the change to set GIT_TEST_CMP=cmp for your platform
> into three separate patches.)
Will do. And then I'll resubmit the whole series.
Cheers,
--
Gary V. Vaughan (gary@xxxxxxxxxxxxxxxxxx)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html