Re: [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update

From: Emily Shaffer <emilyshaffer@google.com>
Date: Aug 12, 2020, 23:10 UTC
Message-ID: <20200812231021.GG2965447@google.com>
In-Reply-To: <83401c52002716084b9c53a77c9d57b6009f84e2.1596731424.git.gitgitgadget@gmail.com>

[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:

Show 7 quoted lines

> 
> +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.

Show 8 quoted lines

> @@ -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)

Show 7 quoted lines

> @@ -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?

Show 5 quoted lines

> +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.

Show 24 quoted lines

> +
> +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.

Show 6 quoted lines

> +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()?

Show 8 quoted lines

> +'
> +
> +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.