On Fri, Nov 18, 2022 at 8:44 PM Jacob Abel <jacobabel@xxxxxxxxxx> 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").
