On Sat, Oct 08, 2016 at 05:38:47PM +0200, René Scharfe wrote:
> 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.
> 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.
> 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. ;)
> 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