On Thu, Sep 26, 2019 at 02:31:56PM -0700, Emily Shaffer wrote:
> ---
> 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@xxxxxxxxxx>
> Helped-by: Jeff King <peff@xxxxxxxx>
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. :)
> 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.
> --- 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
