Re: [PATCH v2 03/10] midx-write: use `revs->repo` inside `read_refs_snapshot`

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 20, 2024, 19:46 UTC
Message-ID: <Zz48r6ZZcol6TQaF@nand.local>
In-Reply-To: <PA4PR07MB74069403837F36FD07D075EB9E212@PA4PR07MB7406.eurprd07.prod.outlook.com>

On Wed, Nov 20, 2024 at 02:26:23PM +0000, Richard Kerry wrote:

Show 9 quoted lines

>
> >> The `read_refs_snapshot` uses the `parse_oid_hex` function which
> >> internally uses global variables. Let's instead use
> >
> >Nit: s/variables/variable
>
> No, that's fine.
> It's plural, so ends with 's'.
> Unless it should be "uses a global variable"

The global variable in question here is just "the_hash_algo", so I think shejialuo's suggestion to use "variable" is correct, but it would need to be "uses a global variable" instead of "uses global variable" (without the article).

But I think we're being unnecessarily vague here, and could instead say:

    The function `read_refs_snapshot()` uses `parse_oid_hex()`, which
    relies on the global `the_hash_algo` variable. Let's instead use
    [...]

Show 7 quoted lines

> >> Also, while here, fix a missing newline after the functions definition.
> >>
> >
> >Nit: s/functions/function
>
> Maybe.
> But it could be "the function's definition" as it could be seen as possessive.

It should be "function's definition", as the possessive is the correct form.

Thanks, Taylor