Re: [PATCH v4 00/10] Compute and consume generation numbers

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <stolee@gmail.com> writes:

> As promised, here is the diff from v3.

What is this strange string "       " in place of tabs in the interdiff?
" " here is Unicode Character 'NO-BREAK SPACE' (U+00A0).

Though it doesn't matter for viewing, my newsreader (Gnus from GNU
Emacs) thinks that it is worth notifying about when replying.

Also, it looks like at least in one place the diff got line-wrapped.

> Thanks,
> -Stolee
>
> -- >8 --
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7e1da6c6ea..b819756946 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1148,6 +1148,7 @@ int cmd_merge(int argc, const char **argv, const
> char *prefix)
>         branch = branch_to_free = resolve_refdup("HEAD", 0, &head_oid,
> NULL);
>         if (branch)
>                 skip_prefix(branch, "refs/heads/", &branch);
> +
>         init_diff_ui_defaults();
>         git_config(git_merge_config, NULL);
>
> @@ -1156,7 +1157,6 @@ int cmd_merge(int argc, const char **argv, const
> char *prefix)
>         else
>                 head_commit = lookup_commit_or_die(&head_oid, "HEAD");
>
> -
>         if (branch_mergeoptions)
>                 parse_branch_merge_options(branch_mergeoptions);
>         argc = parse_options(argc, argv, prefix, builtin_merge_options,

Whitespace fixes, all right.

> diff --git a/commit-graph.c b/commit-graph.c
> index 21e853c21a..aebd242def 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -257,7 +257,7 @@ static int fill_commit_in_graph(struct commit
> *item, struct commit_graph *g, uin
>         uint32_t *parent_data_ptr;
>         uint64_t date_low, date_high;
>         struct commit_list **pptr;
> -       const unsigned char *commit_data = g->chunk_commit_data +
> GRAPH_DATA_WIDTH * pos;
> +       const unsigned char *commit_data = g->chunk_commit_data +
> (g->hash_len + 16) * pos;
>
>         item->object.parsed = 1;
>         item->graph_pos = pos;

This was accidental change in v3 (unrelated to the changes in commit it
were in).  Though I wonder if the symbolic constant route is not better
- though as separate standalone commit.

> @@ -304,7 +304,7 @@ static int find_commit_in_graph(struct commit
> *item, struct commit_graph *g, uin
>                 *pos = item->graph_pos;
>                 return 1;
>         } else {
> -               return bsearch_graph(commit_graph,
> &(item->object.oid), pos);
> +               return bsearch_graph(g, &(item->object.oid), pos);
>         }
>  }
>

Fixup for a commit, that was sent in separate fixup email in v3.  All
right.

Though I wonder if it wouldn't be better to call global variable
'the_commit_graph' to avoid such errors in the future...

> @@ -312,10 +312,10 @@ int parse_commit_in_graph(struct commit *item)
>  {
>         uint32_t pos;
>
> -       if (item->object.parsed)
> -               return 0;
>         if (!core_commit_graph)
>                 return 0;
> +       if (item->object.parsed)
> +               return 1;
>         prepare_commit_graph();
>         if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
>                 return fill_commit_in_graph(item, commit_graph, pos);

Fixed accidental flip-flopping about return value when
item->object.parsed.  I'd have to take a look at actual commits to say
whether I think it is all right or not.

> @@ -454,9 +454,8 @@ static void write_graph_chunk_data(struct hashfile
> *f, int hash_len,
>                 else
>                         packedDate[0] = 0;
>
> -               if ((*list)->generation != GENERATION_NUMBER_INFINITY) {
> +               if ((*list)->generation != GENERATION_NUMBER_INFINITY)
>                         packedDate[0] |= htonl((*list)->generation << 2);
> -               }
>
>                 packedDate[1] = htonl((*list)->date);
>                 hashwrite(f, packedDate, 8);

Coding style change, to be more in line with CodingGuidelines, namely
that we usually do not use block for single-command in conditionals.

All right.

> diff --git a/commit.c b/commit.c
> index 9ef6f699bd..e2e16ea1a7 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -653,7 +653,7 @@ int compare_commits_by_gen_then_commit_date(const
> void *a_, const void *b_, void
>         else if (a->generation > b->generation)
>                 return -1;
>
> -       /* use date as a heuristic when generataions are equal */
> +       /* use date as a heuristic when generations are equal */
>         if (a->date < b->date)
>                 return 1;
>         else if (a->date > b->date)

Fixed typo in comment.  All right.

> @@ -1078,7 +1078,7 @@ int in_merge_bases_many(struct commit *commit,
> int nr_reference, struct commit *
>         }
>
>         if (commit->generation > min_generation)
> -               return 0;
> +               return ret;
>
>         bases = paint_down_to_common(commit, nr_reference, reference,
> commit->generation);
>         if (commit->object.flags & PARENT2)

Unifying way of returning result (to one that was used before this
commit in this fragment of the git code).  Looks all right, from what I
remember.

> diff --git a/ref-filter.c b/ref-filter.c
> index e2fea6d635..fb35067fc9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -16,6 +16,7 @@
>  #include "trailer.h"
>  #include "wt-status.h"
>  #include "commit-slab.h"
> +#include "commit-graph.h"
>
>  static struct ref_msg {
>         const char *gone;
> @@ -1629,7 +1630,7 @@ static enum contains_result
> contains_tag_algo(struct commit *candidate,
>
>         for (p = want; p; p = p->next) {
>                 struct commit *c = p->item;
> -               parse_commit_or_die(c);
> +               load_commit_graph_info(c);
>                 if (c->generation < cutoff)
>                         cutoff = c->generation;
>         }

Avoiding performance penalty when not using commit-graph feature (or
when it is turned off).  Looks good on first glance.

> @@ -1582,7 +1583,7 @@ static int in_commit_list(const struct
> commit_list *want, struct commit *c)
>  }
>
>  /*
> - * Test whether the candidate or one of its parents is contained in
> the list.
> + * Test whether the candidate is contained in the list.
>   * Do not recurse to find out, though, but return -1 if inconclusive.
>   */
>  static enum contains_result contains_test(struct commit *candidate,

Bringing comment in line with the function it is about.  Good.

Best,
-- 
Jakub Narębski