Re: [PATCH v2] fetch: optionally allow disabling FETCH_HEAD update

[Phillip Wood <phillip.wood123@gmail.com>]

On 28/07/2020 17:37, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 7/27/2020 12:13 PM, Junio C Hamano wrote:
>>> Derrick Stolee <stolee@gmail.com> writes:
>>>
>>>> I'll rebase onto jc/no-update-fetch-head for the next version, since
>>>> that branch is based on v2.28.0-rc0, which is recent enough.
>>>
>>> I do not think it is wise to base a work on top of unfinished "you
>>> could do it this way, perhaps?" demonstration patch the original
>>> author does not have much inclination to finish, though.
>>>
>>> When I am really bored, I may go back to the topic to finish it, but
>>> I wouldn't mind if you took ownership of it at all.
>>
>> Ah. I didn't understand the status of that branch. I'll pull it in
>> to this topic.
> 
> So here is with one of the two things that I found missing in the
> first iteration of the patch: documentation.
> 
> The other thing that I found iffy (and still missing from this
> version) was what should be done when "git pull" is explicitly given
> the "--no-write-fetch-head" option.
> 
> I think (but didn't check the recent code) that 'git pull' would
> pass only known-to-make-sense command line options to underlying
> 'git fetch', so it probably will barf with "unknown option", which
> is the best case.  We might want to make it sure with a new test in
> 5521.  On the other hand, if we get anything other than "no such
> option", we may want to think if we want to "fix" it or just leave
> it inside "if it hurts, don't do it" territory.
> 
> Thanks.
> 
> The patch without doc was Reviewed-by: Taylor Blau <me@ttaylorr.com>
> but this round has not been.
> 
> -- >8 --
> 
> 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 `--fetch-write-head` 

Typo, it should be `--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>
> ---
>   Documentation/config/fetch.txt  |  7 ++++++
>   Documentation/fetch-options.txt | 10 +++++++++
>   builtin/fetch.c                 | 19 +++++++++++++---
>   builtin/pull.c                  |  3 ++-
>   t/t5510-fetch.sh                | 39 +++++++++++++++++++++++++++++++--
>   5 files changed, 72 insertions(+), 6 deletions(-)
> 
> 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 6e2a160a47..6775e8499f 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 82ac4be8a5..3ccf69753f 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")),
> @@ -893,7 +900,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));

I was suspicious of this as we haven't cleared write_fetch_head in the 
--dry-run case yet but the test below seems to show that we still don't 
write FETCH_HEAD if --dry-run is given. That makes we wonder what the 
point of setting the filename based on the value of write_fetch_head is.

The rest looks good to me, though it might be worth having a test to 
check that pull does indeed reject --no-write-fetch-head

Best Wishes

Phillip

>   	int want_status;
>   	int summary_width = transport_summary_width(ref_map);
>   
> @@ -1327,7 +1336,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;
> @@ -1594,7 +1603,7 @@ static int fetch_multiple(struct string_list *list, int max_children)
>   	int i, result = 0;
>   	struct argv_array argv = ARGV_ARRAY_INIT;
>   
> -	if (!append && !dry_run) {
> +	if (!append && write_fetch_head) {
>   		int errcode = truncate_fetch_head();
>   		if (errcode)
>   			return errcode;
> @@ -1795,6 +1804,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 8159c5d7c9..e988d92b53 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -527,7 +527,8 @@ static int run_fetch(const char *repo, const char **refspecs)
>   	struct argv_array args = ARGV_ARRAY_INIT;
>   	int ret;
>   
> -	argv_array_pushl(&args, "fetch", "--update-head-ok", NULL);
> +	argv_array_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 a66dbe0bde..3052c2d8d5 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 &&
>   	(
>