Subject: Re: [PATCH v2 4/4] builtin/multi-pack-index.c: split sub-commands

From: Taylor Blau <[email-masked]>
Date: Mon, 15 Feb 2021 18:49:17 -0500

On Tue, Feb 16, 2021 at 12:11:08AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I split this into two patches: one to move the trace2_cmd_mode() calls
> > around, and another to replace the final 'die()' with the usage text.
>
> Thanks for picking it up.

Of course. This has been quite a fun digression :-).

> > Like I said in my review of your patches to the commit-graph builtin
> > here:
> >
> >     https://lore.kernel.org/git/[email-masked]/
> >
> > I don't find the 'if (argc && ...)' style more readable, so the second
> > patch looks like this instead:
>
> *Nod* FWIW (and this is getting way to nit-y) I don't disagree with you
> about the "argc &&" being not very readable,
>
> I just lean more on the side of getting rid of duplicate branches,
> you'll still need the if (!argc) usage(...) case above without that
> pattern, or some replacement for it.
>
> But we can have our cake (not re-check argc all the time) and eat it too
> (not copy/paste usage_with_options()). Isn't it beautiful?

Heh; I'm not sure that I'd call adding a goto "beautiful", but I
actually do find this one more readable. I dunno, honestly, I'm happy to
squash it in to the last commit on top, but honestly I don't really care
strongly one way or another ;).

> > Is it OK if I use your Signed-off-by on both of those two new patches?
>
> Yes please, should have included it to begin with.

Thanks, and no worries.

Thanks,
Taylor