Re: [PATCH v2 4/4] t4059: rewrite to be adaptive to hunk filtering

Max Kirillov [off-list ref] writes:

Looks like there is no exact specification what `diff -c` and
`diff --cc` should output. Considering this it is not reasonable to
hardcode any specific output in test.

OK.

Rather, it should verify that file
selection behaves the same as hunk selection.

Hrm, really?  "git diff --raw" and "git diff -p -w" on two trees
would not show identical set of paths, when the blob object are
different byte-wise but are equivalent at the content level per
given definition of equivalence (e.g. "-w").  Given that --cc is
to look at the differences at hunk/content level to combine and omit
uninteresting ones from the output, relative to -c output, I would
imagine that the file selection and the hunk selection are expected
to behave differently.

So, having said that I am a bit skeptical about the description of
the goals, there are a few nits on the implementation, too.

+# the difference in short file must be returned if and only if it is shown in long file
+for fn in win1 win2 merge delete base only1 only2 only1discard only2discard; do
+	if git diff --cc merge branch1 branch2 mergebase -- long/$fn | grep -q '^[ +-]\{3\}2\(change[12]|merge\)\?$'
+	then

Just like the earlier parts of this patch, write a newline when you
do not have to write a semicolon, and split lines after pipe when
your pipeline gets long, i.e.

	for fn in win2 win2 ...
        do
		if git diff --cc ... |
			grep 'pattern'
		then

Do I smell some GNUism in your "grep" patterns?

You have

    '^[ +-]\{3\}2\(change[12]|merge\)\?$'

but matching zero-or-one repetition with ? is not in BRE, and \? to
use it in BRE is a GNU extension.

Also in BRE , '|' is not an alternation (and making it into such
with '\|' in BRE is a GNU extension IIRC.

Worse, you are not using backslash here to invoke GNU extension, so
I suspect the grep invocations in the patch may not be working as
you expect.

Doubly worse, what is shown by "diff -c" for line 2 seems to be
three [- +] columns, followed by one of

    2
    2change1
    2change2
    2merged

followed by the end of line, so your "|merge" part may never match.

We encourage people to avoid \{m,n\}, because, it is not supported
by some implementations even though it is specified in POSIX for
BRE.

Perhaps spelling out what you are expecting a bit more explicitly,
e.g.

	grep -e "^[-+ ][-+ ][-+ ]2$" \
             -e "^[-+ ][-+ ][-+ ]2change[12]$" \
             -e "^[-+ ][-+ ][-+ ]2merged$"

might be a reasonable way to write this in a more portable and
readable way.  And turn that into a helper shell function to make it
more readable and maintainable for a bonus point.

Thanks.

On Sat, Apr 11, 2015 at 10:48:22PM -0700, Junio C Hamano wrote:

Max Kirillov [off-list ref] writes:

quoted

Rather, it should verify that file
selection behaves the same as hunk selection.

Hrm, really?  "git diff --raw" and "git diff -p -w" on two trees
would not show identical set of paths, when the blob object are
different byte-wise but are equivalent at the content level per
given definition of equivalence (e.g. "-w").  Given that --cc is
to look at the differences at hunk/content level to combine and omit
uninteresting ones from the output, relative to -c output, I would
imagine that the file selection and the hunk selection are expected
to behave differently.

I mean, that if a hunk is shown in some long file, which
have other changes, with some option (-c or --cc), it should
also be shown if it is the only change in file.

So, having said that I am a bit skeptical about the description of
the goals, there are a few nits on the implementation, too.

quoted

+# the difference in short file must be returned if and only if it is shown in long file
+for fn in win1 win2 merge delete base only1 only2 only1discard only2discard; do
+	if git diff --cc merge branch1 branch2 mergebase -- long/$fn | grep -q '^[ +-]\{3\}2\(change[12]|merge\)\?$'
+	then

Just like the earlier parts of this patch, write a newline when you
do not have to write a semicolon, and split lines after pipe when
your pipeline gets long, i.e.

	for fn in win2 win2 ...
        do
		if git diff --cc ... |
			grep 'pattern'
		then

Do I smell some GNUism in your "grep" patterns?

You have

    '^[ +-]\{3\}2\(change[12]|merge\)\?$'

but matching zero-or-one repetition with ? is not in BRE, and \? to
use it in BRE is a GNU extension.

Also in BRE , '|' is not an alternation (and making it into such
with '\|' in BRE is a GNU extension IIRC.

Worse, you are not using backslash here to invoke GNU extension, so
I suspect the grep invocations in the patch may not be working as
you expect.

Thank you, I missed it. I suspect it was working because
single "2" still matching. Will fix it and other issues.

-- 
Max