Hi Eric, On Mon, 24 Feb 2020, Eric Sunshine wrote: > On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > > > This change makes the code unnecessarily confusing and effectively > > > > turns the final line into dead code. I would much rather see the three > > > > cases spelled out explicitly, perhaps like this: > > > > > > > > if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */ > > > > !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */ > > > > strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */ > > > > I would be really cautious about that. > > > > To me, the originally proposed change says: strip `/.`, if any. Then, > > strip `/.git`, and if successful, strip another `/.`, if any. > > That's not at all what the original said, which is reproduced here: > > if (!strbuf_strip_suffix(&worktree_path, "/.git")) > strbuf_strip_suffix(&worktree_path, "/."); > > It says "try stripping '/.git'; if that fails, try stripping '/.'". > That is, it recognizes and handles two distinct cases: (1) the path to > the .git directory of a non-bare repository, which always ends with > "/.git", and (2) the path to a bare git repository, which always ends > with "/.". So, the original code wasn't doing any sort of incremental > stripping of suffixes; it was just handling two known distinct cases. > > Perhaps you missed the '!' in the conditional? I totally did. Sorry! Ciao, Dscho
