On Sun, Sep 29, 2019 at 7:22 PM Junio C Hamano <gits...@pobox.com> wrote:
>
> Alex Henrie <alexhenri...@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
