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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 21, 2021, 15:21 UTC
Message-ID: <211221.86lf0eq8qj.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_McOfm545Xd8hF7YDgzyOjDmcGxpWZ6pQ-yaKAEWMMbgg@mail.gmail.com>

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

Show 25 quoted lines

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

Will fix.

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

*nod*

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

Show 20 quoted lines

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