mirror of
https://github.com/git/git
synced 2024-10-30 14:03:28 +00:00
9d484b92ed
Sergey Organov noticed and reported "--patch --no-patch --raw"
behaves differently from just "--raw". It turns out that there are
a few interesting bugs in the implementation and documentation.
* First, the documentation for "--no-patch" was unclear that it
could be read to mean "--no-patch" countermands an earlier
"--patch" but not other things. The intention of "--no-patch"
ever since it was introduced at d09cd15d (diff: allow --no-patch
as synonym for -s, 2013-07-16) was to serve as a synonym for
"-s", so "--raw --patch --no-patch" should have produced no
output, but it can be (mis)read to allow showing only "--raw"
output.
* Then the interaction between "-s" and other format options were
poorly implemented. Modern versions of Git uses one bit each to
represent formatting options like "--patch", "--stat" in a single
output_format word, but for historical reasons, "-s" also is
represented as another bit in the same word. This allows two
interesting bugs to happen, and we have both X-<.
(1) After setting a format bit, then setting NO_OUTPUT with "-s",
the code to process another "--<format>" option drops the
NO_OUTPUT bit to allow output to be shown again. However,
the code to handle "-s" only set NO_OUTPUT without unsetting
format bits set earlier, so the earlier format bit got
revealed upon seeing the second "--<format>" option. This is
the problem Sergey observed.
(2) After setting NO_OUTPUT with "-s", code to process
"--<format>" option can forget to unset NO_OUTPUT, leaving
the command still silent.
It is tempting to change the meaning of "--no-patch" to mean
"disable only the patch format output" and reimplement "-s" as "not
showing anything", but it would be an end-user visible change in
behavior. Let's fix the interactions of these bits to first make
"-s" work as intended.
The fix is conceptually very simple.
* Whenever we set DIFF_FORMAT_FOO because we saw the "--foo"
option (e.g. DIFF_FORMAT_RAW is set when the "--raw" option is
given), we make sure we drop DIFF_FORMAT_NO_OUTPUT. We forgot to
do so in some of the options and caused (2) above.
* When processing "-s" option, we should not just set
DIFF_FORMAT_NO_OUTPUT bit, but clear other DIFF_FORMAT_* bits.
We didn't do so and retained format bits set by options
previously seen, causing (1) above.
It is even more tempting to lose NO_OUTPUT bit and instead take
output_format word being 0 as its replacement, but that would break
the mechanism "git show" uses to default to "--patch" output, where
the distinction between telling the command to be silent with "-s"
and having no output format specified on the command line matters,
and an explicit output format given on the command line should not
be "combined" with the default "--patch" format.
So, while we cannot lose the NO_OUTPUT bit, as a follow-up work, we
may want to replace it with OPTION_GIVEN bit, and
* make "--patch", "--raw", etc. set DIFF_FORMAT_$format bit and
DIFF_FORMAT_OPTION_GIVEN bit on for each format. "--no-raw",
etc. will set off DIFF_FORMAT_$format bit but still record the
fact that we saw an option from the command line by setting
DIFF_FORMAT_OPTION_GIVEN bit.
* make "-s" (and its synonym "--no-patch") clear all other bits
and set only the DIFF_FORMAT_OPTION_GIVEN bit on.
which I suspect would make the code much cleaner without breaking
any end-user expectations.
Once that is in place, transitioning "--no-patch" to mean the
counterpart of "--patch", just like "--no-raw" only defeats an
earlier "--raw", would be quite simple at the code level. The
social cost of migrating the end-user expectations might be too
great for it to be worth, but at least the "GIVEN" bit clean-up
alone may be worth it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
126 lines
3 KiB
Bash
Executable file
126 lines
3 KiB
Bash
Executable file
#!/bin/sh
|
|
#
|
|
# Copyright (c) 2005 Junio C Hamano
|
|
#
|
|
|
|
test_description='Test built-in diff output engine.
|
|
|
|
We happen to know that all diff plumbing and diff Porcelain share the
|
|
same command line parser, so testing one should be sufficient; pick
|
|
diff-files as a representative.
|
|
'
|
|
|
|
TEST_PASSES_SANITIZE_LEAK=true
|
|
. ./test-lib.sh
|
|
. "$TEST_DIRECTORY"/lib-diff.sh
|
|
|
|
echo >path0 'Line 1
|
|
Line 2
|
|
line 3'
|
|
cat path0 >path1
|
|
chmod +x path1
|
|
mkdir path2
|
|
>path2/path3
|
|
|
|
test_expect_success 'update-index --add two files with and without +x.' '
|
|
git update-index --add path0 path1 path2/path3
|
|
'
|
|
|
|
mv path0 path0-
|
|
sed -e 's/line/Line/' <path0- >path0
|
|
chmod +x path0
|
|
rm -f path1
|
|
test_expect_success 'git diff-files -p after editing work tree.' '
|
|
git diff-files -p >actual
|
|
'
|
|
|
|
# that's as far as it comes
|
|
if [ "$(git config --get core.filemode)" = false ]
|
|
then
|
|
say 'filemode disabled on the filesystem'
|
|
test_done
|
|
fi
|
|
|
|
cat >expected <<\EOF
|
|
diff --git a/path0 b/path0
|
|
old mode 100644
|
|
new mode 100755
|
|
--- a/path0
|
|
+++ b/path0
|
|
@@ -1,3 +1,3 @@
|
|
Line 1
|
|
Line 2
|
|
-line 3
|
|
+Line 3
|
|
diff --git a/path1 b/path1
|
|
deleted file mode 100755
|
|
--- a/path1
|
|
+++ /dev/null
|
|
@@ -1,3 +0,0 @@
|
|
-Line 1
|
|
-Line 2
|
|
-line 3
|
|
EOF
|
|
|
|
test_expect_success 'validate git diff-files -p output.' '
|
|
compare_diff_patch expected actual
|
|
'
|
|
|
|
test_expect_success 'git diff-files -s after editing work tree' '
|
|
git diff-files -s >actual 2>err &&
|
|
test_must_be_empty actual &&
|
|
test_must_be_empty err
|
|
'
|
|
|
|
test_expect_success 'git diff-files --no-patch as synonym for -s' '
|
|
git diff-files --no-patch >actual 2>err &&
|
|
test_must_be_empty actual &&
|
|
test_must_be_empty err
|
|
'
|
|
|
|
test_expect_success 'git diff-files --no-patch --patch shows the patch' '
|
|
git diff-files --no-patch --patch >actual &&
|
|
compare_diff_patch expected actual
|
|
'
|
|
|
|
test_expect_success 'git diff-files --no-patch --patch-with-raw shows the patch and raw data' '
|
|
git diff-files --no-patch --patch-with-raw >actual &&
|
|
grep -q "^:100644 100755 .* $ZERO_OID M path0\$" actual &&
|
|
tail -n +4 actual >actual-patch &&
|
|
compare_diff_patch expected actual-patch
|
|
'
|
|
|
|
test_expect_success 'git diff-files --patch --no-patch does not show the patch' '
|
|
git diff-files --patch --no-patch >actual 2>err &&
|
|
test_must_be_empty actual &&
|
|
test_must_be_empty err
|
|
'
|
|
|
|
|
|
# Smudge path2/path3 so that dirstat has something to show
|
|
date >path2/path3
|
|
|
|
for format in stat raw numstat shortstat summary \
|
|
dirstat cumulative dirstat-by-file \
|
|
patch-with-raw patch-with-stat compact-summary
|
|
do
|
|
test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" '
|
|
git diff-files --no-patch "--$format" >actual &&
|
|
git diff-files "--$format" >expect &&
|
|
test_cmp expect actual
|
|
'
|
|
|
|
test_expect_success "--no-patch clears all previous ones" '
|
|
git diff-files --$format -s -p >actual &&
|
|
git diff-files -p >expect &&
|
|
test_cmp expect actual
|
|
'
|
|
|
|
test_expect_success "--no-patch in 'git diff --no-patch --$format' is a no-op" '
|
|
git diff --no-patch "--$format" >actual &&
|
|
git diff "--$format" >expect &&
|
|
test_cmp expect actual
|
|
'
|
|
done
|
|
|
|
test_done
|