Re: [Bug-vc-dwim] vc-dwim 1.5: external diff died

[Jim Meyering]

Bruno Haible wrote:

> Hi Jim,
>
>> Perhaps GIT_EXTERNAL_DIFF is still set in the environment?
>
> I had unset it in the command line just before invoking vc-dwim.
> You can see from the previous command (with "git diff") that this
> 'unset' command works as expected.
>
> I had modified my copy of vc-dwim like this:
>
> BEGIN
> {
>   my $perllibdir = $ENV{'perllibdir'} || '/arch/x86-linux/gnu/share/vc-dwim';
>   unshift @INC, (split ':', $perllibdir);
>
>   # Override SHELL.  This is required on DJGPP so that system() uses
>   # bash, not COMMAND.COM which doesn't quote arguments properly.
>   # Other systems aren't expected to use $SHELL when Automake
>   # runs, but it should be safe to drop the `if DJGPP' guard if
>   # it turns up other systems need the same thing.  After all,
>   # if SHELL is used, ./configure's SHELL is always better than
>   # the user's SHELL (which may be something like tcsh).
>   $ENV{'SHELL'} = '/bin/sh' if exists $ENV{'DJGPP'};
>
>   undef $ENV{'GIT_EXTERNAL_DIFF'};
> }
>
> Apparently this last statement is causing the trouble.
>
> Admittedly I am trying to modify code that I don't understand - until
> the problem I reported yesterday gets fixed upstream.
>
> When I remove that hack, now I get reasonable output from 'vc-dwim':
>
> $ (unset GIT_EXTERNAL_DIFF ; vc-dwim)
> vc-dwim: lib/relocatable.c is listed in the ChangeLog entry, but not in diffs.
> Did you forget to add it?
>
> But without the 'unset' command, vc-dwim does not recognize the diff format,
> like you predicted:
>
> $ vc-dwim
> vc-dwim: ChangeLog contains no newly added lines

Thanks for the report.  I've pushed the change-sets included below.
I think I've addressed your problem.
Now, the tests pass even when run like this:

    GIT_EXTERNAL_DIFF=echo make check

But that was easy.  Just arrange for the git-using
tests to unset that envvar.  However, I have not tested
whether setting GIT_EXTERNAL_DIFF to some script that actually
does useful work does what you'd expect.  Is there a canonical
or minimal script that does that?  Point me to one and I'll add
a test to exercise it.


>From fe916a93a5216a557b4c938b0f8caa9344824167 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 15:55:28 +0200
Subject: [PATCH 1/6] maint: vc-dwim: remove an unused statement

* vc-dwim.pl (main): Remove unused local.
---
 vc-dwim.pl |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/vc-dwim.pl b/vc-dwim.pl
index 63b100f..ae45219 100755
--- a/vc-dwim.pl
+++ b/vc-dwim.pl
@@ -783,7 +783,6 @@ sub main

   my $vc = VC->new ($changelog_file_name[0]);
   my $vc_name = $vc->name();
-  my @vc_diff = $vc->diff_cmd();

   # Key is ChangeLog file name, value is a ref to list of
   # lines added to that file.
--
1.7.7.rc0.362.g5a14


>From 50632511c320a3d53d502770f3519f61ac2b3d06 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 17:15:18 +0200
Subject: [PATCH 2/6] vc-dwim: do actually remove blank-empty lines

* vc-dwim.pl (get_diffs): Replace a no-op loop with one that
actually does what the comments said.
---
 vc-dwim.pl |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/vc-dwim.pl b/vc-dwim.pl
index ae45219..0d23cdf 100755
--- a/vc-dwim.pl
+++ b/vc-dwim.pl
@@ -128,12 +128,13 @@ sub get_diffs ($$)
           . join (' ', @cmd) . "': $PROCESS_STATUS\n";
     }

