Re: [PATCH 1/1] diffcore: add a filter to find a specific blob

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2017, 15:04 UTC
Message-ID: <xmqq1sk5b6rx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171208002447.20261-2-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 28 quoted lines

> diff --git a/diffcore-blobfind.c b/diffcore-blobfind.c
> new file mode 100644
> index 0000000000..e65c7cad6e
> --- /dev/null
> +++ b/diffcore-blobfind.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2017 Google Inc.
> + */
> +#include "cache.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +
> +static void diffcore_filter_blobs(struct diff_queue_struct *q,
> +				  struct diff_options *options)
> +{
> +	int src, dst;
> +
> +	if (!options->blobfind)
> +		BUG("blobfind oidset not initialized???");
> +
> +	for (src = dst = 0; src < q->nr; src++) {
> +		struct diff_filepair *p = q->queue[src];
> +
> +		if ((DIFF_FILE_VALID(p->one) &&
> +		     oidset_contains(options->blobfind, &p->one->oid)) ||
> +		    (DIFF_FILE_VALID(p->two) &&
> +		     oidset_contains(options->blobfind, &p->two->oid))) {

Shouldn't this make sure that !DIFF_PAIR_UNMERGED(p) before looking at the sides of the pair? I think an unmerged pair is queued with filespecs whose modes are cleared, but we should not be relying on that---I think we even had bugs we fixed by introducing an explicit is_unmerged field to the filepair struct.

Show 8 quoted lines

> @@ -2883,6 +2884,8 @@ int prepare_revision_walk(struct rev_info *revs)
>  		simplify_merges(revs);
>  	if (revs->children.name)
>  		set_children(revs);
> +	if (revs->diffopt.blobfind)
> +		revs->simplify_history = 0;
>  	return 0;
>  }

It makes sense to clear this bit by default, but is this an unconditonal clearing? In other words, is there a way for the user to countermand this default and ask for merge simplification from the command line, e.g. "diff --simplify-history --blobfind=<blob>"?

Show 29 quoted lines

> +test_description='test finding specific blobs in the revision walking'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	git commit --allow-empty -m "empty initial commit" &&
> +
> +	echo "Hello, world!" >greeting &&
> +	git add greeting &&
> +	git commit -m "add the greeting blob" && # borrowed from Git from the Bottom Up
> +	git tag -m "the blob" greeting $(git rev-parse HEAD:greeting) &&
> +
> +	echo asdf >unrelated &&
> +	git add unrelated &&
> +	git commit -m "unrelated history" &&
> +
> +	git revert HEAD^ &&
> +
> +	git commit --allow-empty -m "another unrelated commit"
> +'
> +
> +test_expect_success 'find the greeting blob' '
> +	cat >expect <<-EOF &&
> +	Revert "add the greeting blob"
> +	add the greeting blob
> +	EOF
> +
> +	git log --abbrev=12 --oneline --blobfind=greeting^{blob} >actual.raw &&
> +	cut -c 14- actual.raw >actual &&
> +	test_cmp expect actual

The hardcoded numbers look strange and smell like a workaround of not asking for full object names.

Also, now it has the simplify-history bit in the change, don't we want to check a mergy history, and also running "diff-files" while the index has unmerged entries?

Other than that, the changes look quite sane and pleasnt read.

Thanks.

> +'
> +
> +test_done