[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-vc-dwim] vc-dwim 1.5: external diff died
From: |
Jim Meyering |
Subject: |
Re: [Bug-vc-dwim] vc-dwim 1.5: external diff died |
Date: |
Mon, 03 Oct 2011 19:17:21 +0200 |
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