-  # Remove the single space from what would otherwise be empty
-  # lines in unified diff output.
-  foreach my $line (@added_lines)
+  # This may not be needed for git (for it, just run
+  # git config diff.suppress-blank-empty true), but for other
+  # version control systems, it helps to normalize diff output.
+  foreach my $i (0..$#added_lines)
     {
-      $line eq ' '
-        and $line = '';
+      $added_lines[$i] eq ' '
+        and $added_lines[$i] = '';
     }

   return address@hidden;
--
1.7.7.rc0.362.g5a14


>From b32251d42e0e63a1c73784d538cd5bd968a73d07 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 18:26:21 +0200
Subject: [PATCH 3/6] tests: update init.sh from gnulib; clean up tests

* tests/init.sh: Update from gnulib.
* tests/add-empty: Use compare, not cmp.
* tests/cl-but-no-diff: Likewise.
* tests/dirty-workdir: Likewise.
* tests/git-mv: Likewise.
* tests/no-star: Likewise.
* tests/no-vc: Likewise.
* tests/subdir: Likewise.
* tests/subdir-cvs: Likewise.
* tests/subdir-middle: Likewise.
* tests/symlinked-changelog: Likewise.
* tests/two-vc: Likewise.
* tests/cl-other-user: Likewise.
* tests/leading-comment: Likewise.
* tests/two-line-attr: Likewise.
---
 tests/add-empty           |    3 +--
 tests/cl-but-no-diff      |    3 +--
 tests/cl-other-user       |    3 +--
 tests/dirty-workdir       |    4 ++++
 tests/git-mv              |    5 ++---
 tests/init.sh             |   31 +++++++++++++++++++++++++------
 tests/leading-comment     |    3 +--
 tests/no-star             |    3 +--
 tests/no-vc               |    3 +--
 tests/subdir              |    2 +-
 tests/subdir-cvs          |    3 +--
 tests/subdir-middle       |    3 +--
 tests/symlinked-changelog |    3 +--
 tests/two-line-attr       |    3 +--
 tests/two-vc              |    3 +--
 15 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/tests/add-empty b/tests/add-empty
index c755877..c4cb600 100755
--- a/tests/add-empty
+++ b/tests/add-empty
@@ -42,7 +42,6 @@ set -e

 vc-dwim ChangeLog > out

-cmp out exp \
-  || { diff out exp 2> /dev/null; false; }
+compare out exp

 Exit 0
diff --git a/tests/cl-but-no-diff b/tests/cl-but-no-diff
index 7112506..f99026e 100755
--- a/tests/cl-but-no-diff
+++ b/tests/cl-but-no-diff
@@ -34,7 +34,6 @@ vc-dwim: not-mod is listed in the ChangeLog entry, but not in 
diffs.
 Did you forget to add it?
 EOF

-cmp out exp \
-  || { diff out exp 2> /dev/null; false; }
+compare out exp

 Exit 0
diff --git a/tests/cl-other-user b/tests/cl-other-user
index 61ecb0d..04570a8 100755
--- a/tests/cl-other-user
+++ b/tests/cl-other-user
@@ -40,7 +40,6 @@ index e69de29..de03f25 100644
 +bow
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/dirty-workdir b/tests/dirty-workdir
index 00aa325..9c56583 100644
--- a/tests/dirty-workdir
+++ b/tests/dirty-workdir
@@ -5,6 +5,10 @@
 print_ver_ vc-chlog
 require_git_

+# The use of git diff --cached below would fail if you had, say,
+# GIT_EXTERNAL_DIFF='diff -c'.
+unset GIT_EXTERNAL_DIFF
+
 # Let's see whether this system knows unidiff format.
 # We assume that if diff doesn't, then patch also won't.
 # As long as vc-chlog only works with unidiff, SKIP the test.
diff --git a/tests/git-mv b/tests/git-mv
index 3fba537..37fcbc7 100755
--- a/tests/git-mv
+++ b/tests/git-mv
@@ -49,12 +49,11 @@ set -e

 vc-dwim ChangeLog > out

-if cmp out exp > /dev/null 2>&1 ; then
+if compare out exp > /dev/null 2>&1 ; then
   :
-elif cmp out exp-old; then
+elif compare out exp-old; then
   :
 else
-  diff out exp-old 2> /dev/null
   false
 fi

diff --git a/tests/init.sh b/tests/init.sh
index 71c6516..373d9d4 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -68,15 +68,29 @@ Exit () { set +e; (exit $1); exit $1; }

 # Print warnings (e.g., about skipped and failed tests) to this file number.
 # Override by defining to say, 9, in init.cfg, and putting say,
-# "export ...ENVVAR_SETTINGS...; exec 9>&2; $(SHELL)" in the definition
-# of TESTS_ENVIRONMENT in your tests/Makefile.am file.
+#   export ...ENVVAR_SETTINGS...; $(SHELL) 9>&2
+# in the definition of TESTS_ENVIRONMENT in your tests/Makefile.am file.
 # This is useful when using automake's parallel tests mode, to print
 # the reason for skip/failure to console, rather than to the .log files.
 : ${stderr_fileno_=2}

-warn_ () { echo "$@" 1>&$stderr_fileno_; }
+# Note that correct expansion of "$*" depends on IFS starting with ' '.
+# Always write the full diagnostic to stderr.
+# When stderr_fileno_ is not 2, also emit the first line of the
+# diagnostic to that file descriptor.
+warn_ ()
+{
+  # If IFS does not start with ' ', set it and emit the warning in a subshell.
+  case $IFS in
+    ' '*) printf '%s\n' "$*" >&2
+          test $stderr_fileno_ = 2 \
+            || { printf '%s\n' "$*" | sed 1q >&$stderr_fileno_ ; } ;;
+    *) (IFS=' '; warn_ "$@");;
+  esac
+}
 fail_ () { warn_ "$ME_: failed test: $@"; Exit 1; }
 skip_ () { warn_ "$ME_: skipped test: $@"; Exit 77; }
