{"thread":{"id":"57323","subject":"[PATCH] fetch --prune: exit with error if pruning fails","startedAt":"2022-01-27T15:37:21Z","lastAt":"2022-01-31T19:20:54Z","messageCount":11,"participants":["Thomas Gummerer","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"447088","messageId":"20220127153714.1190894-1-t.gummerer@gmail.com","threadId":"57323","inReplyTo":null,"subject":"[PATCH] fetch --prune: exit with error if pruning fails","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2022-01-27T15:37:14Z","receivedAt":"2022-01-27T15:37:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When pruning refs fails, we currently print an error to stderr, but\nstill exit 0 from 'git fetch'.  Since this is a genuine error fetch\nshould be exiting with some non-zero exit code.  Make it so.\n\nThe --prune option was introduced in f360d844de (\"builtin-fetch: add\n--prune option\", 2009-11-10).  Unfortunately it's unclear from that\ncommit whether ignoring the exit code was an oversight or\nintentional, but it feels like an oversight.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/fetch.c  | 12 ++++++++----\n t/t5510-fetch.sh | 10 ++++++++++\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e..54545efedd 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1609,11 +1609,15 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tprune_refs(rs, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(rs, ref_map, transport->url);\n \t\t} else {\n-\t\t\tprune_refs(&transport->remote->fetch,\n-\t\t\t\t   ref_map,\n-\t\t\t\t   transport->url);\n+\t\t\tretcode = prune_refs(&transport->remote->fetch,\n+\t\t\t\t\t     ref_map,\n+\t\t\t\t\t     transport->url);\n+\t\t}\n+\t\tif (retcode) {\n+\t\t\tfree_refs(ref_map);\n+\t\t\tgoto cleanup;\n \t\t}\n \t}\n \tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 20f7110ec1..df824cc3d0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -164,6 +164,16 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-fail &&\n+\tcd prune-fail &&\n+\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t>.git/packed-refs.new &&\n+\n+\ttest_must_fail git fetch --prune origin\n+'\n+\n test_expect_success 'fetch --atomic works with a single branch' '\n \ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n \n-- \n2.31.1\n\n"},{"id":"447122","messageId":"xmqqmtjh0x5f.fsf@gitster.g","threadId":"57323","inReplyTo":"20220127153714.1190894-1-t.gummerer@gmail.com","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T19:57:32Z","receivedAt":"2022-01-27T19:57:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> When pruning refs fails, we currently print an error to stderr, but\n> still exit 0 from 'git fetch'.  Since this is a genuine error fetch\n> should be exiting with some non-zero exit code.  Make it so.\n>\n> The --prune option was introduced in f360d844de (\"builtin-fetch: add\n> --prune option\", 2009-11-10).  Unfortunately it's unclear from that\n> commit whether ignoring the exit code was an oversight or\n> intentional, but it feels like an oversight.\n\nIt is a good idea to signal, with a non-zero status, that the user\nneeds to inspect the situation and possibly take a corrective\naction.  I however do not know if it is sufficient to just give the\nsame exit status as returned by prune_refs(), which may or may not\nhappen to crash with other possible non-zero exit status to make it\nindistinguishable from other kinds of errors.\n\n>  \t\tif (rs->nr) {\n> -\t\t\tprune_refs(rs, ref_map, transport->url);\n> +\t\t\tretcode = prune_refs(rs, ref_map, transport->url);\n>  \t\t} else {\n> -\t\t\tprune_refs(&transport->remote->fetch,\n> -\t\t\t\t   ref_map,\n> -\t\t\t\t   transport->url);\n> +\t\t\tretcode = prune_refs(&transport->remote->fetch,\n> +\t\t\t\t\t     ref_map,\n> +\t\t\t\t\t     transport->url);\n> +\t\t}\n\nIt seems that it is possible for do_fetch() to return a negative\nvalue, even without this patch.  The return value from prune_refs()\ncomes from refs.c::delete_refs(), which can come from error_errno()\nthat is -1, so this patch adds more of the same problem to existing\nones, though.\n\nAnd the value propagates up to cmd_fetch() via fetch_one().  This\nwill be relayed to exit() without changing via this callchain:\n\n    git.c::cmd_main()\n      git.c::run_argv()\n        git.c::handle_builtin()\n          git.c::run_builtin()\n\nThis is not a new problem, which does not want to be corrected as\npart of this patch, but let's leave a mental note as #leftoverbits\nfor now.\n\n> +\t\tif (retcode) {\n> +\t\t\tfree_refs(ref_map);\n> +\t\t\tgoto cleanup;\n>  \t\t}\n\nThis part is iffy.  We tried to prune refs, we may have removed some\nof the refs missing from the other side but we may still have some\nother refs that are missing from the other side due to the failure\nwe noticed.\n\nIs it sensible to abort the fetching?  I am undecided, but without\nfurther input, my gut reaction is that it is safe and may even be\nbetter to treat this as a soft error and try to go closer to where\nthe user wanted to go as much as possible by continuing to fetch\nfrom the other side, given that we have already paid for the cost of\ndiscovering the refs from the other side.\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 20f7110ec1..df824cc3d0 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -164,6 +164,16 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n>  \tgit rev-parse sometag\n>  '\n>  \n> +test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n> +\tcd \"$D\" &&\n> +\tgit clone . prune-fail &&\n> +\tcd prune-fail &&\n> +\tgit update-ref refs/remotes/origin/extrabranch main &&\n> +\t>.git/packed-refs.new &&\n> +\ttest_must_fail git fetch --prune origin\n\nIs it because the lockfile \".new\" on packed-refs prevents deletion\nof refs but does not block creation and updates to existing refs\nthat it is an effective test for the \"--prune\" issue?  If we somehow\n\"locked\" the whole ref updates, then the fetch would fail even\nwithout \"--prune\", so it may be \"effective\", but smells like knowing\ntoo much implementation detail.  Yuck, but I do not offhand think of\nany better way (it is easier to think of worse ways), so without\nfurther input, I would say that this is the best (or \"least bad\") we\ncan do.\n\nAnother tangent #leftoverbits is that many tests in this script are\nso old-style that chdir around without isolating themselves in\nsubshells, while some do.  We may want to clean them up when the\nfile is quiescent.\n\nThanks.\n"},{"id":"447180","messageId":"nycvar.QRO.7.76.6.2201281110050.347@tvgsbejvaqbjf.bet","threadId":"57323","inReplyTo":"xmqqmtjh0x5f.fsf@gitster.g","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T10:13:56Z","receivedAt":"2022-01-28T10:14:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 27 Jan 2022, Junio C Hamano wrote:\n\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n>\n> > +\t\tif (retcode) {\n> > +\t\t\tfree_refs(ref_map);\n> > +\t\t\tgoto cleanup;\n> >  \t\t}\n>\n> This part is iffy.  We tried to prune refs, we may have removed some\n> of the refs missing from the other side but we may still have some\n> other refs that are missing from the other side due to the failure\n> we noticed.\n>\n> Is it sensible to abort the fetching?  I am undecided, but without\n> further input, my gut reaction is that it is safe and may even be\n> better to treat this as a soft error and try to go closer to where\n> the user wanted to go as much as possible by continuing to fetch\n> from the other side, given that we have already paid for the cost of\n> discovering the refs from the other side.\n\nI am not so sure. When pruning failed, there may very well be directories\nor files in the way of fetching the refs as desired. And it might be even\nworse if pruning failed _without_ the fetch failing afterwards: the user\nspecifically asked for stale refs to be cleaned up, the command succeeded,\nbut did not do what the user asked for.\n\nMaybe Thomas has an even stronger argument in favor of erroring out. In\nany case, I don't think that `--prune` should be a \"best effort, otherwise\njust shrug\" option. If we wanted that, we could introduce\n`--prune-best-effort` or some such...\n\nCiao,\nDscho\n"},{"id":"447186","messageId":"87pmocp1si.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me","threadId":"57323","inReplyTo":"nycvar.QRO.7.76.6.2201281110050.347@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2022-01-28T10:55:41Z","receivedAt":"2022-01-28T10:55:51Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"\nJohannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Thu, 27 Jan 2022, Junio C Hamano wrote:\n>\n>> Thomas Gummerer <t.gummerer@gmail.com> writes:\n>>\n>> > +\t\tif (retcode) {\n>> > +\t\t\tfree_refs(ref_map);\n>> > +\t\t\tgoto cleanup;\n>> >  \t\t}\n>>\n>> This part is iffy.  We tried to prune refs, we may have removed some\n>> of the refs missing from the other side but we may still have some\n>> other refs that are missing from the other side due to the failure\n>> we noticed.\n>>\n>> Is it sensible to abort the fetching?  I am undecided, but without\n>> further input, my gut reaction is that it is safe and may even be\n>> better to treat this as a soft error and try to go closer to where\n>> the user wanted to go as much as possible by continuing to fetch\n>> from the other side, given that we have already paid for the cost of\n>> discovering the refs from the other side.\n\n> I am not so sure. When pruning failed, there may very well be directories\n> or files in the way of fetching the refs as desired. And it might be even\n> worse if pruning failed _without_ the fetch failing afterwards: the user\n> specifically asked for stale refs to be cleaned up, the command succeeded,\n> but did not do what the user asked for.\n\nI was thinking along similar lines here.  I was going back and forth\nbetween letting the fetch continue, and then exiting with a non-zero\nexit code, and just erroring out directly.  I ended up with the latter\nbecause it felt like the right thing to do for the user.  The command\nfailed, so I'd think it's more confusing that it does more work after\nthat (even if we already did part of that work).\n\nBut I don't care too much one way or another, as long as we end up with\na non-zero exit code when the fetch fails.\n\n> Maybe Thomas has an even stronger argument in favor of erroring out. In\n> any case, I don't think that `--prune` should be a \"best effort, otherwise\n> just shrug\" option. If we wanted that, we could introduce\n> `--prune-best-effort` or some such...\n\nI don't have a strong argument on whether we should error out\nimmediately after failing to prune the refs, or just erroring out after\ndoing the rest of the work, other than exiting early feels right to me.\n\nFrom the comments upthread it seems to me that the argument is more\nwhether we should continue after failing to prune, and exit with a\nnon-zero exit code, or if we should just fail early.\n\nThough I'm happy to introduce a '--prune-best-effort' option or\nsomething like that if we still want to preserve that option for users.\nI don't really see people wanting to use that though, and it should be\nvery rare that '--prune' fails, but the rest of the fetch succeeds.\n"},{"id":"447187","messageId":"87lez0p1db.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me","threadId":"57323","inReplyTo":"xmqqmtjh0x5f.fsf@gitster.g","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2022-01-28T11:04:48Z","receivedAt":"2022-01-28T11:04:56Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"\nJunio C Hamano writes:\n\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n>\n>> When pruning refs fails, we currently print an error to stderr, but\n>> still exit 0 from 'git fetch'.  Since this is a genuine error fetch\n>> should be exiting with some non-zero exit code.  Make it so.\n>>\n>> The --prune option was introduced in f360d844de (\"builtin-fetch: add\n>> --prune option\", 2009-11-10).  Unfortunately it's unclear from that\n>> commit whether ignoring the exit code was an oversight or\n>> intentional, but it feels like an oversight.\n>\n> It is a good idea to signal, with a non-zero status, that the user\n> needs to inspect the situation and possibly take a corrective\n> action.  I however do not know if it is sufficient to just give the\n> same exit status as returned by prune_refs(), which may or may not\n> happen to crash with other possible non-zero exit status to make it\n> indistinguishable from other kinds of errors.\n\nNot sure I understand this correctly.  Are you saying that we should\ntake the return value from prune refs, and always munge that to the same\nexit code for 'git fetch --prune' if it fails?  E.g. so prune_refs()\ncould return 1 or 2 or 3 or whatever, but we would always want the exit\ncode for 'fetch' to be 1?\n\nHappy to implement that.\n\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index 20f7110ec1..df824cc3d0 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -164,6 +164,16 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n>>  \tgit rev-parse sometag\n>>  '\n>>\n>> +test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n>> +\tcd \"$D\" &&\n>> +\tgit clone . prune-fail &&\n>> +\tcd prune-fail &&\n>> +\tgit update-ref refs/remotes/origin/extrabranch main &&\n>> +\t>.git/packed-refs.new &&\n>> +\ttest_must_fail git fetch --prune origin\n>\n> Is it because the lockfile \".new\" on packed-refs prevents deletion\n> of refs but does not block creation and updates to existing refs\n> that it is an effective test for the \"--prune\" issue?  If we somehow\n> \"locked\" the whole ref updates, then the fetch would fail even\n> without \"--prune\", so it may be \"effective\", but smells like knowing\n> too much implementation detail.  Yuck, but I do not offhand think of\n> any better way (it is easier to think of worse ways), so without\n> further input, I would say that this is the best (or \"least bad\") we\n> can do.\n\nYes, that's correct.  New refs will be created as loose refs, so they\ndon't care about packed-refs.  However deletions can potentially be\nhappening in packed-refs, and that's why it fails when 'packed-refs.new'\nexists.\n\nI don't love the test either, but I also can't think of a better way to\ndo this.\n\n> Another tangent #leftoverbits is that many tests in this script are\n> so old-style that chdir around without isolating themselves in\n> subshells, while some do.  We may want to clean them up when the\n> file is quiescent.\n>\n> Thanks.\n"},{"id":"447200","messageId":"nycvar.QRO.7.76.6.2201281333410.347@tvgsbejvaqbjf.bet","threadId":"57323","inReplyTo":"87lez0p1db.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T12:34:56Z","receivedAt":"2022-01-28T12:35:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Fri, 28 Jan 2022, Thomas Gummerer wrote:\n\n> Junio C Hamano writes:\n>\n> > Thomas Gummerer <t.gummerer@gmail.com> writes:\n> >\n> >> +test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n> >> +\tcd \"$D\" &&\n> >> +\tgit clone . prune-fail &&\n> >> +\tcd prune-fail &&\n> >> +\tgit update-ref refs/remotes/origin/extrabranch main &&\n> >> +\t>.git/packed-refs.new &&\n> >> +\ttest_must_fail git fetch --prune origin\n> >\n> > Is it because the lockfile \".new\" on packed-refs prevents deletion\n> > of refs but does not block creation and updates to existing refs\n> > that it is an effective test for the \"--prune\" issue?  If we somehow\n> > \"locked\" the whole ref updates, then the fetch would fail even\n> > without \"--prune\", so it may be \"effective\", but smells like knowing\n> > too much implementation detail.  Yuck, but I do not offhand think of\n> > any better way (it is easier to think of worse ways), so without\n> > further input, I would say that this is the best (or \"least bad\") we\n> > can do.\n>\n> Yes, that's correct.  New refs will be created as loose refs, so they\n> don't care about packed-refs.  However deletions can potentially be\n> happening in packed-refs, and that's why it fails when 'packed-refs.new'\n> exists.\n>\n> I don't love the test either, but I also can't think of a better way to\n> do this.\n\nMaybe add a code comment about it? Something like:\n\n\t[...]\n\t: this will prevent --prune from locking packed-refs &&\n\t>.git/packed-refs.new &&\n\t[...]\n\nCiao,\nDscho\n"},{"id":"447201","messageId":"nycvar.QRO.7.76.6.2201281335050.347@tvgsbejvaqbjf.bet","threadId":"57323","inReplyTo":"87pmocp1si.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T12:36:25Z","receivedAt":"2022-01-28T12:36:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas & Junio,\n\nOn Fri, 28 Jan 2022, Thomas Gummerer wrote:\n\n> Johannes Schindelin writes:\n>\n> > On Thu, 27 Jan 2022, Junio C Hamano wrote:\n> >\n> >> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> >>\n> >> > +\t\tif (retcode) {\n> >> > +\t\t\tfree_refs(ref_map);\n> >> > +\t\t\tgoto cleanup;\n> >> >  \t\t}\n> >>\n> >> This part is iffy.  We tried to prune refs, we may have removed some\n> >> of the refs missing from the other side but we may still have some\n> >> other refs that are missing from the other side due to the failure\n> >> we noticed.\n> >>\n> >> Is it sensible to abort the fetching?  I am undecided, but without\n> >> further input, my gut reaction is that it is safe and may even be\n> >> better to treat this as a soft error and try to go closer to where\n> >> the user wanted to go as much as possible by continuing to fetch\n> >> from the other side, given that we have already paid for the cost of\n> >> discovering the refs from the other side.\n>\n> > I am not so sure. When pruning failed, there may very well be directories\n> > or files in the way of fetching the refs as desired. And it might be even\n> > worse if pruning failed _without_ the fetch failing afterwards: the user\n> > specifically asked for stale refs to be cleaned up, the command succeeded,\n> > but did not do what the user asked for.\n>\n> I was thinking along similar lines here.  I was going back and forth\n> between letting the fetch continue, and then exiting with a non-zero\n> exit code, and just erroring out directly.\n\nOh, I think I misunderstood Junio. As long as the failed prune will cause\na non-zero exit code, I am fine with continuing to try to fetch.\n\nCiao,\nDscho\n"},{"id":"447229","messageId":"xmqqpmobwwvg.fsf@gitster.g","threadId":"57323","inReplyTo":"nycvar.QRO.7.76.6.2201281110050.347@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-28T18:14:43Z","receivedAt":"2022-01-28T18:14:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I am not so sure. When pruning failed, there may very well be directories\n> or files in the way of fetching the refs as desired. And it might be even\n> worse if pruning failed _without_ the fetch failing afterwards: the user\n> specifically asked for stale refs to be cleaned up, the command succeeded,\n> but did not do what the user asked for.\n>\n> Maybe Thomas has an even stronger argument in favor of erroring out. In\n> any case, I don't think that `--prune` should be a \"best effort, otherwise\n> just shrug\" option. If we wanted that, we could introduce\n> `--prune-best-effort` or some such...\n\nI am not opposed to reporting an error by exiting with non-zero exit\ncode.  I never said it should be best effort, and doing the \"fetch\" part\nafter a failed prune does not make it best effort.\n\nWhat I am questioning is if it makes sense to stop the fetching\npart.  When we fetch to update multiple refs, we do not stop at the\nfirst ref-update failure, but try to do as much as possible and then\nreport an error, no?  It is the same thing.\n\n"},{"id":"447329","messageId":"87ilu0oxyi.fsf@coati.i-did-not-set--mail-host-address--so-tickle-me","threadId":"57323","inReplyTo":"nycvar.QRO.7.76.6.2201281333410.347@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] fetch --prune: exit with error if pruning fails","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2022-01-31T13:07:33Z","receivedAt":"2022-01-31T13:07:55Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"\nJohannes Schindelin writes:\n\n> Hi Thomas,\n>\n> On Fri, 28 Jan 2022, Thomas Gummerer wrote:\n>\n>> Junio C Hamano writes:\n>>\n>> > Thomas Gummerer <t.gummerer@gmail.com> writes:\n>> >\n>> >> +test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n>> >> +\tcd \"$D\" &&\n>> >> +\tgit clone . prune-fail &&\n>> >> +\tcd prune-fail &&\n>> >> +\tgit update-ref refs/remotes/origin/extrabranch main &&\n>> >> +\t>.git/packed-refs.new &&\n>> >> +\ttest_must_fail git fetch --prune origin\n>> >\n>> > Is it because the lockfile \".new\" on packed-refs prevents deletion\n>> > of refs but does not block creation and updates to existing refs\n>> > that it is an effective test for the \"--prune\" issue?  If we somehow\n>> > \"locked\" the whole ref updates, then the fetch would fail even\n>> > without \"--prune\", so it may be \"effective\", but smells like knowing\n>> > too much implementation detail.  Yuck, but I do not offhand think of\n>> > any better way (it is easier to think of worse ways), so without\n>> > further input, I would say that this is the best (or \"least bad\") we\n>> > can do.\n>>\n>> Yes, that's correct.  New refs will be created as loose refs, so they\n>> don't care about packed-refs.  However deletions can potentially be\n>> happening in packed-refs, and that's why it fails when 'packed-refs.new'\n>> exists.\n>>\n>> I don't love the test either, but I also can't think of a better way to\n>> do this.\n>\n> Maybe add a code comment about it? Something like:\n>\n> \t[...]\n> \t: this will prevent --prune from locking packed-refs &&\n> \t>.git/packed-refs.new &&\n> \t[...]\n\nYeah, I think a comment here is a good idea, thanks!\n"},{"id":"447330","messageId":"20220131133047.1885074-1-t.gummerer@gmail.com","threadId":"57323","inReplyTo":"20220127153714.1190894-1-t.gummerer@gmail.com","subject":"[PATCH v2] fetch --prune: exit with error if pruning fails","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2022-01-31T13:30:47Z","receivedAt":"2022-01-31T13:30:58Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When pruning refs fails, we currently print an error to stderr, but\nstill exit 0 from 'git fetch'.  Since this is a genuine error fetch\nshould be exiting with some non-zero exit code.  Make it so.\n\nThe --prune option was introduced in f360d844de (\"builtin-fetch: add\n--prune option\", 2009-11-10).  Unfortunately it's unclear from that\ncommit whether ignoring the exit code was an oversight or\nintentional, but it feels like an oversight.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nchanges in v2:\n- retcode will now always be 1 if we get an error from prune_refs,\n  no matter what the original error code was.\n- we will now continue the fetch, and only exit with a non-zero\n  exit code after having finished the rest of the fetch operation\n- added a comment in the test explaining why creating a packed-refs.new\n  file will make the fetch --prune fail.\n\n builtin/fetch.c  | 10 ++++++----\n t/t5510-fetch.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e..648f0694f2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1609,12 +1609,14 @@ static int do_fetch(struct transport *transport,\n \t\t * don't care whether --tags was specified.\n \t\t */\n \t\tif (rs->nr) {\n-\t\t\tprune_refs(rs, ref_map, transport->url);\n+\t\t\tretcode = prune_refs(rs, ref_map, transport->url);\n \t\t} else {\n-\t\t\tprune_refs(&transport->remote->fetch,\n-\t\t\t\t   ref_map,\n-\t\t\t\t   transport->url);\n+\t\t\tretcode = prune_refs(&transport->remote->fetch,\n+\t\t\t\t\t     ref_map,\n+\t\t\t\t\t     transport->url);\n \t\t}\n+\t\tif (retcode != 0)\n+\t\t\tretcode = 1;\n \t}\n \tif (fetch_and_consume_refs(transport, ref_map, worktrees)) {\n \t\tfree_refs(ref_map);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 20f7110ec1..ef0da0a63b 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -164,6 +164,17 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'\n \tgit rev-parse sometag\n '\n \n+test_expect_success REFFILES 'fetch --prune fails to delete branches' '\n+\tcd \"$D\" &&\n+\tgit clone . prune-fail &&\n+\tcd prune-fail &&\n+\tgit update-ref refs/remotes/origin/extrabranch main &&\n+\t: this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds  &&\n+\t>.git/packed-refs.new &&\n+\n+\ttest_must_fail git fetch --prune origin\n+'\n+\n test_expect_success 'fetch --atomic works with a single branch' '\n \ttest_when_finished \"rm -rf \\\"$D\\\"/atomic\" &&\n \n-- \n2.31.1\n\n"},{"id":"447357","messageId":"xmqqh79jiuel.fsf@gitster.g","threadId":"57323","inReplyTo":"20220131133047.1885074-1-t.gummerer@gmail.com","subject":"Re: [PATCH v2] fetch --prune: exit with error if pruning fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T19:20:50Z","receivedAt":"2022-01-31T19:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 5f06b21f8e..648f0694f2 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1609,12 +1609,14 @@ static int do_fetch(struct transport *transport,\n>  \t\t * don't care whether --tags was specified.\n>  \t\t */\n>  \t\tif (rs->nr) {\n> -\t\t\tprune_refs(rs, ref_map, transport->url);\n> +\t\t\tretcode = prune_refs(rs, ref_map, transport->url);\n>  \t\t} else {\n> -\t\t\tprune_refs(&transport->remote->fetch,\n> -\t\t\t\t   ref_map,\n> -\t\t\t\t   transport->url);\n> +\t\t\tretcode = prune_refs(&transport->remote->fetch,\n> +\t\t\t\t\t     ref_map,\n> +\t\t\t\t\t     transport->url);\n>  \t\t}\n> +\t\tif (retcode != 0)\n> +\t\t\tretcode = 1;\n>  \t}\n\nLooks trivially correct, even though a few style things look a bit\nirritating to my eyes ;-).\n\nThanks, will queue.\n"}]}