Re: [PATCH v9] show-branch: convert per-branch flags to commit-slab

Gatla Vishweshwar Reddy <[email protected]> writes:

> I took time to read through the code carefully before sending this.

Hmph.  I hate to say this, but I am finding it difficult to trust
your "carefully" at this point.

    $ make
    $ ./git show-branch master next
    Floating point exception (core dumped).

While I have not spent the time to exhaustively find all bugs in
this code [*], it is disturbing that the simplest use of the command
immediately crashes.  The reason for this crash is trivial.

>  static struct commit_name_slab name_slab;
>  
> +define_commit_slab(commit_rev_flags, uint64_t);
> +static struct commit_rev_flags rev_flags_slab;
> +static int flags_stride; /* number of uint64_t words per commit */
> +
>  static struct commit_name *commit_to_name(struct commit *commit)
>  {
>  	return *commit_name_slab_at(&name_slab, commit);
>  }

The code still uses name_slab to associate names with commits, and
commit_to_name() is called by functions like name_commits(), which
is used in cmd_show_branch().  Yet, the patch does this:

> @@ -713,8 +773,6 @@ int cmd_show_branch(int ac,
>  	const char **args_copy = NULL;
>  	int ret;
>  
> -	init_commit_name_slab(&name_slab);
> -

The code simply discards the initialization for that slab, leaving
name_slab BSS-initialized.  Consequently, all members in the struct
are initialized to 0, including the '.slab_size' member.

This init_commit_name_slab() call is not moved elsewhere; it simply
disappeared without an explanation.

When commit_to_name() tries to look up a commit in the slab, it
first attempts to determine which slab should contain the data for
the commit, using this code from <commit-slab-impl.h>:

scope elemtype *slabname## _at_peek(struct slabname *s,			\
						  const struct commit *c, \
						  int add_if_missing)   \
{									\
	unsigned int nth_slab, nth_slot;				\
									\
	nth_slab = c->index / s->slab_size;				\
	nth_slot = c->index % s->slab_size;				\
									\
And we all know what would happen when you divide by zero.



[Footnote]

 * ... and I will not.