From: Junio C Hamano <gitster@pobox.com> To: Emily Shaffer <emilyshaffer@google.com> Cc: Junio C Hamano via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org, sandals@crustytoothpaste.net, steadmon@google.com, jrnieder@gmail.com, peff@peff.net, congdanhqx@gmail.com, phillip.wood123@gmail.com, sluongng@gmail.com, jonathantanmy@google.com, Derrick Stolee <derrickstolee@github.com> Subject: Re: [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update Date: Wed, 12 Aug 2020 17:03:32 -0700 [thread overview] Message-ID: <xmqqo8nf4dkb.fsf@gitster.c.googlers.com> (raw) In-Reply-To: <20200812231021.GG2965447@google.com> (Emily Shaffer's message of "Wed, 12 Aug 2020 16:10:21 -0700") Emily Shaffer <emilyshaffer@google.com> writes: >> +fetch.writeFetchHEAD:: >> + Setting it to false tells `git fetch` not to write the list >> + of remote refs fetched in the `FETCH_HEAD` file directly >> + under `$GIT_DIR`. Can be countermanded from the command >> + line with the `--[no-]write-fetch-head` option. Defaults to >> + true. > > [jrnieder] Is providing a config option creating more trouble than > benefit? Is this a burden on script authors that they need to check the > config state, when instead they could just pass the flag? Or rather, > because of the config, the script author has to pass the flag explicitly > anyways. > [emily] removing the config also clears up the confusion around 'git pull' + > 'fetch.writeFetchHEAD' I commented on later. [A meta comment, but I somehow find this format cumbersome to read and respond to. Would it be possible to dedup and/or summarize comments on a single point?] I do not think "it becomes cumbersome to design interaction between command line option and configuration" is a valid reason not to add configuration variable. It would be a valid reason not to, if we have a convincing argument why people won't pass the command line option very often, though, but you'd need to be prepared for a possible future in which somebody finds a good use case where a configuration is useful, which means you cannot forever depend on the lack of configurability to avoid making a proper design of the interaction between configuration and command line. As the feature itself is primarily designed for scripts that want to always disable writing of FETCH_HEAD, I can certainly understand a short-term/sighted view of not wanting to add configuration, though. >> @@ -895,7 +902,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, >> const char *what, *kind; >> struct ref *rm; >> char *url; >> - const char *filename = dry_run ? "/dev/null" : git_path_fetch_head(the_repository); >> + const char *filename = (!write_fetch_head >> + ? "/dev/null" >> + : git_path_fetch_head(the_repository)); > > [emily] Huh. so what does the dry_run codepath look like now? It looks > like it's been superseded by write_fetch_head but the option parse > doesn't tell dry_run to update write_fetch_head instead. (Found the > answer later, see below) > >> @@ -1797,6 +1806,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) >> if (depth || deepen_since || deepen_not.nr) >> deepen = 1; >> >> + /* FETCH_HEAD never gets updated in --dry-run mode */ >> + if (dry_run) >> + write_fetch_head = 0; > > [emily] Aha, here is where dry_run informs write_fetch_head. I wonder if > there's a way to instead tell the options struct that --dry-run ~= > --write-fetch-head? I do not know with what meaning you used the symbol "~=". Dry-run tells the command to operate differently in a few ways, and one of the (smaller) ways is not to write the FETCH_HEAD file. Did you mean "contains", "includes", etc.? >> +test_expect_success '--write-fetch-head gets defeated by --dry-run' ' >> + rm -f .git/FETCH_HEAD && >> + git fetch --dry-run --write-fetch-head . && >> + ! test -f .git/FETCH_HEAD >> +' > > [emily] Would it make more sense to make these args mutually exclusive? At least, give a hint, if you cannot state in concrete words, why you suspect it might make sense to do things differently. What use case do you think an alternative design would help better? Without it, you can just get "no" and such an exchange would not be useful at all to improve the patch. > [jrnieder] If someone specifies both, then they probably want to say > "show me what I would write to FETCH_HEAD but don't actually do that" - > which isn't info that we print anyways, right now. Do you mean "don't actually write but show it to standard output instead" or something? If we were designing --dry-run from scratch back when there were no "git fetch" command, that would have made tons of sense, I think ;-) To me, the "--write-fetch-head" option tells it to "write to the file", and not "write the info to unfixed destination that is not specified by this option but derived by the presense of other options". While the "don't download and show what would be in FETCH_HEAD" might be a good feature to add, combining these two options may be a good way to invoke the feature. >> +test_expect_success 'git pull --no-write-fetch-head fails' ' >> + mkdir clonedwfh && >> + (cd clonedwfh && git init && >> + test_must_fail git pull --no-write-fetch-head "../parent" >out 2>err && >> + test_must_be_empty out && >> + test_i18ngrep "no-write-fetch-head" err) > > Should this be shown as a usage error instead of a die()? As "pull" does and should not take "--[no-]write-fetch-head" as its option, I think the above command line deserves an usage error, just like "git pull --update-head-ok" does (or "git pull --no-such-option" for that matter).
next prev parent reply other threads:[~2020-08-13 0:03 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-06 16:30 [PATCH 0/9] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget 2020-08-06 16:30 ` [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget 2020-08-12 23:10 ` Emily Shaffer 2020-08-13 0:03 ` Junio C Hamano [this message] 2020-08-13 1:45 ` Jonathan Nieder 2020-08-13 4:37 ` [PATCH v3] " Junio C Hamano 2020-08-14 1:13 ` Derrick Stolee 2020-08-14 1:32 ` Junio C Hamano 2020-08-06 16:30 ` [PATCH 2/9] maintenance: add prefetch task Derrick Stolee via GitGitGadget 2020-08-12 23:10 ` Emily Shaffer 2020-08-14 1:28 ` Derrick Stolee 2020-08-06 16:30 ` [PATCH 3/9] maintenance: add loose-objects task Derrick Stolee via GitGitGadget 2020-08-12 23:10 ` Emily Shaffer 2020-08-14 1:46 ` Derrick Stolee 2020-08-06 16:30 ` [PATCH 4/9] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget 2020-08-06 16:30 ` [PATCH 5/9] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget 2020-08-06 16:30 ` [PATCH 6/9] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget 2020-08-06 16:30 ` [PATCH 7/9] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget 2020-08-06 16:30 ` [PATCH 8/9] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget 2020-08-06 17:02 ` Son Luong Ngoc 2020-08-06 18:13 ` Derrick Stolee 2020-08-06 16:30 ` [PATCH 9/9] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 0/9] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 1/9] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 2/9] maintenance: add prefetch task Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 3/9] maintenance: add loose-objects task Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 4/9] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 5/9] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 6/9] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 7/9] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 8/9] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget 2020-08-18 14:25 ` [PATCH v2 9/9] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget 2020-08-25 18:36 ` [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget 2020-08-25 18:36 ` [PATCH v3 1/8] maintenance: add prefetch task Derrick Stolee via GitGitGadget 2020-09-22 23:05 ` Jonathan Tan 2020-08-25 18:36 ` [PATCH v3 2/8] maintenance: add loose-objects task Derrick Stolee via GitGitGadget 2020-09-22 23:09 ` Jonathan Tan 2020-09-24 13:45 ` Derrick Stolee 2020-08-25 18:36 ` [PATCH v3 3/8] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget 2020-09-22 23:15 ` Jonathan Tan 2020-09-24 13:51 ` Derrick Stolee 2020-08-25 18:36 ` [PATCH v3 4/8] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget 2020-09-22 23:16 ` Jonathan Tan 2020-09-24 13:53 ` Derrick Stolee 2020-08-25 18:36 ` [PATCH v3 5/8] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget 2020-08-25 18:36 ` [PATCH v3 6/8] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget 2020-09-22 23:26 ` Jonathan Tan 2020-09-24 14:05 ` Derrick Stolee 2020-09-24 22:01 ` Jonathan Tan 2020-08-25 18:36 ` [PATCH v3 7/8] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget 2020-08-25 18:36 ` [PATCH v3 8/8] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget 2020-09-22 23:52 ` Jonathan Tan 2020-08-25 20:59 ` [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks Junio C Hamano 2020-08-26 15:15 ` Son Luong Ngoc 2020-08-26 16:21 ` Derrick Stolee 2020-09-25 12:33 ` [PATCH v4 " Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 1/8] maintenance: add prefetch task Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 2/8] maintenance: add loose-objects task Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 3/8] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget 2020-09-25 18:00 ` Junio C Hamano 2020-09-25 18:43 ` Derrick Stolee 2020-09-25 12:33 ` [PATCH v4 4/8] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 5/8] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 6/8] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 7/8] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget 2020-09-25 12:33 ` [PATCH v4 8/8] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=xmqqo8nf4dkb.fsf@gitster.c.googlers.com \ --to=gitster@pobox.com \ --cc=congdanhqx@gmail.com \ --cc=derrickstolee@github.com \ --cc=emilyshaffer@google.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=jonathantanmy@google.com \ --cc=jrnieder@gmail.com \ --cc=peff@peff.net \ --cc=phillip.wood123@gmail.com \ --cc=sandals@crustytoothpaste.net \ --cc=sluongng@gmail.com \ --cc=steadmon@google.com \ --subject='Re: [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).