Re: [PATCH v2] log -G: Ignore binary files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 29, 2018, 07:10 UTC
- Message-ID
- <xmqqa7lsnyu5.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <c4eac0b0ff0812e5aa8b081e603fc8bdd042ddeb.1543403143.git.thomas.braun@virtuell-zuhause.de>
Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> Subject: Re: [PATCH v2] log -G: Ignore binary files
s/Ig/ig/; (will locally munge--this alone is no reason to reroll).
The code changes looked sensible.
Show 16 quoted lines
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index 844df760f7..5c3e2a16b2 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' ' > rm .gitattributes > ' > > +test_expect_success 'log -G ignores binary files' ' > + git checkout --orphan orphan1 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git log -Ga >result && > + test_must_be_empty result > +'
As this is the first mention of data.bin, this is adding a new file data.bin that has two 'a' but is a binary file. And that is the only commit in the history leading to orphan1.
The fact that "log -Ga" won't find any means it missed the creation event, because the blob is binary. Good.
Show 5 quoted lines
> +test_expect_success 'log -G looks into binary files with -a' ' > + git checkout --orphan orphan2 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" &&
This starts from the state left by the previous test piece, i.e. we have a binary data.bin file with two 'a' in it. We pretend to modify and add, but these two steps are no-op if the previous succeeded, but even if the previous step failed, we get what we want in the data.bin file. And then we make an initial commit the same way.
> + git log -a -Ga >actual && > + git log >expected &&
And we ran the same test but this time with "-a" to tell Git that binary-ness should not matter. It will find the sole commit. Good.
Show 6 quoted lines
> + test_cmp actual expected > +' > + > +test_expect_success 'log -G looks into binary files with textconv filter' ' > + git checkout --orphan orphan3 && > + echo "* diff=bin" > .gitattributes &&
s/> />/; (will locally munge--this alone is no reason to reroll).
> + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git -c diff.bin.textconv=cat log -Ga >actual &&
This exposes a slight iffy-ness in the design. The textconv filter used here does not strip the "binary-ness" from the payload, but it is enough to tell the machinery that -G should look into the difference. Is that really desirable, though?
IOW, if this weren't the initial commit (which is handled by the codepath to special-case creation and deletion in diff_grep() function), would "log -Ga" show it without "-a"? Should it?
I think this test piece (and probably the previous ones for "-a" vs "no -a" without textconv, as well) should be using a history with three commits, where
- the root commit introduces "a\0a" to data.bin (creation event)
- the second commit adds another instance of "a\0a" to data.bin
(forces comparison)- the third commit removes data.bin (deletion event)
and make sure that the three are treated identically. If "log -Ga" finds one (with the combination of other conditions like use of textconv or -a option), it should find all three, and vice versa.
Show 13 quoted lines
> + git log >expected && > + test_cmp actual expected > +' > + > +test_expect_success 'log -S looks into binary files' ' > + git checkout --orphan orphan4 && > + printf "a\0a" >data.bin && > + git add data.bin && > + git commit -m "message" && > + git log -Sa >actual && > + git log >expected && > + test_cmp actual expected > +'
Likewise. This would also benefit from a three-commit history.
Perhaps you can create such a history at the beginning of these additions as another "setup -G/-S binary test" step and test different variations in subsequent tests without the setup?
> test_done