diff mbox series

[v2,3/8] fetch: fix missing from-reference when fetching HEAD:foo

Message ID d1fb6eeae76bc550030475e40868b90a2ca6ce8c.1682593865.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series fetch: introduce machine-parseable output | expand

Commit Message

Patrick Steinhardt April 27, 2023, 11:13 a.m. UTC
`store_updated_refs()` parses the remote reference for two purposes:

    - It gets used as a note when writing FETCH_HEAD.

    - It is passed through to `display_ref_update()` to display
      updated references in the following format:

      ```
       * branch               master          -> master
      ```

In most cases, the parsed remote reference is the prettified reference
name and can thus be used for both cases. But if the remote reference is
HEAD, the parsed remote reference becomes empty. This is intended when
we write the FETCH_HEAD, where we skip writing the note in that case.
But it is not intended when displaying the updated references and would
cause us to miss the left-hand side of the displayed reference update:

```
$ git fetch origin HEAD:foo
From https://github.com/git/git
 * [new ref]                          -> foo
```

The HEAD string is clearly missing from the left-hand side of the arrow,
which is further stressed by the point that the following commands show
the left-hand side as expected:

```
$ git fetch origin HEAD
From https://github.com/git/git
 * branch                  HEAD       -> FETCH_HEAD

$ git fetch origin master
From https://github.com/git/git
 * branch                  master     -> FETCH_HEAD
 * branch                  master     -> origin/master
```

The logic of how we compute the remote reference name that we ultimately
pass to `display_ref_update()` is not easy to follow. There are three
different cases here:

    - When the remote reference name is "HEAD" we set the remote
      reference name to the empty string. This is the case that causes
      the bug to occur, where we would indeed want to print "HEAD"
      instead of the empty string. This is what `prettify_refname()`
      would return.

    - When the remote reference name has a well-known prefix then we
      strip this prefix. This matches what `prettify_refname()` does.

    - Otherwise, we keep the fully qualified reference name. This also
      matches what `prettify_refname()` does.

As the return value of `prettify_refname()` would do the correct thing
for us in all three cases, we can fix the bug by passing through the
full remote reference name to `display_ref_update()`, which learns to
call `prettify_refname()`. At the same time, this also simplifies the
code a bit.

Note that this patch also changes formatting of the block that computes
the "kind" and "what" variables. This is done on purpose so that it is
part of the diff, hopefully making the change easier to comprehend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c         | 37 +++++++++++++++++++------------------
 t/t5574-fetch-output.sh | 25 +++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 18 deletions(-)

Comments

Glen Choo April 27, 2023, 5:26 p.m. UTC | #1
For v2, I've only read through this patch for now. I'm still formulating
my thoughts on the output format and the CLI option name.

Patrick Steinhardt <ps@pks.im> writes:

> `store_updated_refs()` parses the remote reference for two purposes:
>
>     - It gets used as a note when writing FETCH_HEAD.
>
>     - It is passed through to `display_ref_update()` to display
>       updated references in the following format:
>
>       ```
>        * branch               master          -> master
>       ```
>
> In most cases, the parsed remote reference is the prettified reference
> name and can thus be used for both cases. But if the remote reference is
> HEAD, the parsed remote reference becomes empty. This is intended when
> we write the FETCH_HEAD, where we skip writing the note in that case.
> But it is not intended when displaying the updated references and would
> cause us to miss the left-hand side of the displayed reference update:
>
> [...]
>
> As the return value of `prettify_refname()` would do the correct thing
> for us in all three cases, we can fix the bug by passing through the
> full remote reference name to `display_ref_update()`, which learns to
> call `prettify_refname()`. At the same time, this also simplifies the
> code a bit.

This rewritten section explains the bug and the fix very nicely :)
Thanks!

> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> index 0e45c27007..b9dcdade63 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -54,6 +54,31 @@ test_expect_success 'fetch compact output' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'fetch output with HEAD and --dry-run' '
> +	test_when_finished "rm -rf head" &&
> +	git clone . head &&
> +
> +	git -C head fetch --dry-run origin HEAD >actual 2>&1 &&
> +	cat >expect <<-EOF &&
> +	From $(test-tool path-utils real_path .)/.
> +	 * branch            HEAD       -> FETCH_HEAD
> +	EOF
> +	test_cmp expect actual &&
> +
> +	git -C head fetch origin HEAD >actual 2>&1 &&
> +	test_cmp expect actual &&

Maybe rename the test to just 'fetch output with HEAD'? The 'and
--dry-run' part seems completely obsolete.
Jacob Keller April 27, 2023, 7:49 p.m. UTC | #2
On 4/27/2023 4:13 AM, Patrick Steinhardt wrote:
> index 0e45c27007..b9dcdade63 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -54,6 +54,31 @@ test_expect_success 'fetch compact output' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'fetch output with HEAD and --dry-run' '
> +	test_when_finished "rm -rf head" &&

The test name could drop the --dry-run mention since this now tests both
variants.


Everything else looks good on this patch. The new commit message is much
better to me now!
diff mbox series

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c310d89878..7c64f0c562 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -918,12 +918,14 @@  static void display_ref_update(struct display_state *display_state, char code,
 	}
 
 	width = (summary_width + strlen(summary) - gettext_width(summary));
+	remote = prettify_refname(remote);
+	local = prettify_refname(local);
 
 	strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary);
 	if (!display_state->compact_format)
-		print_remote_to_local(display_state, remote, prettify_refname(local));
+		print_remote_to_local(display_state, remote, local);
 	else
