Re: [PATCH v3] commit-graph: remove a duplicate assignment

[Alex Henrie <alexhenrie24@gmail.com>]

On Sun, Sep 29, 2019 at 7:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > The variable g was being set to the same value both at the beginning of
> > the function and before the loop. The assignment before the loop was
> > kept because it helps clarify what the loop does, and the redundant
> > assignment at the beginning of the function was removed.
>
> Writing these mostly in the past tense is misleading to those who
> are used to read "git log" from this project.  Give orders to the
> codebase to "become like so" instead.  Perhaps like
>
>         Leave the variable 'g' uninitialized before it is set just
>         before its first use in front of a loop, which is a lot more
>         appropriate place to indicate what it is used for.

Okay, thanks for the guidance. I'll use your text on the next version
of the patch.

> >  static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
> >  {
> > -     struct commit_graph *g = ctx->r->objects->commit_graph;
> > +     struct commit_graph *g;
> >       uint32_t num_commits = ctx->commits.nr;
>
> Stepping back a bit, doesn't the same justification you gave to this
> change apply to 'num_commits'?  If you make it uninitialized before
> its first use and assign ctx->commits.nr to it near where 'g' is
> given its first value, wouldn't it make it even clearer that these
> two variables are almost always used together and how they are used
> in the loop?

Yes, that makes sense. I'll send a revised patch that includes that
change as well.

-Alex