+fatal_ () { warn_ "$ME_: hard error: $@"; Exit 99; }
 framework_failure_ () { warn_ "$ME_: set-up failure: $@"; Exit 99; }

 # Sanitize this shell to POSIX mode, if possible.
@@ -167,7 +181,10 @@ else
     st_=$?

     # $re_shell_ works just fine.  Use it.
-    test $st_ = 10 && break
+    if test $st_ = 10; then
+      gl_set_x_corrupts_stderr_=false
+      break
+    fi

     # If this is our first marginally acceptable shell, remember it.
     if test "$st_:$marginal_" = 9: ; then
@@ -204,8 +221,10 @@ export MALLOC_PERTURB_
 # a partition, or to undo any other global state changes.
 cleanup_ () { :; }

-if ( diff --version < /dev/null 2>&1 | grep GNU ) > /dev/null 2>&1; then
+if ( diff -u "$0" "$0" < /dev/null ) > /dev/null 2>&1; then
   compare () { diff -u "$@"; }
+elif ( diff -c "$0" "$0" < /dev/null ) > /dev/null 2>&1; then
+  compare () { diff -c "$@"; }
 elif ( cmp --version < /dev/null 2>&1 | grep GNU ) > /dev/null 2>&1; then
   compare () { cmp -s "$@"; }
 else
@@ -400,7 +419,7 @@ mktempd_ ()
 {
   case $# in
   2);;
-  *) fail_ "Usage: $ME DIR TEMPLATE";;
+  *) fail_ "Usage: mktempd_ DIR TEMPLATE";;
   esac

   destdir_=$1
diff --git a/tests/leading-comment b/tests/leading-comment
index 4d9b0d8..6ee288f 100755
--- a/tests/leading-comment
+++ b/tests/leading-comment
@@ -36,7 +36,6 @@ index e69de29..de03f25 100644
 +bow
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/no-star b/tests/no-star
index 1a2d1bd..e7f05e4 100755
--- a/tests/no-star
+++ b/tests/no-star
@@ -35,7 +35,6 @@ index e69de29..de03f25 100644
 +bow
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/no-vc b/tests/no-vc
index ab9a344..12698ac 100755
--- a/tests/no-vc
+++ b/tests/no-vc
@@ -19,7 +19,6 @@ cat <<\EOF > exp || fail=1
 FIXME
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/subdir b/tests/subdir
index 7fed101..5e252dd 100755
--- a/tests/subdir
+++ b/tests/subdir
@@ -36,7 +36,7 @@ index e69de29..de03f25 100644
 +bow
 EOF

