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

On Tue, Dec 21 2021, Han-Wen Nienhuys wrote:

On Thu, Dec 16, 2021 at 2:45 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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.

quoted

int a one-line terany instead.

ternary.

quoted

"don't do negative comparison on enum values"

Will fix.

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

*nod*

quoted

collected.reflogs.strdup_strings = 1;

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

Will comment on it. FWWI this is a common pattern with the string_list
API.

If you declare it "dup" and push into it you'll end up double-duping,
but if you don't declare it dup'd and free it you'll leak memory. It
won't free() a non-duped list.

The other option is to declare it "dup" and then
"string_list_append_nodup", will try and see...

quoted

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

Will experiment & see, looks like a wrapper might be easiest here.

Thanks!