Re: [PATCH v3] commit-graph: remove a duplicate assignment
- From
Alex Henrie <alexhenrie24@gmail.com>
- Date
- Sep 30, 2019, 07:34 UTC
- Message-ID
- <CAMMLpeSY9DJbJJDExS2RcdyXbr8dEW5S67zSyNeCHsr5h2upTg@mail.gmail.com>
- In-Reply-To
- <xmqqimpainmj.fsf@gitster-ct.c.googlers.com>
On Sun, Sep 29, 2019 at 7:22 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> > 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.
Show 12 quoted lines
> > 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