| Message ID | 83401c52002716084b9c53a77c9d57b6009f84e2.1596731424.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintenance II: prefetch, loose-objects, incremental-repack tasks | expand |
[Note: like last time, this was the subject of our team's review club this week, so I'll try to cite individual comments. -Emily] On Thu, Aug 06, 2020 at 04:30:16PM +0000, Junio C Hamano via GitGitGadget wrote: > > +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. > @@ -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? > +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? [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. > + > +test_expect_success 'fetch.writeFetchHEAD and FETCH_HEAD' ' > + rm -f .git/FETCH_HEAD && > + git -c fetch.writeFetchHEAD=no fetch . && > + ! test -f .git/FETCH_HEAD > +' > + > +test_expect_success 'fetch.writeFetchHEAD gets defeated by --dry-run' ' > + rm -f .git/FETCH_HEAD && > + git -c fetch.writeFetchHEAD=yes fetch --dry-run . && > + ! test -f .git/FETCH_HEAD > +' > + > +test_expect_success 'fetch.writeFetchHEAD and --no-write-fetch-head' ' > + rm -f .git/FETCH_HEAD && > + git -c fetch.writeFetchHEAD=yes fetch --no-write-fetch-head . && > + ! test -f .git/FETCH_HEAD > +' > + > +test_expect_success 'fetch.writeFetchHEAD and --write-fetch-head' ' > + rm -f .git/FETCH_HEAD && > + git -c fetch.writeFetchHEAD=no fetch --write-fetch-head . && > + test -f .git/FETCH_HEAD > +' [jrnieder] Thanks for being thorough about these. > +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()? > +' > + > +test_expect_success 'git pull succeeds with fetch.writeFetchHEAD=false' ' > + mkdir clonedwfhconfig && > + (cd clonedwfhconfig && git init && > + git config fetch.writeFetchHEAD false && > + git pull "../parent" >out 2>err && > + grep FETCH_HEAD err) [emily] I guess it's a little surprising that my config is being overridden without any warning... although pull can't possibly work if FETCH_HEAD isn't updated. I am conflicted about this use case.. [jrnieder] This is another argument to drop the config from this patch, unless we are thinking of changing the default behavior of --[no-]write-fetch-head.
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).
Junio C Hamano wrote: > 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. Yes, I think that since this feature is primarily designed for scripts, an option is more likely to be useful for them than a config setting. An option kicks in when the calling script requests it; a config setting can kick in even when they didn't intend to request it. My opinion would change if we think that we're going to flip the default to --no-write-fetch-head some day, in which case a config setting would be a good way to request a preview of the future. But I don't believe anyone's brought that up as a direction we want to pursue. [...] >> 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? My take is that the behavior that the patch implements for --dry-run --write-fetch-head is correct and what a user would want: it acts *as though* you passed --write-fetch-head (including producing the same console output), without producing mutations that the user might regret (such as updating FETCH_HEAD). Thanks and hope that helps, Jonathan
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt index b20394038d..0aaa05e8c0 100644 --- a/Documentation/config/fetch.txt +++ b/Documentation/config/fetch.txt @@ -91,3 +91,10 @@ fetch.writeCommitGraph:: merge and the write may take longer. Having an updated commit-graph file helps performance of many Git commands, including `git merge-base`, `git push -f`, and `git log --graph`. Defaults to false. + +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. diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 495bc8ab5a..6972ad2522 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -64,6 +64,16 @@ documented in linkgit:git-config[1]. --dry-run:: Show what would be done, without making any changes. +ifndef::git-pull[] +--[no-]write-fetch-head:: + Write the list of remote refs fetched in the `FETCH_HEAD` + file directly under `$GIT_DIR`. This is the default unless + the configuration variable `fetch.writeFetchHEAD` is set to + false. Passing `--no-write-fetch-head` from the command + line tells Git not to write the file. Under `--dry-run` + option, the file is never written. +endif::git-pull[] + -f:: --force:: When 'git fetch' is used with `<src>:<dst>` refspec it may diff --git a/builtin/fetch.c b/builtin/fetch.c index c8b9366d3c..42ca774ad1 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */ #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */ static int all, append, dry_run, force, keep, multiple, update_head_ok; +static int write_fetch_head = 1; static int verbosity, deepen_relative, set_upstream; static int progress = -1; static int enable_auto_gc = 1; @@ -118,6 +119,10 @@ static int git_fetch_config(const char *k, const char *v, void *cb) return 0; } + if (!strcmp(k, "fetch.writefetchhead")) { + write_fetch_head = git_config_bool(k, v); + return 0; + } return git_default_config(k, v, cb); } @@ -162,6 +167,8 @@ static struct option builtin_fetch_options[] = { PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules), OPT_BOOL(0, "dry-run", &dry_run, N_("dry run")), + OPT_BOOL(0, "write-fetch-head", &write_fetch_head, + N_("write fetched references to the FETCH_HEAD file")), OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")), OPT_BOOL('u', "update-head-ok", &update_head_ok, N_("allow updating of HEAD ref")), @@ -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)); int want_status; int summary_width = transport_summary_width(ref_map); @@ -1329,7 +1338,7 @@ static int do_fetch(struct transport *transport, } /* if not appending, truncate FETCH_HEAD */ - if (!append && !dry_run) { + if (!append && write_fetch_head) { retcode = truncate_fetch_head(); if (retcode) goto cleanup; @@ -1596,7 +1605,7 @@ static int fetch_multiple(struct string_list *list, int max_children) int i, result = 0; struct strvec argv = STRVEC_INIT; - if (!append && !dry_run) { + if (!append && write_fetch_head) { int errcode = truncate_fetch_head(); if (errcode) return errcode; @@ -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; + if (all) { if (argc == 1) die(_("fetch --all does not take a repository argument")); diff --git a/builtin/pull.c b/builtin/pull.c index 015f6ded0b..5ef3434e1f 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -527,7 +527,8 @@ static int run_fetch(const char *repo, const char **refspecs) struct strvec args = STRVEC_INIT; int ret; - strvec_pushl(&args, "fetch", "--update-head-ok", NULL); + strvec_pushl(&args, "fetch", "--update-head-ok", + "--write-fetch-head", NULL); /* Shared options */ argv_push_verbosity(&args); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 9850ecde5d..31c91d0ed2 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -539,13 +539,48 @@ test_expect_success 'fetch into the current branch with --update-head-ok' ' ' -test_expect_success 'fetch --dry-run' ' - +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' ' rm -f .git/FETCH_HEAD && git fetch --dry-run . && ! test -f .git/FETCH_HEAD ' +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' ' + rm -f .git/FETCH_HEAD && + git fetch --no-write-fetch-head . && + ! test -f .git/FETCH_HEAD +' + +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 +' + +test_expect_success 'fetch.writeFetchHEAD and FETCH_HEAD' ' + rm -f .git/FETCH_HEAD && + git -c fetch.writeFetchHEAD=no fetch . && + ! test -f .git/FETCH_HEAD +' + +test_expect_success 'fetch.writeFetchHEAD gets defeated by --dry-run' ' + rm -f .git/FETCH_HEAD && + git -c fetch.writeFetchHEAD=yes fetch --dry-run . && + ! test -f .git/FETCH_HEAD +' + +test_expect_success 'fetch.writeFetchHEAD and --no-write-fetch-head' ' + rm -f .git/FETCH_HEAD && + git -c fetch.writeFetchHEAD=yes fetch --no-write-fetch-head . && + ! test -f .git/FETCH_HEAD +' + +test_expect_success 'fetch.writeFetchHEAD and --write-fetch-head' ' + rm -f .git/FETCH_HEAD && + git -c fetch.writeFetchHEAD=no fetch --write-fetch-head . && + test -f .git/FETCH_HEAD +' + test_expect_success "should be able to fetch with duplicate refspecs" ' mkdir dups && ( diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 159afa7ac8..1acae3b9a4 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -77,6 +77,7 @@ test_expect_success 'git pull -q -v --no-rebase' ' test_must_be_empty out && test -s err) ' + test_expect_success 'git pull --cleanup errors early on invalid argument' ' mkdir clonedcleanup && (cd clonedcleanup && git init && @@ -85,6 +86,21 @@ test_expect_success 'git pull --cleanup errors early on invalid argument' ' test -s err) ' +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) +' + +test_expect_success 'git pull succeeds with fetch.writeFetchHEAD=false' ' + mkdir clonedwfhconfig && + (cd clonedwfhconfig && git init && + git config fetch.writeFetchHEAD false && + git pull "../parent" >out 2>err && + grep FETCH_HEAD err) +' test_expect_success 'git pull --force' ' mkdir clonedoldstyle &&