| Message ID | 20241119-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-v2-3-e2f607174efc@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Change midx.c and midx-write.c to not use global variables | expand |
On Tue, Nov 19, 2024 at 04:36:42PM +0100, Karthik Nayak wrote: > The `read_refs_snapshot` uses the `parse_oid_hex` function which > internally uses global variables. Let's instead use Nit: s/variables/variable > `parse_oid_hex_algop` and provide the hash algo via `revs->repo`. > > Also, while here, fix a missing newline after the functions definition. > Nit: s/functions/function > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> 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" >> >> 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. Regards, Richard.
On Tue, Nov 19, 2024 at 04:36:42PM +0100, Karthik Nayak wrote: > --- > midx-write.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/midx-write.c b/midx-write.c > index 22b5233f51ec6c6d99b8f9613818f1581dca5982..564438f616f59cd24edda956e4af0e0acf167138 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -760,7 +760,7 @@ static int read_refs_snapshot(const char *refs_snapshot, > hex = &buf.buf[1]; > } > > - if (parse_oid_hex(hex, &oid, &end) < 0) > + if (parse_oid_hex_algop(hex, &oid, &end, revs->repo->hash_algo) < 0) Looks obviously correct. > die(_("could not parse line: %s"), buf.buf); > if (*end) > die(_("malformed line: %s"), buf.buf); > @@ -776,6 +776,7 @@ static int read_refs_snapshot(const char *refs_snapshot, > strbuf_release(&buf); > return 0; > } > + Good spotting :-). Thanks, Taylor
On Wed, Nov 20, 2024 at 02:26:23PM +0000, Richard Kerry wrote: > > >> 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 [...] > >> 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
Taylor Blau <me@ttaylorr.com> writes: > On Wed, Nov 20, 2024 at 02:26:23PM +0000, Richard Kerry wrote: >> >> >> 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 > [...] > >> >> 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. > Will fix all of this and re-do the commit message, thanks all. > Thanks, > Taylor
diff --git a/midx-write.c b/midx-write.c index 22b5233f51ec6c6d99b8f9613818f1581dca5982..564438f616f59cd24edda956e4af0e0acf167138 100644 --- a/midx-write.c +++ b/midx-write.c @@ -760,7 +760,7 @@ static int read_refs_snapshot(const char *refs_snapshot, hex = &buf.buf[1]; } - if (parse_oid_hex(hex, &oid, &end) < 0) + if (parse_oid_hex_algop(hex, &oid, &end, revs->repo->hash_algo) < 0) die(_("could not parse line: %s"), buf.buf); if (*end) die(_("malformed line: %s"), buf.buf); @@ -776,6 +776,7 @@ static int read_refs_snapshot(const char *refs_snapshot, strbuf_release(&buf); return 0; } + static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr_p, const char *refs_snapshot, struct write_midx_context *ctx)
The `read_refs_snapshot` uses the `parse_oid_hex` function which internally uses global variables. Let's instead use `parse_oid_hex_algop` and provide the hash algo via `revs->repo`. Also, while here, fix a missing newline after the functions definition. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- midx-write.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)