Re: [PATCH v1 1/2] log -G: Ignore binary files

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 22, 2018, 10:16 UTC
Message-ID: <87k1l5zabd.fsf@evledraar.gmail.com>
In-Reply-To: <590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de>

On Wed, Nov 21 2018, Thomas Braun wrote:

Show 57 quoted lines

> The -G <regex> option of log looks for the differences whose patch text
> contains added/removed lines that match regex.
>
> The concept of differences only makes sense for text files, therefore
> we need to ignore binary files when searching with -G <regex> as well.
>
> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
>  Documentation/gitdiffcore.txt |  2 +-
>  diffcore-pickaxe.c            |  5 +++++
>  t/t4209-log-pickaxe.sh        | 22 ++++++++++++++++++++++
>  3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index c0a60f3158..059ddd3431 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches the given
>  regular expression.  This means that it will detect in-file (or what
>  rename-detection considers the same file) moves, which is noise.  The
>  implementation runs diff twice and greps, and this can be quite
> -expensive.
> +expensive.  Binary files without textconv filter are ignored.
>
>  When `-S` or `-G` are used without `--pickaxe-all`, only filepairs
>  that match their respective criterion are kept in the output.  When
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 69fc55ea1e..8c2558b07d 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
>  		textconv_two = get_textconv(o->repo->index, p->two);
>  	}
>
> +	if ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&
> +	    ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||
> +	     (!textconv_two && diff_filespec_is_binary(o->repo, p->two))))
> +		return 0;
> +
>  	/*
>  	 * If we have an unmodified pair, we know that the count will be the
>  	 * same and don't even have to load the blobs. Unless textconv is in
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 844df760f7..42cc8afd8b 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing textconv tool)' '
>  	rm .gitattributes
>  '
>
> +test_expect_success 'log -G ignores binary files' '
> +	rm -rf .git &&
> +	git init &&
> +	printf "a\0b" >data.bin &&
> +	git add data.bin &&
> +	git commit -m "message" &&
> +	git log -G a >result &&

Would be less confusing as "-Ga" since that's the invocation we document, even though I see (but wasn't aware that...) "-G a" works too.

Show 16 quoted lines

> +	test_must_be_empty result
> +'
> +
> +test_expect_success 'log -G looks into binary files with textconv filter' '
> +	rm -rf .git &&
> +	git init &&
> +	echo "* diff=bin" > .gitattributes &&
> +	printf "a\0b" >data.bin &&
> +	git add data.bin &&
> +	git commit -m "message" &&
> +	git -c diff.bin.textconv=cat log -G a >actual &&
> +	git log >expected &&
> +	test_cmp actual expected
> +'
> +
>  test_done

This patch seems like the wrong direction to me. In particular the assertion that "the concept of differences only makes sense for text files". That's just not true. This patch breaks this:

    (
        rm -rf /tmp/g-test &&
        git init /tmp/g-test &&
        cd /tmp/g-test &&
        for i in {1..10}; do
            echo "Always matching thensome 5" >file &&
            printf "a thensome %d binary \0" $i >>file &&
            git add file &&
            git commit -m"Bump $i"
        done &&
        git log -Gthensome.*5
    )

Right now this will emit 3/10 patches, and the right ones! I.e. "Bump [156]". The 1st one because it introduces the "Always matching thensome 5". Then 5/6 because the add/remove the string "a thensome 5 binary", respectively. Which matches /thensome.*5/.

I.e. in the first one we do a regex match against the content here because we don't have both sides: https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L48-L53

And then for the later ones where we have both sides we end up in diffgrep_consume(): https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L27-L36

I think there may be a real issue here to address, which might be some combination of:

 a) Even though the diffcore can do a binary diff internally, this is
    not what it exposes with "-p", we just say "Binary files differ".

    I don't know how to emit the raw version we'll end up passing to
    diffgrep_consume() in this case. Is it just --binary without the
    encoding? I don't know...

 b) Your test case shows that you're matching a string at a \0
    boundary. Is this perhaps something you ran into? I.e. that we don't
    have some -F version of -G so we can't supply regexes that match
    past a \0? I had some related work on grep for this that hasn't been
    carried over to the diffcore:

        git log --grep='grep:.*\\0' --author=Ævar

 c) Is this binary diff we end up matching against just bad in some
    cases? I haven't dug but that wouldn't surprise me, i.e. that it's
    trying to be line-based so we'll overmatch in many cases.

So maybe this is something that should be passed down as a flag? See a recent discussion at https://public-inbox.org/git/87lg77cmr1.fsf@evledraar.gmail.com/ for how that could be done.

Also if we don't have some tests already that were failing with this patch we really should have those as "let's test the current behavior first". Unfortunately tests in this area are really lacking, see e.g. my:

    git log --author=Junio --min-parents=2 --grep=ab/.*grep

For some series of patches to grep where to get one patch in I needed to often lead with 5-10 test patches to convince reviewers that I knew what I was changing, and also to be comfortable that I'd covered all the edge cases we currently supported, but weren't testing for.