Re: [PATCH] worktree: update is_bare heuristics

From: Jeff King <peff@peff.net>
Date: Apr 18, 2019, 18:42 UTC
Message-ID: <20190418184205.GA12260@sigill.intra.peff.net>
In-Reply-To: <20190418183000.78138-1-jonathantanmy@google.com>

On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote:

Show 13 quoted lines

> > >         strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
> > > -       is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> > > -       if (is_bare)
> > > +       if (!strbuf_strip_suffix(&worktree_path, "/.git"))
> > >                 strbuf_strip_suffix(&worktree_path, "/.");
> > 
> > We can just call these two calls unconditionally, right? No harm done
> > if we don't strip.
> 
> We can, and no harm done. But this if/then pattern is also repeated in
> other parts of the file (e.g. get_linked_worktree()) so I'll leave it in
> for consistency. (Also, for what it's worth, it's slightly faster if
> only one strip is done.)

I also think your version expresses the intent more clearly. We expect to see one or the other, but not "foo/./.git". And so (just as the code prior to your patch) we would not convert that to "foo".

I am not sure of exactly what the "/." is trying to accomplish, so maybe that double-strip _would_ be desirable, but if so it is definitely worthy of its own commit explaining why that is so.

Interestingly, the case in get_linked_worktree() makes a lot more sense because it has added "." as an absolute path itself, and is just cleaning up the results of its strbuf_add_absolute_path()[1]. Which makes me wonder if the "/." stripping in get_main_worktree() is actually cargo-culted and simply unnecessary.

-Peff

[1] It seems like it would be simpler to just use strbuf_getcwd() for
    this, but maybe I am missing something.