Re: [PATCH v3 2/2] worktree add: add --orphan flag

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Nov 18, 2022 at 8:44 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> On 22/11/15 10:08PM, Ævar Arnfjörð Bjarmason wrote:
> > > +   test_must_fail git worktree add --orphan existingbranch orphandir2 &&
> > > +   test ! -d orphandir2
> >
> > I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants
> > to be a "test_path_is_missing"?
> >
> > In general we try to test what a thing is, not what it isn't, in this
> > case don't we fail to create the dir entirely? So "not exists" would
> > cover it?
>
> Ah yes that would be preferable. I've updated it for v4.
>
> This shows up in the file in a few other places in this file as well
> (from before this patch). Should I make the changes there as well and put
> those changes into an additional patch in this patchset?

With my reviewer's hat on, my goal is to help the series land in
Junio's tree, which means I'd like to see fewer changes with each
iteration. Adding a new patch which is only tangentially related to
what the series wants to achieve isn't a priority, and could end up
delaying acceptance of the series if problems in the new patch end up
requiring additional rerolls.

So, yes, you could do that cleanup as a preparatory patch in the
series if you want to tackle it. It would be an appropriate cleanup
since you're working on code nearby. Or it could be done as a follow
up to this series. Given how small the cleanup patch would likely be,
it may not make a difference one way or the other, especially if the
commit message explains the change well (for instance, by paraphrasing
what Ævar said about "testing what a thing is, not what it isn't").