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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 13, 2020, 00:03 UTC
Message-ID: <xmqqo8nf4dkb.fsf@gitster.c.googlers.com>
In-Reply-To: <20200812231021.GG2965447@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 14 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.

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

Show 25 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)
>
>> @@ -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.?

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

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.

Show 8 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()?

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