Re: [PATCH v2 0/9] reftable prep: have builtin/reflog.c handle "--verbose"

[Han-Wen Nienhuys <hanwen@google.com>]

On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> This series refactors various small bits of builtin/reflog.c
> (e.g. using a "struct string_list" now), and finally makes it handle
> the "--verbose" output, instead of telling the files backend to emit
> that same verbose output.
>
> This means that when we start to integrate "reftable" the new backend
> won't need to implement verbose reflog output, it will just work.
>
> This is a sort-of v2[1]. I ejected the changes at the end to add
> better --progress output to "git reflog". Those fixes are worthwhile,
> but hopefully this smaller & easier to review series can be queued up
> first, we can do those UX improvements later.

Thanks for sending this.

I looked over all patches separately. Overall, the series looks good to me.

> int a one-line terany instead.

ternary.

> "don't do negative comparison on enum values"

I would describe it as "use switch over enum values", as this doesn't
involve negative numbers.

> collected.reflogs.strdup_strings = 1;

This puzzled me. Why isn't the init done as _DUP ? Warrants a comment
at the least.

>  .. goto expire
> ..
>    return 0;
> expire:

(personal opinion) this is going overboard with gotos and labels. Either

  if ( .. ) {
    expire = 1;
    goto done;
  }
done:
  if (expire) {  print stuff }
  return expire

Or wrap the existing function (without changes) in a callback that
does the print for you.

your call.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado