[v2] fetch: optionally allow disabling FETCH_HEAD update
diff mbox series

Message ID xmqqr1svegt4.fsf_-_@gitster.c.googlers.com
State New
Headers show
Series
  • [v2] fetch: optionally allow disabling FETCH_HEAD update
Related show

Commit Message

Junio C Hamano July 28, 2020, 4:37 p.m. UTC
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` 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(-)

Comments

Phillip Wood July 29, 2020, 9:12 a.m. UTC | #1
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 &&
>   	(
>
Phillip Wood July 29, 2020, 9:17 a.m. UTC | #2
On 29/07/2020 10:12, Phillip Wood wrote:
> 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.

Sorry ignore that. I misread the hunk header - we have in fact cleared 
write_fetch_head in the --dry-run case by the time we get here.

> 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 &&
>>       (
>>
Derrick Stolee July 30, 2020, 3:17 p.m. UTC | #3
On 7/28/2020 12:37 PM, 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.

Here is the version I applied and updated. Please let me know what
you think.

-->8--

From 3f60a0f0fd388447aa9c2b805b5646039df98d0b Mon Sep 17 00:00:00 2001
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 28 Jul 2020 09:37:59 -0700
Subject: [PATCH] fetch: optionally allow disabling FETCH_HEAD update

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

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index b20394038d1..0aaa05e8c0e 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 495bc8ab5a1..6972ad2522c 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 d8253f66e4c..1d7194aac26 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 argv_array argv = ARGV_ARRAY_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 8159c5d7c96..e988d92b535 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 9850ecde5df..31c91d0ed2e 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 159afa7ac81..1acae3b9a4f 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 &&

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 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));
 	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 &&
 	(