[v3,2/2] git_path(): handle `.lock` files correctly
diff mbox series

Message ID 8b1f4f089a6d4a2293507a1a77668c828598e84f.1571694882.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Handle git_path() with lock files correctly in worktrees
Related show

Commit Message

Johannes Schindelin via GitGitGadget Oct. 21, 2019, 9:54 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Ever since worktrees were introduced, the `git_path()` function _really_
needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
specific to the worktree, and therefore so is its reflog). However, the
wrong path is returned for `logs/HEAD.lock`.

This does not matter as long as the Git executable is doing the asking,
as the path for that `logs/HEAD.lock` file is constructed from
`git_path("logs/HEAD")` by appending the `.lock` suffix.

However, Git GUI just learned to use `--git-path` instead of appending
relative paths to what `git rev-parse --git-dir` returns (and as a
consequence not only using the correct hooks directory, but also using
the correct paths in worktrees other than the main one). While it does
not seem as if Git GUI in particular is asking for `logs/HEAD.lock`,
let's be safe rather than sorry.

Side note: Git GUI _does_ ask for `index.lock`, but that is already
resolved correctly, due to `update_common_dir()` preferring to leave
unknown paths in the (worktree-specific) git directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 path.c               |  8 +++++++-
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

SZEDER Gábor Oct. 22, 2019, 4:01 p.m. UTC | #1
On Mon, Oct 21, 2019 at 09:54:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Ever since worktrees were introduced, the `git_path()` function _really_
> needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
> specific to the worktree, and therefore so is its reflog). However, the
> wrong path is returned for `logs/HEAD.lock`.
> 
> This does not matter as long as the Git executable is doing the asking,
> as the path for that `logs/HEAD.lock` file is constructed from
> `git_path("logs/HEAD")` by appending the `.lock` suffix.
> 
> However, Git GUI just learned to use `--git-path` instead of appending
> relative paths to what `git rev-parse --git-dir` returns (and as a
> consequence not only using the correct hooks directory, but also using
> the correct paths in worktrees other than the main one). While it does
> not seem as if Git GUI in particular is asking for `logs/HEAD.lock`,
> let's be safe rather than sorry.
> 
> Side note: Git GUI _does_ ask for `index.lock`, but that is already
> resolved correctly, due to `update_common_dir()` preferring to leave
> unknown paths in the (worktree-specific) git directory.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  path.c               |  8 +++++++-
>  t/t1500-rev-parse.sh | 15 +++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/path.c b/path.c
> index e3da1f3c4e..5ff64e7a8a 100644
> --- a/path.c
> +++ b/path.c
> @@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void *value, void *baton)
>  static void update_common_dir(struct strbuf *buf, int git_dir_len,
>  			      const char *common_dir)
>  {
> -	char *base = buf->buf + git_dir_len;
> +	char *base = buf->buf + git_dir_len, *base2 = NULL;
> +
> +	if (ends_with(base, ".lock"))
> +		base = base2 = xstrndup(base, strlen(base) - 5);

Hm, this adds the magic number 5 and calls strlen(base) twice, because
ends_with() calls strip_suffix(), which calls strlen().  Calling
strip_suffix() directly would allow us to avoid the magic number and
the second strlen():

  size_t len;
  if (strip_suffix(base, ".lock", &len))
          base = base2 = xstrndup(base, len);

>  	init_common_trie();
>  	if (trie_find(&common_trie, base, check_common, NULL) > 0)
>  		replace_dir(buf, git_dir_len, common_dir);
> +
> +	free(base2);
>  }
>  
>  void report_linked_checkout_garbage(void)
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 01abee533d..d318a1eeef 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'git-path in worktree' '
> +	test_tick &&
> +	git commit --allow-empty -m empty &&
> +	git worktree add --detach wt &&
> +	test_write_lines >expect \
> +		"$(pwd)/.git/worktrees/wt/logs/HEAD" \
> +		"$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
> +		"$(pwd)/.git/worktrees/wt/index" \
> +		"$(pwd)/.git/worktrees/wt/index.lock" &&
> +	git -C wt rev-parse >actual \
> +		--git-path logs/HEAD --git-path logs/HEAD.lock \
> +		--git-path index --git-path index.lock &&
> +	test_cmp expect actual
> +'

