fetch: optionally allow disabling FETCH_HEAD update
diff mbox series

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

Commit Message

Junio C Hamano July 13, 2020, 6:06 p.m. UTC
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 without having 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.

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

    Junio C Hamano <gitster@pobox.com> writes:

    > Johannes Sixt <j6t@kdbg.org> writes:
    >
    >> Nah, really??? It's one of the benefits of git-fetch that it writes
    >> FETCH_HEAD and the primary reason in many cases where I use the command!
    >> So, either I don't care that FETCH_HEAD is written, or I do use it. IMO,
    >> not wanting to write FETCH_HEAD is the odd case and would need a
    >> configuration tweak, not the other way round.
    >
    > Yeah, that's even easier to arrange.  
    >
    > Just the "--[no-]write-fetch-head" command line option and the
    > fetch.writeFetchHEAD configuration variable are introduced and left
    > off by default forever.

    Something like this, perhaps.  Obviously I won't be pushing this
    topic further while in prerelase freeze, but Konstantin or
    anybody else interested can locally apply the patch to their own
    copy of Git to test it out.

    Thanks.

 builtin/fetch.c  | 19 ++++++++++++++++---
 builtin/pull.c   |  3 ++-
 t/t5510-fetch.sh | 39 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 6 deletions(-)

Comments

Taylor Blau July 13, 2020, 7:08 p.m. UTC | #1
On Mon, Jul 13, 2020 at 11:06:09AM -0700, Junio C Hamano wrote:
> 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 without having 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.
>
> 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>
> ---
>
>     Junio C Hamano <gitster@pobox.com> writes:
>
>     > Johannes Sixt <j6t@kdbg.org> writes:
>     >
>     >> Nah, really??? It's one of the benefits of git-fetch that it writes
>     >> FETCH_HEAD and the primary reason in many cases where I use the command!
>     >> So, either I don't care that FETCH_HEAD is written, or I do use it. IMO,
>     >> not wanting to write FETCH_HEAD is the odd case and would need a
>     >> configuration tweak, not the other way round.
>     >
>     > Yeah, that's even easier to arrange.
>     >
>     > Just the "--[no-]write-fetch-head" command line option and the
>     > fetch.writeFetchHEAD configuration variable are introduced and left
>     > off by default forever.
>
>     Something like this, perhaps.  Obviously I won't be pushing this
>     topic further while in prerelase freeze, but Konstantin or
>     anybody else interested can locally apply the patch to their own
>     copy of Git to test it out.
>
>     Thanks.
>
>  builtin/fetch.c  | 19 ++++++++++++++++---
>  builtin/pull.c   |  3 ++-
>  t/t5510-fetch.sh | 39 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 55 insertions(+), 6 deletions(-)
>
> 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));

Ah, because dry_run ==> !write_fetch_head, so this is an equivalent
translation. Makes sense.

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

...and here we pass '--write-fetch-head' explicitly, because we don't
want a user who has set 'fetch.writeFetchHead' to 'false' to suddenly
have their 'git pull's stop working. Makes sense.

>
>  	/* 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 &&
>  	(

Test coverage all looks good, thanks for working on this. I don't think
there's anything left, so this would be great after 2.28 is released.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

> --
> 2.28.0-rc0
>

Thanks,
Taylor
Junio C Hamano July 13, 2020, 7:45 p.m. UTC | #2
Taylor Blau <me@ttaylorr.com> writes:

>> -	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));
>
> Ah, because dry_run ==> !write_fetch_head, so this is an equivalent
> translation. Makes sense.

Yup, now dry_run is merely one of the two cases we turn
write_fetch_head off, and the logic to prevent us from writing
FETCH_HEAD (hopefully without any other side effect) was already
there in this function.

> Test coverage all looks good, thanks for working on this. I don't think
> there's anything left, so this would be great after 2.28 is released.
>
>   Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks.

Patch
diff mbox series

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