Re: [PATCH v4 00/10] Compute and consume generation numbers
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Apr 28, 2018, 17:28 UTC
- Message-ID
- <86in8bjl4j.fsf@gmail.com>
- In-Reply-To
- <527a8d47-15d4-78a1-4320-97ddc27bce48@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.
Show 28 quoted lines
> 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.
Show 16 quoted lines
> 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.
Show 11 quoted lines
> @@ -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...
Show 13 quoted lines
> @@ -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.
Show 12 quoted lines
> @@ -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.
Show 14 quoted lines
> 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.
Show 11 quoted lines
> @@ -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.
Show 22 quoted lines
> 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.
Show 11 quoted lines
> @@ -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