-		print_compact(display_state, remote, prettify_refname(local));
+		print_compact(display_state, remote, local);
 	if (error)
 		strbuf_addf(&display_state->buf, "  (%s)", error);
 	strbuf_addch(&display_state->buf, '\n');
@@ -934,7 +936,7 @@  static void display_ref_update(struct display_state *display_state, char code,
 static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    struct display_state *display_state,
-			    const char *remote, const struct ref *remote_ref,
+			    const struct ref *remote_ref,
 			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
@@ -946,7 +948,7 @@  static int update_local_ref(struct ref *ref,
 	if (oideq(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
 			display_ref_update(display_state, '=', _("[up to date]"), NULL,
-					   remote, ref->name, summary_width);
+					   remote_ref->name, ref->name, summary_width);
 		return 0;
 	}
 
@@ -959,7 +961,7 @@  static int update_local_ref(struct ref *ref,
 		 */
 		display_ref_update(display_state, '!', _("[rejected]"),
 				   _("can't fetch into checked-out branch"),
-				   remote, ref->name, summary_width);
+				   remote_ref->name, ref->name, summary_width);
 		return 1;
 	}
 
@@ -970,12 +972,12 @@  static int update_local_ref(struct ref *ref,
 			r = s_update_ref("updating tag", ref, transaction, 0);
 			display_ref_update(display_state, r ? '!' : 't', _("[tag update]"),
 					   r ? _("unable to update local ref") : NULL,
-					   remote, ref->name, summary_width);
+					   remote_ref->name, ref->name, summary_width);
 			return r;
 		} else {
 			display_ref_update(display_state, '!', _("[rejected]"),
 					   _("would clobber existing tag"),
-					   remote, ref->name, summary_width);
+					   remote_ref->name, ref->name, summary_width);
 			return 1;
 		}
 	}
@@ -1008,7 +1010,7 @@  static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, transaction, 0);
 		display_ref_update(display_state, r ? '!' : '*', what,
 				   r ? _("unable to update local ref") : NULL,
-				   remote, ref->name, summary_width);
+				   remote_ref->name, ref->name, summary_width);
 		return r;
 	}
 
@@ -1030,7 +1032,7 @@  static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, transaction, 1);
 		display_ref_update(display_state, r ? '!' : ' ', quickref.buf,
 				   r ? _("unable to update local ref") : NULL,
-				   remote, ref->name, summary_width);
+				   remote_ref->name, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {
@@ -1042,12 +1044,12 @@  static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, transaction, 1);
 		display_ref_update(display_state, r ? '!' : '+', quickref.buf,
 				   r ? _("unable to update local ref") : _("forced update"),
-				   remote, ref->name, summary_width);
+				   remote_ref->name, ref->name, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
 		display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"),
-				   remote, ref->name, summary_width);
+				   remote_ref->name, ref->name, summary_width);
 		return 1;
 	}
 }
@@ -1252,14 +1254,13 @@  static int store_updated_refs(struct display_state *display_state,
 			if (!strcmp(rm->name, "HEAD")) {
 				kind = "";
 				what = "";
-			}
-			else if (skip_prefix(rm->name, "refs/heads/", &what))
+			} else if (skip_prefix(rm->name, "refs/heads/", &what)) {
 				kind = "branch";
-			else if (skip_prefix(rm->name, "refs/tags/", &what))
+			} else if (skip_prefix(rm->name, "refs/tags/", &what)) {
 				kind = "tag";
-			else if (skip_prefix(rm->name, "refs/remotes/", &what))
+			} else if (skip_prefix(rm->name, "refs/remotes/", &what)) {
 				kind = "remote-tracking branch";
-			else {
+			} else {
 				kind = "";
 				what = rm->name;
 			}
@@ -1277,7 +1278,7 @@  static int store_updated_refs(struct display_state *display_state,
 					  display_state->url_len);
 
 			if (ref) {
-				rc |= update_local_ref(ref, transaction, display_state, what,
+				rc |= update_local_ref(ref, transaction, display_state,
 						       rm, summary_width);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
@@ -1288,7 +1289,7 @@  static int store_updated_refs(struct display_state *display_state,
 				 */
 				display_ref_update(display_state, '*',
 						   *kind ? kind : "branch", NULL,
-						   *what ? what : "HEAD",
+						   rm->name,
 						   "FETCH_HEAD", summary_width);
 			}
 		}
diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 0e45c27007..b9dcdade63 100755
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -54,6 +54,31 @@  test_expect_success 'fetch compact output' '
 	test_cmp expect actual
 '
 
+test_expect_success 'fetch output with HEAD and --dry-run' '
+	test_when_finished "rm -rf head" &&
+	git clone . head &&
+
+	git -C head fetch --dry-run origin HEAD >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	From $(test-tool path-utils real_path .)/.
+	 * branch            HEAD       -> FETCH_HEAD
+	EOF
+	test_cmp expect actual &&
+
+	git -C head fetch origin HEAD >actual 2>&1 &&
+	test_cmp expect actual &&
+
+	git -C head fetch --dry-run origin HEAD:foo >actual 2>&1 &&
+	cat >expect <<-EOF &&
+	From $(test-tool path-utils real_path .)/.
+	 * [new ref]         HEAD       -> foo
+	EOF
+	test_cmp expect actual &&
+
+	git -C head fetch origin HEAD:foo >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success '--no-show-forced-updates' '
 	mkdir forced-updates &&
 	(