| Message ID | 20241009135747.3563204-2-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/6] refs_update_symref: atomically record overwritten ref | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > Instead of calling get_main_ref_store(the_repository) multiple times, > call it once and store in a new refs variable. Although this change > probably offers some performance benefits, the main purpose is to > shorten the line lengths of function calls using this variable for > better readability. > --- > > Notes: > v5: new patch (split from the next patch as a preparatory step) It is a good idea to help making it obvious that we are accessing the same ref store. The patch needs to be signed-off, though. > builtin/remote.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index d8ff440027..353ffd2c43 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1404,6 +1404,7 @@ static int set_head(int argc, const char **argv, const char *prefix) > int i, opt_a = 0, opt_d = 0, result = 0; > struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; > char *head_name = NULL; > + struct ref_store *refs = get_main_ref_store(the_repository); > > struct option options[] = { > OPT_BOOL('a', "auto", &opt_a, > @@ -1434,7 +1435,7 @@ static int set_head(int argc, const char **argv, const char *prefix) > head_name = xstrdup(states.heads.items[0].string); > free_remote_ref_states(&states); > } else if (opt_d && !opt_a && argc == 1) { > - if (refs_delete_ref(get_main_ref_store(the_repository), NULL, buf.buf, NULL, REF_NO_DEREF)) > + if (refs_delete_ref(refs, NULL, buf.buf, NULL, REF_NO_DEREF)) > result |= error(_("Could not delete %s"), buf.buf); > } else > usage_with_options(builtin_remote_sethead_usage, options); > @@ -1442,9 +1443,9 @@ static int set_head(int argc, const char **argv, const char *prefix) > if (head_name) { > strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name); > /* make sure it's valid */ > - if (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf)) > + if (!refs_ref_exists(refs, buf2.buf)) > result |= error(_("Not a valid ref: %s"), buf2.buf); > - else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head", NULL)) > + else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", NULL)) > result |= error(_("Could not setup %s"), buf.buf); > else if (opt_a) > printf("%s/HEAD set to %s\n", argv[0], head_name);
On Wed Oct 09, 2024 at 21:26, Junio C Hamano <gitster@pobox.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > >> Instead of calling get_main_ref_store(the_repository) multiple times, >> call it once and store in a new refs variable. Although this change >> probably offers some performance benefits, the main purpose is to >> shorten the line lengths of function calls using this variable for >> better readability. >> --- >> >> Notes: >> v5: new patch (split from the next patch as a preparatory step) > > It is a good idea to help making it obvious that we are accessing > the same ref store. Indeed, although it was your idea ;) > > The patch needs to be signed-off, though. Damn :/ Sorry, another oversight ... Should I send a v6 for the series with this fixed now or wait a bit to see if there are other comments? Thanks again for the patience! Best, Bence
diff --git a/builtin/remote.c b/builtin/remote.c index d8ff440027..353ffd2c43 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1404,6 +1404,7 @@ static int set_head(int argc, const char **argv, const char *prefix) int i, opt_a = 0, opt_d = 0, result = 0; struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; char *head_name = NULL; + struct ref_store *refs = get_main_ref_store(the_repository); struct option options[] = { OPT_BOOL('a', "auto", &opt_a, @@ -1434,7 +1435,7 @@ static int set_head(int argc, const char **argv, const char *prefix) head_name = xstrdup(states.heads.items[0].string); free_remote_ref_states(&states); } else if (opt_d && !opt_a && argc == 1) { - if (refs_delete_ref(get_main_ref_store(the_repository), NULL, buf.buf, NULL, REF_NO_DEREF)) + if (refs_delete_ref(refs, NULL, buf.buf, NULL, REF_NO_DEREF)) result |= error(_("Could not delete %s"), buf.buf); } else usage_with_options(builtin_remote_sethead_usage, options); @@ -1442,9 +1443,9 @@ static int set_head(int argc, const char **argv, const char *prefix) if (head_name) { strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name); /* make sure it's valid */ - if (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf)) + if (!refs_ref_exists(refs, buf2.buf)) result |= error(_("Not a valid ref: %s"), buf2.buf); - else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head", NULL)) + else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", NULL)) result |= error(_("Could not setup %s"), buf.buf); else if (opt_a) printf("%s/HEAD set to %s\n", argv[0], head_name);
Instead of calling get_main_ref_store(the_repository) multiple times, call it once and store in a new refs variable. Although this change probably offers some performance benefits, the main purpose is to shorten the line lengths of function calls using this variable for better readability. --- Notes: v5: new patch (split from the next patch as a preparatory step) builtin/remote.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)