[PATCH] worktree: avoid dead-code in conditional

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 24, 2020, 19:05 UTC
Message-ID: <20200624190541.5253-1-sunshine@sunshineco.com>

get_worktrees() retrieves a list of all worktrees associated with a repository, including the main worktree. The location of the main worktree is determined by get_main_worktree() which needs to handle three distinct cases for the main worktree after absolute-path conversion:

    * <bare-repository>/.
    * <main-worktree>/.git/. (when $CWD is .git)
    * <main-worktree>/.git (when $CWD is any worktree)

They all need to be normalized to just the <path> portion, dropping any "/." or "/.git" suffix.

It turns out, however, that get_main_worktree() was only handling the first and last cases, i.e.:

    if (!strip_suffix(path, "/.git"))
        strip_suffix(path, "/.");

This shortcoming was addressed by 45f274fbb1 (get_main_worktree(): allow it to be called in the Git directory, 2020-02-23) by changing the logic to:

    strip_suffix(path, "/.");
    if (!strip_suffix(path, "/.git"))
        strip_suffix(path, "/.");

which makes the final strip_suffix() invocation dead-code.

Fix this oversight by enumerating the three distinct cases explicitly rather than attempting to strip the suffix(es) incrementally.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

This "dead-code problem" was caught during review[1], however, the patch was never re-rolled and ended up being accepted as-is. Dscho had raised an objection[2] to the suggested way to fix the dead-code problem but his objection was based upon a misunderstanding[3] thus (implicitly) withdrawn.

An alternative to enumerating the three distinct cases, as this patch does, would be to strip suffix components incrementally like this:

    strip_suffix(path, "/.");
    strip_suffix(path, "/.git");

However, such code is relatively opaque and would require a largish in-code comment mirroring a chunk of the comment message of this patch. The chosen approach, on the other hand, is relatively self-documenting, requiring only very small inline source-code comments.

[1]: https://lore.kernel.org/git/CAPig+cTh-uu-obh9aeDOV9ptbVwRmkujgucbu9ei1Qa3qSNG_A@mail.gmail.com/ [2]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet/ [3]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002271658250.46@tvgsbejvaqbjf.bet/

 worktree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/worktree.c b/worktree.c
index ee82235f26..b5cdee34de 100644
--- a/worktree.c
+++ b/worktree.c
@@ -50,9 +50,9 @@ static struct worktree *get_main_worktree(void)
 	struct strbuf worktree_path = STRBUF_INIT;
 
 	strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
-	strbuf_strip_suffix(&worktree_path, "/.");
-	if (!strbuf_strip_suffix(&worktree_path, "/.git"))
-		strbuf_strip_suffix(&worktree_path, "/.");
+	if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git */
+	    !strbuf_strip_suffix(&worktree_path, "/.git")) /* in worktree */
+		strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
 
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);

-- 
2.27.0.288.g4b34aa94c7