{"thread":{"id":"53750","subject":"[PATCH] worktree: avoid dead-code in conditional","startedAt":"2020-06-24T19:06:45Z","lastAt":"2020-06-25T00:38:27Z","messageCount":4,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"400552","messageId":"20200624190541.5253-1-sunshine@sunshineco.com","threadId":"53750","inReplyTo":null,"subject":"[PATCH] worktree: avoid dead-code in conditional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-24T19:05:41Z","receivedAt":"2020-06-24T19:06:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"get_worktrees() retrieves a list of all worktrees associated with a\nrepository, including the main worktree. The location of the main\nworktree is determined by get_main_worktree() which needs to handle\nthree distinct cases for the main worktree after absolute-path\nconversion:\n\n    * <bare-repository>/.\n    * <main-worktree>/.git/. (when $CWD is .git)\n    * <main-worktree>/.git (when $CWD is any worktree)\n\nThey all need to be normalized to just the <path> portion, dropping any\n\"/.\" or \"/.git\" suffix.\n\nIt turns out, however, that get_main_worktree() was only handling the\nfirst and last cases, i.e.:\n\n    if (!strip_suffix(path, \"/.git\"))\n        strip_suffix(path, \"/.\");\n\nThis shortcoming was addressed by 45f274fbb1 (get_main_worktree(): allow\nit to be called in the Git directory, 2020-02-23) by changing the logic\nto:\n\n    strip_suffix(path, \"/.\");\n    if (!strip_suffix(path, \"/.git\"))\n        strip_suffix(path, \"/.\");\n\nwhich makes the final strip_suffix() invocation dead-code.\n\nFix this oversight by enumerating the three distinct cases explicitly\nrather than attempting to strip the suffix(es) incrementally.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nThis \"dead-code problem\" was caught during review[1], however, the patch\nwas never re-rolled and ended up being accepted as-is. Dscho had raised\nan objection[2] to the suggested way to fix the dead-code problem but\nhis objection was based upon a misunderstanding[3] thus (implicitly)\nwithdrawn.\n\nAn alternative to enumerating the three distinct cases, as this patch\ndoes, would be to strip suffix components incrementally like this:\n\n    strip_suffix(path, \"/.\");\n    strip_suffix(path, \"/.git\");\n\nHowever, such code is relatively opaque and would require a largish\nin-code comment mirroring a chunk of the comment message of this patch.\nThe chosen approach, on the other hand, is relatively self-documenting,\nrequiring only very small inline source-code comments.\n\n[1]: https://lore.kernel.org/git/CAPig+cTh-uu-obh9aeDOV9ptbVwRmkujgucbu9ei1Qa3qSNG_A@mail.gmail.com/\n[2]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet/\n[3]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002271658250.46@tvgsbejvaqbjf.bet/\n\n worktree.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex ee82235f26..b5cdee34de 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -50,9 +50,9 @@ static struct worktree *get_main_worktree(void)\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n-\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n-\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n-\t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n+\tif (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git */\n+\t    !strbuf_strip_suffix(&worktree_path, \"/.git\")) /* in worktree */\n+\t\tstrbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n-- \n2.27.0.288.g4b34aa94c7\n\n"},{"id":"400557","messageId":"xmqqsgek5fg3.fsf@gitster.c.googlers.com","threadId":"53750","inReplyTo":"20200624190541.5253-1-sunshine@sunshineco.com","subject":"Re: [PATCH] worktree: avoid dead-code in conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T21:20:12Z","receivedAt":"2020-06-24T21:20:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> get_worktrees() retrieves a list of all worktrees associated with a\n> repository, including the main worktree. The location of the main\n> worktree is determined by get_main_worktree() which needs to handle\n> three distinct cases for the main worktree after absolute-path\n> conversion:\n>\n>     * <bare-repository>/.\n>     * <main-worktree>/.git/. (when $CWD is .git)\n>     * <main-worktree>/.git (when $CWD is any worktree)\n\nIt is unclear from the above but I would assume that you are talking\nabout the returned path from get_git_common_dir().\n\nI can certainly understand why there needs two distinct cases\n(i.e.. bare vs non-bare), but why is this codepath (or any caller of\nget_git_common_dir()) forced to care about the two cases?\n\nI wonder if the right \"fix\" to this instance, at the same time\npreventing similar breakages in the future, is rather make sure\nget_git_common_dir() not to return the redundant path with \".git/.\"\nsuffixed?  For that matter, I do not know why the bare case must\nneed \"/.\" suffix.  There seem to be about a dozen callers of the\nfunction, but don't some of them share a similar issue?  \n\nLet's look at the other two grep hits from worktree.c\n\n\tstrbuf_reset(&path);\n\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n\n\tworktree = xcalloc(1, sizeof(*worktree));\n\tworktree->path = strbuf_detach(&worktree_path, NULL);\n\tworktree->id = xstrdup(id);\n\tadd_head_info(worktree);\n\ndone:\n\tstrbuf_release(&path);\n\tstrbuf_release(&worktree_path);\n\treturn worktree;\n}\n\nThis looks somewhat bogus.  \"sturct strbuf path\" is populated, but\nis released without ever getting used, isn't it?  Am I grossly\nmisreading the code?\n\nThe other one\n\n\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n\tdir = opendir(path.buf);\n\tstrbuf_release(&path);\n\tif (dir) {\n\t\twhile ((d = readdir(dir)) != NULL) {\n\t\t...\n\nis a regular filesystem access, but it ends up opening the directory\nwith a path like \"foo/.git/./worktrees\", where we should be using a\nmore reasonable \"foo/.git/worktrees\" to access it.  The redundant\n\"/./\" is not wrong per-se, but it looks sloppy to depend on \"not\nwrong per-se\".\n\nPuzzled.\n"},{"id":"400563","messageId":"CAPig+cRdRyWAjU7QADPt39rDgJ4njDtN_pwo8JMVD=YX09a-Vg@mail.gmail.com","threadId":"53750","inReplyTo":"xmqqsgek5fg3.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] worktree: avoid dead-code in conditional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-24T23:00:37Z","receivedAt":"2020-06-24T23:00:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 24, 2020 at 5:20 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > get_worktrees() retrieves a list of all worktrees associated with a\n> > repository, including the main worktree. The location of the main\n> > worktree is determined by get_main_worktree() which needs to handle\n> > three distinct cases for the main worktree after absolute-path\n> > conversion:\n> >\n> >   * <bare-repository>/.\n> >   * <main-worktree>/.git/. (when $CWD is .git)\n> >   * <main-worktree>/.git (when $CWD is any worktree)\n>\n> It is unclear from the above but I would assume that you are talking\n> about the returned path from get_git_common_dir().\n\nNo, I am talking about the result of strbuf_add_absolute_path()\napplied to get_git_common_dir(). I intentionally explicitly stated\n\"after absolute-path conversion\" in the commit message to be clear\nabout this, but perhaps I should have mentioned\nstrbuf_add_absolute_path() by name to avoid confusion.\n\n> I can certainly understand why there needs two distinct cases\n> (i.e.. bare vs non-bare), but why is this codepath (or any caller of\n> get_git_common_dir()) forced to care about the two cases?\n\nCallers of get_git_common_dir() aren't forced to care. But after\napplying strbuf_add_absolute_path() we are forced to care. This is the\nresult of get_git_common_dir() for the three cases:\n\n    .     (within bare repo)\n    .git  (within any worktree)\n    .     (within .git)\n\nAfter applying strbuf_add_absolute_path(), we get:\n\n    /whatever/proj.git/.   (within bare proj)\n    /whatever/proj/.git    (within any worktree)\n    /whatever/proj/.git/.  (within .git)\n\nThese need to be normalized, respectively, to:\n\n    /whatever/proj.git  (within bare proj)\n    /whatever/proj      (within any worktree)\n    /whatever/proj      (within .git)\n\n> I wonder if the right \"fix\" to this instance, at the same time\n> preventing similar breakages in the future, is rather make sure\n> get_git_common_dir() not to return the redundant path with \".git/.\"\n> suffixed? For that matter, I do not know why the bare case must\n> need \"/.\" suffix. There seem to be about a dozen callers of the\n> function, but don't some of them share a similar issue?\n>\n> Puzzled.\n\nYour puzzlement may arise from the misunderstanding regarding\nget_git_common_dir() vs. strbuf_add_absolute_path()?\n\nIt might be nice to refine strbuf_add_absolute_path() to return more\naesthetically pleasing results (i.e. not add trailing \".\" when\nunneeded), but such a change is outside the scope of this small patch\nand may have repercussions which could be a time sink (and I haven't\nthe time to devote to it).\n\n> Let's look at the other two grep hits from worktree.c\n>\n>     strbuf_reset(&path);\n>     strbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n>\n>     worktree = xcalloc(1, sizeof(*worktree));\n>     worktree->path = strbuf_detach(&worktree_path, NULL);\n>     worktree->id = xstrdup(id);\n>     add_head_info(worktree);\n>\n> done:\n>     strbuf_release(&path);\n>     strbuf_release(&worktree_path);\n>     return worktree;\n> }\n>\n> This looks somewhat bogus. \"sturct strbuf path\" is populated, but\n> is released without ever getting used, isn't it? Am I grossly\n> misreading the code?\n\nI think you're reading it just fine; that code is indeed dead. This\nappears to be fallout from 5193490442 (worktree: add a function to get\nworktree details, 2015-10-08), which unfortunately (I think) got\nlittle or no review[1].\n\nThis could indeed use a \"dead-code removal\" patch of its own, though\nit's unrelated to the current patch.\n\nFOOTNOTES\n\n[1] I had reviewed the very first version of the patch series which\n    introduced these changes, but then effectively went offline for\n    many, many months, thus was unable to review any of the re-rolls.\n"},{"id":"400567","messageId":"xmqqbll8569x.fsf@gitster.c.googlers.com","threadId":"53750","inReplyTo":"CAPig+cRdRyWAjU7QADPt39rDgJ4njDtN_pwo8JMVD=YX09a-Vg@mail.gmail.com","subject":"Re: [PATCH] worktree: avoid dead-code in conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-25T00:38:18Z","receivedAt":"2020-06-25T00:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Callers of get_git_common_dir() aren't forced to care. But after\n> applying strbuf_add_absolute_path() we are forced to care. This is the\n> result of get_git_common_dir() for the three cases:\n>\n>     .     (within bare repo)\n>     .git  (within any worktree)\n>     .     (within .git)\n>\n> After applying strbuf_add_absolute_path(), we get:\n>\n>     /whatever/proj.git/.   (within bare proj)\n>     /whatever/proj/.git    (within any worktree)\n>     /whatever/proj/.git/.  (within .git)\n\nOK, but the point still stands.  \n\nShouldn't strbuf_add_absolute_path() be what we want to normalize?\n\n> Your puzzlement may arise from the misunderstanding regarding\n> get_git_common_dir() vs. strbuf_add_absolute_path()?\n\nYes, exactly.\n\nThanks.\n"}]}