@matheusbsbs / matheus-bagliano / commits / 1c31764dda

fetch: print left-hand side when fetching HEAD:foo

`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 when displaying the updated references this leads to inconsistent output where the left-hand side of reference updates is missing in some cases: ``` $ git fetch origin HEAD HEAD:explicit-head :implicit-head main From https://github.com/git/git * branch HEAD -> FETCH_HEAD * [new ref] -> explicit-head * [new ref] -> implicit-head * branch main -> FETCH_HEAD ``` This behaviour has existed ever since the table-based output has been introduced for git-fetch(1) via 165f390250 (git-fetch: more terse fetch output, 2007-11-03) and was never explicitly documented either in the commit message or in any of our tests. So while it may not be a bug per se, it feels like a weird inconsistency and not like it was a concious design decision. 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 left-hand side to go missing, 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 thus fix the inconsistency 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" (which is the category like "branch" or "tag") and "what" (which is the prettified reference name like "master" or "v1.0") 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed May 10, 2023 at 14:34 UTC 1c31764dda4fd4aacdcc95c4a53b8c778a2f9de2

2 files changed +66 -18

builtin/fetch.c

+19 -18

@@ -920,12 +920,14 @@ static void display_ref_update(struct display_state *display_state, char code,

920 }

921

922 width = (summary_width + strlen(summary) - gettext_width(summary));

923 + remote = prettify_refname(remote);

924 + local = prettify_refname(local);

925

926 strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary);

927 if (!display_state->compact_format)

926 - print_remote_to_local(display_state, remote, prettify_refname(local));

928 + print_remote_to_local(display_state, remote, local);

929 else

928 - print_compact(display_state, remote, prettify_refname(local));

930 + print_compact(display_state, remote, local);

931 if (error)

932 strbuf_addf(&display_state->buf, " (%s)", error);

933 strbuf_addch(&display_state->buf, '\n');

@@ -936,7 +938,7 @@ static void display_ref_update(struct display_state *display_state, char code,

938 static int update_local_ref(struct ref *ref,

939 struct ref_transaction *transaction,

940 struct display_state *display_state,

939 - const char *remote, const struct ref *remote_ref,

941 + const struct ref *remote_ref,

942 int summary_width)

943 {

944 struct commit *current = NULL, *updated;

@@ -948,7 +950,7 @@ static int update_local_ref(struct ref *ref,

950 if (oideq(&ref->old_oid, &ref->new_oid)) {

951 if (verbosity > 0)

952 display_ref_update(display_state, '=', _("[up to date]"), NULL,

951 - remote, ref->name, summary_width);

953 + remote_ref->name, ref->name, summary_width);

954 return 0;

955 }

956

@@ -961,7 +963,7 @@ static int update_local_ref(struct ref *ref,

963 */

964 display_ref_update(display_state, '!', _("[rejected]"),

965 _("can't fetch into checked-out branch"),

964 - remote, ref->name, summary_width);

966 + remote_ref->name, ref->name, summary_width);

967 return 1;

968 }

969

@@ -972,12 +974,12 @@ static int update_local_ref(struct ref *ref,

974 r = s_update_ref("updating tag", ref, transaction, 0);

975 display_ref_update(display_state, r ? '!' : 't', _("[tag update]"),

976 r ? _("unable to update local ref") : NULL,

975 - remote, ref->name, summary_width);

977 + remote_ref->name, ref->name, summary_width);

978 return r;

979 } else {

980 display_ref_update(display_state, '!', _("[rejected]"),

981 _("would clobber existing tag"),

980 - remote, ref->name, summary_width);

982 + remote_ref->name, ref->name, summary_width);

983 return 1;

984 }

985 }

@@ -1010,7 +1012,7 @@ static int update_local_ref(struct ref *ref,

1012 r = s_update_ref(msg, ref, transaction, 0);

1013 display_ref_update(display_state, r ? '!' : '*', what,

1014 r ? _("unable to update local ref") : NULL,

1013 - remote, ref->name, summary_width);

1015 + remote_ref->name, ref->name, summary_width);

1016 return r;

1017 }

1018

@@ -1033,7 +1035,7 @@ static int update_local_ref(struct ref *ref,

1035 r = s_update_ref("fast-forward", ref, transaction, 1);

1036 display_ref_update(display_state, r ? '!' : ' ', quickref.buf,

1037 r ? _("unable to update local ref") : NULL,

1036 - remote, ref->name, summary_width);

1038 + remote_ref->name, ref->name, summary_width);

1039 strbuf_release(&quickref);

1040 return r;

