git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Junio C Hamano
Message 3 of 3 in “commit-graph: remove a duplicate assignment”
  1. commit-graph: remove a duplicate assignmentAlex Henrie, Sep 29, 2019
  2. Junio C HamanoSep 30, 2019
  3. Alex HenrieSep 30, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.