I think this test better fits into 't0060-path-utils.sh': it already
checks 'logs/HEAD' and 'index' in a different working tree (well, with
GIT_COMMON_DIR set, but that's the same), and it has a helper function
to make each of the two new .lock tests a one-liner.

>  test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
>  	test_commit test_commit &&
>  	echo true >expect &&
> -- 
> gitgitgadget
Junio C Hamano Oct. 23, 2019, 3:38 a.m. UTC | #2
SZEDER Gábor <szeder.dev@gmail.com> writes:

>> +	char *base = buf->buf + git_dir_len, *base2 = NULL;
>> +
>> +	if (ends_with(base, ".lock"))
>> +		base = base2 = xstrndup(base, strlen(base) - 5);
>
> Hm, this adds the magic number 5 and calls strlen(base) twice, because
> ends_with() calls strip_suffix(), which calls strlen().  Calling
> strip_suffix() directly would allow us to avoid the magic number and
> the second strlen():
>
>   size_t len;
>   if (strip_suffix(base, ".lock", &len))
>           base = base2 = xstrndup(base, len);

Makes sense, and is easy to squash in.
Johannes Schindelin Oct. 28, 2019, 12:01 p.m. UTC | #3
Hi,

On Tue, 22 Oct 2019, SZEDER Gábor wrote:

> On Mon, Oct 21, 2019 at 09:54:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Ever since worktrees were introduced, the `git_path()` function _really_
> > needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
> > specific to the worktree, and therefore so is its reflog). However, the
> > wrong path is returned for `logs/HEAD.lock`.
> >
> > This does not matter as long as the Git executable is doing the asking,
> > as the path for that `logs/HEAD.lock` file is constructed from
> > `git_path("logs/HEAD")` by appending the `.lock` suffix.
> >
> > However, Git GUI just learned to use `--git-path` instead of appending
> > relative paths to what `git rev-parse --git-dir` returns (and as a
> > consequence not only using the correct hooks directory, but also using
> > the correct paths in worktrees other than the main one). While it does
> > not seem as if Git GUI in particular is asking for `logs/HEAD.lock`,
> > let's be safe rather than sorry.
> >
> > Side note: Git GUI _does_ ask for `index.lock`, but that is already
> > resolved correctly, due to `update_common_dir()` preferring to leave
> > unknown paths in the (worktree-specific) git directory.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  path.c               |  8 +++++++-
> >  t/t1500-rev-parse.sh | 15 +++++++++++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..5ff64e7a8a 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void *value, void *baton)
> >  static void update_common_dir(struct strbuf *buf, int git_dir_len,
> >  			      const char *common_dir)
> >  {
> > -	char *base = buf->buf + git_dir_len;
> > +	char *base = buf->buf + git_dir_len, *base2 = NULL;
> > +
> > +	if (ends_with(base, ".lock"))
> > +		base = base2 = xstrndup(base, strlen(base) - 5);
>
> Hm, this adds the magic number 5 and calls strlen(base) twice, because
> ends_with() calls strip_suffix(), which calls strlen().  Calling
> strip_suffix() directly would allow us to avoid the magic number and
> the second strlen():
>
>   size_t len;
>   if (strip_suffix(base, ".lock", &len))
>           base = base2 = xstrndup(base, len);

Actually, we should probably avoid the extra allocation. Earlier, I was
concerned about multi-threading issues when attempting to modify the
`strbuf`. But then, we modify that `strbuf` a couple of lines later
anyway, so my fears were totally unfounded. Therefore, my current
version reads like this:

-- snip --
	int has_lock_suffix = strbuf_strip_suffix(buf, LOCK_SUFFIX);

	init_common_trie();
	if (trie_find(&common_trie, base, check_common, NULL) > 0)
		replace_dir(buf, git_dir_len, common_dir);

	if (has_lock_suffix)
		strbuf_addstr(buf, LOCK_SUFFIX);
-- snap --


>
> >  	init_common_trie();
> >  	if (trie_find(&common_trie, base, check_common, NULL) > 0)
> >  		replace_dir(buf, git_dir_len, common_dir);
> > +
> > +	free(base2);
> >  }
> >
> >  void report_linked_checkout_garbage(void)
> > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> > index 01abee533d..d318a1eeef 100755
> > --- a/t/t1500-rev-parse.sh
> > +++ b/t/t1500-rev-parse.sh
> > @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'git-path in worktree' '
> > +	test_tick &&
> > +	git commit --allow-empty -m empty &&
> > +	git worktree add --detach wt &&
> > +	test_write_lines >expect \
> > +		"$(pwd)/.git/worktrees/wt/logs/HEAD" \
> > +		"$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
> > +		"$(pwd)/.git/worktrees/wt/index" \
> > +		"$(pwd)/.git/worktrees/wt/index.lock" &&
> > +	git -C wt rev-parse >actual \
> > +		--git-path logs/HEAD --git-path logs/HEAD.lock \
> > +		--git-path index --git-path index.lock &&
> > +	test_cmp expect actual
> > +'
>
> I think this test better fits into 't0060-path-utils.sh': it already
> checks 'logs/HEAD' and 'index' in a different working tree (well, with
> GIT_COMMON_DIR set, but that's the same), and it has a helper function
> to make each of the two new .lock tests a one-liner.

Excellent point. Thank you for helping me improve the patch!

Ciao,
Dscho

>
> >  test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
> >  	test_commit test_commit &&
> >  	echo true >expect &&
> > --
> > gitgitgadget
>
SZEDER Gábor Oct. 28, 2019, 12:32 p.m. UTC | #4
On Mon, Oct 28, 2019 at 01:01:49PM +0100, Johannes Schindelin wrote:
> > > @@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void *value, void *baton)
> > >  static void update_common_dir(struct strbuf *buf, int git_dir_len,
> > >  			      const char *common_dir)
> > >  {
> > > -	char *base = buf->buf + git_dir_len;
> > > +	char *base = buf->buf + git_dir_len, *base2 = NULL;
> > > +
> > > +	if (ends_with(base, ".lock"))
> > > +		base = base2 = xstrndup(base, strlen(base) - 5);
> >
> > Hm, this adds the magic number 5 and calls strlen(base) twice, because
> > ends_with() calls strip_suffix(), which calls strlen().  Calling
> > strip_suffix() directly would allow us to avoid the magic number and
> > the second strlen():
> >
> >   size_t len;
> >   if (strip_suffix(base, ".lock", &len))
> >           base = base2 = xstrndup(base, len);
> 
> Actually, we should probably avoid the extra allocation. Earlier, I was
> concerned about multi-threading issues when attempting to modify the
> `strbuf`. But then, we modify that `strbuf` a couple of lines later
> anyway,

Do we?  You're right, we do indeed, because in replace_dir(buf, ...)
we call strbuf_splice(buf, ...).
OK, then your suggestion below is even better.

> so my fears were totally unfounded. Therefore, my current
> version reads like this:
> 
> -- snip --
> 	int has_lock_suffix = strbuf_strip_suffix(buf, LOCK_SUFFIX);
> 
> 	init_common_trie();
> 	if (trie_find(&common_trie, base, check_common, NULL) > 0)
> 		replace_dir(buf, git_dir_len, common_dir);
> 
> 	if (has_lock_suffix)
> 		strbuf_addstr(buf, LOCK_SUFFIX);
> -- snap --
Junio C Hamano Oct. 28, 2019, 5:30 p.m. UTC | #5
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>   size_t len;
>>   if (strip_suffix(base, ".lock", &len))
>>           base = base2 = xstrndup(base, len);
>
> Actually, we should probably avoid the extra allocation. Earlier, I was
> concerned about multi-threading issues when attempting to modify the
> `strbuf`. But then, we modify that `strbuf` a couple of lines later
> anyway, so my fears were totally unfounded. Therefore, my current
> version reads like this:
>
> -- snip --
> 	int has_lock_suffix = strbuf_strip_suffix(buf, LOCK_SUFFIX);
>
> 	init_common_trie();
> 	if (trie_find(&common_trie, base, check_common, NULL) > 0)
> 		replace_dir(buf, git_dir_len, common_dir);
>
> 	if (has_lock_suffix)
> 		strbuf_addstr(buf, LOCK_SUFFIX);
> -- snap --

Makes sense.  Thanks for thinking it through.

Patch
diff mbox series

diff --git a/path.c b/path.c
index e3da1f3c4e..5ff64e7a8a 100644
--- a/path.c
+++ b/path.c
@@ -349,10 +349,16 @@  static int check_common(const char *unmatched, void *value, void *baton)
 static void update_common_dir(struct strbuf *buf, int git_dir_len,
 			      const char *common_dir)
 {
-	char *base = buf->buf + git_dir_len;
+	char *base = buf->buf + git_dir_len, *base2 = NULL;
+
+	if (ends_with(base, ".lock"))
+		base = base2 = xstrndup(base, strlen(base) - 5);
+
 	init_common_trie();
 	if (trie_find(&common_trie, base, check_common, NULL) > 0)
 		replace_dir(buf, git_dir_len, common_dir);
+
+	free(base2);
 }
 
 void report_linked_checkout_garbage(void)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 01abee533d..d318a1eeef 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -116,6 +116,21 @@  test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git-path in worktree' '
+	test_tick &&
+	git commit --allow-empty -m empty &&
+	git worktree add --detach wt &&
+	test_write_lines >expect \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD" \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
+		"$(pwd)/.git/worktrees/wt/index" \
+		"$(pwd)/.git/worktrees/wt/index.lock" &&
+	git -C wt rev-parse >actual \
+		--git-path logs/HEAD --git-path logs/HEAD.lock \
+		--git-path index --git-path index.lock &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&