[1/9] fetch: optionally allow disabling FETCH_HEAD update
diff mbox series

Message ID 83401c52002716084b9c53a77c9d57b6009f84e2.1596731424.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Maintenance II: prefetch, loose-objects, incremental-repack tasks
Related show

Commit Message

Johannes Schindelin via GitGitGadget Aug. 6, 2020, 4:30 p.m. UTC
From: Junio C Hamano <gitster@pobox.com>

If you run fetch but record the result in remote-tracking branches,
and either if you do nothing with the fetched refs (e.g. you are
merely mirroring) or if you always work from the remote-tracking
refs (e.g. you fetch and then merge origin/branchname separately),
you can get away with having no FETCH_HEAD at all.

Teach "git fetch" a command line option "--[no-]write-fetch-head"
and "fetch.writeFetchHEAD" configuration variable.  Without either,
the default is to write FETCH_HEAD, and the usual rule that the
command line option defeats configured default applies.

Note that under "--dry-run" mode, FETCH_HEAD is never written;
otherwise you'd see list of objects in the file that you do not
actually have.  Passing `--write-fetch-head` does not force `git
fetch` to write the file.

Also note that this option is explicitly passed when "git pull"
internally invokes "git fetch", so that those who configured their
"git fetch" not to write FETCH_HEAD would not be able to break the
cooperation between these two commands.  "git pull" must see what
"git fetch" got recorded in FETCH_HEAD to work correctly.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config/fetch.txt  |  7 ++++++
 Documentation/fetch-options.txt | 10 +++++++++
 builtin/fetch.c                 | 19 +++++++++++++---
 builtin/pull.c                  |  3 ++-
 t/t5510-fetch.sh                | 39 +++++++++++++++++++++++++++++++--
 t/t5521-pull-options.sh         | 16 ++++++++++++++
 6 files changed, 88 insertions(+), 6 deletions(-)

Comments

Emily Shaffer Aug. 12, 2020, 11:10 p.m. UTC | #1
[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.
Junio C Hamano Aug. 13, 2020, 12:03 a.m. UTC | #2
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).
Jonathan Nieder Aug. 13, 2020, 1:45 a.m. UTC | #3
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

Patch
diff mbox series

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 &&