1041 } else if (force || ref->force) {

@@ -1045,12 +1047,12 @@ static int update_local_ref(struct ref *ref,

1047 r = s_update_ref("forced-update", ref, transaction, 1);

1048 display_ref_update(display_state, r ? '!' : '+', quickref.buf,

1049 r ? _("unable to update local ref") : _("forced update"),

1048 - remote, ref->name, summary_width);

1050 + remote_ref->name, ref->name, summary_width);

1051 strbuf_release(&quickref);

1052 return r;

1053 } else {

1054 display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"),

1053 - remote, ref->name, summary_width);

1055 + remote_ref->name, ref->name, summary_width);

1056 return 1;

1057 }

1058 }

@@ -1255,14 +1257,13 @@ static int store_updated_refs(struct display_state *display_state,

1257 if (!strcmp(rm->name, "HEAD")) {

1258 kind = "";

1259 what = "";

1258 - }

1259 - else if (skip_prefix(rm->name, "refs/heads/", &what))

1260 + } else if (skip_prefix(rm->name, "refs/heads/", &what)) {

1261 kind = "branch";

1261 - else if (skip_prefix(rm->name, "refs/tags/", &what))

1262 + } else if (skip_prefix(rm->name, "refs/tags/", &what)) {

1263 kind = "tag";

1263 - else if (skip_prefix(rm->name, "refs/remotes/", &what))

1264 + } else if (skip_prefix(rm->name, "refs/remotes/", &what)) {

1265 kind = "remote-tracking branch";

1265 - else {

1266 + } else {

1267 kind = "";

1268 what = rm->name;

1269 }

@@ -1280,7 +1281,7 @@ static int store_updated_refs(struct display_state *display_state,

1281 display_state->url_len);

1282

1283 if (ref) {

1283 - rc |= update_local_ref(ref, transaction, display_state, what,

1284 + rc |= update_local_ref(ref, transaction, display_state,

1285 rm, summary_width);

1286 free(ref);

1287 } else if (write_fetch_head || dry_run) {

@@ -1291,7 +1292,7 @@ static int store_updated_refs(struct display_state *display_state,

1292 */

1293 display_ref_update(display_state, '*',

1294 *kind ? kind : "branch", NULL,

1294 - *what ? what : "HEAD",

1295 + rm->name,

1296 "FETCH_HEAD", summary_width);

1297 }

1298 }

t/t5574-fetch-output.sh

+47

@@ -61,6 +61,53 @@ test_expect_success 'fetch compact output' '

61 test_cmp expect actual

62 '

64 +test_expect_success 'fetch output with HEAD' '

65 + test_when_finished "rm -rf head" &&

66 + git clone . head &&

67 +

68 + git -C head fetch --dry-run origin HEAD >actual.out 2>actual.err &&

69 + cat >expect <<-EOF &&

70 + From $(test-tool path-utils real_path .)/.

71 + * branch HEAD -> FETCH_HEAD

72 + EOF

73 + test_must_be_empty actual.out &&

74 + test_cmp expect actual.err &&

75 +

76 + git -C head fetch origin HEAD >actual.out 2>actual.err &&

77 + test_must_be_empty actual.out &&

78 + test_cmp expect actual.err &&

79 +

80 + git -C head fetch --dry-run origin HEAD:foo >actual.out 2>actual.err &&

81 + cat >expect <<-EOF &&

82 + From $(test-tool path-utils real_path .)/.

83 + * [new ref] HEAD -> foo

84 + EOF

85 + test_must_be_empty actual.out &&

86 + test_cmp expect actual.err &&

87 +

88 + git -C head fetch origin HEAD:foo >actual.out 2>actual.err &&

89 + test_must_be_empty actual.out &&

90 + test_cmp expect actual.err

91 +'

92 +

93 +test_expect_success 'fetch output with object ID' '

94 + test_when_finished "rm -rf object-id" &&

95 + git clone . object-id &&

96 + commit=$(git rev-parse HEAD) &&

97 +

98 + git -C object-id fetch --dry-run origin $commit:object-id >actual.out 2>actual.err &&

99 + cat >expect <<-EOF &&

100 + From $(test-tool path-utils real_path .)/.

101 + * [new ref] $commit -> object-id

102 + EOF

103 + test_must_be_empty actual.out &&

104 + test_cmp expect actual.err &&

105 +

106 + git -C object-id fetch origin $commit:object-id >actual.out 2>actual.err &&

107 + test_must_be_empty actual.out &&

108 + test_cmp expect actual.err

109 +'

110 +

111 test_expect_success '--no-show-forced-updates' '

112 mkdir forced-updates &&

113 (