Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2021, 18:51 UTC
Message-ID: <xmqq7dnn7jjz.fsf@gitster.c.googlers.com>
In-Reply-To: <448e6a64fa157990fcc973ce2fe4a9fc2ba1ab32.1612431093.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines

>  int is_range_diff_range(const char *arg)
>  {
> -	return !!strstr(arg, "..");
> +	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
> +	const char *argv[] = { "", copy, "--", NULL };
> +	int i, positive = 0, negative = 0;
> +	struct rev_info revs;
> +
> +	init_revisions(&revs, NULL);
> +	if (setup_revisions(3, argv, &revs, 0) == 1) {
> +		for (i = 0; i < revs.pending.nr; i++)
> +			if (revs.pending.objects[i].item->flags & UNINTERESTING)
> +				negative++;
> +			else
> +				positive++;
> +	}
> +
> +	free(copy);
> +	object_array_clear(&revs.pending);
> +	return negative > 0 && positive > 0;
>  }

One thing that worries me with this code is that I do not see anybody that clears UNINTERESTING bit in the flags. In-core objects are singletons, so if a user fed the command two ranges,

	git range-diff A..B C..A

and this code first handled "A..B", smudging the in-core instance of the commit object A with UNINTERESTING bit, that in-core instance will be reused when the second range argument "C..A" is given to this function again.

At that point, has anybody cleared the UNINTERESTING bit in the flags word for the in-core commit A? I do not see it done in this function, but perhaps I am missing it done in the init/setup functions (I somehow doubt it, though)?

Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the commits in revs.pending[] array before we clear it? Depending on the shape of "arg" that is end-user supplied, we may have walked the history in handle_dotdot_1() to parse it (e.g. "A...B").

Also we'd want to see what needs to be cleared in revs.cmdline that would have been populated by calls to add_rev_cmdline().

Other than that, I quite like the way the actual code turned out to be.

Show 19 quoted lines

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6eb344be0312..e217cecac9ed 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'A^! and A^-<n> (unmodified)' '
> +	git range-diff --no-color topic^! unmodified^-1 >actual &&
> +	cat >expect <<-EOF &&
> +	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'trivial reordering' '
>  	git range-diff --no-color master topic reordered >actual &&
>  	cat >expect <<-EOF &&