On 22/11/22 01:00AM, Eric Sunshine wrote: > 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"). In that case I'll make a note and send in a cleanup patch with that change (referencing this thread) some time down the road after this series.
