From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Jonathan Tan <jonathantanmy@google.com>
Subject: [PATCH v2 0/6] fetch: refactor code that prints reference updates
Date: Mon, 20 Mar 2023 13:35:16 +0100 [thread overview]
Message-ID: <cover.1679315383.git.ps@pks.im> (raw)
In-Reply-To: <cover.1678878623.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 41718 bytes --]
Hi,
This is the second version of my patch series to refactor the code that
prints reference updates during git-fetch(1). Its aim is it to make the
code more self-contained so that it can be easily amended for a new
machine-parseable format.
Changes compared to v1:
- I've dropped the first patch to rename the preexisting `display`
buffer variable. Instead, I'm now just picking the longer
`display_state` variable name for the newly introduced structure
in order to avoid naming conflicts.
- I've touched up the commit messages of patches 4-6 to hopefully
clarify their intent a little bit better.
- I've dropped patch 7/8 that unified the logic to calculate the
summary width. Even though it fixes a bug in one case, Jonathan
correctly pointed out that it's weird in the case where there are
only reference deletions without any reference updates. I may have
another go in a separate patch series after the dust has settled.
Thanks for the feedback so far!
Patrick
Patrick Steinhardt (6):
fetch: move reference width calculation into `display_state`
fetch: move output format into `display_state`
fetch: pass the full local reference name to `format_display`
fetch: centralize handling of per-reference format
fetch: centralize logic to print remote URL
fetch: centralize printing of reference updates
builtin/fetch.c | 267 +++++++++++++++++++++++++-----------------------
1 file changed, 138 insertions(+), 129 deletions(-)
Range-diff against v1:
1: 692206f7ff < -: ---------- fetch: rename `display` buffer to avoid name conflict
2: aa792b12a4 ! 1: ce2c4b61ae fetch: move reference width calculation into `display_state`
@@ builtin/fetch.c: static void adjust_refcol_width(const struct ref *ref)
}
-static void prepare_format_display(struct ref *ref_map)
-+static void display_state_init(struct display_state *display, struct ref *ref_map)
++static void display_state_init(struct display_state *display_state, struct ref *ref_map)
{
struct ref *rm;
const char *format = "full";
-+ memset(display, 0, sizeof(*display));
++ memset(display_state, 0, sizeof(*display_state));
+
if (verbosity < 0)
return;
@@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
die(_("invalid value for '%s': '%s'"),
"fetch.output", format);
-+ display->refcol_width = 10;
++ display_state->refcol_width = 10;
for (rm = ref_map; rm; rm = rm->next) {
+ int width;
+
@@ builtin/fetch.c: static void prepare_format_display(struct ref *ref_map)
+ * appear on the left hand side of '->' and shrink the column
+ * back.
+ */
-+ if (display->refcol_width < width)
-+ display->refcol_width = width;
++ if (display_state->refcol_width < width)
++ display_state->refcol_width = width;
}
}
--static void print_remote_to_local(struct strbuf *display_buffer,
-+static void print_remote_to_local(struct display_state *display,
+-static void print_remote_to_local(struct strbuf *display,
++static void print_remote_to_local(struct display_state *display_state,
+ struct strbuf *display_buffer,
const char *remote, const char *local)
{
-- strbuf_addf(display_buffer, "%-*s -> %s", refcol_width, remote, local);
-+ strbuf_addf(display_buffer, "%-*s -> %s", display->refcol_width, remote, local);
+- strbuf_addf(display, "%-*s -> %s", refcol_width, remote, local);
++ strbuf_addf(display_buffer, "%-*s -> %s", display_state->refcol_width, remote, local);
}
static int find_and_replace(struct strbuf *haystack,
@@ builtin/fetch.c: static int find_and_replace(struct strbuf *haystack,
return 1;
}
--static void print_compact(struct strbuf *display_buffer,
-+static void print_compact(struct display_state *display, struct strbuf *display_buffer,
+-static void print_compact(struct strbuf *display,
++static void print_compact(struct display_state *display_state, struct strbuf *display_buffer,
const char *remote, const char *local)
{
struct strbuf r = STRBUF_INIT;
struct strbuf l = STRBUF_INIT;
if (!strcmp(remote, local)) {
-- strbuf_addf(display_buffer, "%-*s -> *", refcol_width, remote);
-+ strbuf_addf(display_buffer, "%-*s -> *", display->refcol_width, remote);
+- strbuf_addf(display, "%-*s -> *", refcol_width, remote);
++ strbuf_addf(display_buffer, "%-*s -> *", display_state->refcol_width, remote);
return;
}
-@@ builtin/fetch.c: static void print_compact(struct strbuf *display_buffer,
+@@ builtin/fetch.c: static void print_compact(struct strbuf *display,
if (!find_and_replace(&r, local, "*"))
find_and_replace(&l, remote, "*");
-- print_remote_to_local(display_buffer, r.buf, l.buf);
-+ print_remote_to_local(display, display_buffer, r.buf, l.buf);
+- print_remote_to_local(display, r.buf, l.buf);
++ print_remote_to_local(display_state, display_buffer, r.buf, l.buf);
strbuf_release(&r);
strbuf_release(&l);
}
--static void format_display(struct strbuf *display_buffer, char code,
-+static void format_display(struct display_state *display,
+-static void format_display(struct strbuf *display, char code,
++static void format_display(struct display_state *display_state,
+ struct strbuf *display_buffer, char code,
const char *summary, const char *error,
const char *remote, const char *local,
int summary_width)
-@@ builtin/fetch.c: static void format_display(struct strbuf *display_buffer, char code,
+@@ builtin/fetch.c: static void format_display(struct strbuf *display, char code,
- strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
+ width = (summary_width + strlen(summary) - gettext_width(summary));
+
+- strbuf_addf(display, "%c %-*s ", code, width, summary);
++ strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
if (!compact_format)
-- print_remote_to_local(display_buffer, remote, local);
-+ print_remote_to_local(display, display_buffer, remote, local);
+- print_remote_to_local(display, remote, local);
++ print_remote_to_local(display_state, display_buffer, remote, local);
else
-- print_compact(display_buffer, remote, local);
-+ print_compact(display, display_buffer, remote, local);
+- print_compact(display, remote, local);
++ print_compact(display_state, display_buffer, remote, local);
if (error)
- strbuf_addf(display_buffer, " (%s)", error);
+- strbuf_addf(display, " (%s)", error);
++ strbuf_addf(display_buffer, " (%s)", error);
}
static int update_local_ref(struct ref *ref,
struct ref_transaction *transaction,
-+ struct display_state *display,
++ struct display_state *display_state,
const char *remote, const struct ref *remote_ref,
- struct strbuf *display_buffer, int summary_width)
+ struct strbuf *display, int summary_width)
{
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
if (oideq(&ref->old_oid, &ref->new_oid)) {
if (verbosity > 0)
-- format_display(display_buffer, '=', _("[up to date]"), NULL,
-+ format_display(display, display_buffer, '=', _("[up to date]"), NULL,
+- format_display(display, '=', _("[up to date]"), NULL,
++ format_display(display_state, display, '=', _("[up to date]"), NULL,
remote, pretty_ref, summary_width);
return 0;
}
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
-- format_display(display_buffer, '!', _("[rejected]"),
-+ format_display(display, display_buffer, '!', _("[rejected]"),
+- format_display(display, '!', _("[rejected]"),
++ format_display(display_state, display, '!', _("[rejected]"),
_("can't fetch into checked-out branch"),
remote, pretty_ref, summary_width);
return 1;
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
if (force || ref->force) {
int r;
r = s_update_ref("updating tag", ref, transaction, 0);
-- format_display(display_buffer, r ? '!' : 't', _("[tag update]"),
-+ format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
+- format_display(display, r ? '!' : 't', _("[tag update]"),
++ format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
return r;
} else {
-- format_display(display_buffer, '!', _("[rejected]"), _("would clobber existing tag"),
-+ format_display(display, display_buffer, '!', _("[rejected]"),
+- format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
++ format_display(display_state, display, '!', _("[rejected]"),
+ _("would clobber existing tag"),
remote, pretty_ref, summary_width);
return 1;
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
}
r = s_update_ref(msg, ref, transaction, 0);
-- format_display(display_buffer, r ? '!' : '*', what,
-+ format_display(display, display_buffer, r ? '!' : '*', what,
+- format_display(display, r ? '!' : '*', what,
++ format_display(display_state, display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
return r;
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
strbuf_addstr(&quickref, "..");
strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
r = s_update_ref("fast-forward", ref, transaction, 1);
-- format_display(display_buffer, r ? '!' : ' ', quickref.buf,
-+ format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
+- format_display(display, r ? '!' : ' ', quickref.buf,
++ format_display(display_state, display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
strbuf_release(&quickref);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
strbuf_addstr(&quickref, "...");
strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
r = s_update_ref("forced-update", ref, transaction, 1);
-- format_display(display_buffer, r ? '!' : '+', quickref.buf,
-+ format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
+- format_display(display, r ? '!' : '+', quickref.buf,
++ format_display(display_state, display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced update"),
remote, pretty_ref, summary_width);
strbuf_release(&quickref);
return r;
} else {
-- format_display(display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
-+ format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
+- format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
++ format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
remote, pretty_ref, summary_width);
return 1;
}
@@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n
"to avoid this check\n");
-static int store_updated_refs(const char *raw_url, const char *remote_name,
-+static int store_updated_refs(struct display_state *display,
++static int store_updated_refs(struct display_state *display_state,
+ const char *raw_url, const char *remote_name,
int connectivity_checked,
struct ref_transaction *transaction, struct ref *ref_map,
@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
strbuf_reset(¬e);
if (ref) {
- rc |= update_local_ref(ref, transaction, what,
-+ rc |= update_local_ref(ref, transaction, display, what,
++ rc |= update_local_ref(ref, transaction, display_state, what,
rm, ¬e, summary_width);
free(ref);
} else if (write_fetch_head || dry_run) {
@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *
* is set).
*/
- format_display(¬e, '*',
-+ format_display(display, ¬e, '*',
++ format_display(display_state, ¬e, '*',
*kind ? kind : "branch", NULL,
*what ? what : "HEAD",
"FETCH_HEAD", summary_width);
@@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)
}
-static int fetch_and_consume_refs(struct transport *transport,
-+static int fetch_and_consume_refs(struct display_state *display,
++static int fetch_and_consume_refs(struct display_state *display_state,
+ struct transport *transport,
struct ref_transaction *transaction,
struct ref *ref_map,
@@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,
trace2_region_enter("fetch", "consume_refs", the_repository);
- ret = store_updated_refs(transport->url, transport->remote->name,
-+ ret = store_updated_refs(display, transport->url, transport->remote->name,
++ ret = store_updated_refs(display_state, transport->url, transport->remote->name,
connectivity_checked, transaction, ref_map,
fetch_head);
trace2_region_leave("fetch", "consume_refs", the_repository);
@@ builtin/fetch.c: static int fetch_and_consume_refs(struct transport *transport,
}
-static int prune_refs(struct refspec *rs,
-+static int prune_refs(struct display_state *display,
++static int prune_refs(struct display_state *display_state,
+ struct refspec *rs,
struct ref_transaction *transaction,
struct ref *ref_map,
@@ builtin/fetch.c: static int prune_refs(struct refspec *rs,
shown_url = 1;
}
- format_display(&sb, '-', _("[deleted]"), NULL,
-+ format_display(display, &sb, '-', _("[deleted]"), NULL,
++ format_display(display_state, &sb, '-', _("[deleted]"), NULL,
_("(none)"), prettify_refname(ref->name),
summary_width);
fprintf(stderr, " %s\n",sb.buf);
@@ builtin/fetch.c: static struct transport *prepare_transport(struct remote *remot
}
-static int backfill_tags(struct transport *transport,
-+static int backfill_tags(struct display_state *display,
++static int backfill_tags(struct display_state *display_state,
+ struct transport *transport,
struct ref_transaction *transaction,
struct ref *ref_map,
@@ builtin/fetch.c: static int backfill_tags(struct transport *transport,
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head);
-+ retcode = fetch_and_consume_refs(display, transport, transaction, ref_map, fetch_head);
++ retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
{
struct ref_transaction *transaction = NULL;
struct ref *ref_map = NULL;
-+ struct display_state display;
++ struct display_state display_state;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
-+ display_state_init(&display, ref_map);
++ display_state_init(&display_state, ref_map);
+
if (atomic_fetch) {
transaction = ref_transaction_begin(&err);
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
*/
if (rs->nr) {
- retcode = prune_refs(rs, transaction, ref_map, transport->url);
-+ retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
++ retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
} else {
- retcode = prune_refs(&transport->remote->fetch,
-+ retcode = prune_refs(&display, &transport->remote->fetch,
++ retcode = prune_refs(&display_state, &transport->remote->fetch,
transaction, ref_map,
transport->url);
}
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
}
- if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) {
-+ if (fetch_and_consume_refs(&display, transport, transaction, ref_map, &fetch_head)) {
++ if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {
retcode = 1;
goto cleanup;
}
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
* the transaction and don't commit anything.
*/
- if (backfill_tags(transport, transaction, tags_ref_map,
-+ if (backfill_tags(&display, transport, transaction, tags_ref_map,
++ if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
&fetch_head))
retcode = 1;
}
3: a4fd935c40 ! 2: 34eedb882c fetch: move output format into `display_state`
@@ builtin/fetch.c: static int s_update_ref(const char *action,
{
int max, rlen, llen, len;
-@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
+@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
git_config_get_string_tmp("fetch.output", &format);
if (!strcasecmp(format, "full"))
- compact_format = 0;
-+ display->compact_format = 0;
++ display_state->compact_format = 0;
else if (!strcasecmp(format, "compact"))
- compact_format = 1;
-+ display->compact_format = 1;
++ display_state->compact_format = 1;
else
die(_("invalid value for '%s': '%s'"),
"fetch.output", format);
-@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
+@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
!strcmp(rm->name, "HEAD"))
continue;
- width = refcol_width(rm);
-+ width = refcol_width(rm, display->compact_format);
++ width = refcol_width(rm, display_state->compact_format);
/*
* Not precise calculation for compact mode because '*' can
-@@ builtin/fetch.c: static void format_display(struct display_state *display,
+@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
width = (summary_width + strlen(summary) - gettext_width(summary));
strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
- if (!compact_format)
-+ if (!display->compact_format)
- print_remote_to_local(display, display_buffer, remote, local);
++ if (!display_state->compact_format)
+ print_remote_to_local(display_state, display_buffer, remote, local);
else
- print_compact(display, display_buffer, remote, local);
+ print_compact(display_state, display_buffer, remote, local);
4: 0998173b57 ! 3: ec355b8b8d fetch: pass the full local reference name to `format_display`
@@ Commit message
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/fetch.c ##
-@@ builtin/fetch.c: static void format_display(struct display_state *display,
+@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
- if (!display->compact_format)
-- print_remote_to_local(display, display_buffer, remote, local);
-+ print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
+ if (!display_state->compact_format)
+- print_remote_to_local(display_state, display_buffer, remote, local);
++ print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
else
-- print_compact(display, display_buffer, remote, local);
-+ print_compact(display, display_buffer, remote, prettify_refname(local));
+- print_compact(display_state, display_buffer, remote, local);
++ print_compact(display_state, display_buffer, remote, prettify_refname(local));
if (error)
strbuf_addf(display_buffer, " (%s)", error);
}
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
- struct strbuf *display_buffer, int summary_width)
+ struct strbuf *display, int summary_width)
{
struct commit *current = NULL, *updated;
- const char *pretty_ref = prettify_refname(ref->name);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
if (oideq(&ref->old_oid, &ref->new_oid)) {
if (verbosity > 0)
- format_display(display, display_buffer, '=', _("[up to date]"), NULL,
+ format_display(display_state, display, '=', _("[up to date]"), NULL,
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
return 0;
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
*/
- format_display(display, display_buffer, '!', _("[rejected]"),
+ format_display(display_state, display, '!', _("[rejected]"),
_("can't fetch into checked-out branch"),
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
r = s_update_ref("updating tag", ref, transaction, 0);
- format_display(display, display_buffer, r ? '!' : 't', _("[tag update]"),
+ format_display(display_state, display, r ? '!' : 't', _("[tag update]"),
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
return r;
} else {
- format_display(display, display_buffer, '!', _("[rejected]"),
+ format_display(display_state, display, '!', _("[rejected]"),
_("would clobber existing tag"),
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
}
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
r = s_update_ref(msg, ref, transaction, 0);
- format_display(display, display_buffer, r ? '!' : '*', what,
+ format_display(display_state, display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
r = s_update_ref("fast-forward", ref, transaction, 1);
- format_display(display, display_buffer, r ? '!' : ' ', quickref.buf,
+ format_display(display_state, display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
} else if (force || ref->force) {
@@ builtin/fetch.c: static int update_local_ref(struct ref *ref,
r = s_update_ref("forced-update", ref, transaction, 1);
- format_display(display, display_buffer, r ? '!' : '+', quickref.buf,
+ format_display(display_state, display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced update"),
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
strbuf_release(&quickref);
return r;
} else {
- format_display(display, display_buffer, '!', _("[rejected]"), _("non-fast-forward"),
+ format_display(display_state, display, '!', _("[rejected]"), _("non-fast-forward"),
- remote, pretty_ref, summary_width);
+ remote, ref->name, summary_width);
return 1;
}
}
-@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
+@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
shown_url = 1;
}
- format_display(display, &sb, '-', _("[deleted]"), NULL,
+ format_display(display_state, &sb, '-', _("[deleted]"), NULL,
- _("(none)"), prettify_refname(ref->name),
+ _("(none)"), ref->name,
summary_width);
5: d45ec31eea ! 4: e4c1ed4ad5 fetch: deduplicate handling of per-reference format
@@ Metadata
Author: Patrick Steinhardt <ps@pks.im>
## Commit message ##
- fetch: deduplicate handling of per-reference format
+ fetch: centralize handling of per-reference format
- Both callsites that call `format_display()` and then print the result to
- standard error use the same formatting directive " %s\n" to print the
- reference to disk, thus duplicating a small part of the logic.
+ The function `format_display()` is used to print a single reference
+ update to a buffer which will then ultimately be printed by the caller.
+ This architecture causes us to duplicate some logic across the different
+ callsites of this function. This makes it hard to follow the code as
+ some parts of the logic are located in one place, while other parts of
+ the logic are located in a different place. Furthermore, by having the
+ logic scattered around it becomes quite hard to implement a new output
+ format for the reference updates.
- Refactor the code to handle this in `format_display()` itself. This
- paves the way for handling the printing logic in that function
- completely.
+ We can make the logic a whole lot easier to understand by making the
+ `format_display()` function self-contained so that it handles formatting
+ and printing of the references. This will eventually allow us to easily
+ implement a completely different output format, but also opens the door
+ to conditionally print to either stdout or stderr depending on the
+ output format.
+
+ As a first step towards that goal we move the formatting directive used
+ by both callers to print a single reference update into this function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/fetch.c ##
-@@ builtin/fetch.c: static void format_display(struct display_state *display,
+@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
width = (summary_width + strlen(summary) - gettext_width(summary));
- strbuf_addf(display_buffer, "%c %-*s ", code, width, summary);
+ strbuf_addf(display_buffer, " %c %-*s ", code, width, summary);
- if (!display->compact_format)
- print_remote_to_local(display, display_buffer, remote, prettify_refname(local));
+ if (!display_state->compact_format)
+ print_remote_to_local(display_state, display_buffer, remote, prettify_refname(local));
else
- print_compact(display, display_buffer, remote, prettify_refname(local));
+ print_compact(display_state, display_buffer, remote, prettify_refname(local));
if (error)
strbuf_addf(display_buffer, " (%s)", error);
+ strbuf_addch(display_buffer, '\n');
}
static int update_local_ref(struct ref *ref,
-@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
+@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
url_len, url);
shown_url = 1;
}
@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
}
}
}
-@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
- format_display(display, &sb, '-', _("[deleted]"), NULL,
+@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
+ format_display(display_state, &sb, '-', _("[deleted]"), NULL,
_("(none)"), ref->name,
summary_width);
- fprintf(stderr, " %s\n",sb.buf);
6: 2ea3a4e308 ! 5: 98b799af71 fetch: deduplicate logic to print remote URL
@@ Metadata
Author: Patrick Steinhardt <ps@pks.im>
## Commit message ##
- fetch: deduplicate logic to print remote URL
+ fetch: centralize logic to print remote URL
When fetching from a remote, we not only print the actual references
that have changed, but will also print the URL from which we have
@@ Commit message
can convert the global variable into a member of `display_state`. And
second, we can deduplicate the logic to compute the anonymized URL.
+ This also works as expected when fetching from multiple remotes, for
+ example via a group of remotes, as we do this by forking a standalone
+ git-fetch(1) process per remote that is to be fetched.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/fetch.c ##
@@ builtin/fetch.c: static int refcol_width(const struct ref *ref, int compact_form
return rlen;
}
--static void display_state_init(struct display_state *display, struct ref *ref_map)
-+static void display_state_init(struct display_state *display, struct ref *ref_map,
+-static void display_state_init(struct display_state *display_state, struct ref *ref_map)
++static void display_state_init(struct display_state *display_state, struct ref *ref_map,
+ const char *raw_url)
{
struct ref *rm;
const char *format = "full";
+ int i;
- memset(display, 0, sizeof(*display));
+ memset(display_state, 0, sizeof(*display_state));
+ if (raw_url)
-+ display->url = transport_anonymize_url(raw_url);
++ display_state->url = transport_anonymize_url(raw_url);
+ else
-+ display->url = xstrdup("foreign");
++ display_state->url = xstrdup("foreign");
+
-+ display->url_len = strlen(display->url);
-+ for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
++ display_state->url_len = strlen(display_state->url);
++ for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
+ ;
-+ display->url_len = i + 1;
-+ if (4 < i && !strncmp(".git", display->url + i - 3, 4))
-+ display->url_len = i - 3;
++ display_state->url_len = i + 1;
++ if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))
++ display_state->url_len = i - 3;
+
if (verbosity < 0)
return;
-@@ builtin/fetch.c: static void display_state_init(struct display_state *display, struct ref *ref_ma
+@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
}
}
-+static void display_state_release(struct display_state *display)
++static void display_state_release(struct display_state *display_state)
+{
-+ free(display->url);
++ free(display_state->url);
+}
+
- static void print_remote_to_local(struct display_state *display,
+ static void print_remote_to_local(struct display_state *display_state,
struct strbuf *display_buffer,
const char *remote, const char *local)
-@@ builtin/fetch.c: static void format_display(struct display_state *display,
+@@ builtin/fetch.c: static void format_display(struct display_state *display_state,
if (verbosity < 0)
return;
-+ if (!display->shown_url) {
-+ strbuf_addf(display_buffer, _("From %.*s\n"), display->url_len, display->url);
-+ display->shown_url = 1;
++ if (!display_state->shown_url) {
++ strbuf_addf(display_buffer, _("From %.*s\n"),
++ display_state->url_len, display_state->url);
++ display_state->shown_url = 1;
+ }
+
width = (summary_width + strlen(summary) - gettext_width(summary));
@@ builtin/fetch.c: static void format_display(struct display_state *display,
@@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n"
"to avoid this check\n");
- static int store_updated_refs(struct display_state *display,
+ static int store_updated_refs(struct display_state *display_state,
- const char *raw_url, const char *remote_name,
+ const char *remote_name,
int connectivity_checked,
@@ builtin/fetch.c: N_("it took %.2f seconds to check forced updates; you can use\n
rm = ref_map;
if (check_connected(iterate_ref_map, &rm, &opt)) {
- rc = error(_("%s did not send all necessary objects\n"), url);
-+ rc = error(_("%s did not send all necessary objects\n"), display->url);
++ rc = error(_("%s did not send all necessary objects\n"),
++ display_state->url);
goto abort;
}
}
-@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
+@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
what = rm->name;
}
@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
strbuf_reset(¬e);
if (*what) {
if (*kind)
-@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
+@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
append_fetch_head(fetch_head, &rm->old_oid,
rm->fetch_head_status,
- note.buf, url, url_len);
-+ note.buf, display->url, display->url_len);
++ note.buf, display_state->url,
++ display_state->url_len);
strbuf_reset(¬e);
if (ref) {
-@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
+@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
*what ? what : "HEAD",
"FETCH_HEAD", summary_width);
}
@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
}
}
-@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
+@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display_state,
abort:
strbuf_release(¬e);
@@ builtin/fetch.c: static int store_updated_refs(struct display_state *display,
return rc;
}
-@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,
+@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,
}
trace2_region_enter("fetch", "consume_refs", the_repository);
-- ret = store_updated_refs(display, transport->url, transport->remote->name,
-+ ret = store_updated_refs(display, transport->remote->name,
+- ret = store_updated_refs(display_state, transport->url, transport->remote->name,
++ ret = store_updated_refs(display_state, transport->remote->name,
connectivity_checked, transaction, ref_map,
fetch_head);
trace2_region_leave("fetch", "consume_refs", the_repository);
-@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display,
- static int prune_refs(struct display_state *display,
+@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display_state,
+ static int prune_refs(struct display_state *display_state,
struct refspec *rs,
struct ref_transaction *transaction,
- struct ref *ref_map,
@@ builtin/fetch.c: static int fetch_and_consume_refs(struct display_state *display
if (!dry_run) {
if (transaction) {
for (ref = stale_refs; ref; ref = ref->next) {
-@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
+@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
for (ref = stale_refs; ref; ref = ref->next) {
struct strbuf sb = STRBUF_INIT;
@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
- fprintf(stderr, _("From %.*s\n"), url_len, url);
- shown_url = 1;
- }
- format_display(display, &sb, '-', _("[deleted]"), NULL,
+ format_display(display_state, &sb, '-', _("[deleted]"), NULL,
_("(none)"), ref->name,
summary_width);
-@@ builtin/fetch.c: static int prune_refs(struct display_state *display,
+@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
cleanup:
strbuf_release(&err);
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
{
struct ref_transaction *transaction = NULL;
struct ref *ref_map = NULL;
-- struct display_state display;
-+ struct display_state display = { 0 };
+- struct display_state display_state;
++ struct display_state display_state = { 0 };
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
-- display_state_init(&display, ref_map);
-+ display_state_init(&display, ref_map, transport->url);
+- display_state_init(&display_state, ref_map);
++ display_state_init(&display_state, ref_map, transport->url);
if (atomic_fetch) {
transaction = ref_transaction_begin(&err);
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
* don't care whether --tags was specified.
*/
if (rs->nr) {
-- retcode = prune_refs(&display, rs, transaction, ref_map, transport->url);
-+ retcode = prune_refs(&display, rs, transaction, ref_map);
+- retcode = prune_refs(&display_state, rs, transaction, ref_map, transport->url);
++ retcode = prune_refs(&display_state, rs, transaction, ref_map);
} else {
- retcode = prune_refs(&display, &transport->remote->fetch,
+ retcode = prune_refs(&display_state, &transport->remote->fetch,
- transaction, ref_map,
- transport->url);
+ transaction, ref_map);
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
error("%s", err.buf);
}
-+ display_state_release(&display);
++ display_state_release(&display_state);
close_fetch_head(&fetch_head);
strbuf_release(&err);
free_refs(ref_map);
7: f67f9640a8 < -: ---------- fetch: fix inconsistent summary width for pruned and updated refs
8: 9667301711 < -: ---------- fetch: centralize printing of reference updates
-: ---------- > 6: fe7e2e85eb fetch: centralize printing of reference updates
--
2.40.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-03-20 12:35 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 11:21 [PATCH 0/8] fetch: refactor code that prints reference updates Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 1/8] fetch: rename `display` buffer to avoid name conflict Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 2/8] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-15 20:59 ` Junio C Hamano
2023-03-16 15:05 ` Patrick Steinhardt
2023-03-16 16:18 ` Junio C Hamano
2023-03-17 10:03 ` Patrick Steinhardt
2023-03-16 16:19 ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 3/8] fetch: move output format " Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 4/8] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-15 22:18 ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 5/8] fetch: deduplicate handling of per-reference format Patrick Steinhardt
2023-03-15 22:45 ` Junio C Hamano
2023-03-16 15:06 ` Patrick Steinhardt
2023-03-16 16:50 ` Junio C Hamano
2023-03-17 9:51 ` Patrick Steinhardt
2023-03-17 15:41 ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 6/8] fetch: deduplicate logic to print remote URL Patrick Steinhardt
2023-03-15 23:02 ` Junio C Hamano
2023-03-16 15:06 ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs Patrick Steinhardt
2023-03-15 23:12 ` Junio C Hamano
2023-03-16 15:06 ` Patrick Steinhardt
2023-03-16 16:30 ` Junio C Hamano
2023-03-17 9:55 ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 8/8] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-17 20:24 ` [PATCH 0/8] fetch: refactor code that prints " Jonathan Tan
2023-03-20 6:57 ` Patrick Steinhardt
2023-03-20 12:26 ` Patrick Steinhardt
2023-03-20 12:35 ` Patrick Steinhardt [this message]
2023-03-20 12:35 ` [PATCH v2 1/6] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 2/6] fetch: move output format " Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 3/6] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 4/6] fetch: centralize handling of per-reference format Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 5/6] fetch: centralize logic to print remote URL Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 6/6] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-20 22:57 ` Jonathan Tan
2023-03-22 9:04 ` Patrick Steinhardt
2023-03-29 18:45 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1679315383.git.ps@pks.im \
--to=ps@pks.im \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.