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

From

Han-Wen Nienhuys <hanwen@google.com>

Date

Dec 21, 2021, 14:24 UTC

Message-ID

<CAFQ2z_McOfm545Xd8hF7YDgzyOjDmcGxpWZ6pQ-yaKAEWMMbgg@mail.gmail.com>

In-Reply-To

<cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com>

>
> 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"

> collected.reflogs.strdup_strings = 1;

>  .. 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

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