Re: [PATCH 01/12] blame: use commit-slab for blame suspects instead of commit->util

[Duy Nguyen]

On Sat, May 12, 2018 at 11:22 AM, Jeff King  wrote:
> On Sat, May 12, 2018 at 10:00:17AM +0200, Nguyễn Thái Ngọc Duy wrote:
>
>> +define_commit_slab(blame_suspects, struct blame_origin *);
>> +static struct blame_suspects blame_suspects;
>> +
>> +struct blame_origin *get_blame_suspects(struct commit *commit)
>> +{
>> + struct blame_origin **result;
>> +
>> + result = blame_suspects_peek(_suspects, commit);
>> +
>> + return result ? *result : NULL;
>> +}
>
> Hmm. You need this helper because you want to be able to peek and get a
> NULL. But that's already what _at() would return, with the only
> difference that we may extend the slab just to return NULL.
>
> I wonder how much it matters in practice. We'd generally be extending
> the slab to hit every commit anyway in this case, I would think.

I don't know much about blame so I stay very conservative ;-) If it's
safe to just do _at() here, I'll update this patch.

> I suppose it doesn't actually simplify the code that much to do it that
> way, though. We could get rid of this helper, but the caller would still
> look like:
>
>   for (p = *blame_suspects_at(o->commit); p; p = p->next)
>
> which is actually slightly uglier than get_blame_suspects(), because we
> have to do the pointer-dereference ourselves.

And the caller would need to include commit-slab.h too. I added
get_blame_suspects() because I wanted to avoid that.
-- 
Duy

Re: [PATCH 01/12] blame: use commit-slab for blame suspects instead of commit->util

[Jeff King]

On Sat, May 12, 2018 at 10:00:17AM +0200, Nguyễn Thái Ngọc Duy wrote:

> +define_commit_slab(blame_suspects, struct blame_origin *);
> +static struct blame_suspects blame_suspects;
> +
> +struct blame_origin *get_blame_suspects(struct commit *commit)
> +{
> + struct blame_origin **result;
> +
> + result = blame_suspects_peek(_suspects, commit);
> +
> + return result ? *result : NULL;
> +}

Hmm. You need this helper because you want to be able to peek and get a
NULL. But that's already what _at() would return, with the only
difference that we may extend the slab just to return NULL.

I wonder how much it matters in practice. We'd generally be extending
the slab to hit every commit anyway in this case, I would think.

I suppose it doesn't actually simplify the code that much to do it that
way, though. We could get rid of this helper, but the caller would still
look like:

  for (p = *blame_suspects_at(o->commit); p; p = p->next)

which is actually slightly uglier than get_blame_suspects(), because we
have to do the pointer-dereference ourselves.

-Peff

[PATCH 01/12] blame: use commit-slab for blame suspects instead of commit->util

[Nguyễn Thái Ngọc Duy]

It's done so that commit->util can be removed. See more explanation in
the commit that removes commit->util.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 blame.c | 42 +++---
 blame.h |  2 ++
 builtin/blame.c |  2 +-
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/blame.c b/blame.c
index 78c9808bd1..18e8bd996a 100644
--- a/blame.c
+++ b/blame.c
@@ -6,6 +6,24 @@
 #include "diffcore.h"
 #include "tag.h"
 #include "blame.h"
+#include "commit-slab.h"
+
+define_commit_slab(blame_suspects, struct blame_origin *);
+static struct blame_suspects blame_suspects;
+
+struct blame_origin *get_blame_suspects(struct commit *commit)
+{
+   struct blame_origin **result;
+
+   result = blame_suspects_peek(_suspects, commit);
+
+   return result ? *result : NULL;
+}
+
+static void set_blame_suspects(struct commit *commit, struct blame_origin 
*origin)
+{
+   *blame_suspects_at(_suspects, commit) = origin;
+}
 
 void blame_origin_decref(struct blame_origin *o)
 {
@@ -15,12 +33,12 @@ void blame_origin_decref(struct blame_origin *o)
blame_origin_decref(o->previous);
free(o->file.ptr);
/* Should be present exactly once in commit chain */
-   for (p = o->commit->util; p; l = p, p = p->next) {
+   for (p = get_blame_suspects(o->commit); p; l = p, p = p->next) {
if (p == o) {
if (l)
l->next = p->next;
else
-   o->commit->util = p->next;
+   set_blame_suspects(o->commit, p->next);
free(o);
return;
}
@@ -41,8 +59,8 @@ static struct blame_origin *make_origin(struct commit 
*commit, const char *path)
FLEX_ALLOC_STR(o, path, path);
o->commit = commit;
o->refcnt = 1;
-   o->next = commit->util;
-   commit->util = o;
+   o->next = get_blame_suspects(commit);
+   set_blame_suspects(commit, o);
return o;
 }
 
@@ -54,13 +72,13 @@ static struct blame_origin *get_origin(struct commit 
*commit, const char *path)
 {
struct blame_origin *o, *l;
 
-   for (o = commit->util, l = NULL; o; l = o, o = o->next) {
+   for (o = get_blame_suspects(commit), l = NULL; o; l = o, o = o->next) {
if (!strcmp(o->path, path)) {
/* bump to front */
if (l) {
l->next = o->next;
-   o->next = commit->util;
-   commit->util = o;
+   o->next = get_blame_suspects(commit);
+   set_blame_suspects(commit, o);
}
return blame_origin_incref(o);
}
@@ -478,7 +496,7 @@ static void queue_blames(struct blame_scoreboard *sb, 
struct blame_origin *porig
porigin->suspects = blame_merge(porigin->suspects, sorted);
else {
struct blame_origin *o;
-   for (o = porigin->commit->util; o; o = o->next) {
+   for (o = get_blame_suspects(porigin->commit); o; o = o->next) {
if (o->suspects) {
porigin->suspects = sorted;
return;
@@ -525,7 +543,7 @@ static struct blame_origin *find_origin(struct commit 
*parent,
const char *paths[2];
 
/* First check any existing origins */
-   for (porigin = parent->util; porigin; porigin = porigin->next)
+   for (porigin = get_blame_suspects(parent); porigin; porigin = 
porigin->next)
if (!strcmp(porigin->path, origin->path)) {
/*
 * The same path between origin and its parent
@@ -1550,7 +1568,7 @@ void assign_blame(struct blame_scoreboard *sb, int opt)
 
while (commit) {
struct blame_entry *ent;
-   struct blame_origin *suspect = commit->util;
+   struct blame_origin *suspect = get_blame_suspects(commit);
 
/* find one suspect to break down */
while (suspect && !suspect->suspects)
@@ -1752,6 +1770,8 @@ void setup_scoreboard(struct blame_scoreboard *sb, const 
char *path, struct blam
struct commit *final_commit = NULL;
enum object_type type;
 
+   init_blame_suspects(_suspects);
+
if (sb->reverse && sb->contents_from)
die(_("--contents and --reverse do not blend well."));
 
@@ -1815,7 +1835,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, const 
char *path, struct blam
}
 
if (is_null_oid(>final->object.oid)) {
-   o = sb->final->util;
+   o =