| Message ID | 20220131133047.1885074-1-t.gummerer@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c9e04d905edb5487c43b03304704e8d1248f9ac0 |
| Headers | show |
| Series | [v2] fetch --prune: exit with error if pruning fails | expand |
Thomas Gummerer <t.gummerer@gmail.com> writes: > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 5f06b21f8e..648f0694f2 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1609,12 +1609,14 @@ static int do_fetch(struct transport *transport, > * don't care whether --tags was specified. > */ > if (rs->nr) { > - prune_refs(rs, ref_map, transport->url); > + retcode = prune_refs(rs, ref_map, transport->url); > } else { > - prune_refs(&transport->remote->fetch, > - ref_map, > - transport->url); > + retcode = prune_refs(&transport->remote->fetch, > + ref_map, > + transport->url); > } > + if (retcode != 0) > + retcode = 1; > } Looks trivially correct, even though a few style things look a bit irritating to my eyes ;-). Thanks, will queue.
diff --git a/builtin/fetch.c b/builtin/fetch.c index 5f06b21f8e..648f0694f2 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1609,12 +1609,14 @@ static int do_fetch(struct transport *transport, * don't care whether --tags was specified. */ if (rs->nr) { - prune_refs(rs, ref_map, transport->url); + retcode = prune_refs(rs, ref_map, transport->url); } else { - prune_refs(&transport->remote->fetch, - ref_map, - transport->url); + retcode = prune_refs(&transport->remote->fetch, + ref_map, + transport->url); } + if (retcode != 0) + retcode = 1; } if (fetch_and_consume_refs(transport, ref_map, worktrees)) { free_refs(ref_map); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 20f7110ec1..ef0da0a63b 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -164,6 +164,17 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' git rev-parse sometag ' +test_expect_success REFFILES 'fetch --prune fails to delete branches' ' + cd "$D" && + git clone . prune-fail && + cd prune-fail && + git update-ref refs/remotes/origin/extrabranch main && + : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds && + >.git/packed-refs.new && + + test_must_fail git fetch --prune origin +' + test_expect_success 'fetch --atomic works with a single branch' ' test_when_finished "rm -rf \"$D\"/atomic" &&
When pruning refs fails, we currently print an error to stderr, but still exit 0 from 'git fetch'. Since this is a genuine error fetch should be exiting with some non-zero exit code. Make it so. The --prune option was introduced in f360d844de ("builtin-fetch: add --prune option", 2009-11-10). Unfortunately it's unclear from that commit whether ignoring the exit code was an oversight or intentional, but it feels like an oversight. Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- changes in v2: - retcode will now always be 1 if we get an error from prune_refs, no matter what the original error code was. - we will now continue the fetch, and only exit with a non-zero exit code after having finished the rest of the fetch operation - added a comment in the test explaining why creating a packed-refs.new file will make the fetch --prune fail. builtin/fetch.c | 10 ++++++---- t/t5510-fetch.sh | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-)