On Fri, Feb 11, 2022 at 12:03 PM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> When the backfilling of tags fails we do not report this error to the
> caller, but only report it implicitly at a later point when reporting
> updated references. This leaves callers unable to act upon the
> information of whether the backfilling succeeded or not.
>
> Refactor the function to return an error code and pass it up the
> callstack. This causes us to correctly propagate the error back to the
> user of git-fetch(1).
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/fetch.c | 29 +++++++++++++++++++++--------
> t/t5503-tagfollow.sh | 4 +---
> 2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 627847e2f8..1eda0b68ff 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1495,12 +1495,12 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
> return transport;
> }
>
> -static void backfill_tags(struct transport *transport,
> - struct ref *ref_map,
> - struct fetch_head *fetch_head,
> - struct worktree **worktrees)
> +static int backfill_tags(struct transport *transport,
> + struct ref *ref_map,
> + struct fetch_head *fetch_head,
> + struct worktree **worktrees)
> {
> - int cannot_reuse;
> + int retcode, cannot_reuse;
>
> /*
> * Once we have set TRANS_OPT_DEEPEN_SINCE, we can't unset it
> @@ -1519,12 +1519,14 @@ static void backfill_tags(struct transport *transport,
> transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
> transport_set_option(transport, TRANS_OPT_DEPTH, "0");
> transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
> - fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
> + retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
>
> if (gsecondary) {
> transport_disconnect(gsecondary);
> gsecondary = NULL;
> }
> +
> + return retcode;
> }
>
> static int do_fetch(struct transport *transport,
> @@ -1628,8 +1630,19 @@ static int do_fetch(struct transport *transport,
> struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>
> find_non_local_tags(remote_refs, &tags_ref_map, &tail);
> - if (tags_ref_map)
> - backfill_tags(transport, tags_ref_map, &fetch_head, worktrees);
> + if (tags_ref_map) {
> + /*
> + * If backfilling tags succeeds we used to not return
> + * an error code to the user at all. Instead, we
> + * silently swallowed that error and updated the local
> + * state of the repository. We now notify the user of
> + * any such errors, but we continue to make sure that
> + * FETCH_HEAD and the upstream branch are configured as
> + * expected.
> + */
nit: I'd prefer to see code comments explain what we currently do, and
move history lessons to the commit message.
> + if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
> + retcode = 1;
> + }
>
> free_refs(tags_ref_map);
> }
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 888305ad4d..549f908b90 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -237,9 +237,7 @@ test_expect_success 'backfill failure causes command to fail' '
> done
> EOF
>
> - # Even though we fail to create refs/tags/tag1 the below command
> - # unexpectedly succeeds.
> - git -C clone5 fetch .. $B:refs/heads/something &&
> + test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
> test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&
> test $S = $(git -C clone5 rev-parse --verify tag2) &&
> test_must_fail git -C clone5 rev-parse --verify tag1
> --
> 2.35.1
>
