* [PATCH 1/4] worktree: drop pointless strbuf_release()
2020-07-31 23:32 [PATCH 0/4] worktree: cleanups & simplification Eric Sunshine
@ 2020-07-31 23:32 ` Eric Sunshine
2020-07-31 23:32 ` [PATCH 2/4] worktree: drop unused code from get_linked_worktree() Eric Sunshine
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-07-31 23:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Duy Nguyen, Michael Rappazzo, Eric Sunshine
The content of this strbuf is unconditionally detached several lines
before the strbuf_release() and the strbuf is never touched again after
that point.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Notes:
I'm on the fence about this change. On the one hand, I spent extra
cycles studying the code to determine if the strbuf was used again
after being detached. On the other hand, the strbuf_release() at the
end of the function protects against a leak if someone ever inserts
code which re-uses the strbuf. So, I wouldn't be bothered if this
patch is dropped from the series.
worktree.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/worktree.c b/worktree.c
index cba2e54598..c0df5e2c79 100644
--- a/worktree.c
+++ b/worktree.c
@@ -66,8 +66,6 @@ static struct worktree *get_main_worktree(void)
worktree->is_bare = (is_bare_repository_cfg == 1) ||
is_bare_repository();
add_head_info(worktree);
-
- strbuf_release(&worktree_path);
return worktree;
}
--
2.28.0.203.gce1f2e0ef1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/4] worktree: drop unused code from get_linked_worktree()
2020-07-31 23:32 [PATCH 0/4] worktree: cleanups & simplification Eric Sunshine
2020-07-31 23:32 ` [PATCH 1/4] worktree: drop pointless strbuf_release() Eric Sunshine
@ 2020-07-31 23:32 ` Eric Sunshine
2020-07-31 23:32 ` [PATCH 3/4] worktree: drop bogus and unnecessary path munging Eric Sunshine
2020-07-31 23:32 ` [PATCH 4/4] worktree: retire special-case normalization of main worktree path Eric Sunshine
3 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-07-31 23:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Duy Nguyen, Michael Rappazzo, Eric Sunshine
This code has been unused since fa099d2322 (worktree.c: kill parse_ref()
in favor of refs_resolve_ref_unsafe(), 2017-04-24), so drop it.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Notes:
This is a companion to 02bbbe9df9 (worktree: drop unused code from
get_main_worktree(), 2020-02-23), which also cleaned up fallout from
fa099d2322 (worktree.c: kill parse_ref() in favor of
refs_resolve_ref_unsafe(), 2017-04-24).
worktree.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/worktree.c b/worktree.c
index c0df5e2c79..fa8366a3ca 100644
--- a/worktree.c
+++ b/worktree.c
@@ -90,9 +90,6 @@ static struct worktree *get_linked_worktree(const char *id)
strbuf_strip_suffix(&worktree_path, "/.");
}
- strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
-
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->id = xstrdup(id);
--
2.28.0.203.gce1f2e0ef1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/4] worktree: drop bogus and unnecessary path munging
2020-07-31 23:32 [PATCH 0/4] worktree: cleanups & simplification Eric Sunshine
2020-07-31 23:32 ` [PATCH 1/4] worktree: drop pointless strbuf_release() Eric Sunshine
2020-07-31 23:32 ` [PATCH 2/4] worktree: drop unused code from get_linked_worktree() Eric Sunshine
@ 2020-07-31 23:32 ` Eric Sunshine
2020-07-31 23:32 ` [PATCH 4/4] worktree: retire special-case normalization of main worktree path Eric Sunshine
3 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-07-31 23:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Duy Nguyen, Michael Rappazzo, Eric Sunshine
The content of .git/worktrees/<id>/gitdir must be a path of the form
"/path/to/worktree/.git". Any other content would be indicative of a
corrupt "gitdir" file. To determine the path of the worktree itself one
merely strips the "/.git" suffix, and this is indeed how the worktree
path was determined from inception.
However, 5193490442 (worktree: add a function to get worktree details,
2015-10-08) extended the path manipulation in a mysterious way. If it is
unable to strip "/.git" from the path, then it instead reports the
current working directory as the linked worktree's path:
if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
strbuf_reset(&worktree_path);
strbuf_add_absolute_path(&worktree_path, ".");
strbuf_strip_suffix(&worktree_path, "/.");
}
This logic is clearly bogus; it can never be generally correct behavior.
It materialized out of thin air in 5193490442 with neither explanation
nor tests to illustrate a case in which it would be desirable.
It's possible that this logic was introduced to somehow deal with a
corrupt "gitdir" file, so that it returns _some_ sort of meaningful
value, but returning the current working directory is not helpful. In
fact, it is quite misleading (except in the one specific case when the
current directory is the worktree whose "gitdir" entry is corrupt).
Moreover, reporting the corrupt value to the user, rather than fibbing
about it and hiding it outright, is more helpful since it may aid in
diagnosing the problem.
Therefore, drop this bogus path munging and restore the logic to the
original behavior of merely stripping "/.git".
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
worktree.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/worktree.c b/worktree.c
index fa8366a3ca..355824bf87 100644
--- a/worktree.c
+++ b/worktree.c
@@ -82,13 +82,8 @@ static struct worktree *get_linked_worktree(const char *id)
if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
-
strbuf_rtrim(&worktree_path);
- if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
- strbuf_reset(&worktree_path);
- strbuf_add_absolute_path(&worktree_path, ".");
- strbuf_strip_suffix(&worktree_path, "/.");
- }
+ strbuf_strip_suffix(&worktree_path, "/.git");
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
--
2.28.0.203.gce1f2e0ef1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4/4] worktree: retire special-case normalization of main worktree path
2020-07-31 23:32 [PATCH 0/4] worktree: cleanups & simplification Eric Sunshine
` (2 preceding siblings ...)
2020-07-31 23:32 ` [PATCH 3/4] worktree: drop bogus and unnecessary path munging Eric Sunshine
@ 2020-07-31 23:32 ` Eric Sunshine
3 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-07-31 23:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Duy Nguyen, Michael Rappazzo, Eric Sunshine
In order for "git-worktree list" to present consistent results,
get_main_worktree() performs manual normalization on the repository
path (returned by get_common_dir()) after passing it through
strbuf_add_absolute_path(). In particular, it cleans up the path for
three distinct cases when the current working directory is (1) the main
worktree, (2) the .git/ subdirectory, or (3) a bare repository.
The need for such special-cases is a direct consequence of employing
strbuf_add_absolute_path() which, for the sake of efficiency, doesn't
bother normalizing the path (such as folding out redundant path
components) after making it absolute. Lack of normalization is not
typically a problem since redundant path elements make no difference
when working with paths at the filesystem level. However, when preparing
paths for presentation, possible redundant path components make it
difficult to ensure consistency.
Eliminate the need for these special cases by instead making the path
absolute via strbuf_add_real_path() which normalizes the path for us.
Once normalized, the only case we need to handle manually is converting
it to the path of the main worktree by stripping the "/.git" suffix.
This stripping of the "/.git" suffix is a regular idiom in
worktree-related code; for instance, it is employed by
get_linked_worktree(), as well.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
Notes:
This is a followup to 5f4ee57ad9 (worktree: avoid dead-code in
conditional, 2020-06-24) which dropped dead code from the manual
normalization done by get_main_worktree() but which did not
eliminate the special cases.
It's also a direct response to Junio's observation[1] that it would
be better if we didn't have to handle these special cases in the
first place.
[1]: https://lore.kernel.org/git/xmqqbll8569x.fsf@gitster.c.googlers.com/
worktree.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/worktree.c b/worktree.c
index 355824bf87..62217b4a6b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -49,10 +49,8 @@ static struct worktree *get_main_worktree(void)
struct worktree *worktree = NULL;
struct strbuf worktree_path = STRBUF_INIT;
- strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
- 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 */
+ strbuf_add_real_path(&worktree_path, get_git_common_dir());
+ strbuf_strip_suffix(&worktree_path, "/.git");
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
--
2.28.0.203.gce1f2e0ef1
^ permalink raw reply related [flat|nested] 5+ messages in thread