{"thread":{"id":"59358","subject":"[PATCH] fetch: pass --no-write-fetch-head to subprocesses","startedAt":"2023-03-08T10:04:47Z","lastAt":"2023-03-09T21:32:49Z","messageCount":8,"participants":["Eric Wong","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473186","messageId":"20230308100438.908471-1-e@80x24.org","threadId":"59358","inReplyTo":null,"subject":"[PATCH] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-03-08T10:04:38Z","receivedAt":"2023-03-08T10:04:47Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"It seems a user would expect this option would work regardless\nof whether it's fetching from a single remote or many.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n I haven't checked if there's other suitable options which could\n go into add_options_to_argv(); hopefully someone else can check :>\n\n builtin/fetch.c           | 2 ++\n t/t5514-fetch-multiple.sh | 7 +++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a09606b472..78513f1708 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1880,6 +1880,8 @@ static void add_options_to_argv(struct strvec *argv)\n \t\tstrvec_push(argv, \"--ipv4\");\n \telse if (family == TRANSPORT_FAMILY_IPV6)\n \t\tstrvec_push(argv, \"--ipv6\");\n+\tif (!write_fetch_head)\n+\t\tstrvec_push(argv, \"--no-write-fetch-head\");\n }\n \n /* Fetch multiple remotes in parallel */\ndiff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\nindex 54f422ced3..98f034aa77 100755\n--- a/t/t5514-fetch-multiple.sh\n+++ b/t/t5514-fetch-multiple.sh\n@@ -58,6 +58,13 @@ test_expect_success 'git fetch --all' '\n \t test_cmp expect output)\n '\n \n+test_expect_success 'git fetch --all --no-write-fetch-head' '\n+\t(cd test &&\n+\trm -f .git/FETCH_HEAD &&\n+\tgit fetch --all --no-write-fetch-head &&\n+\ttest_path_is_missing .git/FETCH_HEAD)\n+'\n+\n test_expect_success 'git fetch --all should continue if a remote has errors' '\n \t(git clone one test2 &&\n \t cd test2 &&\n"},{"id":"473205","messageId":"xmqqwn3rta2c.fsf@gitster.g","threadId":"59358","inReplyTo":"20230308100438.908471-1-e@80x24.org","subject":"Re: [PATCH] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T17:41:31Z","receivedAt":"2023-03-08T17:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Subject: Re: [PATCH] fetch: pass --no-write-fetch-head to subprocesses\n\nI read the title as saying that \"git fetch --recurse-submodules\n--no-write-fetch-head\" should propagate the latter option down to\nfetches done in submodules, but looking at the added test, you are\naddressing a different use case, aren't you?  Or are you covering\nboth \"fetch: honor --no-write-fetch-head when fetching from multiple\nremotes\" and \"fetch: pass --no-write-fetch-head down to submodules\"?\n\n> It seems a user would expect this option would work regardless\n> of whether it's fetching from a single remote or many.\n\nThis hints that it is only the latter, but if we are covering both\n\n (1) the title we have here may be alright.\n\n (2) the proposed log message should state the change affects both\n     (in a good way).\n\n (3) the other half may want to be tested in new test as well.\n\nThanks.\n\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  I haven't checked if there's other suitable options which could\n>  go into add_options_to_argv(); hopefully someone else can check :>\n>\n>  builtin/fetch.c           | 2 ++\n>  t/t5514-fetch-multiple.sh | 7 +++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index a09606b472..78513f1708 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1880,6 +1880,8 @@ static void add_options_to_argv(struct strvec *argv)\n>  \t\tstrvec_push(argv, \"--ipv4\");\n>  \telse if (family == TRANSPORT_FAMILY_IPV6)\n>  \t\tstrvec_push(argv, \"--ipv6\");\n> +\tif (!write_fetch_head)\n> +\t\tstrvec_push(argv, \"--no-write-fetch-head\");\n>  }\n>  \n>  /* Fetch multiple remotes in parallel */\n> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\n> index 54f422ced3..98f034aa77 100755\n> --- a/t/t5514-fetch-multiple.sh\n> +++ b/t/t5514-fetch-multiple.sh\n> @@ -58,6 +58,13 @@ test_expect_success 'git fetch --all' '\n>  \t test_cmp expect output)\n>  '\n>  \n> +test_expect_success 'git fetch --all --no-write-fetch-head' '\n> +\t(cd test &&\n> +\trm -f .git/FETCH_HEAD &&\n> +\tgit fetch --all --no-write-fetch-head &&\n> +\ttest_path_is_missing .git/FETCH_HEAD)\n> +'\n> +\n>  test_expect_success 'git fetch --all should continue if a remote has errors' '\n>  \t(git clone one test2 &&\n>  \t cd test2 &&\n"},{"id":"473230","messageId":"20230308222205.M679514@dcvr","threadId":"59358","inReplyTo":"xmqqwn3rta2c.fsf@gitster.g","subject":"[PATCH v2] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-03-08T22:22:05Z","receivedAt":"2023-03-08T22:22:34Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> \n> > Subject: Re: [PATCH] fetch: pass --no-write-fetch-head to subprocesses\n> \n> I read the title as saying that \"git fetch --recurse-submodules\n> --no-write-fetch-head\" should propagate the latter option down to\n> fetches done in submodules, but looking at the added test, you are\n> addressing a different use case, aren't you?  Or are you covering\n> both \"fetch: honor --no-write-fetch-head when fetching from multiple\n> remotes\" and \"fetch: pass --no-write-fetch-head down to submodules\"?\n\nJust multiple remotes, I hardly deal with submodules.\n\n> > It seems a user would expect this option would work regardless\n> > of whether it's fetching from a single remote or many.\n> \n> This hints that it is only the latter, but if we are covering both\n> \n>  (1) the title we have here may be alright.\n\nYes, I figured so.  I actually considered just using the title\nand didn't really feel the need to add a message body\n\n>  (2) the proposed log message should state the change affects both\n>      (in a good way).\n\nUpdated.\n\n>  (3) the other half may want to be tested in new test as well.\n\nOK, updated t5526, hope it's portable.  I mimicked the\nformatting style of each respective test so the diff itself\nlooks odd between changes to t5514 and t5526 :x\n\n> Thanks.\n\nv2: revised commit message body, test submodules in t5526\n---8<---\nSubject: [PATCH] fetch: pass --no-write-fetch-head to subprocesses\n\nIt seems a user would expect this option would work regardless\nof whether it's fetching from a single remote, many remotes,\nor recursing into submodules.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n builtin/fetch.c             |  2 ++\n t/t5514-fetch-multiple.sh   |  7 +++++++\n t/t5526-fetch-submodules.sh | 13 +++++++++++++\n 3 files changed, 22 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a09606b472..78513f1708 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1880,6 +1880,8 @@ static void add_options_to_argv(struct strvec *argv)\n \t\tstrvec_push(argv, \"--ipv4\");\n \telse if (family == TRANSPORT_FAMILY_IPV6)\n \t\tstrvec_push(argv, \"--ipv6\");\n+\tif (!write_fetch_head)\n+\t\tstrvec_push(argv, \"--no-write-fetch-head\");\n }\n \n /* Fetch multiple remotes in parallel */\ndiff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh\nindex 54f422ced3..98f034aa77 100755\n--- a/t/t5514-fetch-multiple.sh\n+++ b/t/t5514-fetch-multiple.sh\n@@ -58,6 +58,13 @@ test_expect_success 'git fetch --all' '\n \t test_cmp expect output)\n '\n \n+test_expect_success 'git fetch --all --no-write-fetch-head' '\n+\t(cd test &&\n+\trm -f .git/FETCH_HEAD &&\n+\tgit fetch --all --no-write-fetch-head &&\n+\ttest_path_is_missing .git/FETCH_HEAD)\n+'\n+\n test_expect_success 'git fetch --all should continue if a remote has errors' '\n \t(git clone one test2 &&\n \t cd test2 &&\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex b9546ef8e5..8ffb300f2d 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -167,6 +167,19 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n \tverify_fetch_result actual.err\n '\n \n+test_expect_success \"fetch --recurse-submodules honors --no-write-fetch-head\" '\n+\t(\n+\t\tcd downstream &&\n+\t\tfh=$(find . -name FETCH_HEAD -type f) &&\n+\t\trm -f $fh &&\n+\t\tgit fetch --recurse-submodules --no-write-fetch-head &&\n+\t\tfor f in $fh\n+\t\tdo\n+\t\t\ttest_path_is_missing $f || return 1\n+\t\tdone\n+\t)\n+'\n+\n test_expect_success \"submodule.recurse option triggers recursive fetch\" '\n \tadd_submodule_commits &&\n \t(\n"},{"id":"473237","messageId":"xmqqttyurg4w.fsf@gitster.g","threadId":"59358","inReplyTo":"20230308222205.M679514@dcvr","subject":"Re: [PATCH v2] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T23:13:19Z","receivedAt":"2023-03-08T23:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> +test_expect_success 'git fetch --all --no-write-fetch-head' '\n> +\t(cd test &&\n> +\trm -f .git/FETCH_HEAD &&\n> +\tgit fetch --all --no-write-fetch-head &&\n> +\ttest_path_is_missing .git/FETCH_HEAD)\n> +'\n\nThe style used in the other script might be more modern, but given\nthat the existing one (in the post context) uses the same older\nstyle, I think that would be OK.\n\n>  test_expect_success 'git fetch --all should continue if a remote has errors' '\n>  \t(git clone one test2 &&\n>  \t cd test2 &&\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index b9546ef8e5..8ffb300f2d 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -167,6 +167,19 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n>  \tverify_fetch_result actual.err\n>  '\n>  \n> +test_expect_success \"fetch --recurse-submodules honors --no-write-fetch-head\" '\n> +\t(\n> +\t\tcd downstream &&\n> +\t\tfh=$(find . -name FETCH_HEAD -type f) &&\n> +\t\trm -f $fh &&\n\nI do not like this part.  The \"rm -f\" we saw in the \"fetch --all\" test\nwas \"make sure it is missing, so that we can be sure that presence\nafter running 'git fetch' *is* a bug\".  But using $fh later ...\n\n> +\t\tgit fetch --recurse-submodules --no-write-fetch-head &&\n> +\t\tfor f in $fh\n> +\t\tdo\n> +\t\t\ttest_path_is_missing $f || return 1\n> +\t\tdone\n\n... like this means now we depend on FETCH_HEAD being in all\nsubmodule repositories before we start this step.\n\nI think we should instead enumerate submodule repositories, instead\nof enumerating existing .git/FETCH_HEAD files.\n\n> +\t)\n> +'\n> +\n>  test_expect_success \"submodule.recurse option triggers recursive fetch\" '\n>  \tadd_submodule_commits &&\n>  \t(\n"},{"id":"473239","messageId":"xmqqjzzqrevv.fsf@gitster.g","threadId":"59358","inReplyTo":"xmqqttyurg4w.fsf@gitster.g","subject":"Re: [PATCH v2] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T23:40:20Z","receivedAt":"2023-03-08T23:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think we should instead enumerate submodule repositories, instead\n> of enumerating existing .git/FETCH_HEAD files.\n\nPerhaps something along this line?\n\n t/t5526-fetch-submodules.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git c/t/t5526-fetch-submodules.sh w/t/t5526-fetch-submodules.sh\nindex 8ffb300f2d..dcdbe26a08 100755\n--- c/t/t5526-fetch-submodules.sh\n+++ w/t/t5526-fetch-submodules.sh\n@@ -170,13 +170,13 @@ test_expect_success \"fetch --recurse-submodules recurses into submodules\" '\n test_expect_success \"fetch --recurse-submodules honors --no-write-fetch-head\" '\n \t(\n \t\tcd downstream &&\n-\t\tfh=$(find . -name FETCH_HEAD -type f) &&\n-\t\trm -f $fh &&\n+\t\tgit submodule foreach --recursive \\\n+\t\tsh -c \"cd \\\"\\$(git rev-parse --git-dir)\\\" && rm -f FETCH_HEAD\" &&\n+\n \t\tgit fetch --recurse-submodules --no-write-fetch-head &&\n-\t\tfor f in $fh\n-\t\tdo\n-\t\t\ttest_path_is_missing $f || return 1\n-\t\tdone\n+\n+\t\tgit submodule foreach --recursive \\\n+\t\tsh -c \"cd \\\"\\$(git rev-parse --git-dir)\\\" && ! test -f FETCH_HEAD\"\n \t)\n '\n \n"},{"id":"473240","messageId":"20230308234857.M503278@dcvr","threadId":"59358","inReplyTo":"xmqqjzzqrevv.fsf@gitster.g","subject":"Re: [PATCH v2] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-03-08T23:48:57Z","receivedAt":"2023-03-08T23:49:01Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I think we should instead enumerate submodule repositories, instead\n> > of enumerating existing .git/FETCH_HEAD files.\n> \n> Perhaps something along this line?\n\nSure, can you squash it into mine?  Thanks.\n"},{"id":"473249","messageId":"ZAlOB0XZaGPUJMS7@coredump.intra.peff.net","threadId":"59358","inReplyTo":"20230308100438.908471-1-e@80x24.org","subject":"Re: [PATCH] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-09T03:09:59Z","receivedAt":"2023-03-09T03:10:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 08, 2023 at 10:04:38AM +0000, Eric Wong wrote:\n\n> It seems a user would expect this option would work regardless\n> of whether it's fetching from a single remote or many.\n> \n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  I haven't checked if there's other suitable options which could\n>  go into add_options_to_argv(); hopefully someone else can check :>\n\nThere's at least one that came up before:\n\n  https://lore.kernel.org/git/DM5PR1701MB1724CCBB1AC5CF342BA9ADD5898E9@DM5PR1701MB1724.namprd17.prod.outlook.com/\n\nbut it never got turned into a real patch.\n\nThis is obviously an error-prone mechanism.  It would be nice if there\nwas a way to avoid it, but after some discussion in this thread, we\ndidn't come up with anything clever:\n\n  https://lore.kernel.org/git/20200914121906.GD4705@pflmari/\n\n-Peff\n"},{"id":"473317","messageId":"xmqqzg8l8vbr.fsf@gitster.g","threadId":"59358","inReplyTo":"20230308234857.M503278@dcvr","subject":"Re: [PATCH v2] fetch: pass --no-write-fetch-head to subprocesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T21:32:24Z","receivedAt":"2023-03-09T21:32:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > I think we should instead enumerate submodule repositories, instead\n>> > of enumerating existing .git/FETCH_HEAD files.\n>> \n>> Perhaps something along this line?\n>\n> Sure, can you squash it into mine?  Thanks.\n\nHeh, I left it at \"something along this line\" because I didn't want\nto debug it myself, or think about the longer-term ramifications\nwhen the tests before this part eventually change in the future.\n\nI've squashed it in and merged the result to 'next', together with a\nfew other topics.\n\nThanks.\n"}]}