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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2018, 01:29 UTC
Message-ID: <xmqq4lc96gsg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <590f2ca6b5323c17365a1645b5d10e9ab30623c4.1542833244.git.thomas.braun@virtuell-zuhause.de>

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:

Show 12 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(-)

OK.

Show 10 quoted lines

> 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.

OK.

Show 16 quoted lines

> 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

Shouldn't this new test come after the existing optimization, which allows us to leave without loading the blob contents (which is needed once you call diff_filespec_is_binary())?

Show 11 quoted lines

> 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 &&

Please never never ever do the above two unless you are writing a test that checks low-level repository details.

If you want a clean history that has specific lineage of commits without getting affected by commits that have been made by the previous test pieces, it is OK to "checkout --orphan" to create an empty history to work with.

Show 20 quoted lines

> +	printf "a\0b" >data.bin &&
> +	git add data.bin &&
> +	git commit -m "message" &&
> +	git log -G a >result &&
> +	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