Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 1, 2016, 08:58 UTC
- Message-ID
- <alpine.DEB.2.20.1608011055180.149069@virtualbox>
- In-Reply-To
- <xmqqa8h0m82f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 29 Jul 2016, Junio C Hamano wrote:
Show 21 quoted lines
> Kevin Willford <kcwillford@gmail.com> writes:
>
> > static int patch_id_cmp(struct patch_id *a,
> > struct patch_id *b,
> > - void *keydata)
> > + struct diff_options *opt)
> > {
> > + if (is_null_sha1(a->patch_id) &&
> > + commit_patch_id(a->commit, opt, a->patch_id, 0))
> > + return error("Could not get patch ID for %s",
> > + oid_to_hex(&a->commit->object.oid));
> > + if (is_null_sha1(b->patch_id) &&
> > + commit_patch_id(b->commit, opt, b->patch_id, 0))
> > + return error("Could not get patch ID for %s",
> > + oid_to_hex(&b->commit->object.oid));
> > return hashcmp(a->patch_id, b->patch_id);
> > }
>
> These error returns initially looks slightly iffy in that in general
> the caller of any_cmp_fn() wants to know how a/b compares, but by
> returning error(), it always says "a is smaller than b".I am to blame, as this is my design.
And yes, it is kind of funny that we require a cmpfn that returns <0, ==0 and >0 for comparisons, when hashmaps try to avoid any order.
Do you want a note in the commit message about this "abuse" of a negative return value, or a code comment?
> The idea of using the two level hash, computing the more expensive > one only when the hashmap hashes of the result of the cheaper hash > function collide, is excellent.
Thanks :-)
Ciao, Dscho