{"thread":{"id":"49878","subject":"[PATCH 0/2] Teach log -G to ignore binary files","startedAt":"2018-11-21T21:00:59Z","lastAt":"2018-12-28T20:13:05Z","messageCount":30,"participants":["Thomas Braun","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"363847","messageId":"1535679074.141165.1542834055343@ox.hosteurope.de","threadId":"49878","inReplyTo":"84cbbfbd213b358d1e2d7cce8b4685b09efac3de.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"[PATCH 0/2] Teach log -G to ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-21T21:00:54Z","receivedAt":"2018-11-21T21:00:59Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Based on the previous discussion in [1] I've prepared patches which teach\nlog -G to ignore binary files. log -S keeps its behaviour but got a test to ensure that.\n\nFeedback welcome!\n\n[1]: https://public-inbox.org/git/7a0992eb-adb9-a7a1-cfaa-3384bc4d3e5c@virtuell-zuhause.de/\n\nPS: This is the (possibly missing) cover letter.\n"},{"id":"363848","messageId":"84cbbfbd213b358d1e2d7cce8b4685b09efac3de.1542833244.git.thomas.braun@virtuell-zuhause.de","threadId":"49878","inReplyTo":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"[PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-21T20:52:28Z","receivedAt":"2018-11-21T21:08:06Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"The -S option of log looks for differences that changes the\nnumber of occurrences of the specified string (i.e. addition/deletion)\nin a file.\n\nAdd a test to ensure that we keep looking into binary files with -S\nas changing that would break backwards compatibility in unexpected ways.\n\nSigned-off-by: Thomas Braun \n---\n t/t4209-log-pickaxe.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex 42cc8afd8b..d430f6f2f9 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with textconv filter' '\n \ttest_cmp actual expected\n '\n \n+test_expect_success 'log -S looks into binary files' '\n+\trm -rf .git &&\n+\tgit init &&\n+\tprintf \"a\\0b\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log -S a >actual &&\n+\tgit log >expected &&\n+\ttest_cmp actual expected\n+'\n+\n test_done\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"363849","messageId":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","threadId":"49878","inReplyTo":"cover.1542831544.git.thomas.braun@virtuell-zuhause.de","subject":"[PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-21T20:52:27Z","receivedAt":"2018-11-21T21:08:09Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"The -G option of log looks for the differences whose patch text\ncontains added/removed lines that match regex.\n\nThe concept of differences only makes sense for text files, therefore\nwe need to ignore binary files when searching with -G as well.\n\nSigned-off-by: Thomas Braun \n---\n Documentation/gitdiffcore.txt | 2 +-\n diffcore-pickaxe.c | 5 +++++\n t/t4209-log-pickaxe.sh | 22 ++++++++++++++++++++++\n 3 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\nindex c0a60f3158..059ddd3431 100644\n--- a/Documentation/gitdiffcore.txt\n+++ b/Documentation/gitdiffcore.txt\n@@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n regular expression. This means that it will detect in-file (or what\n rename-detection considers the same file) moves, which is noise. The\n implementation runs diff twice and greps, and this can be quite\n-expensive.\n+expensive. Binary files without textconv filter are ignored.\n \n When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n that match their respective criterion are kept in the output. When\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 69fc55ea1e..8c2558b07d 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n \t}\n \n+\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n+\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n+\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n+\t\treturn 0;\n+\n \t/*\n \t * If we have an unmodified pair, we know that the count will be the\n \t * same and don't even have to load the blobs. Unless textconv is in\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex 844df760f7..42cc8afd8b 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n \trm .gitattributes\n '\n \n+test_expect_success 'log -G ignores binary files' '\n+\trm -rf .git &&\n+\tgit init &&\n+\tprintf \"a\\0b\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log -G a >result &&\n+\ttest_must_be_empty result\n+'\n+\n+test_expect_success 'log -G looks into binary files with textconv filter' '\n+\trm -rf .git &&\n+\tgit init &&\n+\techo \"* diff=bin\" > .gitattributes &&\n+\tprintf \"a\\0b\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit -c diff.bin.textconv=cat log -G a >actual &&\n+\tgit log >expected &&\n+\ttest_cmp actual expected\n+'\n+\n test_done\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"363850","messageId":"cover.1542831544.git.thomas.braun@virtuell-zuhause.de","threadId":"49878","inReplyTo":null,"subject":"[PATCH 0/2] Teach log -G to ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-21T20:52:26Z","receivedAt":"2018-11-21T21:08:10Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Based on the previous discussion in [1] I've prepared patches which teach \nlog -G to ignore binary files. log -S keeps its behaviour but got a test to ensure that.\n\nFeedback welcome!\n\n[1]: https://public-inbox.org/git/7a0992eb-adb9-a7a1-cfaa-3384bc4d3e5c@virtuell-zuhause.de/\n\nThomas Braun (2):\n log -G: Ignore binary files\n log -S: Add test which searches in binary files\n\n Documentation/gitdiffcore.txt | 2 +-\n diffcore-pickaxe.c | 5 +++++\n t/t4209-log-pickaxe.sh | 21 +++++++++++++++++++++\n 3 files changed, 27 insertions(+), 1 deletion(-)\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"363860","messageId":"xmqq4lc96gsg.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-22T01:29:35Z","receivedAt":"2018-11-22T01:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Braun writes:\n\n> The -G option of log looks for the differences whose patch text\n> contains added/removed lines that match regex.\n>\n> The concept of differences only makes sense for text files, therefore\n> we need to ignore binary files when searching with -G as well.\n>\n> Signed-off-by: Thomas Braun \n> ---\n> Documentation/gitdiffcore.txt | 2 +-\n> diffcore-pickaxe.c | 5 +++++\n> t/t4209-log-pickaxe.sh | 22 ++++++++++++++++++++++\n> 3 files changed, 28 insertions(+), 1 deletion(-)\n\nOK.\n\n> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> index c0a60f3158..059ddd3431 100644\n> --- a/Documentation/gitdiffcore.txt\n> +++ b/Documentation/gitdiffcore.txt\n> @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n> regular expression. This means that it will detect in-file (or what\n> rename-detection considers the same file) moves, which is noise. The\n> implementation runs diff twice and greps, and this can be quite\n> -expensive.\n> +expensive. Binary files without textconv filter are ignored.\n\nOK.\n\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 69fc55ea1e..8c2558b07d 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n> \t}\n> \n> +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> +\t\treturn 0;\n> +\n> \t/*\n> \t * If we have an unmodified pair, we know that the count will be the\n> \t * same and don't even have to load the blobs. Unless textconv is in\n\nShouldn't this new test come after the existing optimization, which\nallows us to leave without loading the blob contents (which is\nneeded once you call diff_filespec_is_binary())?\n\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..42cc8afd8b 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> \trm .gitattributes\n> '\n> \n> +test_expect_success 'log -G ignores binary files' '\n> +\trm -rf .git &&\n> +\tgit init &&\n\nPlease never never ever do the above two unless you are writing a\ntest that checks low-level repository details.\n\nIf you want a clean history that has specific lineage of commits\nwithout getting affected by commits that have been made by the\nprevious test pieces, it is OK to \"checkout --orphan\" to create an\nempty history to work with.\n\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -G a >result &&\n> +\ttest_must_be_empty result\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n> +\trm -rf .git &&\n> +\tgit init &&\n> +\techo \"* diff=bin\" > .gitattributes &&\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> test_done\n"},{"id":"363861","messageId":"xmqqzhu1520k.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"84cbbfbd213b358d1e2d7cce8b4685b09efac3de.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-22T01:34:03Z","receivedAt":"2018-11-22T01:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Braun writes:\n\n> The -S option of log looks for differences that changes the\n> number of occurrences of the specified string (i.e. addition/deletion)\n> in a file.\n\ns/-S /-S/ and\ns/the specified string/the specified block of text/ would make it\nmore in line with how Documentation/gitdiffcore.txt explains it.\nThe original discussion from early 2017 also explains with a pointer\nwhy the primary mode of -S is not but is .\n\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 42cc8afd8b..d430f6f2f9 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with textconv filter' '\n> \ttest_cmp actual expected\n> '\n> \n> +test_expect_success 'log -S looks into binary files' '\n> +\trm -rf .git &&\n> +\tgit init &&\n\nSame comment as the one for 1/2 applies here.\n\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -S a >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> test_done\n\nOther than these, I think both patches look sensible. Thanks for\nresurrecting the old topic and reigniting it.\n"},{"id":"363875","messageId":"87lg5lzd7i.fsf@evledraar.gmail.com","threadId":"49878","inReplyTo":"84cbbfbd213b358d1e2d7cce8b4685b09efac3de.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-22T09:14:09Z","receivedAt":"2018-11-22T09:14:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 21 2018, Thomas Braun wrote:\n\n> The -S option of log looks for differences that changes the\n> number of occurrences of the specified string (i.e. addition/deletion)\n> in a file.\n>\n> Add a test to ensure that we keep looking into binary files with -S\n> as changing that would break backwards compatibility in unexpected ways.\n>\n> Signed-off-by: Thomas Braun \n> ---\n> t/t4209-log-pickaxe.sh | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n>\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 42cc8afd8b..d430f6f2f9 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with textconv filter' '\n> \ttest_cmp actual expected\n> '\n>\n> +test_expect_success 'log -S looks into binary files' '\n> +\trm -rf .git &&\n> +\tgit init &&\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -S a >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> test_done\n\nThis should just be part of 1/2 since the behavior is changed there &\nthe commit message should describe both cases.\n"},{"id":"363876","messageId":"87k1l5zabd.fsf@evledraar.gmail.com","threadId":"49878","inReplyTo":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-22T10:16:38Z","receivedAt":"2018-11-22T10:16:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":">\nOn Wed, Nov 21 2018, Thomas Braun wrote:\n\n> The -G option of log looks for the differences whose patch text\n> contains added/removed lines that match regex.\n>\n> The concept of differences only makes sense for text files, therefore\n> we need to ignore binary files when searching with -G as well.\n>\n> Signed-off-by: Thomas Braun \n> ---\n> Documentation/gitdiffcore.txt | 2 +-\n> diffcore-pickaxe.c | 5 +++++\n> t/t4209-log-pickaxe.sh | 22 ++++++++++++++++++++++\n> 3 files changed, 28 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> index c0a60f3158..059ddd3431 100644\n> --- a/Documentation/gitdiffcore.txt\n> +++ b/Documentation/gitdiffcore.txt\n> @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n> regular expression. This means that it will detect in-file (or what\n> rename-detection considers the same file) moves, which is noise. The\n> implementation runs diff twice and greps, and this can be quite\n> -expensive.\n> +expensive. Binary files without textconv filter are ignored.\n>\n> When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n> that match their respective criterion are kept in the output. When\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 69fc55ea1e..8c2558b07d 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n> \t}\n>\n> +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> +\t\treturn 0;\n> +\n> \t/*\n> \t * If we have an unmodified pair, we know that the count will be the\n> \t * same and don't even have to load the blobs. Unless textconv is in\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..42cc8afd8b 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> \trm .gitattributes\n> '\n>\n> +test_expect_success 'log -G ignores binary files' '\n> +\trm -rf .git &&\n> +\tgit init &&\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -G a >result &&\n\nWould be less confusing as \"-Ga\" since that's the invocation we\ndocument, even though I see (but wasn't aware that...) \"-G a\" works too.\n\n> +\ttest_must_be_empty result\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n> +\trm -rf .git &&\n> +\tgit init &&\n> +\techo \"* diff=bin\" > .gitattributes &&\n> +\tprintf \"a\\0b\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> test_done\n\nThis patch seems like the wrong direction to me. In particular the\nassertion that \"the concept of differences only makes sense for text\nfiles\". That's just not true. This patch breaks this:\n\n (\n rm -rf /tmp/g-test &&\n git init /tmp/g-test &&\n cd /tmp/g-test &&\n for i in {1..10}; do\n echo \"Always matching thensome 5\" >file &&\n printf \"a thensome %d binary \\0\" $i >>file &&\n git add file &&\n git commit -m\"Bump $i\"\n done &&\n git log -Gthensome.*5\n )\n\nRight now this will emit 3/10 patches, and the right ones! I.e. \"Bump\n[156]\". The 1st one because it introduces the \"Always matching thensome\n5\". Then 5/6 because the add/remove the string \"a thensome 5 binary\",\nrespectively. Which matches /thensome.*5/.\n\nI.e. in the first one we do a regex match against the content here\nbecause we don't have both sides:\nhttps://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L48-L53\n\nAnd then for the later ones where we have both sides we end up in\ndiffgrep_consume():\nhttps://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L27-L36\n\nI think there may be a real issue here to address, which might be some\ncombination of:\n\n a) Even though the diffcore can do a binary diff internally, this is\n not what it exposes with \"-p\", we just say \"Binary files differ\".\n\n I don't know how to emit the raw version we'll end up passing to\n diffgrep_consume() in this case. Is it just --binary without the\n encoding? I don't know...\n\n b) Your test case shows that you're matching a string at a \\0\n boundary. Is this perhaps something you ran into? I.e. that we don't\n have some -F version of -G so we can't supply regexes that match\n past a \\0? I had some related work on grep for this that hasn't been\n carried over to the diffcore:\n\n git log --grep='grep:.*\\\\0' --author=Ævar\n\n c) Is this binary diff we end up matching against just bad in some\n cases? I haven't dug but that wouldn't surprise me, i.e. that it's\n trying to be line-based so we'll overmatch in many cases.\n\nSo maybe this is something that should be passed down as a flag? See a\nrecent discussion at\nhttps://public-inbox.org/git/87lg77cmr1.fsf@evledraar.gmail.com/ for how\nthat could be done.\n\nAlso if we don't have some tests already that were failing with this\npatch we really should have those as \"let's test the current behavior\nfirst\". Unfortunately tests in this area are really lacking, see\ne.g. my:\n\n git log --author=Junio --min-parents=2 --grep=ab/.*grep\n\nFor some series of patches to grep where to get one patch in I needed to\noften lead with 5-10 test patches to convince reviewers that I knew what\nI was changing, and also to be comfortable that I'd covered all the edge\ncases we currently supported, but weren't testing for.\n"},{"id":"363911","messageId":"20181122162024.GD28192@sigill.intra.peff.net","threadId":"49878","inReplyTo":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-22T16:20:24Z","receivedAt":"2018-11-22T16:20:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 21, 2018 at 09:52:27PM +0100, Thomas Braun wrote:\n\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 69fc55ea1e..8c2558b07d 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n> \t}\n> \n> +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> +\t\treturn 0;\n\nIf the user passes \"-a\" to treat binary files as text, we should\nprobably skip the binary check. I think we'd need to check\n\"o->flags.text\" here.\n\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..42cc8afd8b 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> [...]\n> +test_expect_success 'log -G ignores binary files' '\n> [...]\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n\nAnd likewise add a test here similar to the textconv one.\n\n-Peff\n"},{"id":"363912","messageId":"20181122162705.GE28192@sigill.intra.peff.net","threadId":"49878","inReplyTo":"87k1l5zabd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-22T16:27:05Z","receivedAt":"2018-11-22T16:27:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 22, 2018 at 11:16:38AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n> \n> This patch seems like the wrong direction to me. In particular the\n> assertion that \"the concept of differences only makes sense for text\n> files\". That's just not true. This patch breaks this:\n\nBut \"-G\" is defined as \"look for differences whose patch text contains\nadded/removed lines that match \". We don't have patch text here,\nlet alone added/removed lines.\n\nFor binary files, \"-Sfoo\" is better defined. I think we _could_ define\n\"search for in the added/removed bytes of a binary file\". But\nI don't think that's what the current code does (it really does a line\ndiff on a binary file, which is likely to put tons of unchanged crap\ninto the \"added and removed\" lines, because the line divisions aren't\nmeaningful in the first place).\n\n> (\n> rm -rf /tmp/g-test &&\n> git init /tmp/g-test &&\n> cd /tmp/g-test &&\n> for i in {1..10}; do\n> echo \"Always matching thensome 5\" >file &&\n> printf \"a thensome %d binary \\0\" $i >>file &&\n> git add file &&\n> git commit -m\"Bump $i\"\n> done &&\n> git log -Gthensome.*5\n> )\n> \n> Right now this will emit 3/10 patches, and the right ones! I.e. \"Bump\n> [156]\". The 1st one because it introduces the \"Always matching thensome\n> 5\". Then 5/6 because the add/remove the string \"a thensome 5 binary\",\n> respectively. Which matches /thensome.*5/.\n\nRight, this will sometimes do the right thing. But it will also often do\nthe wrong thing. It's also very expensive (we specifically avoid feeding\nlarge binary files to xdiff, but I think \"-G\" will happily do so -- I\ndidn't double check, though).\n\n-Peff\n"},{"id":"363982","messageId":"xmqq5zwn43c9.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"87lg5lzd7i.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-24T02:27:34Z","receivedAt":"2018-11-24T02:27:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Wed, Nov 21 2018, Thomas Braun wrote:\n>\n>> The -S option of log looks for differences that changes the\n>> number of occurrences of the specified string (i.e. addition/deletion)\n>> in a file.\n>>\n> ...\n> This should just be part of 1/2 since the behavior is changed there &\n> the commit message should describe both cases.\n\nSensible suggestion. Thanks.\n"},{"id":"363983","messageId":"xmqq1s7b434u.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"20181122162024.GD28192@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-24T02:32:01Z","receivedAt":"2018-11-24T02:32:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n>> +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n>> +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n>> +\t\treturn 0;\n>\n> If the user passes \"-a\" to treat binary files as text, we should\n> probably skip the binary check. I think we'd need to check\n> \"o->flags.text\" here.\n\nYeah, I forgot about that option. It would give an escape hatch\nthat has a sane explanation.\n"},{"id":"364098","messageId":"CAGZ79kaaLeaffMnkyd7Z1gF5yxq=GDH_Fy_PG1=FfcL=9b9nRQ@mail.gmail.com","threadId":"49878","inReplyTo":"590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-26T20:19:01Z","receivedAt":"2018-11-26T20:19:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 21, 2018 at 1:08 PM Thomas Braun\n wrote:\n>\n> The -G option of log looks for the differences whose patch text\n> contains added/removed lines that match regex.\n>\n> The concept of differences only makes sense for text files, therefore\n> we need to ignore binary files when searching with -G as well.\n\nWhat about partial text/partial binary files?\n\nI recall using text searching tools (not necessarily git machinery,\nmy memory is fuzzy) to check for strings in pdf files, which are\nusually marked binary in context of git, such that we do not\nsee their diffs in `log -p`.\n\nBut I would expect a search with -G or -S to still work...\nuntil I find the exception in the docs, only to wonder if\nthere is a switch to turn off this optimisation for this\ncorner case.\n\nStefan\n"},{"id":"364113","messageId":"xmqq4lc3uyuh.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"CAGZ79kaaLeaffMnkyd7Z1gF5yxq=GDH_Fy_PG1=FfcL=9b9nRQ@mail.gmail.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-27T00:51:34Z","receivedAt":"2018-11-27T00:51:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Wed, Nov 21, 2018 at 1:08 PM Thomas Braun\n> wrote:\n>>\n>> The -G option of log looks for the differences whose patch text\n>> contains added/removed lines that match regex.\n>>\n>> The concept of differences only makes sense for text files, therefore\n>> we need to ignore binary files when searching with -G as well.\n>\n> What about partial text/partial binary files?\n\nGood point. You'd use \"-a\" (or \"--text\") to tell the diff machinery\nto treat the contents as text, and the new logic must pay attention\nto that command line option.\n"},{"id":"364207","messageId":"1687320635.1640.1543404663287@ox.hosteurope.de","threadId":"49878","inReplyTo":"xmqq4lc96gsg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:03Z","receivedAt":"2018-11-28T11:31:09Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Junio C Hamano hat am 22. November 2018 um 02:29 geschrieben:\n> \n> \n> Thomas Braun writes:\n> \n> > The -G option of log looks for the differences whose patch text\n> > contains added/removed lines that match regex.\n> >\n> > The concept of differences only makes sense for text files, therefore\n> > we need to ignore binary files when searching with -G as well.\n> >\n> > Signed-off-by: Thomas Braun \n> > ---\n> > Documentation/gitdiffcore.txt | 2 +-\n> > diffcore-pickaxe.c | 5 +++++\n> > t/t4209-log-pickaxe.sh | 22 ++++++++++++++++++++++\n> > 3 files changed, 28 insertions(+), 1 deletion(-)\n> \n> OK.\n> \n> > diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> > index c0a60f3158..059ddd3431 100644\n> > --- a/Documentation/gitdiffcore.txt\n> > +++ b/Documentation/gitdiffcore.txt\n> > @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n> > regular expression. This means that it will detect in-file (or what\n> > rename-detection considers the same file) moves, which is noise. The\n> > implementation runs diff twice and greps, and this can be quite\n> > -expensive.\n> > +expensive. Binary files without textconv filter are ignored.\n> \n> OK.\n> \n> > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> > index 69fc55ea1e..8c2558b07d 100644\n> > --- a/diffcore-pickaxe.c\n> > +++ b/diffcore-pickaxe.c\n> > @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> > \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n> > \t}\n> > \n> > +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> > +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> > +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> > +\t\treturn 0;\n> > +\n> > \t/*\n> > \t * If we have an unmodified pair, we know that the count will be the\n> > \t * same and don't even have to load the blobs. Unless textconv is in\n> \n> Shouldn't this new test come after the existing optimization, which\n> allows us to leave without loading the blob contents (which is\n> needed once you call diff_filespec_is_binary())?\n\nYes, good point.\n\n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 844df760f7..42cc8afd8b 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> > \trm .gitattributes\n> > '\n> > \n> > +test_expect_success 'log -G ignores binary files' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> \n> Please never never ever do the above two unless you are writing a\n> test that checks low-level repository details.\n> \n> If you want a clean history that has specific lineage of commits\n> without getting affected by commits that have been made by the\n> previous test pieces, it is OK to \"checkout --orphan\" to create an\n> empty history to work with.\n\nThanks for the hint. I thought I had seen a less intrusive way for getting an empty history. \nChanged.\n\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -G a >result &&\n> > +\ttest_must_be_empty result\n> > +'\n> > +\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n>\n"},{"id":"364208","messageId":"2133674367.1646.1543404670839@ox.hosteurope.de","threadId":"49878","inReplyTo":"xmqqzhu1520k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:10Z","receivedAt":"2018-11-28T11:31:14Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Junio C Hamano hat am 22. November 2018 um 02:34 geschrieben:\n> \n> \n> Thomas Braun writes:\n> \n> > The -S option of log looks for differences that changes the\n> > number of occurrences of the specified string (i.e. addition/deletion)\n> > in a file.\n> \n> s/-S /-S/ and\n> s/the specified string/the specified block of text/ would make it\n> more in line with how Documentation/gitdiffcore.txt explains it.\n> The original discussion from early 2017 also explains with a pointer\n> why the primary mode of -S is not but is .\n\nThanks for the pointer. I've updated the commit message.\n \n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 42cc8afd8b..d430f6f2f9 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with textconv filter' '\n> > \ttest_cmp actual expected\n> > '\n> > \n> > +test_expect_success 'log -S looks into binary files' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> \n> Same comment as the one for 1/2 applies here.\n\nFixed as well.\n\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -S a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n> \n> Other than these, I think both patches look sensible. Thanks for\n> resurrecting the old topic and reigniting it.\n>\n"},{"id":"364209","messageId":"849207555.1650.1543404675371@ox.hosteurope.de","threadId":"49878","inReplyTo":"87lg5lzd7i.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1 2/2] log -S: Add test which searches in binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:15Z","receivedAt":"2018-11-28T11:31:19Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Ævar Arnfjörð Bjarmason hat am 22. November 2018 um 10:14 geschrieben:\n> \n> \n> \n> On Wed, Nov 21 2018, Thomas Braun wrote:\n> \n> > The -S option of log looks for differences that changes the\n> > number of occurrences of the specified string (i.e. addition/deletion)\n> > in a file.\n> >\n> > Add a test to ensure that we keep looking into binary files with -S\n> > as changing that would break backwards compatibility in unexpected ways.\n> >\n> > Signed-off-by: Thomas Braun \n> > ---\n> > t/t4209-log-pickaxe.sh | 11 +++++++++++\n> > 1 file changed, 11 insertions(+)\n> >\n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 42cc8afd8b..d430f6f2f9 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with textconv filter' '\n> > \ttest_cmp actual expected\n> > '\n> >\n> > +test_expect_success 'log -S looks into binary files' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -S a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n> \n> This should just be part of 1/2 since the behavior is changed there &\n> the commit message should describe both cases.\n\nMy reasoning was that this is a separate test which does not fit in with the other part.\nBut I'm happy in folding both into one patch. Done.\n"},{"id":"364210","messageId":"1544218571.1651.1543404679825@ox.hosteurope.de","threadId":"49878","inReplyTo":"87k1l5zabd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:19Z","receivedAt":"2018-11-28T11:31:25Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Ævar Arnfjörð Bjarmason hat am 22. November 2018 um 11:16 geschrieben:\n\n[...]\n\n> >\n> > +test_expect_success 'log -G ignores binary files' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -G a >result &&\n> \n> Would be less confusing as \"-Ga\" since that's the invocation we\n> document, even though I see (but wasn't aware that...) \"-G a\" works too.\n\nDone.\n\n> > +\ttest_must_be_empty result\n> > +'\n> > +\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n> \n> This patch seems like the wrong direction to me. In particular the\n> assertion that \"the concept of differences only makes sense for text\n> files\". That's just not true. This patch breaks this:\n> \n> (\n> rm -rf /tmp/g-test &&\n> git init /tmp/g-test &&\n> cd /tmp/g-test &&\n> for i in {1..10}; do\n> echo \"Always matching thensome 5\" >file &&\n> printf \"a thensome %d binary \\0\" $i >>file &&\n> git add file &&\n> git commit -m\"Bump $i\"\n> done &&\n> git log -Gthensome.*5\n> )\n> \n> Right now this will emit 3/10 patches, and the right ones! I.e. \"Bump\n> [156]\". The 1st one because it introduces the \"Always matching thensome\n> 5\". Then 5/6 because the add/remove the string \"a thensome 5 binary\",\n> respectively. Which matches /thensome.*5/.\n\nlog -p does not show you the patch text in your example because it is treated\nas binary. And currently \"log -G\" has a different opinion into what it looks\nand what it ignores. My patch tries to bring both more in line.\n \n> I.e. in the first one we do a regex match against the content here\n> because we don't have both sides:\n> https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L48-L53\n> \n> And then for the later ones where we have both sides we end up in\n> diffgrep_consume():\n> https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L27-L36\n> \n> I think there may be a real issue here to address, which might be some\n> combination of:\n> \n> a) Even though the diffcore can do a binary diff internally, this is\n> not what it exposes with \"-p\", we just say \"Binary files differ\".\n> \n> I don't know how to emit the raw version we'll end up passing to\n> diffgrep_consume() in this case. Is it just --binary without the\n> encoding? I don't know...\n> \n> b) Your test case shows that you're matching a string at a \\0\n> boundary. Is this perhaps something you ran into? I.e. that we don't\n> have some -F version of -G so we can't supply regexes that match\n> past a \\0? I had some related work on grep for this that hasn't been\n> carried over to the diffcore:\n> \n> git log --grep='grep:.*\\\\0' --author=Ævar\n> \n> c) Is this binary diff we end up matching against just bad in some\n> cases? I haven't dug but that wouldn't surprise me, i.e. that it's\n> trying to be line-based so we'll overmatch in many cases.\n> \n> So maybe this is something that should be passed down as a flag? See a\n> recent discussion at\n> https://public-inbox.org/git/87lg77cmr1.fsf@evledraar.gmail.com/ for how\n> that could be done.\n\nIt is not about the \\0 boundary. v2 of the patches will clarify that. My main\nmotiviation is to speed up \"log -G\" as that takes a considerable amount of time \nwhen it wades through MBs of binary files which change often. And in multiple places\nI can already treat binary files differently (e.g. turn off delta compression, skip\ntrying to diff them, no EOL normalization). And for me making log -G ignore what git \nthinks are binary files is making the line clearer between what should be treated as binary\nand what as text.\n\n> Also if we don't have some tests already that were failing with this\n> patch we really should have those as \"let's test the current behavior\n> first\". Unfortunately tests in this area are really lacking, see\n> e.g. my:\n> \n> git log --author=Junio --min-parents=2 --grep=ab/.*grep\n> \n> For some series of patches to grep where to get one patch in I needed to\n> often lead with 5-10 test patches to convince reviewers that I knew what\n> I was changing, and also to be comfortable that I'd covered all the edge\n> cases we currently supported, but weren't testing for.\n\nI'm happy to add more test cases to convince everyone involved :)\n"},{"id":"364211","messageId":"1148695380.1653.1543404686042@ox.hosteurope.de","threadId":"49878","inReplyTo":"20181122162024.GD28192@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:26Z","receivedAt":"2018-11-28T11:31:30Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Jeff King hat am 22. November 2018 um 17:20 geschrieben:\n> \n> \n> On Wed, Nov 21, 2018 at 09:52:27PM +0100, Thomas Braun wrote:\n> \n> > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> > index 69fc55ea1e..8c2558b07d 100644\n> > --- a/diffcore-pickaxe.c\n> > +++ b/diffcore-pickaxe.c\n> > @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> > \t\ttextconv_two = get_textconv(o->repo->index, p->two);\n> > \t}\n> > \n> > +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> > +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> > +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> > +\t\treturn 0;\n> \n> If the user passes \"-a\" to treat binary files as text, we should\n> probably skip the binary check. I think we'd need to check\n> \"o->flags.text\" here.\n\nGood point. I missed that flag. Added.\n\n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 844df760f7..42cc8afd8b 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> > [...]\n> > +test_expect_success 'log -G ignores binary files' '\n> > [...]\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> \n> And likewise add a test here similar to the textconv one.\n\nAdded as well.\n"},{"id":"364212","messageId":"992623969.1654.1543404690644@ox.hosteurope.de","threadId":"49878","inReplyTo":"xmqq4lc3uyuh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:30Z","receivedAt":"2018-11-28T11:31:34Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"\n> Junio C Hamano hat am 27. November 2018 um 01:51 geschrieben:\n> \n> \n> Stefan Beller writes:\n> \n> > On Wed, Nov 21, 2018 at 1:08 PM Thomas Braun\n> > wrote:\n> >>\n> >> The -G option of log looks for the differences whose patch text\n> >> contains added/removed lines that match regex.\n> >>\n> >> The concept of differences only makes sense for text files, therefore\n> >> we need to ignore binary files when searching with -G as well.\n> >\n> > What about partial text/partial binary files?\n> \n> Good point. You'd use \"-a\" (or \"--text\") to tell the diff machinery\n> to treat the contents as text, and the new logic must pay attention\n> to that command line option.\n\nYes exactly. Either use -a for the occasional use or a textconv filter\nfor permanent use.\n\nComing from the opposite side: I usually mark svg files as binary as the\ntextual diff is well, let's say uninspiring.\n"},{"id":"364213","messageId":"910459876.1655.1543404700658@ox.hosteurope.de","threadId":"49878","inReplyTo":"87k1l5zabd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1 1/2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:31:40Z","receivedAt":"2018-11-28T11:31:44Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Ævar Arnfjörð Bjarmason hat am 22. November 2018 um 11:16 geschrieben:\n\n[...]\n\n> >\n> > +test_expect_success 'log -G ignores binary files' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -G a >result &&\n> \n> Would be less confusing as \"-Ga\" since that's the invocation we\n> document, even though I see (but wasn't aware that...) \"-G a\" works too.\n\nDone.\n\n> > +\ttest_must_be_empty result\n> > +'\n> > +\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\trm -rf .git &&\n> > +\tgit init &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> > +\tprintf \"a\\0b\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -G a >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n> \n> This patch seems like the wrong direction to me. In particular the\n> assertion that \"the concept of differences only makes sense for text\n> files\". That's just not true. This patch breaks this:\n> \n> (\n> rm -rf /tmp/g-test &&\n> git init /tmp/g-test &&\n> cd /tmp/g-test &&\n> for i in {1..10}; do\n> echo \"Always matching thensome 5\" >file &&\n> printf \"a thensome %d binary \\0\" $i >>file &&\n> git add file &&\n> git commit -m\"Bump $i\"\n> done &&\n> git log -Gthensome.*5\n> )\n> \n> Right now this will emit 3/10 patches, and the right ones! I.e. \"Bump\n> [156]\". The 1st one because it introduces the \"Always matching thensome\n> 5\". Then 5/6 because the add/remove the string \"a thensome 5 binary\",\n> respectively. Which matches /thensome.*5/.\n\nlog -p does not show you the patch text in your example because it is treated\nas binary. And currently \"log -G\" has a different opinion into what it looks\nand what it ignores. My patch tries to bring both more in line.\n \n> I.e. in the first one we do a regex match against the content here\n> because we don't have both sides:\n> https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L48-L53\n> \n> And then for the later ones where we have both sides we end up in\n> diffgrep_consume():\n> https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L27-L36\n> \n> I think there may be a real issue here to address, which might be some\n> combination of:\n> \n> a) Even though the diffcore can do a binary diff internally, this is\n> not what it exposes with \"-p\", we just say \"Binary files differ\".\n> \n> I don't know how to emit the raw version we'll end up passing to\n> diffgrep_consume() in this case. Is it just --binary without the\n> encoding? I don't know...\n> \n> b) Your test case shows that you're matching a string at a \\0\n> boundary. Is this perhaps something you ran into? I.e. that we don't\n> have some -F version of -G so we can't supply regexes that match\n> past a \\0? I had some related work on grep for this that hasn't been\n> carried over to the diffcore:\n> \n> git log --grep='grep:.*\\\\0' --author=Ævar\n> \n> c) Is this binary diff we end up matching against just bad in some\n> cases? I haven't dug but that wouldn't surprise me, i.e. that it's\n> trying to be line-based so we'll overmatch in many cases.\n> \n> So maybe this is something that should be passed down as a flag? See a\n> recent discussion at\n> https://public-inbox.org/git/87lg77cmr1.fsf@evledraar.gmail.com/ for how\n> that could be done.\n\nIt is not about the \\0 boundary. v2 of the patches will clarify that. My main\nmotiviation is to speed up \"log -G\" as that takes a considerable amount of time \nwhen it wades through MBs of binary files which change often. And in multiple places\nI can already treat binary files differently (e.g. turn off delta compression, skip\ntrying to diff them, no EOL normalization). And for me making log -G ignore what git \nthinks are binary files is making the line clearer between what should be treated\nas binary and what as text.\n\n> Also if we don't have some tests already that were failing with this\n> patch we really should have those as \"let's test the current behavior\n> first\". Unfortunately tests in this area are really lacking, see\n> e.g. my:\n> \n> git log --author=Junio --min-parents=2 --grep=ab/.*grep\n> \n> For some series of patches to grep where to get one patch in I needed to\n> often lead with 5-10 test patches to convince reviewers that I knew what\n> I was changing, and also to be comfortable that I'd covered all the edge\n> cases we currently supported, but weren't testing for.\n\nI'm happy to add more test cases to convince everyone involved :)\n"},{"id":"364214","messageId":"c4eac0b0ff0812e5aa8b081e603fc8bdd042ddeb.1543403143.git.thomas.braun@virtuell-zuhause.de","threadId":"49878","inReplyTo":"1535679074.141165.1542834055343@ox.hosteurope.de","subject":"[PATCH v2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-11-28T11:32:57Z","receivedAt":"2018-11-28T11:33:16Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"The -G option of log looks for the differences whose patch text\ncontains added/removed lines that match regex.\n\nAs the concept of patch text only makes sense for text files, we need to\nignore binary files when searching with -G as well.\n\nThe -S option of log looks for differences that changes\nthe number of occurrences of the specified block of text (i.e.\naddition/deletion) in a file. As we want to keep the current behaviour,\nadd a test to ensure it.\n\nSigned-off-by: Thomas Braun \n---\n\nChanges since v1:\n- Merged both patches into one\n- Adapted commit messages\n- Added missing support for -a flag with tests\n- Placed new code into correct location to be able to reuse an existing\n optimization\n- Uses help-suggested -Ga writing without spaces\n- Uses orphan branches instead of cannonball cleanup with rm -rf\n- Changed search text to make it clear that it is not about the \\0 boundary\n\n Documentation/gitdiffcore.txt | 2 +-\n diffcore-pickaxe.c | 6 ++++++\n t/t4209-log-pickaxe.sh | 40 +++++++++++++++++++++++++++++++++++\n 3 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\nindex c0a60f3158..059ddd3431 100644\n--- a/Documentation/gitdiffcore.txt\n+++ b/Documentation/gitdiffcore.txt\n@@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n regular expression. This means that it will detect in-file (or what\n rename-detection considers the same file) moves, which is noise. The\n implementation runs diff twice and greps, and this can be quite\n-expensive.\n+expensive. Binary files without textconv filter are ignored.\n \n When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n that match their respective criterion are kept in the output. When\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 69fc55ea1e..4cea086f80 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -154,6 +154,12 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n+\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n+\t !o->flags.text &&\n+\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n+\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n+\t\treturn 0;\n+\n \tmf1.size = fill_textconv(o->repo, textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(o->repo, textconv_two, p->two, &mf2.ptr);\n \ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex 844df760f7..5c3e2a16b2 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n \trm .gitattributes\n '\n \n+test_expect_success 'log -G ignores binary files' '\n+\tgit checkout --orphan orphan1 &&\n+\tprintf \"a\\0a\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log -Ga >result &&\n+\ttest_must_be_empty result\n+'\n+\n+test_expect_success 'log -G looks into binary files with -a' '\n+\tgit checkout --orphan orphan2 &&\n+\tprintf \"a\\0a\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log -a -Ga >actual &&\n+\tgit log >expected &&\n+\ttest_cmp actual expected\n+'\n+\n+test_expect_success 'log -G looks into binary files with textconv filter' '\n+\tgit checkout --orphan orphan3 &&\n+\techo \"* diff=bin\" > .gitattributes &&\n+\tprintf \"a\\0a\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n+\tgit log >expected &&\n+\ttest_cmp actual expected\n+'\n+\n+test_expect_success 'log -S looks into binary files' '\n+\tgit checkout --orphan orphan4 &&\n+\tprintf \"a\\0a\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log -Sa >actual &&\n+\tgit log >expected &&\n+\ttest_cmp actual expected\n+'\n+\n test_done\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364218","messageId":"87a7ltz7jh.fsf@evledraar.gmail.com","threadId":"49878","inReplyTo":"c4eac0b0ff0812e5aa8b081e603fc8bdd042ddeb.1543403143.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-28T12:54:42Z","receivedAt":"2018-11-28T12:54:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 28 2018, Thomas Braun wrote:\n\nLooks much better this time around.\n\n> The -G option of log looks for the differences whose patch text\n> contains added/removed lines that match regex.\n>\n> As the concept of patch text only makes sense for text files, we need to\n> ignore binary files when searching with -G as well.\n>\n> The -S option of log looks for differences that changes\n> the number of occurrences of the specified block of text (i.e.\n> addition/deletion) in a file. As we want to keep the current behaviour,\n> add a test to ensure it.\n> [...]\n> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> index c0a60f3158..059ddd3431 100644\n> --- a/Documentation/gitdiffcore.txt\n> +++ b/Documentation/gitdiffcore.txt\n> @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n> regular expression. This means that it will detect in-file (or what\n> rename-detection considers the same file) moves, which is noise. The\n> implementation runs diff twice and greps, and this can be quite\n> -expensive.\n> +expensive. Binary files without textconv filter are ignored.\n\nNow that we support --text that should be documented. I tried to come up\nwith something on top:\n\n diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n index 0378cd574e..42ae65fb57 100644\n --- a/Documentation/diff-options.txt\n +++ b/Documentation/diff-options.txt\n @@ -524,6 +524,10 @@ struct), and want to know the history of that block since it first\n came into being: use the feature iteratively to feed the interesting\n block in the preimage back into `-S`, and keep going until you get the\n very first version of the block.\n ++\n +Unlike `-G` the `-S` option will always search through binary files\n +without a textconv filter. [[TODO: Don't we want to support --no-text\n +then as an optimization?]].\n\n -G::\n \tLook for differences whose patch text contains added/removed\n @@ -545,6 +549,15 @@ occurrences of that string did not change).\n +\n See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more\n information.\n ++\n +Unless `--text` is supplied binary files without a textconv filter\n +will be ignored. This was not the case before Git version 2.21..\n ++\n +With `--text`, instead of patch lines we ::\n \tLook for differences that change the number of occurrences of\n diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n index c0a60f3158..26880b4149 100644\n --- a/Documentation/gitdiffcore.txt\n +++ b/Documentation/gitdiffcore.txt\n @@ -251,6 +251,10 @@ criterion in a changeset, the entire changeset is kept. This behavior\n is designed to make reviewing changes in the context of the whole\n changeset easier.\n\n +Both `-S' and `-G' will ignore binary files without a textconv filter\n +by default, this can be overriden with `--text`. With `--text` the\n +binary patch we look through is generated as [[TODO: ???]].\n +\n diffcore-order: For Sorting the Output Based on Filenames\n ---------------------------------------------------------\n\nBut as you can see given the TODO comments I don't know how this works\nexactly. I *could* dig, but that's my main outstanding problem with this\npatch, the commit message / docs aren't being updated to reflect the new\nbehavior.\n\nI.e. let's leave the docs in some state where the reader can as\nunambiguously know what to expect with -G and these binary diffs we've\nbeen implicitly supporting as with the textual diffs. Ideally with some\nexamples of how to generate them (re my question about the base85 output\nin v1).\n\nPart of that's obviously behavior we've had all along, but it's much\nmore convincing to say:\n\n We are changing X which we've done for ages, it works exactly like\n this, and here's a switch to get it back.\n\nInstead of:\n\n X doesn't make sense, let's turn it off.\n\nAlso the diffcore docs already say stuff about how slow/fast things are,\nand in a side-thread you said:\n\n My main motiviation is to speed up \"log -G\" as that takes a\n considerable amount of time when it wades through MBs of binary\n files which change often.\n\nMakes sense, but then let's say something about that in that section of\nthe docs.\n\n> When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n> that match their respective criterion are kept in the output. When\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 69fc55ea1e..4cea086f80 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -154,6 +154,12 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n> \t\treturn 0;\n>\n> +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> +\t !o->flags.text &&\n> +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> +\t\treturn 0;\n> +\n> \tmf1.size = fill_textconv(o->repo, textconv_one, p->one, &mf1.ptr);\n> \tmf2.size = fill_textconv(o->repo, textconv_two, p->two, &mf2.ptr);\n>\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..5c3e2a16b2 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> \trm .gitattributes\n> '\n>\n> +test_expect_success 'log -G ignores binary files' '\n> +\tgit checkout --orphan orphan1 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -Ga >result &&\n> +\ttest_must_be_empty result\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with -a' '\n> +\tgit checkout --orphan orphan2 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -a -Ga >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n\nA large part of the question(s) I have above & future readers would\npresumably have would be answered by these tests using more realistic\ntest data. I.e. also with \\n in there to see whether -G is also\nline-based in this binary case.\n\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n> +\tgit checkout --orphan orphan3 &&\n> +\techo \"* diff=bin\" > .gitattributes &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> +test_expect_success 'log -S looks into binary files' '\n> +\tgit checkout --orphan orphan4 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -Sa >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> test_done\n\nThese tests have way to much repeated boilerplate for no reason. This\ncould just be (as-is, without the better test data suggested above):\n\ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex 844df760f7..23ed6cc4b1 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -106,4 +106,34 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n \trm .gitattributes\n '\n\n+test_expect_success 'setup log -[GS] binary & --text' '\n+\tgit checkout --orphan GS-binary-and-text &&\n+\tprintf \"a\\0a\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"message\" &&\n+\tgit log >full-log\n+'\n+\n+test_expect_success 'log -G ignores binary files' '\n+\tgit log -Ga >result &&\n+\ttest_must_be_empty result\n+'\n+\n+test_expect_success 'log -G looks into binary files with -a' '\n+\tgit log -a -Ga >actual &&\n+\ttest_cmp actual full-log\n+'\n+\n+test_expect_success 'log -G looks into binary files with textconv filter' '\n+\techo \"* diff=bin\" >.gitattributes &&\n+\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n+\ttest_cmp actual full-log\n+'\n+\n+test_expect_success 'log -S looks into binary files' '\n+\t>.gitattributes &&\n+\tgit log -Sa >actual &&\n+\ttest_cmp actual full-log\n+'\n+\n test_done\n"},{"id":"364294","messageId":"xmqqa7lsnyu5.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"c4eac0b0ff0812e5aa8b081e603fc8bdd042ddeb.1543403143.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-29T07:10:26Z","receivedAt":"2018-11-29T07:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Braun writes:\n\n> Subject: Re: [PATCH v2] log -G: Ignore binary files\n\ns/Ig/ig/; (will locally munge--this alone is no reason to reroll).\n\nThe code changes looked sensible.\n\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..5c3e2a16b2 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> \trm .gitattributes\n> '\n> \n> +test_expect_success 'log -G ignores binary files' '\n> +\tgit checkout --orphan orphan1 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -Ga >result &&\n> +\ttest_must_be_empty result\n> +'\n\nAs this is the first mention of data.bin, this is adding a new file\ndata.bin that has two 'a' but is a binary file. And that is the\nonly commit in the history leading to orphan1.\n\nThe fact that \"log -Ga\" won't find any means it missed the creation\nevent, because the blob is binary. Good.\n\n> +test_expect_success 'log -G looks into binary files with -a' '\n> +\tgit checkout --orphan orphan2 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n\nThis starts from the state left by the previous test piece, i.e. we\nhave a binary data.bin file with two 'a' in it. We pretend to\nmodify and add, but these two steps are no-op if the previous\nsucceeded, but even if the previous step failed, we get what we want\nin the data.bin file. And then we make an initial commit the same\nway.\n\n> +\tgit log -a -Ga >actual &&\n> +\tgit log >expected &&\n\nAnd we ran the same test but this time with \"-a\" to tell Git that\nbinary-ness should not matter. It will find the sole commit. Good.\n\n> +\ttest_cmp actual expected\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n> +\tgit checkout --orphan orphan3 &&\n> +\techo \"* diff=bin\" > .gitattributes &&\n\ns/> />/; (will locally munge--this alone is no reason to reroll).\n\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n\nThis exposes a slight iffy-ness in the design. The textconv filter\nused here does not strip the \"binary-ness\" from the payload, but it\nis enough to tell the machinery that -G should look into the\ndifference. Is that really desirable, though?\n\nIOW, if this weren't the initial commit (which is handled by the\ncodepath to special-case creation and deletion in diff_grep()\nfunction), would \"log -Ga\" show it without \"-a\"? Should it?\n\nI think this test piece (and probably the previous ones for \"-a\" vs\n\"no -a\" without textconv, as well) should be using a history with\nthree commits, where\n\n - the root commit introduces \"a\\0a\" to data.bin (creation event)\n\n - the second commit adds another instance of \"a\\0a\" to data.bin\n (forces comparison)\n\n - the third commit removes data.bin (deletion event)\n\nand make sure that the three are treated identically. If \"log -Ga\"\nfinds one (with the combination of other conditions like use of\ntextconv or -a option), it should find all three, and vice versa.\n\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n> +\n> +test_expect_success 'log -S looks into binary files' '\n> +\tgit checkout --orphan orphan4 &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log -Sa >actual &&\n> +\tgit log >expected &&\n> +\ttest_cmp actual expected\n> +'\n\nLikewise. This would also benefit from a three-commit history.\n\nPerhaps you can create such a history at the beginning of these\nadditions as another \"setup -G/-S binary test\" step and test\ndifferent variations in subsequent tests without the setup?\n\n> test_done\n"},{"id":"364295","messageId":"xmqq5zwgny9d.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"xmqqa7lsnyu5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-29T07:22:54Z","receivedAt":"2018-11-29T07:23:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +test_expect_success 'log -G ignores binary files' '\n>> +\tgit checkout --orphan orphan1 &&\n>> +\tprintf \"a\\0a\" >data.bin &&\n>> +\tgit add data.bin &&\n>> +\tgit commit -m \"message\" &&\n>> +\tgit log -Ga >result &&\n>> +\ttest_must_be_empty result\n>> +'\n>\n> As this is the first mention of data.bin, this is adding a new file\n> data.bin that has two 'a' but is a binary file. And that is the\n> only commit in the history leading to orphan1.\n>\n> The fact that \"log -Ga\" won't find any means it missed the creation\n> event, because the blob is binary. Good.\n\nBy the way, this root commit records another file whose path is\n\"file\" and has \"Picked\" in it. If the file had 'a' in it, it\nwould have been included in \"git log\" output, but that is too subtle\na point to be noticed by the readers who are only reading this patch\nwithout seeing what has been done to the index before this test\npiece.\n\nIf you are going to restructure these tests to create a three-commit\nhistory in a single expect_success that is inspected with various\n\"log -Ga\" invocations in subsequent tests, it is worth removing that\nother file (or rather, starting with \"read-tree --empty\" immediately\nafter checking out the orphan branch, to clarify to the readers that\nthere is nothing but what you add in the set-up step in the index)\nto make the test more robust.\n\n"},{"id":"365362","messageId":"448171361.95154.1544813075679@ox.hosteurope.de","threadId":"49878","inReplyTo":"87a7ltz7jh.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-12-14T18:44:35Z","receivedAt":"2018-12-14T18:44:43Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Ævar Arnfjörð Bjarmason hat am 28. November 2018 um 13:54 geschrieben:\n> \n> \n> \n> On Wed, Nov 28 2018, Thomas Braun wrote:\n> \n> Looks much better this time around.\n\nThanks.\n \n> > The -G option of log looks for the differences whose patch text\n> > contains added/removed lines that match regex.\n> >\n> > As the concept of patch text only makes sense for text files, we need to\n> > ignore binary files when searching with -G as well.\n> >\n> > The -S option of log looks for differences that changes\n> > the number of occurrences of the specified block of text (i.e.\n> > addition/deletion) in a file. As we want to keep the current behaviour,\n> > add a test to ensure it.\n> > [...]\n> > diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> > index c0a60f3158..059ddd3431 100644\n> > --- a/Documentation/gitdiffcore.txt\n> > +++ b/Documentation/gitdiffcore.txt\n> > @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given\n> > regular expression. This means that it will detect in-file (or what\n> > rename-detection considers the same file) moves, which is noise. The\n> > implementation runs diff twice and greps, and this can be quite\n> > -expensive.\n> > +expensive. Binary files without textconv filter are ignored.\n> \n> Now that we support --text that should be documented. I tried to come up\n> with something on top:\n> \n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 0378cd574e..42ae65fb57 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -524,6 +524,10 @@ struct), and want to know the history of that block since it first\n> came into being: use the feature iteratively to feed the interesting\n> block in the preimage back into `-S`, and keep going until you get the\n> very first version of the block.\n> ++\n> +Unlike `-G` the `-S` option will always search through binary files\n> +without a textconv filter. [[TODO: Don't we want to support --no-text\n> +then as an optimization?]].\n> \n> -G::\n> \tLook for differences whose patch text contains added/removed\n> @@ -545,6 +549,15 @@ occurrences of that string did not change).\n> +\n> See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more\n> information.\n> ++\n> +Unless `--text` is supplied binary files without a textconv filter\n> +will be ignored. This was not the case before Git version 2.21..\n> ++\n> +With `--text`, instead of patch lines we +above diff showing what we actually do for binary files. [[TODO: How\n> +does that work?. Could just link to the \"diffcore-pickaxe: For\n> +Detecting Addition/Deletion of Specified String\" section in\n> +gitdiffcore(7) which could explain it]]\n> \n> --find-object=::\n> \tLook for differences that change the number of occurrences of\n> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\n> index c0a60f3158..26880b4149 100644\n> --- a/Documentation/gitdiffcore.txt\n> +++ b/Documentation/gitdiffcore.txt\n> @@ -251,6 +251,10 @@ criterion in a changeset, the entire changeset is kept. This behavior\n> is designed to make reviewing changes in the context of the whole\n> changeset easier.\n> \n> +Both `-S' and `-G' will ignore binary files without a textconv filter\n> +by default, this can be overriden with `--text`. With `--text` the\n> +binary patch we look through is generated as [[TODO: ???]].\n> +\n> diffcore-order: For Sorting the Output Based on Filenames\n> ---------------------------------------------------------\n> \n> But as you can see given the TODO comments I don't know how this works\n> exactly. I *could* dig, but that's my main outstanding problem with this\n> patch, the commit message / docs aren't being updated to reflect the new\n> behavior.\n\nv3 will have some more documentation which took inspiration by your sketches here.\nI've not included a reference to the git version 2.21 in which that patch will hopefully\nland as that seems to be not common in the documentation.\n\nI see tweaking the behaviour of -S outside of this patch series.\n \n> I.e. let's leave the docs in some state where the reader can as\n> unambiguously know what to expect with -G and these binary diffs we've\n> been implicitly supporting as with the textual diffs. Ideally with some\n> examples of how to generate them (re my question about the base85 output\n> in v1).\n> \n> Part of that's obviously behavior we've had all along, but it's much\n> more convincing to say:\n> \n> We are changing X which we've done for ages, it works exactly like\n> this, and here's a switch to get it back.\n> \n> Instead of:\n> \n> X doesn't make sense, let's turn it off.\n> \n> Also the diffcore docs already say stuff about how slow/fast things are,\n> and in a side-thread you said:\n> \n> My main motiviation is to speed up \"log -G\" as that takes a\n> considerable amount of time when it wades through MBs of binary\n> files which change often.\n> \n> Makes sense, but then let's say something about that in that section of\n> the docs.\n\nDone.\n\n> > When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n> > that match their respective criterion are kept in the output. When\n> > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> > index 69fc55ea1e..4cea086f80 100644\n> > --- a/diffcore-pickaxe.c\n> > +++ b/diffcore-pickaxe.c\n> > @@ -154,6 +154,12 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n> > \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n> > \t\treturn 0;\n> >\n> > +\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n> > +\t !o->flags.text &&\n> > +\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n> > +\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n> > +\t\treturn 0;\n> > +\n> > \tmf1.size = fill_textconv(o->repo, textconv_one, p->one, &mf1.ptr);\n> > \tmf2.size = fill_textconv(o->repo, textconv_two, p->two, &mf2.ptr);\n> >\n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 844df760f7..5c3e2a16b2 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> > \trm .gitattributes\n> > '\n> >\n> > +test_expect_success 'log -G ignores binary files' '\n> > +\tgit checkout --orphan orphan1 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -Ga >result &&\n> > +\ttest_must_be_empty result\n> > +'\n> > +\n> > +test_expect_success 'log -G looks into binary files with -a' '\n> > +\tgit checkout --orphan orphan2 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -a -Ga >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> \n> A large part of the question(s) I have above & future readers would\n> presumably have would be answered by these tests using more realistic\n> test data. I.e. also with \\n in there to see whether -G is also\n> line-based in this binary case.\n> \n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\tgit checkout --orphan orphan3 &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > +test_expect_success 'log -S looks into binary files' '\n> > +\tgit checkout --orphan orphan4 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -Sa >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > test_done\n\nDone.\n\n> These tests have way to much repeated boilerplate for no reason. This\n> could just be (as-is, without the better test data suggested above):\n> \n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index 844df760f7..23ed6cc4b1 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -106,4 +106,34 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> \trm .gitattributes\n> '\n> \n> +test_expect_success 'setup log -[GS] binary & --text' '\n> +\tgit checkout --orphan GS-binary-and-text &&\n> +\tprintf \"a\\0a\" >data.bin &&\n> +\tgit add data.bin &&\n> +\tgit commit -m \"message\" &&\n> +\tgit log >full-log\n> +'\n> +\n> +test_expect_success 'log -G ignores binary files' '\n> +\tgit log -Ga >result &&\n> +\ttest_must_be_empty result\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with -a' '\n> +\tgit log -a -Ga >actual &&\n> +\ttest_cmp actual full-log\n> +'\n> +\n> +test_expect_success 'log -G looks into binary files with textconv filter' '\n> +\techo \"* diff=bin\" >.gitattributes &&\n> +\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n> +\ttest_cmp actual full-log\n> +'\n> +\n> +test_expect_success 'log -S looks into binary files' '\n> +\t>.gitattributes &&\n> +\tgit log -Sa >actual &&\n> +\ttest_cmp actual full-log\n> +'\n> +\n> test_done\n\nThanks for pointer. This is resolved in v3 as well. I'm not used to test cases which\ndepend on each other but your are totally right.\n\nThanks for the review.\n"},{"id":"365364","messageId":"894759854.95158.1544813134339@ox.hosteurope.de","threadId":"49878","inReplyTo":"xmqqa7lsnyu5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-12-14T18:45:34Z","receivedAt":"2018-12-14T18:45:39Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Junio C Hamano hat am 29. November 2018 um 08:10 geschrieben:\n> \n> \n> Thomas Braun writes:\n> \n> > Subject: Re: [PATCH v2] log -G: Ignore binary files\n> \n> s/Ig/ig/; (will locally munge--this alone is no reason to reroll).\n\nDone.\n \n> The code changes looked sensible.\n\nThanks.\n\n> > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> > index 844df760f7..5c3e2a16b2 100755\n> > --- a/t/t4209-log-pickaxe.sh\n> > +++ b/t/t4209-log-pickaxe.sh\n> > @@ -106,4 +106,44 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n> > \trm .gitattributes\n> > '\n> > \n> > +test_expect_success 'log -G ignores binary files' '\n> > +\tgit checkout --orphan orphan1 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -Ga >result &&\n> > +\ttest_must_be_empty result\n> > +'\n> \n> As this is the first mention of data.bin, this is adding a new file\n> data.bin that has two 'a' but is a binary file. And that is the\n> only commit in the history leading to orphan1.\n> \n> The fact that \"log -Ga\" won't find any means it missed the creation\n> event, because the blob is binary. Good.\n> \n> > +test_expect_success 'log -G looks into binary files with -a' '\n> > +\tgit checkout --orphan orphan2 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> \n> This starts from the state left by the previous test piece, i.e. we\n> have a binary data.bin file with two 'a' in it. We pretend to\n> modify and add, but these two steps are no-op if the previous\n> succeeded, but even if the previous step failed, we get what we want\n> in the data.bin file. And then we make an initial commit the same\n> way.\n> \n> > +\tgit log -a -Ga >actual &&\n> > +\tgit log >expected &&\n> \n> And we ran the same test but this time with \"-a\" to tell Git that\n> binary-ness should not matter. It will find the sole commit. Good.\n> \n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > +test_expect_success 'log -G looks into binary files with textconv filter' '\n> > +\tgit checkout --orphan orphan3 &&\n> > +\techo \"* diff=bin\" > .gitattributes &&\n> \n> s/> />/; (will locally munge--this alone is no reason to reroll).\n\nDone.\n\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit -c diff.bin.textconv=cat log -Ga >actual &&\n> \n> This exposes a slight iffy-ness in the design. The textconv filter\n> used here does not strip the \"binary-ness\" from the payload, but it\n> is enough to tell the machinery that -G should look into the\n> difference. Is that really desirable, though?\n> \n> IOW, if this weren't the initial commit (which is handled by the\n> codepath to special-case creation and deletion in diff_grep()\n> function), would \"log -Ga\" show it without \"-a\"? Should it?\n\nYes \"log -Ga\" will find all three commits (creation, modification, deletion)\nwhich are present in v3 without \"-a\" and cat as textconv filter.\n\nI can make that more explicit with a textconv filter which removes the binary-ness\n\ngit -c diff.bin.textconv=\"sed -e \\\"s/\\x00//g\\\"\" log -Ga >log &&\n\n(diff.bin.textconv=\"cat -v\" works here as well but seems non-portable)\n\nNow we could also search for \"aa\" as the NUL separating them is gone but that could\nbe getting too clever or?\n\n> I think this test piece (and probably the previous ones for \"-a\" vs\n> \"no -a\" without textconv, as well) should be using a history with\n> three commits, where\n> \n> - the root commit introduces \"a\\0a\" to data.bin (creation event)\n> \n> - the second commit adds another instance of \"a\\0a\" to data.bin\n> (forces comparison)\n> \n> - the third commit removes data.bin (deletion event)\n> \n> and make sure that the three are treated identically. If \"log -Ga\"\n> finds one (with the combination of other conditions like use of\n> textconv or -a option), it should find all three, and vice versa.\n\nGood point. I've added that.\n\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> > +\n> > +test_expect_success 'log -S looks into binary files' '\n> > +\tgit checkout --orphan orphan4 &&\n> > +\tprintf \"a\\0a\" >data.bin &&\n> > +\tgit add data.bin &&\n> > +\tgit commit -m \"message\" &&\n> > +\tgit log -Sa >actual &&\n> > +\tgit log >expected &&\n> > +\ttest_cmp actual expected\n> > +'\n> \n> Likewise. This would also benefit from a three-commit history.\n> \n> Perhaps you can create such a history at the beginning of these\n> additions as another \"setup -G/-S binary test\" step and test\n> different variations in subsequent tests without the setup?\n\nDone.\n"},{"id":"365365","messageId":"1758521848.95160.1544813154194@ox.hosteurope.de","threadId":"49878","inReplyTo":"xmqq5zwgny9d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] log -G: Ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-12-14T18:45:54Z","receivedAt":"2018-12-14T18:45:59Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"> Junio C Hamano hat am 29. November 2018 um 08:22 geschrieben:\n> \n> \n> Junio C Hamano writes:\n> \n> >> +test_expect_success 'log -G ignores binary files' '\n> >> +\tgit checkout --orphan orphan1 &&\n> >> +\tprintf \"a\\0a\" >data.bin &&\n> >> +\tgit add data.bin &&\n> >> +\tgit commit -m \"message\" &&\n> >> +\tgit log -Ga >result &&\n> >> +\ttest_must_be_empty result\n> >> +'\n> >\n> > As this is the first mention of data.bin, this is adding a new file\n> > data.bin that has two 'a' but is a binary file. And that is the\n> > only commit in the history leading to orphan1.\n> >\n> > The fact that \"log -Ga\" won't find any means it missed the creation\n> > event, because the blob is binary. Good.\n> \n> By the way, this root commit records another file whose path is\n> \"file\" and has \"Picked\" in it. If the file had 'a' in it, it\n> would have been included in \"git log\" output, but that is too subtle\n> a point to be noticed by the readers who are only reading this patch\n> without seeing what has been done to the index before this test\n> piece.\n> \n> If you are going to restructure these tests to create a three-commit\n> history in a single expect_success that is inspected with various\n> \"log -Ga\" invocations in subsequent tests, it is worth removing that\n> other file (or rather, starting with \"read-tree --empty\" immediately\n> after checking out the orphan branch, to clarify to the readers that\n> there is nothing but what you add in the set-up step in the index)\n> to make the test more robust.\n\nThanks for the explanation. First I though that \"checkout --orphan\"\nalready takes care of everything but \"read-tree --empty\" is the way to go.\n\nDone.\n"},{"id":"365368","messageId":"f7cb34c6268f556772fa4ee374e2541ba518e2b8.1544811828.git.thomas.braun@virtuell-zuhause.de","threadId":"49878","inReplyTo":"1535679074.141165.1542834055343@ox.hosteurope.de","subject":"[PATCH v3] log -G: ignore binary files","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-12-14T18:49:12Z","receivedAt":"2018-12-14T18:49:36Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"The -G option of log looks for the differences whose patch text\ncontains added/removed lines that match regex.\n\nCurrently -G looks also into patches of binary files (which\naccording to [1]) is binary as well.\n\nThis has a couple of issues:\n\n- It makes the pickaxe search slow. In a proprietary repository of the\n author with only ~5500 commits and a total .git size of ~300MB\n searching takes ~13 seconds\n\n $time git log -Gwave > /dev/null\n\n real 0m13,241s\n user 0m12,596s\n sys 0m0,644s\n\n whereas when we ignore binary files with this patch it takes ~4s\n\n $time ~/devel/git/git log -Gwave > /dev/null\n\n real 0m3,713s\n user 0m3,608s\n sys 0m0,105s\n\n which is a speedup of more than fourfold.\n\n- The internally used algorithm for generating patch text is based on\n xdiff and its states in [1]\n\n > The output format of the binary patch file is proprietary\n > (and binary) and it is basically a collection of copy and insert\n > commands [..]\n\n which means that the current format could change once the internal\n algorithm is changed as the format is not standardized. In addition\n the git binary patch format used for preparing patches for git apply\n is *different* from the xdiff format as can be seen by comparing\n\n git log -p -a\n\n commit 6e95bf4bafccf14650d02ab57f3affe669be10cf\n Author: A U Thor \n Date: Thu Apr 7 15:14:13 2005 -0700\n\n modify binary file\n\n diff --git a/data.bin b/data.bin\n index f414c84..edfeb6f 100644\n --- a/data.bin\n +++ b/data.bin\n @@ -1,2 +1,4 @@\n a\n a^@a\n +a\n +a^@a\n\n with git log --binary\n\n commit 6e95bf4bafccf14650d02ab57f3affe669be10cf\n Author: A U Thor \n Date: Thu Apr 7 15:14:13 2005 -0700\n\n modify binary file\n\n diff --git a/data.bin b/data.bin\n index f414c84bd3aa25fa07836bb1fb73db784635e24b..edfeb6f501[..]\n GIT binary patch\n literal 12\n QcmYe~N@Pgn0zx1O01)N^ZvX%Q\n\n literal 6\n NcmYe~N@Pgn0ssWg0XP5v\n\n which seems unexpected.\n\nTo resolve these issues this patch makes -G ignore binary files\nby default. Textconv filters are supported and also -a/--text for\ngetting the old and broken behaviour back.\n\nThe -S option of log looks for differences that changes\nthe number of occurrences of the specified block of text (i.e.\naddition/deletion) in a file. As we want to keep the current behaviour,\nadd a test to ensure it stays that way.\n\n[1]: http://www.xmailserver.org/xdiff.html\n\nSigned-off-by: Thomas Braun \n---\n\nChanges since v2:\n - Introduce a setup step for the new tests \n - Really start with a clean history in the tests\n - Added more complex commit history for the tests\n - Use test_when_finished for cleanup instead of doing nothing\n - Enhanced commit message to motivate the change better\n - Added some more documentation\n\n Documentation/diff-options.txt | 5 +++++\n Documentation/gitdiffcore.txt | 3 ++-\n diffcore-pickaxe.c | 6 ++++++\n t/t4209-log-pickaxe.sh | 35 ++++++++++++++++++++++++++++++++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 0378cd574e..b94d332f71 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -524,6 +524,8 @@ struct), and want to know the history of that block since it first\n came into being: use the feature iteratively to feed the interesting\n block in the preimage back into `-S`, and keep going until you get the\n very first version of the block.\n++\n+Binary files are searched as well.\n \n -G::\n \tLook for differences whose patch text contains added/removed\n@@ -543,6 +545,9 @@ While `git log -G\"regexec\\(regexp\"` will show this commit, `git log\n -S\"regexec\\(regexp\" --pickaxe-regex` will not (because the number of\n occurrences of that string did not change).\n +\n+Unless `--text` is supplied patches of binary files without a textconv\n+filter will be ignored.\n++\n See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more\n information.\n \ndiff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\nindex c0a60f3158..c970d9fe43 100644\n--- a/Documentation/gitdiffcore.txt\n+++ b/Documentation/gitdiffcore.txt\n@@ -242,7 +242,8 @@ textual diff has an added or a deleted line that matches the given\n regular expression. This means that it will detect in-file (or what\n rename-detection considers the same file) moves, which is noise. The\n implementation runs diff twice and greps, and this can be quite\n-expensive.\n+expensive. To speed things up binary files without textconv filters\n+will be ignored.\n \n When `-S` or `-G` are used without `--pickaxe-all`, only filepairs\n that match their respective criterion are kept in the output. When\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 69fc55ea1e..4cea086f80 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -154,6 +154,12 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \tif (textconv_one == textconv_two && diff_unmodified_pair(p))\n \t\treturn 0;\n \n+\tif ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&\n+\t !o->flags.text &&\n+\t ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||\n+\t (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))\n+\t\treturn 0;\n+\n \tmf1.size = fill_textconv(o->repo, textconv_one, p->one, &mf1.ptr);\n \tmf2.size = fill_textconv(o->repo, textconv_two, p->two, &mf2.ptr);\n \ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex 844df760f7..5d06f5f45e 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -106,4 +106,39 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '\n \trm .gitattributes\n '\n \n+test_expect_success 'setup log -[GS] binary & --text' '\n+\tgit checkout --orphan GS-binary-and-text &&\n+\tgit read-tree --empty &&\n+\tprintf \"a\\na\\0a\\n\" >data.bin &&\n+\tgit add data.bin &&\n+\tgit commit -m \"create binary file\" data.bin &&\n+\tprintf \"a\\na\\0a\\n\" >>data.bin &&\n+\tgit commit -m \"modify binary file\" data.bin &&\n+\tgit rm data.bin &&\n+\tgit commit -m \"delete binary file\" data.bin &&\n+\tgit log >full-log\n+'\n+\n+test_expect_success 'log -G ignores binary files' '\n+\tgit log -Ga >log &&\n+\ttest_must_be_empty log\n+'\n+\n+test_expect_success 'log -G looks into binary files with -a' '\n+\tgit log -a -Ga >log &&\n+\ttest_cmp log full-log\n+'\n+\n+test_expect_success 'log -G looks into binary files with textconv filter' '\n+\ttest_when_finished \"rm .gitattributes\" &&\n+\techo \"* diff=bin\" >.gitattributes &&\n+\tgit -c diff.bin.textconv=cat log -Ga >log &&\n+\ttest_cmp log full-log\n+'\n+\n+test_expect_success 'log -S looks into binary files' '\n+\tgit log -Sa >log &&\n+\ttest_cmp log full-log\n+'\n+\n test_done\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365924","messageId":"xmqqlg49mmus.fsf@gitster-ct.c.googlers.com","threadId":"49878","inReplyTo":"f7cb34c6268f556772fa4ee374e2541ba518e2b8.1544811828.git.thomas.braun@virtuell-zuhause.de","subject":"Re: [PATCH v3] log -G: ignore binary files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T23:24:05Z","receivedAt":"2018-12-28T20:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Braun writes:\n\n> - The internally used algorithm for generating patch text is based on\n> xdiff and its states in [1]\n>\n> > The output format of the binary patch file is proprietary\n> > (and binary) and it is basically a collection of copy and insert\n> > commands [..]\n>\n> which means that the current format could change once the internal\n> algorithm is changed as the format is not standardized. In addition\n> the git binary patch format used for preparing patches for git apply\n> is *different* from the xdiff format as can be seen by comparing\n\nThis particular argument sounds like a red herring. After all, when\nthe --text option is given\n\n>\n> git log -p -a\n>\n> commit 6e95bf4bafccf14650d02ab57f3affe669be10cf\n> Author: A U Thor \n> Date: Thu Apr 7 15:14:13 2005 -0700\n>\n> modify binary file\n>\n> diff --git a/data.bin b/data.bin\n> index f414c84..edfeb6f 100644\n> --- a/data.bin\n> +++ b/data.bin\n> @@ -1,2 +1,4 @@\n> a\n> a^@a\n> +a\n> +a^@a\n\nwe will see 'a' in the output no matter how xdiff internally works;\nthere is no way to express the above change textually without\nshowing \"+a\" somewhere in the patch output.\n\nThe rest of the log message looks good, and ...\n\n> Changes since v2:\n> - Introduce a setup step for the new tests \n> - Really start with a clean history in the tests\n> - Added more complex commit history for the tests\n> - Use test_when_finished for cleanup instead of doing nothing\n> - Enhanced commit message to motivate the change better\n> - Added some more documentation\n\n... the tests are certainly a lot easier to follow.\n\nThanks.\n"}]}