Re: [PATCH v2] promisor-remote: skip move_to_tail when no-op

From: Jeff King <peff@peff.net>
Date: Sep 27, 2019, 00:32 UTC
Message-ID: <20190927003223.GA22473@sigill.intra.peff.net>
In-Reply-To: <20190926213156.88185-1-emilyshaffer@google.com>

On Thu, Sep 26, 2019 at 02:31:56PM -0700, Emily Shaffer wrote:

Show 8 quoted lines

> ---
> Thanks Peff for pointing out this bug occurs any time the tail is being
> moved to tail in the promisors list. Modified patch to check just for
> "already at tail" condition.
> 
> Added a test case based on Peff's reproduction steps (thanks!) and
> confirmed it failed before the patch, exiting gracelessly with a
> SIGABRT, and now fails gracefully (and passes test_must_fail condition).

Thanks, this is looking good, though I have a few nits.

> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Helped-by: Jeff King <peff@peff.net>

The trailers are supposed to be chronological. It probably doesn't matter a lot here, but it's more important for something like a signoff chain. I suppose you could also argue that you signed off and then I helped but I think you signed off again after. :)

Show 14 quoted lines

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 9bc296cdde..9bd5b79d59 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -89,6 +89,9 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,
>  static void promisor_remote_move_to_tail(struct promisor_remote *r,
>  					 struct promisor_remote *previous)
>  {
> +	if (r->next == NULL)
> +		return;
> +
>  	if (previous)
>  		previous->next = r->next;
>  	else

Yeah, I think of all the discussed options, this one is pretty easy to understand.

Show 17 quoted lines

> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -429,6 +429,18 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
>  	done
>  '
>  
> +test_expect_success 'single promisor remote can be re-initialized gracefully' '
> +	# ensure one promisor is in the promisors list
> +	rm -rf repo &&
> +	test_create_repo repo &&
> +	git -C repo remote add foo /wherever &&
> +	git -C repo config remote.foo.promisor true &&
> +	git -C repo config extensions.partialclone foo &&
> +
> +	# reinitialize the promisors list; this must fail gracefully
> +	test_must_fail git -C repo fetch --filter=blob:none foo 2>fetch_err
> +'

Could we make this a little more robust by using a real repo instead of "/wherever", and confirming that the command actually succeeds?

As a side note, if we're not going to check the content of fetch_err, I think we're better off not redirecting it (it goes to /dev/null by default, or the user or log if running with "-v" or "--verbose-log").

-Peff