-cmp out exp || fail=1
+compare out exp || fail=1
 test $fail = 1 && diff out exp 2> /dev/null

 Exit $fail
diff --git a/tests/subdir-cvs b/tests/subdir-cvs
index 915c398..aedabb1 100755
--- a/tests/subdir-cvs
+++ b/tests/subdir-cvs
@@ -53,7 +53,6 @@ diff -u -p -r1.1.1.1 f
 +bow
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/subdir-middle b/tests/subdir-middle
index a24ad75..acf7525 100755
--- a/tests/subdir-middle
+++ b/tests/subdir-middle
@@ -36,7 +36,6 @@ index e69de29..b680253 100644
 +z
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/symlinked-changelog b/tests/symlinked-changelog
index 6d930b6..aab5f41 100755
--- a/tests/symlinked-changelog
+++ b/tests/symlinked-changelog
@@ -44,8 +44,7 @@ cat <<\EOF > exp || fail=1

 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 (cd m && vc-dwim --commit ChangeLog) > out || fail=1

diff --git a/tests/two-line-attr b/tests/two-line-attr
index b1862f7..6f6dd8c 100755
--- a/tests/two-line-attr
+++ b/tests/two-line-attr
@@ -33,7 +33,6 @@ index e69de29..de03f25 100644
 +bow
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
diff --git a/tests/two-vc b/tests/two-vc
index 65a338a..88061dd 100755
--- a/tests/two-vc
+++ b/tests/two-vc
@@ -32,7 +32,6 @@ vc-dwim: ChangeLog files are managed by more than one 
version-control system:
   sub/ChangeLog: cvs
 EOF

-cmp out exp || fail=1
-test $fail = 1 && diff out exp 2> /dev/null
+compare out exp || fail=1

 Exit $fail
--
1.7.7.rc0.362.g5a14


>From a326e72a00082990341a12465f7714f1b59529fa Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 18:57:48 +0200
Subject: [PATCH 4/6] tests: don't let GIT_EXTERNAL_DIFF induce spurious test
 failures

* tests/init.cfg (require_git_): Protect against any setting of
the GIT_EXTERNAL_DIFF envvar.
---
 tests/init.cfg |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tests/init.cfg b/tests/init.cfg
index c1cb6a2..f5963e9 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -39,6 +39,10 @@ require_git_()

   test $have_git = 0 &&
     skip_ "this test requires git"
+
+  # Numerous uses of git diff in these tests would fail if you had, say,
+  # GIT_EXTERNAL_DIFF set in your environment, so unset it.
+  unset GIT_EXTERNAL_DIFF
 }

 require_cvs_()
--
1.7.7.rc0.362.g5a14


>From ec8fb84e04ff652e2195daff2e7bd7cd9cc13959 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 19:11:11 +0200
Subject: [PATCH 5/6] vc-dwim: accommodate those who set GIT_EXTERNAL_DIFF

* vc-dwim.pl (main): If the GIT_EXTERNAL_DIFF envvar is set, arrange
to run "git diff --no-ext-diff ..." to produce internally-consumed
diff output and "git diff ..." for diff output that is output.
This means that if you set GIT_EXTERNAL_DIFF, vc-dwim must run
git-diff twice in some modes: once for the output it consumes and
a second time for the diff output it prints.
* VC.pm (diff_pristine): New method.
($vc_cmd->GIT): Define its DIFF_PRISTINE member.
(diff_is_pristine): New method.
Reported by Bruno Haible.
---
 VC.pm      |   22 ++++++++++++++++++++++
 vc-dwim.pl |   17 ++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/VC.pm b/VC.pm
