On Fri, Apr 19, 2019 at 1:42 AM Jeff King <peff@xxxxxxxx> wrote: > > On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote: > > > > > 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. Yeah. It's added the same time get_linked_worktree() adds absolute paths and trims "/." in 5193490442 (worktree: add a function to get worktree details - 2015-10-08). Maybe it's because he wasn't sure if get_git_common_dir() could return ".", which makes it exactly the same as get_linked_worktree(). It's probably very unlikely that git_git_common_dir() could return ".", but I can't be sure either. -- Duy
