[v2] fetch: pass --no-write-fetch-head to subprocesses

Commit Message

Eric Wong March 8, 2023, 10:22 p.m. UTC

Junio C Hamano <[e-mail]> wrote:
> Eric Wong <[e-mail]> writes:
> 
> > Subject: Re: [PATCH] fetch: pass --no-write-fetch-head to subprocesses
> 
> I read the title as saying that "git fetch --recurse-submodules
> --no-write-fetch-head" should propagate the latter option down to
> fetches done in submodules, but looking at the added test, you are
> addressing a different use case, aren't you?  Or are you covering
> both "fetch: honor --no-write-fetch-head when fetching from multiple
> remotes" and "fetch: pass --no-write-fetch-head down to submodules"?

Just multiple remotes, I hardly deal with submodules.

> > It seems a user would expect this option would work regardless
> > of whether it's fetching from a single remote or many.
> 
> This hints that it is only the latter, but if we are covering both
> 
>  (1) the title we have here may be alright.

Yes, I figured so.  I actually considered just using the title
and didn't really feel the need to add a message body

>  (2) the proposed log message should state the change affects both
>      (in a good way).

Updated.

>  (3) the other half may want to be tested in new test as well.

OK, updated t5526, hope it's portable.  I mimicked the
formatting style of each respective test so the diff itself
looks odd between changes to t5514 and t5526 :x

> Thanks.

v2: revised commit message body, test submodules in t5526
---8<---
Subject: [PATCH] fetch: pass --no-write-fetch-head to subprocesses

It seems a user would expect this option would work regardless
of whether it's fetching from a single remote, many remotes,
or recursing into submodules.

Signed-off-by: Eric Wong <[e-mail]>
---
 builtin/fetch.c             |  2 ++
 t/t5514-fetch-multiple.sh   |  7 +++++++
 t/t5526-fetch-submodules.sh | 13 +++++++++++++
 3 files changed, 22 insertions(+)

Comments

Junio C Hamano March 8, 2023, 11:13 p.m. UTC | #1

Eric Wong <[e-mail]> writes:

> +test_expect_success 'git fetch --all --no-write-fetch-head' '
> +	(cd test &&
> +	rm -f .git/FETCH_HEAD &&
> +	git fetch --all --no-write-fetch-head &&
> +	test_path_is_missing .git/FETCH_HEAD)
> +'

The style used in the other script might be more modern, but given
that the existing one (in the post context) uses the same older
style, I think that would be OK.

>  test_expect_success 'git fetch --all should continue if a remote has errors' '
>  	(git clone one test2 &&
>  	 cd test2 &&
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index b9546ef8e5..8ffb300f2d 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -167,6 +167,19 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
>  	verify_fetch_result actual.err
>  '
>  
> +test_expect_success "fetch --recurse-submodules honors --no-write-fetch-head" '
> +	(
> +		cd downstream &&
> +		fh=$(find . -name FETCH_HEAD -type f) &&
> +		rm -f $fh &&

I do not like this part.  The "rm -f" we saw in the "fetch --all" test
was "make sure it is missing, so that we can be sure that presence
after running 'git fetch' *is* a bug".  But using $fh later ...

> +		git fetch --recurse-submodules --no-write-fetch-head &&
> +		for f in $fh
> +		do
> +			test_path_is_missing $f || return 1
> +		done

... like this means now we depend on FETCH_HEAD being in all
submodule repositories before we start this step.

I think we should instead enumerate submodule repositories, instead
of enumerating existing .git/FETCH_HEAD files.

> +	)
> +'
> +
>  test_expect_success "submodule.recurse option triggers recursive fetch" '
>  	add_submodule_commits &&
>  	(

Junio C Hamano March 8, 2023, 11:40 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> I think we should instead enumerate submodule repositories, instead
> of enumerating existing .git/FETCH_HEAD files.

Perhaps something along this line?

 t/t5526-fetch-submodules.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git c/t/t5526-fetch-submodules.sh w/t/t5526-fetch-submodules.sh
index 8ffb300f2d..dcdbe26a08 100755
--- c/t/t5526-fetch-submodules.sh
+++ w/t/t5526-fetch-submodules.sh
@@ -170,13 +170,13 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
 test_expect_success "fetch --recurse-submodules honors --no-write-fetch-head" '
 	(
 		cd downstream &&
-		fh=$(find . -name FETCH_HEAD -type f) &&
-		rm -f $fh &&
+		git submodule foreach --recursive \
+		sh -c "cd \"\$(git rev-parse --git-dir)\" && rm -f FETCH_HEAD" &&
+
 		git fetch --recurse-submodules --no-write-fetch-head &&
-		for f in $fh
-		do
-			test_path_is_missing $f || return 1
-		done
+
+		git submodule foreach --recursive \
+		sh -c "cd \"\$(git rev-parse --git-dir)\" && ! test -f FETCH_HEAD"
 	)
 '

Eric Wong March 8, 2023, 11:48 p.m. UTC | #3

Junio C Hamano <[e-mail]> wrote:
> Junio C Hamano <[e-mail]> writes:
> 
> > I think we should instead enumerate submodule repositories, instead
> > of enumerating existing .git/FETCH_HEAD files.
> 
> Perhaps something along this line?

Sure, can you squash it into mine?  Thanks.

Junio C Hamano March 9, 2023, 9:32 p.m. UTC | #4

Eric Wong <[e-mail]> writes:

> Junio C Hamano <[e-mail]> wrote:
>> Junio C Hamano <[e-mail]> writes:
>> 
>> > I think we should instead enumerate submodule repositories, instead
>> > of enumerating existing .git/FETCH_HEAD files.
>> 
>> Perhaps something along this line?
>
> Sure, can you squash it into mine?  Thanks.

Heh, I left it at "something along this line" because I didn't want
to debug it myself, or think about the longer-term ramifications
when the tests before this part eventually change in the future.

I've squashed it in and merged the result to 'next', together with a
few other topics.

Thanks.

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a09606b472..78513f1708 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1880,6 +1880,8 @@  static void add_options_to_argv(struct strvec *argv)
 		strvec_push(argv, "--ipv4");
 	else if (family == TRANSPORT_FAMILY_IPV6)
 		strvec_push(argv, "--ipv6");
+	if (!write_fetch_head)
+		strvec_push(argv, "--no-write-fetch-head");
 }
 
 /* Fetch multiple remotes in parallel */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index 54f422ced3..98f034aa77 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -58,6 +58,13 @@  test_expect_success 'git fetch --all' '
 	 test_cmp expect output)
 '
 
+test_expect_success 'git fetch --all --no-write-fetch-head' '
+	(cd test &&
+	rm -f .git/FETCH_HEAD &&
+	git fetch --all --no-write-fetch-head &&
+	test_path_is_missing .git/FETCH_HEAD)
+'
+
 test_expect_success 'git fetch --all should continue if a remote has errors' '
 	(git clone one test2 &&
 	 cd test2 &&
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index b9546ef8e5..8ffb300f2d 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -167,6 +167,19 @@  test_expect_success "fetch --recurse-submodules recurses into submodules" '
 	verify_fetch_result actual.err
 '
 
+test_expect_success "fetch --recurse-submodules honors --no-write-fetch-head" '
+	(
+		cd downstream &&
+		fh=$(find . -name FETCH_HEAD -type f) &&
+		rm -f $fh &&
+		git fetch --recurse-submodules --no-write-fetch-head &&
+		for f in $fh
+		do
+			test_path_is_missing $f || return 1
+		done
+	)
+'
+
 test_expect_success "submodule.recurse option triggers recursive fetch" '
 	add_submodule_commits &&
 	(