index 17e3f1b..a24d68b 100644
--- a/VC.pm
+++ b/VC.pm
@@ -56,6 +56,7 @@ my $vc_cmd =
    GIT() =>
    {
     DIFF_COMMAND => [qw(git diff -B -C HEAD --)],
+    DIFF_PRISTINE => [qw(git diff --no-ext-diff -B -C HEAD --)],
     VALID_DIFF_EXIT_STATUS => {0 => 1},
     COMMIT_COMMAND => [qw(git commit -q -F)],
     # is-version-controlled-file: true, if "git cat-file -t HEAD:$file"
@@ -188,6 +189,27 @@ sub diff_cmd()
   return @$cmd_ref;
 }

+# Print diff -u style diffs, regardless of envvar settings
+# like GIT_EXTERNAL_DIFF or options like git's --ext-diff.
+# If no DIFF_PRISTINE member is specified, just use DIFF_COMMAND.
+sub diff_pristine()
+{
+  my $self = shift;
+  my $cmd_ref = $vc_cmd->{$self->name()}->{DIFF_PRISTINE}
+    || $vc_cmd->{$self->name()}->{DIFF_COMMAND};
+  return @$cmd_ref;
+}
+
+# Return true if the diff output is not customized.
+# FIXME: currently all it knows about is git's GIT_EXTERNAL_DIFF.
+# I'm sure there are other ways to configure git's diff output
+# as well as the other version control tools.
+sub diff_is_pristine()
+{
+  my $self = shift;
+  return $self->name() ne GIT || !defined $ENV{GIT_EXTERNAL_DIFF};
+}
+
 sub valid_diff_exit_status
 {
   my $self = shift;
diff --git a/vc-dwim.pl b/vc-dwim.pl
index 0d23cdf..e5eb0e5 100755
--- a/vc-dwim.pl
+++ b/vc-dwim.pl
@@ -107,11 +107,12 @@ sub verbose_cmd ($)
 }

 # Return an array of lines from running $VC diff -u on the named files.
-sub get_diffs ($$)
+# If $PRISTINE, do not honor e.g., git's --ext-diff option.
+sub get_diffs ($$$)
 {
-  my ($vc, $f) = @_;
+  my ($vc, $f, $pristine) = @_;

-  my @cmd = ($vc->diff_cmd(), @$f);
+  my @cmd = ($pristine ? $vc->diff_pristine() : $vc->diff_cmd(), @$f);
   $verbose
     and verbose_cmd address@hidden;
   open PIPE, '-|', @cmd
@@ -145,7 +146,7 @@ sub get_new_changelog_lines ($$)
 {
   my ($vc, $f) = @_;

-  my $diff_lines = get_diffs ($vc, [$f]);
+  my $diff_lines = get_diffs ($vc, [$f], 1);
   if (@$diff_lines == 0)
     {
       my $vc_name = $vc->name();
@@ -1026,11 +1027,17 @@ sub main

   # Collect diffs of non-ChangeLog files.
   # But don't print diff output unless we're sure everything is ok.
-  my $diff_lines = get_diffs $vc, address@hidden;
+  my $diff_lines = get_diffs $vc, address@hidden, 1;

   cross_check $vc, address@hidden, $diff_lines;

   print join ("\n", @log_msg_lines), "\n";
+
+  # If a user's diff settings may produce non-default-formatted diffs,
+  # then recompute those diffs now, but using their settings (not pristine).
+  $vc->diff_is_pristine
+    or $diff_lines = get_diffs $vc, address@hidden, 0;
+
   print join ("\n", @$diff_lines), "\n";

   # FIXME: add an option to take ChangeLog-style lines from a file,
--
1.7.7.rc0.362.g5a14


>From 191f4109c8456e1bfcac5e7a2858bed0f5653f44 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 3 Oct 2011 19:16:50 +0200
Subject: [PATCH 6/6] doc: mention this vc-dwim-vs-GIT_EXTERNAL_DIFF fix

* NEWS (Bug fixes): Mention this fix.
---
 NEWS |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index 571b4be..d1564a0 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ vc-dwim NEWS                                          -*- 
outline -*-

 ** Bug fixes

+  vc-dwim now works even when you set GIT_EXTERNAL_DIFF
+
   vc-dwim no longer fails when it encounters a ChangeLog line like
   "* file (...)" (i.e., with no colon) when that line ends with a ")".

--
1.7.7.rc0.362.g5a14