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

[Max Kirillov <max@max630.net>]

On Sat, Apr 11, 2015 at 10:48:22PM -0700, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:
>> 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.
> 
> > +# 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