On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> 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
