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

[Junio C Hamano <gitster@pobox.com>]

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