git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: git@vger.kernel.org
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Hariom verma <hariom18599@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH] worktree: avoid dead-code in conditional
Date: Wed, 24 Jun 2020 15:05:41 -0400	[thread overview]
Message-ID: <20200624190541.5253-1-sunshine@sunshineco.com> (raw)

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


             reply	other threads:[~2020-06-24 19:06 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 19:05 Eric Sunshine [this message]
2020-06-24 21:20 ` [PATCH] worktree: avoid dead-code in conditional Junio C Hamano
2020-06-24 23:00   ` Eric Sunshine
2020-06-25  0:38     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200624190541.5253-1-sunshine@sunshineco.com \
    --to=sunshine@sunshineco.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=hariom18599@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).