git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head
@ 2020-09-02 19:02 Jonathan Tan
  2020-09-02 20:07 ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Tan @ 2020-09-02 19:02 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, derrickstolee

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous. Therefore, suppress this message when
--no-write-fetch-head is passed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

This might be important for Stolee's maintenance prefetch patch [1] too
- presumably we don't want to show FETCH_HEAD there, as it would be
misleading and would clutter in the same way (albeit to a lesser
extent).

[1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/
---
 builtin/fetch.c          | 3 ++-
 t/t0410-partial-clone.sh | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..442df05f5a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (write_fetch_head) {
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
-- 
2.28.0.402.g5ffc5be6b7-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-02 19:02 [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head Jonathan Tan
@ 2020-09-02 20:07 ` Junio C Hamano
  2020-09-02 21:05   ` [PATCH v2] " Jonathan Tan
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2020-09-02 20:07 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, derrickstolee

Jonathan Tan <jonathantanmy@google.com> writes:

> 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD
> during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
> when this ability is used. This message is misleading in this case,
> because FETCH_HEAD is not written.

Thanks for noticing, but I wonder if we should keep this for users
of dry-run, which tends to give more output to what _would_ have
been done if it were not dry-run?

> This might be important for Stolee's maintenance prefetch patch [1] too
> - presumably we don't want to show FETCH_HEAD there, as it would be
> misleading and would clutter in the same way (albeit to a lesser
> extent).

Yes, that makes sense.

> Also, because "fetch" is used to
> lazy-fetch missing objects in a partial clone, this significantly
> clutters up the output in that case since the objects to be fetched are
> potentially numerous. Therefore, suppress this message when
> --no-write-fetch-head is passed.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This is on origin/jt/lazy-fetch.

I think this patch, with possible correction for "we are not writing
FETCH_HEAD but we do want to show what would have been done when the
user asked --dry-run", should be done as a brown-paper-bag bugfix
directly on jc/no-update-fetch-head topic and merged quicly down to
'master', instead of taken hostage of some other topic in flight,
but it would soon already be a week since jt/lazy-fetch was merged
to 'next', so it probably is OK to apply on jt/lazy-fetch, and it
indeed is easier to manage that way.

> [1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/
> ---
>  builtin/fetch.c          | 3 ++-
>  t/t0410-partial-clone.sh | 7 +++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 320ba9471d..442df05f5a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				rc |= update_local_ref(ref, what, rm, &note,
>  						       summary_width);
>  				free(ref);
> -			} else
> +			} else if (write_fetch_head) {
>  				format_display(&note, '*',
>  					       *kind ? kind : "branch", NULL,
>  					       *what ? what : "HEAD",
>  					       "FETCH_HEAD", summary_width);
> +			}
>  			if (note.len) {
>  				if (verbosity >= 0 && !shown_url) {
>  					fprintf(stderr, _("From %.*s\n"),
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index d681e90640..584a039b85 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
>  '
>  
>  test_expect_success 'fetching of missing objects' '
> -	rm -rf repo &&
> +	rm -rf repo err &&
>  	test_create_repo server &&
>  	test_commit -C server foo &&
>  	git -C server repack -a -d --write-bitmap-index &&
> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
>  
>  	git -C repo config core.repositoryformatversion 1 &&
>  	git -C repo config extensions.partialclone "origin" &&
> -	git -C repo cat-file -p "$HASH" &&
> +	git -C repo cat-file -p "$HASH" 2>err &&
> +
> +	# Ensure that no spurious FETCH_HEAD messages are written
> +	! grep FETCH_HEAD err &&

Test also --dry-run, but that perhaps needs to be done outside the
context of partial-clone.  The above "lazy fetching should be silent
and should not bother users with mention of FETCH_HEAD" is good test
in the context of partial-clone, though.

jc/no-update-fetch-head added its own test to t/t5510, and both the
"output lacks FETCH_HEAD when --no-write-fetch-head is given" test
and the "output still mentions FETCH_HEAD with --dry-run" test
belong there.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-02 20:07 ` Junio C Hamano
@ 2020-09-02 21:05   ` Jonathan Tan
  2020-09-02 21:27     ` Junio C Hamano
  2020-09-02 23:56     ` Jonathan Nieder
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Tan @ 2020-09-02 21:05 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous.

Therefore, suppress this message when --no-write-fetch-head is passed
(but not when --dry-run is set).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

Junio writes [1]:

> Test also --dry-run, but that perhaps needs to be done outside the
> context of partial-clone.  The above "lazy fetching should be silent
> and should not bother users with mention of FETCH_HEAD" is good test
> in the context of partial-clone, though.
>
> jc/no-update-fetch-head added its own test to t/t5510, and both the
> "output lacks FETCH_HEAD when --no-write-fetch-head is given" test
> and the "output still mentions FETCH_HEAD with --dry-run" test
> belong there.

Ah, thanks for catching that. Here's an updated version.

[1] https://lore.kernel.org/git/xmqq7dtcaqob.fsf@gitster.c.googlers.com/
---
 builtin/fetch.c          |  8 +++++++-
 t/t0410-partial-clone.sh |  7 +++++--
 t/t5510-fetch.sh         | 18 ++++++++++--------
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..c6c4689250 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (write_fetch_head || dry_run) {
+				/*
+				 * Display fetches written to FETCH_HEAD (or
+				 * would be written to FETCH_HEAD, if --dry-run
+				 * is set).
+				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 2a1abe91f0..759aec9305 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
-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 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	grep FETCH_HEAD err
 '
 
-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 '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
 '
 
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
-- 
2.28.0.402.g5ffc5be6b7-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-02 21:05   ` [PATCH v2] " Jonathan Tan
@ 2020-09-02 21:27     ` Junio C Hamano
  2020-09-02 23:56     ` Jonathan Nieder
  1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2020-09-02 21:27 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

Thanks; this round looks good.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-02 21:05   ` [PATCH v2] " Jonathan Tan
  2020-09-02 21:27     ` Junio C Hamano
@ 2020-09-02 23:56     ` Jonathan Nieder
  2020-09-03  2:03       ` Junio C Hamano
  1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Nieder @ 2020-09-02 23:56 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, gitster

Jonathan Tan wrote:

> 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
> 2020-08-18) introduced the ability to disable writing to FETCH_HEAD
> during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
> when this ability is used. This message is misleading in this case,
> because FETCH_HEAD is not written. Also, because "fetch" is used to
> lazy-fetch missing objects in a partial clone, this significantly
> clutters up the output in that case since the objects to be fetched are
> potentially numerous.
>
> Therefore, suppress this message when --no-write-fetch-head is passed
> (but not when --dry-run is set).
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
[...]
>  builtin/fetch.c          |  8 +++++++-
>  t/t0410-partial-clone.sh |  7 +++++--
>  t/t5510-fetch.sh         | 18 ++++++++++--------
>  3 files changed, 22 insertions(+), 11 deletions(-)

Thanks for fixing it, and sorry I didn't catch it during initial
review.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 320ba9471d..c6c4689250 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>  				rc |= update_local_ref(ref, what, rm, &note,
>  						       summary_width);
>  				free(ref);
> -			} else
> +			} else if (write_fetch_head || dry_run) {
> +				/*
> +				 * Display fetches written to FETCH_HEAD (or
> +				 * would be written to FETCH_HEAD, if --dry-run

nit: to fix the parallel construction, s/would/that would/ or
s/written/that were written/

> +				 * is set).
> +				 */
>  				format_display(&note, '*',
>  					       *kind ? kind : "branch", NULL,
>  					       *what ? what : "HEAD",
>  					       "FETCH_HEAD", summary_width);
> +			}
>  			if (note.len) {
>  				if (verbosity >= 0 && !shown_url) {
>  					fprintf(stderr, _("From %.*s\n"),
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index d681e90640..584a039b85 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
>  '
>  
>  test_expect_success 'fetching of missing objects' '
> -	rm -rf repo &&
> +	rm -rf repo err &&
>  	test_create_repo server &&
>  	test_commit -C server foo &&
>  	git -C server repack -a -d --write-bitmap-index &&
> @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
>  
>  	git -C repo config core.repositoryformatversion 1 &&
>  	git -C repo config extensions.partialclone "origin" &&
> -	git -C repo cat-file -p "$HASH" &&
> +	git -C repo cat-file -p "$HASH" 2>err &&
> +
> +	# Ensure that no spurious FETCH_HEAD messages are written
> +	! grep FETCH_HEAD err &&

This test feels very specific to the bug being addressed.  That said,
since we *also* have a test below for the generic behavior I don't mind.

>  
>  	# Ensure that the .promisor file is written, and check that its
>  	# associated packfile contains the object
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 2a1abe91f0..759aec9305 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
>  
>  '
>  
> -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 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
> +	rm -f .git/FETCH_HEAD err &&
> +	git fetch --dry-run . 2>err &&
> +	! test -f .git/FETCH_HEAD &&
> +	grep FETCH_HEAD err
>  '
>  
> -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 '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
> +	rm -f .git/FETCH_HEAD err &&
> +	git fetch --no-write-fetch-head . 2>err &&
> +	! test -f .git/FETCH_HEAD &&
> +	! grep FETCH_HEAD err

Nice.

With or without the following squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/builtin/fetch.c w/builtin/fetch.c
index c6c46892503..387eeb1ec08 100644
--- i/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -1025,9 +1025,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
-				 * Display fetches written to FETCH_HEAD (or
-				 * would be written to FETCH_HEAD, if --dry-run
-				 * is set).
+				 * Display fetches that wrote to FETCH_HEAD (or,
+				 * if --dry-run is set, that would write to
+				 * FETCH_HEAD).
 				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-02 23:56     ` Jonathan Nieder
@ 2020-09-03  2:03       ` Junio C Hamano
  2020-09-03 19:41         ` [PATCH v3] " Jonathan Tan
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2020-09-03  2:03 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Jonathan Tan, git

Jonathan Nieder <jrnieder@gmail.com> writes:

>>  builtin/fetch.c          |  8 +++++++-
>>  t/t0410-partial-clone.sh |  7 +++++--
>>  t/t5510-fetch.sh         | 18 ++++++++++--------
>>  3 files changed, 22 insertions(+), 11 deletions(-)
>
> Thanks for fixing it, and sorry I didn't catch it during initial
> review.
> ...
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 320ba9471d..c6c4689250 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>>  				rc |= update_local_ref(ref, what, rm, &note,
>>  						       summary_width);
>>  				free(ref);
>> -			} else
>> +			} else if (write_fetch_head || dry_run) {
>> +				/*
>> +				 * Display fetches written to FETCH_HEAD (or
>> +				 * would be written to FETCH_HEAD, if --dry-run
>
> nit: to fix the parallel construction, s/would/that would/ or
> s/written/that were written/

True.

>> +				 * is set).
>> +				 */
>>  				format_display(&note, '*',
>>  					       *kind ? kind : "branch", NULL,
>>  					       *what ? what : "HEAD",
>>  					       "FETCH_HEAD", summary_width);
>> +			}

Strictly speaking, I suspect that this is still broken when the user
says "fetch --no-write-fetch-head --dry-run" in which case we should
skip this block.

And to fix it properly, we would probably need to keep track of
three things semi-independently.

 - were we told this is a "dry-run"? (current 'dry_run' variable)

 - were we told not to store fetch-head? (missing)

 - after all, are we going to write or not write fetch-head (current
   'write_fetch_head' variable)

And the conditional to protect this block would be fixed to use only
the second and new "have we seen --no-fetch-head on the command
line?" variable, and ignore the settings of the dry_run variable, I
think.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-03  2:03       ` Junio C Hamano
@ 2020-09-03 19:41         ` Jonathan Tan
  2020-09-03 21:00           ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Tan @ 2020-09-03 19:41 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster, jrnieder

887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update",
2020-08-18) introduced the ability to disable writing to FETCH_HEAD
during fetch, but did not suppress the "<source> -> FETCH_HEAD" message
when this ability is used. This message is misleading in this case,
because FETCH_HEAD is not written. Also, because "fetch" is used to
lazy-fetch missing objects in a partial clone, this significantly
clutters up the output in that case since the objects to be fetched are
potentially numerous.

Therefore, suppress this message when --no-write-fetch-head is passed.
In order to do this, a new variable that keeps track of what the user
said explicitly about FETCH_HEAD (which may or may not have the same
value as the existing one tracking whether FETCH_HEAD is written) is
introduced.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on origin/jt/lazy-fetch.

OK - updated the code, added a test for the "--dry-run
--no-write-fetch-head" case, and updated commit message and code
comment.
---
 builtin/fetch.c          | 21 +++++++++++++++++----
 t/t0410-partial-clone.sh |  7 +++++--
 t/t5510-fetch.sh         | 25 +++++++++++++++++--------
 3 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 320ba9471d..9addd1f2d4 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 user_specified_write_fetch_head = 1;
 static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream;
 static int progress = -1;
@@ -164,7 +165,7 @@ 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,
+	OPT_BOOL(0, "write-fetch-head", &user_specified_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,
@@ -1023,11 +1024,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else
+			} else if (user_specified_write_fetch_head) {
+				/*
+				 * If the user specified --write-fetch-head
+				 * (or, equivalently, did not specify
+				 * --no-write-fetch-head), inform the user that
+				 * this ref was written to FETCH_HEAD (or, if
+				 * --dry-run was specified, would have been
+				 *  written).
+				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
 					       "FETCH_HEAD", summary_width);
+			}
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),
@@ -1822,8 +1832,11 @@ 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)
+	/*
+	 * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed
+	 * --no-write-fetch-head
+	 */
+	if (dry_run || !user_specified_write_fetch_head)
 		write_fetch_head = 0;
 
 	if (all) {
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index d681e90640..584a039b85 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 '
 
 test_expect_success 'fetching of missing objects' '
-	rm -rf repo &&
+	rm -rf repo err &&
 	test_create_repo server &&
 	test_commit -C server foo &&
 	git -C server repack -a -d --write-bitmap-index &&
@@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&
-	git -C repo cat-file -p "$HASH" &&
+	git -C repo cat-file -p "$HASH" 2>err &&
+
+	# Ensure that no spurious FETCH_HEAD messages are written
+	! grep FETCH_HEAD err &&
 
 	# Ensure that the .promisor file is written, and check that its
 	# associated packfile contains the object
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 2a1abe91f0..83b2504519 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -543,16 +543,25 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
-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 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	grep FETCH_HEAD err
 '
 
-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 '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
+'
+
+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
 '
 
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
-- 
2.28.0.526.ge36021eeef-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-03 19:41         ` [PATCH v3] " Jonathan Tan
@ 2020-09-03 21:00           ` Junio C Hamano
  2020-09-03 21:06             ` Jonathan Tan
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2020-09-03 21:00 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, jrnieder

Jonathan Tan <jonathantanmy@google.com> writes:

> OK - updated the code, added a test for the "--dry-run
> --no-write-fetch-head" case, and updated commit message and code
> comment.

Unfortunately our actions crossed X-< and the previous one that was
good enough is already in 'master', together with the lazy fetch
topic.

Let's turn this into an incremental fix only for "Ouch, we still say
FETCH_HEAD when both --dry-run and --no-write-fetch-head are given"
bug.

Thanks.

-- >8 --
Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction
From: Jonathan Tan <jonathantanmy@google.com>

Recently we introduced "--[no-]-write-fetch-head" option to tell
"git fetch" not to write FETCH_HEAD file.  The command reported that
FETCH_HEAD file is written, even with the "--no-write-fetch-head"
option.

db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,
2020-09-02) tried to squelch this, but the fix was not sufficient.
Because we never write the FETCH_HEAD file when "--dry-run" is
given, the addition of "--[no-]write-fetch-head" option was made by
directly fliping the internal variable 'write_fetch_head' (which
defaults to 'on') to 'off' upon seeing "--dry-run", which allowed
the condition to decide if we write FETCH_HEAD to be a simple
reference to the variable.  But now, we need to tell if the user
explicitly asked "--no-write-fetch-head" with "--dry-run" to
decide when to show the report about FETCH_HEAD correctly.

Introduce an extra 'user_specified_write_fetch_head' variable, which
is 'on' by default and is turned 'off' with '--no-write-fetch-head'.
The 'write_fetch_head' variable that decides if we actually write
FETCH_HEAD remains there, retaining its meaning, but use this new
variable and 'dry_run' to decide if we report about FETCH_HEAD.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

 builtin/fetch.c  | 21 ++++++++++++++-------
 t/t5510-fetch.sh |  7 +++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git c/builtin/fetch.c w/builtin/fetch.c
index c6c4689250..9addd1f2d4 100644
--- c/builtin/fetch.c
+++ w/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 user_specified_write_fetch_head = 1;
 static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream;
 static int progress = -1;
@@ -164,7 +165,7 @@ 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,
+	OPT_BOOL(0, "write-fetch-head", &user_specified_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,
@@ -1023,11 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				rc |= update_local_ref(ref, what, rm, &note,
 						       summary_width);
 				free(ref);
-			} else if (write_fetch_head || dry_run) {
+			} else if (user_specified_write_fetch_head) {
 				/*
-				 * Display fetches written to FETCH_HEAD (or
-				 * would be written to FETCH_HEAD, if --dry-run
-				 * is set).
+				 * If the user specified --write-fetch-head
+				 * (or, equivalently, did not specify
+				 * --no-write-fetch-head), inform the user that
+				 * this ref was written to FETCH_HEAD (or, if
+				 * --dry-run was specified, would have been
+				 *  written).
 				 */
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
@@ -1828,8 +1832,11 @@ 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)
+	/*
+	 * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed
+	 * --no-write-fetch-head
+	 */
+	if (dry_run || !user_specified_write_fetch_head)
 		write_fetch_head = 0;
 
 	if (all) {
diff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh
index 759aec9305..83b2504519 100755
--- c/t/t5510-fetch.sh
+++ w/t/t5510-fetch.sh
@@ -557,6 +557,13 @@ test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does n
 	! grep FETCH_HEAD err
 '
 
+test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' '
+	rm -f .git/FETCH_HEAD err &&
+	git fetch --dry-run --no-write-fetch-head . 2>err &&
+	! test -f .git/FETCH_HEAD &&
+	! grep FETCH_HEAD err
+'
+
 test_expect_success '--write-fetch-head gets defeated by --dry-run' '
 	rm -f .git/FETCH_HEAD &&
 	git fetch --dry-run --write-fetch-head . &&

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head
  2020-09-03 21:00           ` Junio C Hamano
@ 2020-09-03 21:06             ` Jonathan Tan
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Tan @ 2020-09-03 21:06 UTC (permalink / raw)
  To: gitster; +Cc: jonathantanmy, git, jrnieder

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > OK - updated the code, added a test for the "--dry-run
> > --no-write-fetch-head" case, and updated commit message and code
> > comment.
> 
> Unfortunately our actions crossed X-< and the previous one that was
> good enough is already in 'master', together with the lazy fetch
> topic.
> 
> Let's turn this into an incremental fix only for "Ouch, we still say
> FETCH_HEAD when both --dry-run and --no-write-fetch-head are given"
> bug.
> 
> Thanks.
> 
> -- >8 --
> Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction
> From: Jonathan Tan <jonathantanmy@google.com>
> 
> Recently we introduced "--[no-]-write-fetch-head" option to tell
> "git fetch" not to write FETCH_HEAD file.  The command reported that
> FETCH_HEAD file is written, even with the "--no-write-fetch-head"
> option.
> 
> db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head,
> 2020-09-02) tried to squelch this, but the fix was not sufficient.
> Because we never write the FETCH_HEAD file when "--dry-run" is
> given, the addition of "--[no-]write-fetch-head" option was made by
> directly fliping the internal variable 'write_fetch_head' (which
> defaults to 'on') to 'off' upon seeing "--dry-run", which allowed
> the condition to decide if we write FETCH_HEAD to be a simple
> reference to the variable.  But now, we need to tell if the user
> explicitly asked "--no-write-fetch-head" with "--dry-run" to
> decide when to show the report about FETCH_HEAD correctly.
> 
> Introduce an extra 'user_specified_write_fetch_head' variable, which
> is 'on' by default and is turned 'off' with '--no-write-fetch-head'.
> The 'write_fetch_head' variable that decides if we actually write
> FETCH_HEAD remains there, retaining its meaning, but use this new
> variable and 'dry_run' to decide if we report about FETCH_HEAD.
> 
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

Thanks - this looks good.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-09-03 21:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02 19:02 [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head Jonathan Tan
2020-09-02 20:07 ` Junio C Hamano
2020-09-02 21:05   ` [PATCH v2] " Jonathan Tan
2020-09-02 21:27     ` Junio C Hamano
2020-09-02 23:56     ` Jonathan Nieder
2020-09-03  2:03       ` Junio C Hamano
2020-09-03 19:41         ` [PATCH v3] " Jonathan Tan
2020-09-03 21:00           ` Junio C Hamano
2020-09-03 21:06             ` Jonathan Tan

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git