On Tue, Dec 21 2021, Han-Wen Nienhuys wrote:
> 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"
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...
>> .. 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!
