Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes, part 3
- From
Jeff King <peff@peff.net>
- Date
- Oct 10, 2016, 00:00 UTC
- Message-ID
- <20161010000035.mfcf55wqfcbcnarh@sigill.intra.peff.net>
- In-Reply-To
- <c830e99d-4fc9-b001-cce3-38f66dc2c678@web.de>
On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:
Show 7 quoted lines
> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs > instead of taking detours through find_unique_abbrev() and its static > buffer. This is shorter in most cases and a bit more efficient. > > The changes here are not easily handled by a semantic patch because > they involve removing temporary variables and deconstructing format > strings for strbuf_addf().
Yeah, the thing to look for here is whether the sha1 variable holds the same value at both times.
These all look OK to me. Mild rambling below.
Show 14 quoted lines
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 5200d5c..9041c2f 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -202,9 +202,9 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
> strbuf_addf(&o->obuf, "virtual %s\n",
> merge_remote_util(commit)->name);
> else {
> - strbuf_addf(&o->obuf, "%s ",
> - find_unique_abbrev(commit->object.oid.hash,
> - DEFAULT_ABBREV));
> + strbuf_add_unique_abbrev(&o->obuf, commit->object.oid.hash,
> + DEFAULT_ABBREV);
> + strbuf_addch(&o->obuf, ' ');I've often wondered whether a big strbuf_addf() is more efficient than several strbuf_addstrs. It amortizes the size-checks, certainly, though those are probably not very big. It shouldn't matter much for amortizing the cost of malloc, as it's very unlikely to have a case where:
strbuf_addf("%s%s", foo, bar);would require one malloc, but:
strbuf_addstr(foo); strbuf_addstr(bar);
would require two (one of the strings would have to be around the same size as the ALLOC_GROW() doubling).
So it probably doesn't matter much in practice (not that most of these cases are very performance sensitive anyway). Mostly just something I've pondered while tweaking strbuf invocations.
And anyway, in this case replacing find_unique_abbrev lets us write directly into the final buffer rather than copying through a static buffer, so it's almost certainly a win there.
Show 16 quoted lines
> diff --git a/pretty.c b/pretty.c
> index 25efbca..0c31495 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -544,15 +544,13 @@ static void add_merge_info(const struct pretty_print_context *pp,
> strbuf_addstr(sb, "Merge:");
>
> while (parent) {
> - struct commit *p = parent->item;
> - const char *hex = NULL;
> + struct object_id *oidp = &parent->item->object.oid;
> + strbuf_addch(sb, ' ');
> if (pp->abbrev)
> - hex = find_unique_abbrev(p->object.oid.hash, pp->abbrev);
> - if (!hex)
> - hex = oid_to_hex(&p->object.oid);Wow, this existing code was hard to follow. I wondered when find_unique_abbrev() would return NULL, but it never does. This "if (!hex)" should have been an "else" all along.
> + strbuf_add_unique_abbrev(sb, oidp->hash, pp->abbrev); > + else > + strbuf_addstr(sb, oid_to_hex(oidp));
...which I see you changed here, though perhaps it is not immediately obvious that it is correct without knowing find_unique_abbrev's return value assumptions.
Just thinking aloud, I've also wondered if strbuf_addoid() would be handy. We already have oid_to_hex_r(). In fact, you could write it already as:
strbuf_add_unique_abbrev(sb, oidp->hash, 0);
but that is probably not helping maintainability. ;)
Show 15 quoted lines
> diff --git a/submodule.c b/submodule.c > index 2de06a3..476f60b 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -392,10 +392,9 @@ static void show_submodule_header(FILE *f, const char *path, > } > > output_header: > - strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path, > - find_unique_abbrev(one->hash, DEFAULT_ABBREV)); > - if (!fast_backward && !fast_forward) > - strbuf_addch(&sb, '.'); > + strbuf_addf(&sb, "%s%sSubmodule %s ", line_prefix, meta, path); > + strbuf_add_unique_abbrev(&sb, one->hash, DEFAULT_ABBREV); > + strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
Spelling out ".." and "..." as you do makes this much clearer, IMHO.
-Peff