{"thread":{"id":"53957","subject":"[PATCH 0/4] worktree: cleanups & simplification","startedAt":"2020-07-31T23:33:20Z","lastAt":"2020-07-31T23:33:28Z","messageCount":5,"participants":["Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"402609","messageId":"20200731233214.22131-1-sunshine@sunshineco.com","threadId":"53957","inReplyTo":null,"subject":"[PATCH 0/4] worktree: cleanups & simplification","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-31T23:32:10Z","receivedAt":"2020-07-31T23:33:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The primary change of this series is a response to Junio's\nobservation[1] that it would be nice if get_main_worktree() didn't\nemploy special cases for path normalization. I was planning on\ninvestigating the topic eventually anyway but ended up having to\ntackle the same sort of manual special case normalization in another\nworktree-related topic I'm working on, thus this series fell out\nnaturally from that other work. Along the way, while re-studying the\nworktree code, I came across a few other issues which deserved\nattention, but which are not directly related to the other topic I'm\nworking on, so I bundled all these small changes together here.\n\nEric Sunshine (4):\n worktree: drop pointless strbuf_release()\n worktree: drop unused code from get_linked_worktree()\n worktree: drop bogus and unnecessary path munging\n worktree: retire special-case normalization of main worktree path\n\n worktree.c | 18 +++---------------\n 1 file changed, 3 insertions(+), 15 deletions(-)\n\n-- \n2.28.0.203.gce1f2e0ef1\n\n"},{"id":"402610","messageId":"20200731233214.22131-4-sunshine@sunshineco.com","threadId":"53957","inReplyTo":"20200731233214.22131-1-sunshine@sunshineco.com","subject":"[PATCH 3/4] worktree: drop bogus and unnecessary path munging","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-31T23:32:13Z","receivedAt":"2020-07-31T23:33:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The content of .git/worktrees//gitdir must be a path of the form\n\"/path/to/worktree/.git\". Any other content would be indicative of a\ncorrupt \"gitdir\" file. To determine the path of the worktree itself one\nmerely strips the \"/.git\" suffix, and this is indeed how the worktree\npath was determined from inception.\n\nHowever, 5193490442 (worktree: add a function to get worktree details,\n2015-10-08) extended the path manipulation in a mysterious way. If it is\nunable to strip \"/.git\" from the path, then it instead reports the\ncurrent working directory as the linked worktree's path:\n\n if (!strbuf_strip_suffix(&worktree_path, \"/.git\")) {\n strbuf_reset(&worktree_path);\n strbuf_add_absolute_path(&worktree_path, \".\");\n strbuf_strip_suffix(&worktree_path, \"/.\");\n }\n\nThis logic is clearly bogus; it can never be generally correct behavior.\nIt materialized out of thin air in 5193490442 with neither explanation\nnor tests to illustrate a case in which it would be desirable.\n\nIt's possible that this logic was introduced to somehow deal with a\ncorrupt \"gitdir\" file, so that it returns _some_ sort of meaningful\nvalue, but returning the current working directory is not helpful. In\nfact, it is quite misleading (except in the one specific case when the\ncurrent directory is the worktree whose \"gitdir\" entry is corrupt).\nMoreover, reporting the corrupt value to the user, rather than fibbing\nabout it and hiding it outright, is more helpful since it may aid in\ndiagnosing the problem.\n\nTherefore, drop this bogus path munging and restore the logic to the\noriginal behavior of merely stripping \"/.git\".\n\nSigned-off-by: Eric Sunshine \n---\n worktree.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex fa8366a3ca..355824bf87 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -82,13 +82,8 @@ static struct worktree *get_linked_worktree(const char *id)\n \tif (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)\n \t\t/* invalid gitdir file */\n \t\tgoto done;\n-\n \tstrbuf_rtrim(&worktree_path);\n-\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\")) {\n-\t\tstrbuf_reset(&worktree_path);\n-\t\tstrbuf_add_absolute_path(&worktree_path, \".\");\n-\t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n-\t}\n+\tstrbuf_strip_suffix(&worktree_path, \"/.git\");\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n-- \n2.28.0.203.gce1f2e0ef1\n\n"},{"id":"402611","messageId":"20200731233214.22131-3-sunshine@sunshineco.com","threadId":"53957","inReplyTo":"20200731233214.22131-1-sunshine@sunshineco.com","subject":"[PATCH 2/4] worktree: drop unused code from get_linked_worktree()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-31T23:32:12Z","receivedAt":"2020-07-31T23:33:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This code has been unused since fa099d2322 (worktree.c: kill parse_ref()\nin favor of refs_resolve_ref_unsafe(), 2017-04-24), so drop it.\n\nSigned-off-by: Eric Sunshine \n---\n\nNotes:\n This is a companion to 02bbbe9df9 (worktree: drop unused code from\n get_main_worktree(), 2020-02-23), which also cleaned up fallout from\n fa099d2322 (worktree.c: kill parse_ref() in favor of\n refs_resolve_ref_unsafe(), 2017-04-24).\n\n worktree.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex c0df5e2c79..fa8366a3ca 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -90,9 +90,6 @@ static struct worktree *get_linked_worktree(const char *id)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \t}\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-- \n2.28.0.203.gce1f2e0ef1\n\n"},{"id":"402612","messageId":"20200731233214.22131-5-sunshine@sunshineco.com","threadId":"53957","inReplyTo":"20200731233214.22131-1-sunshine@sunshineco.com","subject":"[PATCH 4/4] worktree: retire special-case normalization of main worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-31T23:32:14Z","receivedAt":"2020-07-31T23:33:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In order for \"git-worktree list\" to present consistent results,\nget_main_worktree() performs manual normalization on the repository\npath (returned by get_common_dir()) after passing it through\nstrbuf_add_absolute_path(). In particular, it cleans up the path for\nthree distinct cases when the current working directory is (1) the main\nworktree, (2) the .git/ subdirectory, or (3) a bare repository.\n\nThe need for such special-cases is a direct consequence of employing\nstrbuf_add_absolute_path() which, for the sake of efficiency, doesn't\nbother normalizing the path (such as folding out redundant path\ncomponents) after making it absolute. Lack of normalization is not\ntypically a problem since redundant path elements make no difference\nwhen working with paths at the filesystem level. However, when preparing\npaths for presentation, possible redundant path components make it\ndifficult to ensure consistency.\n\nEliminate the need for these special cases by instead making the path\nabsolute via strbuf_add_real_path() which normalizes the path for us.\nOnce normalized, the only case we need to handle manually is converting\nit to the path of the main worktree by stripping the \"/.git\" suffix.\nThis stripping of the \"/.git\" suffix is a regular idiom in\nworktree-related code; for instance, it is employed by\nget_linked_worktree(), as well.\n\nSigned-off-by: Eric Sunshine \n---\n\nNotes:\n This is a followup to 5f4ee57ad9 (worktree: avoid dead-code in\n conditional, 2020-06-24) which dropped dead code from the manual\n normalization done by get_main_worktree() but which did not\n eliminate the special cases.\n \n It's also a direct response to Junio's observation[1] that it would\n be better if we didn't have to handle these special cases in the\n first place.\n \n [1]: https://lore.kernel.org/git/xmqqbll8569x.fsf@gitster.c.googlers.com/\n\n worktree.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 355824bf87..62217b4a6b 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -49,10 +49,8 @@ static struct worktree *get_main_worktree(void)\n \tstruct worktree *worktree = NULL;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \n-\tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\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+\tstrbuf_add_real_path(&worktree_path, get_git_common_dir());\n+\tstrbuf_strip_suffix(&worktree_path, \"/.git\");\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n-- \n2.28.0.203.gce1f2e0ef1\n\n"},{"id":"402613","messageId":"20200731233214.22131-2-sunshine@sunshineco.com","threadId":"53957","inReplyTo":"20200731233214.22131-1-sunshine@sunshineco.com","subject":"[PATCH 1/4] worktree: drop pointless strbuf_release()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-31T23:32:11Z","receivedAt":"2020-07-31T23:33:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The content of this strbuf is unconditionally detached several lines\nbefore the strbuf_release() and the strbuf is never touched again after\nthat point.\n\nSigned-off-by: Eric Sunshine \n---\n\nNotes:\n I'm on the fence about this change. On the one hand, I spent extra\n cycles studying the code to determine if the strbuf was used again\n after being detached. On the other hand, the strbuf_release() at the\n end of the function protects against a leak if someone ever inserts\n code which re-uses the strbuf. So, I wouldn't be bothered if this\n patch is dropped from the series.\n\n worktree.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex cba2e54598..c0df5e2c79 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -66,8 +66,6 @@ static struct worktree *get_main_worktree(void)\n \tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n \t\tis_bare_repository();\n \tadd_head_info(worktree);\n-\n-\tstrbuf_release(&worktree_path);\n \treturn worktree;\n }\n \n-- \n2.28.0.203.gce1f2e0ef1\n\n"}]}