| Message ID | cover-v2-0.9-00000000000-20211216T134028Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | reftable prep: have builtin/reflog.c handle "--verbose" | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. I was quite confused even after spending quite some time until I realized that you are primarily talking about the "-v" option of "git reflog expire" (and "delete", perhaps) and not about "git reflog -v" having different output from the same command without "-v" option. I'll refrain from commenting on, or picking up, the series until Han-Wen has a chance to take a look and give his input as it is unclear if it helps or crashes with his plan.
On Thu, Dec 16 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 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. > > I was quite confused even after spending quite some time until I > realized that you are primarily talking about the "-v" option of > "git reflog expire" (and "delete", perhaps) and not about "git > reflog -v" having different output from the same command without > "-v" option. Will try to clear that up in an eventual re-roll, but considering this a "look at only if re-rolling for other reasons" for now. > I'll refrain from commenting on, or picking up, the series until > Han-Wen has a chance to take a look and give his input as it is > unclear if it helps or crashes with his plan. He hasn't reviewed this series. But I think I/he have looked at it sufficiently that you might want to consider picking it up, particularly since he doesn't work on Friday's & with the holidays etc. it might be a while. He commented on this helping with his plans recently here: https://lore.kernel.org/git/CAFQ2z_PSS9zOzR6nGYZ8DBK+6oOQzkJsEy_7y+NprwJ1OHNs7w@mail.gmail.com/ The snippet he's referring to can (last seen on-list, AFAICT) be seen here: https://lore.kernel.org/git/3d57f7c443082fd2a7f01aee003a9cd3ca2dd910.1629207607.git.gitgitgadget@gmail.com/ Which from my reading of the code (I haven't actually tried to combine this with reftable/* for real) would Just Work when combined with this series. I.e. he had "dry_run" implemented, but not "verbose". Now he'll get "verbose" for free. The "XXX" comment there about dry_run appears to be a TODO about not doing needless work within the reflog backend.
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" 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.
On Tue, Dec 21 2021, Han-Wen Nienhuys wrote: > 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... >> .. 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!