| Message ID | 54fdee845bea7f67f46817417f8e5a504bd39665.1644565025.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: improve atomicity of `--atomic` flag | expand |
On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt <ps@pks.im> 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. Probably stupid question: are we sure that it's a bug and not a feature? > 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). Even if it would have been the right behavior when backfilling tags was implemented to return an error when backfilling tags fails, I think it's interesting to ask ourselves if this change could be seen as a regression by some users.
Patrick Steinhardt <ps@pks.im> writes: > @@ -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. > + */ > + if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees)) > + retcode = 1; I think s/succeeds/fails/ Having said that, I don't think that we need to describe the past here. Notifying the user if something has failed is expected and doesn't require an explanation about how we didn't do that in the past. Everything up to this patch looks good to me.
On Fri, Feb 11, 2022 at 12:03 PM Patrick Steinhardt <ps@pks.im> 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@pks.im> > --- > 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 >
On Tue, Feb 15, 2022 at 08:52:14AM +0100, Christian Couder wrote: > On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt <ps@pks.im> 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. > > Probably stupid question: are we sure that it's a bug and not a feature? Good question, and I don't have a definitive answer for it. But to me it very much smells like a bug: if I ask for a fetch and the fetch fails to populate some of the data I have asked for, then I want to get a notification on that failure. > > 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). > > Even if it would have been the right behavior when backfilling tags > was implemented to return an error when backfilling tags fails, I > think it's interesting to ask ourselves if this change could be seen > as a regression by some users. Yeah, it's not all that clear-cut because auto-following of tags is a bit obscure. But our default behaviour is to fetch tags pointing into the history, and if a user didn't want that they should've passed `--no-tags` to git-fetch(1). So conversely, we should assume that a user is asking for auto-filling of tags if we're not told otherwise, which also means that it is a failure if this fails. At least that's my take, but I'm happy to hear arguments against my viewpoint. Patrick
On Thu, Feb 17, 2022 at 12:27:15PM +0100, Patrick Steinhardt wrote: > On Tue, Feb 15, 2022 at 08:52:14AM +0100, Christian Couder wrote: > > On Fri, Feb 11, 2022 at 9:03 PM Patrick Steinhardt <ps@pks.im> 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. > > > > Probably stupid question: are we sure that it's a bug and not a feature? > > Good question, and I don't have a definitive answer for it. But to me it > very much smells like a bug: if I ask for a fetch and the fetch fails to > populate some of the data I have asked for, then I want to get a > notification on that failure. > > > > 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). > > > > Even if it would have been the right behavior when backfilling tags > > was implemented to return an error when backfilling tags fails, I > > think it's interesting to ask ourselves if this change could be seen > > as a regression by some users. > > Yeah, it's not all that clear-cut because auto-following of tags is a > bit obscure. But our default behaviour is to fetch tags pointing into > the history, and if a user didn't want that they should've passed > `--no-tags` to git-fetch(1). So conversely, we should assume that a user > is asking for auto-filling of tags if we're not told otherwise, which > also means that it is a failure if this fails. > > At least that's my take, but I'm happy to hear arguments against my > viewpoint. > > Patrick I just noticed that we have in fact landed a change in the exact same spirit on `main` via c9e04d905e (fetch --prune: exit with error if pruning fails, 2022-01-31). So there is precedent that we fix up these missing error codes, and that gives me more confidence that doing the same fixup for the tag-backfill is the correct thing to do. Patrick
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. + */ + 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
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@pks.im> --- builtin/fetch.c | 29 +++++++++++++++++++++-------- t/t5503-tagfollow.sh | 4 +--- 2 files changed, 22 insertions(+), 11 deletions(-)