# [PATCH 1/2] t1400: wrap setup code in test case 31 messages from 2019-10-16 to 2019-10-29. Participants: Johannes Schindelin via GitGitGadget, SZEDER Gábor, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/52043 ## Johannes Schindelin via GitGitGadget, 2019-10-16 07:07 Subject: [PATCH 1/2] t1400: wrap setup code in test case Message-ID: URL: https://gitlist.dev/e/cf97c5182eb98cc0ae72f94d4331abcb4486ca83.1571209637.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Without this, you cannot use `--run=<...>` to skip that part, and a run with `--run=0` (which is a common way to determine the test case number corresponding to a given test case title). Signed-off-by: Johannes Schindelin --- t/t1400-update-ref.sh | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 1fbd940408..69a7f27311 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" ' test_cmp expect .git/logs/$m ' -git update-ref $m $D -cat >.git/logs/$m < 1117150320 -0500 -$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 -$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 -$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 -EOF +test_expect_success 'set up for querying the reflog' ' + git update-ref $m $D && + cat >.git/logs/$m <<-EOF + $Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500 + $C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 + $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 + $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 + $Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 + EOF +' ed="Thu, 26 May 2005 18:32:00 -0500" gd="Thu, 26 May 2005 18:33:00 -0500" -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-16 07:07 Subject: [PATCH 0/2] Handle git_path() with lock files correctly in worktrees Message-ID: URL: https://gitlist.dev/e/pull.401.git.1571209637.gitgitgadget%40gmail.com ``` I stumbled over this during my recent work in Git GUI [https://github.com/gitgitgadget/git/pull/361] that was originally really only intended to use the correct hooks directory. Technically, the first patch is not needed (because I decided against adding a test to t1400 in the end, in favor of t1500), but it shouldn't hurt, either. Johannes Schindelin (2): t1400: wrap setup code in test case git_path(): handle `.lock` files correctly path.c | 4 ++-- t/t1400-update-ref.sh | 18 ++++++++++-------- t/t1500-rev-parse.sh | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/401 -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-16 07:07 Subject: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/f08c90ea02f6811b310a01301acf46da133f38a9.1571209637.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Ever since worktrees were introduced, the `git_path()` function _really_ needed to be called e.g. to get at the `index`. However, the wrong path is returned for `index.lock`. This does not matter as long as the Git executable is doing the asking, as the path for that `index.lock` file is constructed from `git_path("index")` 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). And one of the paths it is looking for is... you guessed it... `index.lock`. So let's make that work as script writers would expect it to. Signed-off-by: Johannes Schindelin --- path.c | 4 ++-- t/t1500-rev-parse.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/path.c b/path.c index e3da1f3c4e..ff85692b45 100644 --- a/path.c +++ b/path.c @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, int result; struct trie *child; - if (!*key) { + if (!*key || !strcmp(key, ".lock")) { /* we have reached the end of the key */ if (root->value && !root->len) return fn(key, root->value, baton); @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, /* Matched the entire compressed section */ key += i; - if (!*key) + if (!*key || !strcmp(key, ".lock")) /* End of key */ return fn(key, root->value, baton); 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 && -- gitgitgadget ``` ## SZEDER Gábor, 2019-10-16 11:04 Subject: Re: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191016110440.GV29845@szeder.dev> URL: https://gitlist.dev/e/20191016110440.GV29845%40szeder.dev In-Reply-To: ``` On Wed, Oct 16, 2019 at 07:07:17AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > Ever since worktrees were introduced, the `git_path()` function _really_ > needed to be called e.g. to get at the `index`. However, the wrong path > is returned for `index.lock`. Could you give an example where it returns the wrong path for 'index.lock'? I tried to reproduce this issue in a working tree, but no matter what I've tried, 'git rev-parse --git-dir index.lock' always returned the right path. > This does not matter as long as the Git executable is doing the asking, > as the path for that `index.lock` file is constructed from > `git_path("index")` 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). And one of the > paths it is looking for is... you guessed it... `index.lock`. > > So let's make that work as script writers would expect it to. > > Signed-off-by: Johannes Schindelin > --- > path.c | 4 ++-- > t/t1500-rev-parse.sh | 15 +++++++++++++++ > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/path.c b/path.c > index e3da1f3c4e..ff85692b45 100644 > --- a/path.c > +++ b/path.c > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > int result; > struct trie *child; > > - if (!*key) { > + if (!*key || !strcmp(key, ".lock")) { > /* we have reached the end of the key */ > if (root->value && !root->len) > return fn(key, root->value, baton); > @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > > /* Matched the entire compressed section */ > key += i; > - if (!*key) > + if (!*key || !strcmp(key, ".lock")) > /* End of key */ > return fn(key, root->value, baton); > > 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 Without the fix applied this test fails with: + test_cmp expect actual --- expect 2019-10-16 10:20:31.047229423 +0000 +++ actual 2019-10-16 10:20:31.051229519 +0000 @@ -1,4 +1,4 @@ /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD -/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD.lock +/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/logs/HEAD.lock /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index.lock error: last command exited with $?=1 So the path of 'index.lock' seems to be fine already, it's the path of the lockfile for HEAD's reflog that's indeed wrong and makes the test fail. On a related note, I'm not sure whether the path of the reflogs directory is right while in a different working tree... Both with and without this patch I get a path pointing to the main working tree: $ ./git -C WT/ rev-parse --git-path logs /home/szeder/src/git/.git/logs However, I'm not sure what the right path should be in the first place, given that each working tree has its own 'logs' directory, but only for HEAD's reflog, while everything else goes to the main working tree's 'logs' directory. > +' > + > test_expect_success 'rev-parse --is-shallow-repository in shallow repo' ' > test_commit test_commit && > echo true >expect && > -- > gitgitgadget ``` ## Junio C Hamano, 2019-10-17 07:15 Subject: Re: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/xmqqzhhz7sh8.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191016110440.GV29845@szeder.dev> ``` SZEDER Gábor writes: > However, I'm not sure what the right path should be in the first > place, given that each working tree has its own 'logs' directory, but > only for HEAD's reflog, while everything else goes to the main working > tree's 'logs' directory. As all worktrees should share the same view of where 'master' (for example) branch points at, what commit it was pointing at before, etc., the reflogs should also be shared for refs. The exception is the HEAD where each worktree can point at its own commit (when detached) or a branch (when not). The above "should" does not mean "I know the code works that way so if your git behaves differently your compiler is wrong"; it just means "that's the logical conclusion of the basic design, and if your git does not behave that way, we have a bug (or two)". Thanks. ``` ## Johannes Schindelin, 2019-10-17 22:05 Subject: Re: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1910172333360.46%40tvgsbejvaqbjf.bet In-Reply-To: <20191016110440.GV29845@szeder.dev> ``` Hi Gábor, On Wed, 16 Oct 2019, SZEDER Gábor wrote: > On Wed, Oct 16, 2019 at 07:07:17AM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin > > > > Ever since worktrees were introduced, the `git_path()` function _really_ > > needed to be called e.g. to get at the `index`. However, the wrong path > > is returned for `index.lock`. > > Could you give an example where it returns the wrong path for > 'index.lock'? Oh wow, this was a left-over from an early draft, before I got the regression test to work... What I meant was of course logs/HEAD.lock. Will fix. > I tried to reproduce this issue in a working tree, but > no matter what I've tried, 'git rev-parse --git-dir index.lock' always > returned the right path. With `s/--git-dir/--git-path/`, I agree. > > This does not matter as long as the Git executable is doing the asking, > > as the path for that `index.lock` file is constructed from > > `git_path("index")` 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). And one of the > > paths it is looking for is... you guessed it... `index.lock`. > > > > So let's make that work as script writers would expect it to. > > > > Signed-off-by: Johannes Schindelin > > --- > > path.c | 4 ++-- > > t/t1500-rev-parse.sh | 15 +++++++++++++++ > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/path.c b/path.c > > index e3da1f3c4e..ff85692b45 100644 > > --- a/path.c > > +++ b/path.c > > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > > int result; > > struct trie *child; > > > > - if (!*key) { > > + if (!*key || !strcmp(key, ".lock")) { > > /* we have reached the end of the key */ > > if (root->value && !root->len) > > return fn(key, root->value, baton); > > @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > > > > /* Matched the entire compressed section */ > > key += i; > > - if (!*key) > > + if (!*key || !strcmp(key, ".lock")) > > /* End of key */ > > return fn(key, root->value, baton); > > > > 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 > > Without the fix applied this test fails with: > > + test_cmp expect actual > --- expect 2019-10-16 10:20:31.047229423 +0000 > +++ actual 2019-10-16 10:20:31.051229519 +0000 > @@ -1,4 +1,4 @@ > /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD > -/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD.lock > +/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/logs/HEAD.lock > /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index > /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index.lock > error: last command exited with $?=1 > > So the path of 'index.lock' seems to be fine already, it's the path of > the lockfile for HEAD's reflog that's indeed wrong and makes the test > fail. Indeed, and this makes this patch much less important than I previosly thought. It's not like it would break Git GUI in worktrees, which is what I thought, which in turn is the reason I sent this so close to -rc0. > On a related note, I'm not sure whether the path of the reflogs > directory is right while in a different working tree... Both with and > without this patch I get a path pointing to the main working tree: > > $ ./git -C WT/ rev-parse --git-path logs > /home/szeder/src/git/.git/logs > > However, I'm not sure what the right path should be in the first > place, given that each working tree has its own 'logs' directory, but > only for HEAD's reflog, while everything else goes to the main working > tree's 'logs' directory. It's like Junio said, the reflog for `HEAD` is special because `HEAD` is special. Look for `common_list` in `path.c` (it is a bit confusing, I admit, you have to look for the 3rd column of numbers: if it is a `1`, then it is a worktree-specific path, if it is `0`, it is supposed to live in the "commondir", i.e. in the gitdir of the main worktree). Thanks, Dscho > > > +' > > + > > test_expect_success 'rev-parse --is-shallow-repository in shallow repo' ' > > test_commit test_commit && > > echo true >expect && > > -- > > gitgitgadget > ``` ## Johannes Schindelin via GitGitGadget, 2019-10-17 22:07 Subject: [PATCH v2 0/2] Handle git_path() with lock files correctly in worktrees Message-ID: URL: https://gitlist.dev/e/pull.401.v2.git.1571350077.gitgitgadget%40gmail.com In-Reply-To: ``` I stumbled over this during my recent work in Git GUI [https://github.com/gitgitgadget/git/pull/361] that was originally really only intended to use the correct hooks directory. It turns out that my fears that index.lock was mishandled were unfounded, hence this patch series has a lot lower priority for me than "OMG we must push this into v2.24.0!". Technically, the first patch is not needed (because I decided against adding a test to t1400 in the end, in favor of t1500), but it shouldn't hurt, either. Changes since v1: * Clarified the commit message to state that index.lock is fine, it is logs/HEAD.lock that isn't. Johannes Schindelin (2): t1400: wrap setup code in test case git_path(): handle `.lock` files correctly path.c | 4 ++-- t/t1400-update-ref.sh | 18 ++++++++++-------- t/t1500-rev-parse.sh | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/401 Range-diff vs v1: 1: cf97c5182e = 1: cf97c5182e t1400: wrap setup code in test case 2: f08c90ea02 ! 2: 93dba5a3a3 git_path(): handle `.lock` files correctly @@ -3,8 +3,9 @@ git_path(): handle `.lock` files correctly Ever since worktrees were introduced, the `git_path()` function _really_ - needed to be called e.g. to get at the `index`. However, the wrong path - is returned for `index.lock`. + needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is + specific to the worktree). 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 `index.lock` file is constructed from @@ -13,10 +14,12 @@ 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). And one of the - paths it is looking for is... you guessed it... `index.lock`. + 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. - So let's make that work as script writers would expect it to. + Side note: Git GUI _does_ ask for `index.lock`, but that is already + resolved correctly. Signed-off-by: Johannes Schindelin -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-17 22:07 Subject: [PATCH v2 1/2] t1400: wrap setup code in test case Message-ID: URL: https://gitlist.dev/e/cf97c5182eb98cc0ae72f94d4331abcb4486ca83.1571350077.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Without this, you cannot use `--run=<...>` to skip that part, and a run with `--run=0` (which is a common way to determine the test case number corresponding to a given test case title). Signed-off-by: Johannes Schindelin --- t/t1400-update-ref.sh | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 1fbd940408..69a7f27311 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" ' test_cmp expect .git/logs/$m ' -git update-ref $m $D -cat >.git/logs/$m < 1117150320 -0500 -$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 -$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 -$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 -EOF +test_expect_success 'set up for querying the reflog' ' + git update-ref $m $D && + cat >.git/logs/$m <<-EOF + $Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500 + $C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 + $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 + $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 + $Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 + EOF +' ed="Thu, 26 May 2005 18:32:00 -0500" gd="Thu, 26 May 2005 18:33:00 -0500" -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-17 22:07 Subject: [PATCH v2 2/2] git_path(): handle `.lock` files correctly Message-ID: <93dba5a3a38d75ba79329383a1d50419a1990c8d.1571350077.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/93dba5a3a38d75ba79329383a1d50419a1990c8d.1571350077.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin 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). 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 `index.lock` file is constructed from `git_path("index")` 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. Signed-off-by: Johannes Schindelin --- path.c | 4 ++-- t/t1500-rev-parse.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/path.c b/path.c index e3da1f3c4e..ff85692b45 100644 --- a/path.c +++ b/path.c @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, int result; struct trie *child; - if (!*key) { + if (!*key || !strcmp(key, ".lock")) { /* we have reached the end of the key */ if (root->value && !root->len) return fn(key, root->value, baton); @@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, /* Matched the entire compressed section */ key += i; - if (!*key) + if (!*key || !strcmp(key, ".lock")) /* End of key */ return fn(key, root->value, baton); 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 && -- gitgitgadget ``` ## Junio C Hamano, 2019-10-18 01:23 Subject: Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/xmqq1rva7sp7.fsf%40gitster-ct.c.googlers.com In-Reply-To: <93dba5a3a38d75ba79329383a1d50419a1990c8d.1571350077.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > 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). 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 `index.lock` file is constructed from > `git_path("index")` by appending the `.lock` suffix. Is this still git_path("index") or is it now HEAD? > Side note: Git GUI _does_ ask for `index.lock`, but that is already > resolved correctly. Is that s/but/and/? > diff --git a/path.c b/path.c > index e3da1f3c4e..ff85692b45 100644 > --- a/path.c > +++ b/path.c > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > int result; > struct trie *child; > > - if (!*key) { > + if (!*key || !strcmp(key, ".lock")) { We only do strcmp for the tail part at the end of the path, so this should probably OK from performance point of view but semantically it is not very satisfying to see a special case for a single .suffix this deep in the callchain. I wonder if it is nicer to have the higher level callers notice ".lock" or whatever other suffixes they care about and ask the lower layer for a key with the suffix stripped? Will queue. Thanks. ``` ## SZEDER Gábor, 2019-10-18 11:06 Subject: Re: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191018110618.GZ29845@szeder.dev> URL: https://gitlist.dev/e/20191018110618.GZ29845%40szeder.dev In-Reply-To: ``` On Fri, Oct 18, 2019 at 12:05:20AM +0200, Johannes Schindelin wrote: > > I tried to reproduce this issue in a working tree, but > > no matter what I've tried, 'git rev-parse --git-dir index.lock' always > > returned the right path. > > With `s/--git-dir/--git-path/`, I agree. Right. I mistyped it a few times on the command line as well, but then the command's output reminded me that I messed up. Alas, no such reminder when writing the email... > > On a related note, I'm not sure whether the path of the reflogs > > directory is right while in a different working tree... Both with and > > without this patch I get a path pointing to the main working tree: > > > > $ ./git -C WT/ rev-parse --git-path logs > > /home/szeder/src/git/.git/logs > > > > However, I'm not sure what the right path should be in the first > > place, given that each working tree has its own 'logs' directory, but > > only for HEAD's reflog, while everything else goes to the main working > > tree's 'logs' directory. > > It's like Junio said, the reflog for `HEAD` is special because `HEAD` is > special. Look for `common_list` in `path.c` (it is a bit confusing, I > admit, you have to look for the 3rd column of numbers: if it is a `1`, > then it is a worktree-specific path, if it is `0`, it is supposed to > live in the "commondir", i.e. in the gitdir of the main worktree). OK, got it. I didn't look yesterday at all, but now I did, and, unfortunately, see two more bugs, and one of them is a "proper" bug leading to bogus output: $ git -C WT/ rev-parse --git-path logs/refs --git-path logs/refs/ /home/szeder/src/git/.git/logs/refs /home/szeder/src/git/.git/worktrees/WT/logs/refs/ ``` ## SZEDER Gábor, 2019-10-18 11:35 Subject: Re: [PATCH 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191018113557.GA29845@szeder.dev> URL: https://gitlist.dev/e/20191018113557.GA29845%40szeder.dev In-Reply-To: <20191018110618.GZ29845@szeder.dev> ``` On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote: > > > On a related note, I'm not sure whether the path of the reflogs > > > directory is right while in a different working tree... Both with and > > > without this patch I get a path pointing to the main working tree: > > > > > > $ ./git -C WT/ rev-parse --git-path logs > > > /home/szeder/src/git/.git/logs > > > > > > However, I'm not sure what the right path should be in the first > > > place, given that each working tree has its own 'logs' directory, but > > > only for HEAD's reflog, while everything else goes to the main working > > > tree's 'logs' directory. > > > > It's like Junio said, the reflog for `HEAD` is special because `HEAD` is > > special. Look for `common_list` in `path.c` (it is a bit confusing, I > > admit, you have to look for the 3rd column of numbers: if it is a `1`, > > then it is a worktree-specific path, if it is `0`, it is supposed to > > live in the "commondir", i.e. in the gitdir of the main worktree). > > OK, got it. > > I didn't look yesterday at all, but now I did, and, unfortunately, see > two more bugs, and one of them is a "proper" bug leading to bogus > output: > > $ git -C WT/ rev-parse --git-path logs/refs --git-path logs/refs/ > /home/szeder/src/git/.git/logs/refs > /home/szeder/src/git/.git/worktrees/WT/logs/refs/ This one-liner below fixes it, but I haven't yet made up my mind about whether this is the right fix or whether there could be any fallout (at least the test suite doesn't show any). $ ./git -C WT/ rev-parse --git-path logs/refs --git-path logs/refs/ /home/szeder/src/git/.git/logs/refs /home/szeder/src/git/.git/logs/refs/ diff --git a/path.c b/path.c index 04b69b9feb..9019169418 100644 --- a/path.c +++ b/path.c @@ -335,7 +335,7 @@ static int check_common(const char *unmatched, void *value, void *baton) struct common_dir *dir = value; if (!dir) - return 0; + return -1; if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/')) return !dir->exclude; ``` ## SZEDER Gábor, 2019-10-18 11:42 Subject: [PATCH 1/2] path.c: fix field name in 'struct common_dir' Message-ID: <20191018114202.5452-2-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20191018114202.5452-2-szeder.dev%40gmail.com In-Reply-To: <20191018114202.5452-1-szeder.dev@gmail.com> ``` An array of 'struct common_dir' instances is used to specify whether various paths in the '.git' directory are worktree-specific or belong to the main worktree. Confusingly, the path is recorded in the struct's 'dirname' field, even though it can be a regular file, e.g. 'gc.pid'. Rename this 'dirname' field to 'path' to avoid future confusion. Signed-off-by: SZEDER Gábor --- path.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/path.c b/path.c index e3da1f3c4e..eeb43e1d25 100644 --- a/path.c +++ b/path.c @@ -103,7 +103,7 @@ struct common_dir { unsigned is_dir:1; /* Not common even though its parent is */ unsigned exclude:1; - const char *dirname; + const char *path; }; static struct common_dir common_list[] = { @@ -320,8 +320,8 @@ static void init_common_trie(void) if (common_trie_done_setup) return; - for (p = common_list; p->dirname; p++) - add_to_trie(&common_trie, p->dirname, p); + for (p = common_list; p->path; p++) + add_to_trie(&common_trie, p->path, p); common_trie_done_setup = 1; } @@ -365,8 +365,8 @@ void report_linked_checkout_garbage(void) return; strbuf_addf(&sb, "%s/", get_git_dir()); len = sb.len; - for (p = common_list; p->dirname; p++) { - const char *path = p->dirname; + for (p = common_list; p->path; p++) { + const char *path = p->path; if (p->ignore_garbage) continue; strbuf_setlen(&sb, len); -- 2.23.0.1084.gae250eaa40 ``` ## SZEDER Gábor, 2019-10-18 11:42 Subject: [PATCH 2/2] path.c: mark 'logs/HEAD' in 'common_list' as file Message-ID: <20191018114202.5452-3-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20191018114202.5452-3-szeder.dev%40gmail.com In-Reply-To: <20191018114202.5452-1-szeder.dev@gmail.com> ``` 'logs/HEAD', i.e. HEAD's reflog, is a file, but its entry in 'common_list' has the 'is_dir' bit set. Unset that bit to make it consistent with what 'logs/HEAD' is supposed to be. This doesn't make a difference in behavior: check_common() is the only function that looks at the 'is_dir' bit, and that function either returns 0, or '!exclude', which for 'logs/HEAD' results in 0 as well. Signed-off-by: SZEDER Gábor --- path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/path.c b/path.c index eeb43e1d25..04b69b9feb 100644 --- a/path.c +++ b/path.c @@ -113,7 +113,7 @@ static struct common_dir common_list[] = { { 0, 1, 0, "info" }, { 0, 0, 1, "info/sparse-checkout" }, { 1, 1, 0, "logs" }, - { 1, 1, 1, "logs/HEAD" }, + { 1, 0, 1, "logs/HEAD" }, { 0, 1, 1, "logs/refs/bisect" }, { 0, 1, 1, "logs/refs/rewritten" }, { 0, 1, 1, "logs/refs/worktree" }, -- 2.23.0.1084.gae250eaa40 ``` ## SZEDER Gábor, 2019-10-18 11:42 Subject: [PATCH 0/2] path.c: minor common_list fixes Message-ID: <20191018114202.5452-1-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20191018114202.5452-1-szeder.dev%40gmail.com In-Reply-To: <20191018110618.GZ29845@szeder.dev> ``` On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote: > I didn't look yesterday at all, but now I did, and, unfortunately, see > two more bugs The second patch is kind of a bugfix, though luckily the bug doesn't actually manifest in undesired behavior. The first is a minor cleanup, so future readers won't have a 'Huh?' moment like I just did. SZEDER Gábor (2): path.c: fix field name in 'struct common_dir' path.c: mark 'logs/HEAD' in 'common_list' as file path.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.23.0.1084.gae250eaa40 ``` ## SZEDER Gábor, 2019-10-18 12:35 Subject: Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191018123504.GB29845@szeder.dev> URL: https://gitlist.dev/e/20191018123504.GB29845%40szeder.dev In-Reply-To: ``` On Fri, Oct 18, 2019 at 10:23:00AM +0900, Junio C Hamano wrote: > > diff --git a/path.c b/path.c > > index e3da1f3c4e..ff85692b45 100644 > > --- a/path.c > > +++ b/path.c > > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > > int result; > > struct trie *child; > > > > - if (!*key) { > > + if (!*key || !strcmp(key, ".lock")) { > > We only do strcmp for the tail part at the end of the path, so this > should probably OK from performance point of view but semantically > it is not very satisfying to see a special case for a single .suffix > this deep in the callchain. I wonder if it is nicer to have the > higher level callers notice ".lock" or whatever other suffixes they > care about and ask the lower layer for a key with the suffix > stripped? I was wondering the same. Despite all of its functions being file static, this is a fairly general trie implementation for paths, which could potentially be useful in other parts of Git as well. Special casing any suffix within its implementation makes it less (re)usable in other cases. ``` ## Johannes Schindelin, 2019-10-21 19:35 Subject: Re: [PATCH 0/2] path.c: minor common_list fixes Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1910212133540.46%40tvgsbejvaqbjf.bet In-Reply-To: <20191018114202.5452-1-szeder.dev@gmail.com> ``` Hi Gábor, On Fri, 18 Oct 2019, SZEDER Gábor wrote: > On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote: > > I didn't look yesterday at all, but now I did, and, unfortunately, see > > two more bugs > > The second patch is kind of a bugfix, though luckily the bug doesn't > actually manifest in undesired behavior. > The first is a minor cleanup, so future readers won't have a 'Huh?' > moment like I just did. > > SZEDER Gábor (2): > path.c: fix field name in 'struct common_dir' > path.c: mark 'logs/HEAD' in 'common_list' as file Those patches look very reasonable to me. Thanks, Dscho > > path.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > -- > 2.23.0.1084.gae250eaa40 > > ``` ## Johannes Schindelin, 2019-10-21 20:26 Subject: Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1910212157350.46%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Junio, On Fri, 18 Oct 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > 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). 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 `index.lock` file is constructed from > > `git_path("index")` by appending the `.lock` suffix. > > Is this still git_path("index") or is it now HEAD? Darn. It is "logs/HEAD", of course. > > Side note: Git GUI _does_ ask for `index.lock`, but that is already > > resolved correctly. > > Is that s/but/and/? It sounds better with an "and", you're right. > > diff --git a/path.c b/path.c > > index e3da1f3c4e..ff85692b45 100644 > > --- a/path.c > > +++ b/path.c > > @@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, > > int result; > > struct trie *child; > > > > - if (!*key) { > > + if (!*key || !strcmp(key, ".lock")) { > > We only do strcmp for the tail part at the end of the path, so this > should probably OK from performance point of view but semantically > it is not very satisfying to see a special case for a single .suffix > this deep in the callchain. I wonder if it is nicer to have the > higher level callers notice ".lock" or whatever other suffixes they > care about and ask the lower layer for a key with the suffix > stripped? Hmm. The parameter is provided as a `const char *`, so I cannot just edit it. My first attempt at fixing this resulted in this commit: -- snip -- path: switch `trie_find()` to a counted string Before this patch, the `trie_find()` function would be fed a regular, NUL-terminated string. However, in the next commit, we want to strip any `.lock` suffix from the argument, and the argument is provided as a `const char *` (i.e. we do not want to modify it). Let's use a string + length pair of parameters instead of a single NUL-terminated string to open the door for this kind of manipulation. Signed-off-by: Johannes Schindelin diff --git a/path.c b/path.c index e3da1f3c4e2..b18d552c890 100644 --- a/path.c +++ b/path.c @@ -236,7 +236,8 @@ static void *add_to_trie(struct trie *root, const char *key, void *value) return old; } -typedef int (*match_fn)(const char *unmatched, void *data, void *baton); +typedef int (*match_fn)(const char *unmatched, size_t unmatched_len, + void *data, void *baton); /* * Search a trie for some key. Find the longest /-or-\0-terminated @@ -261,22 +262,22 @@ typedef int (*match_fn)(const char *unmatched, void *data, void *baton); * |-----------------|------------|---------------|--------------| * */ -static int trie_find(struct trie *root, const char *key, match_fn fn, - void *baton) +static int trie_find(struct trie *root, const char *key, size_t key_len, + match_fn fn, void *baton) { int i; int result; struct trie *child; - if (!*key) { + if (!key_len) { /* we have reached the end of the key */ if (root->value && !root->len) - return fn(key, root->value, baton); + return fn(key, key_len, root->value, baton); else return -1; } - for (i = 0; i < root->len; i++) { + for (i = 0; i < key_len && i < root->len; i++) { /* Partial path normalization: skip consecutive slashes. */ if (key[i] == '/' && key[i+1] == '/') { key++; @@ -288,24 +289,26 @@ static int trie_find(struct trie *root, const char *key, match_fn fn, /* Matched the entire compressed section */ key += i; - if (!*key) + key_len -= i; + + if (!key_len) /* End of key */ - return fn(key, root->value, baton); + return fn(key, key_len, root->value, baton); /* Partial path normalization: skip consecutive slashes */ - while (key[0] == '/' && key[1] == '/') - key++; + while (key_len > 0 && key[0] == '/' && key[1] == '/') + key++, key_len--; - child = root->children[(unsigned char)*key]; + child = root->children[!key_len ? '0' : (unsigned char)*key]; if (child) - result = trie_find(child, key + 1, fn, baton); + result = trie_find(child, key + 1, key_len - 1, fn, baton); else result = -1; - if (result >= 0 || (*key != '/' && *key != 0)) + if (result >= 0 || (key_len && *key != '/')) return result; if (root->value) - return fn(key, root->value, baton); + return fn(key, key_len, root->value, baton); else return -1; } @@ -330,17 +333,18 @@ static void init_common_trie(void) * Helper function for update_common_dir: returns 1 if the dir * prefix is common. */ -static int check_common(const char *unmatched, void *value, void *baton) +static int check_common(const char *unmatched, size_t unmatched_len, + void *value, void *baton) { struct common_dir *dir = value; if (!dir) return 0; - if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/')) + if (dir->is_dir && (!unmatched_len || unmatched[0] == '/')) return !dir->exclude; - if (!dir->is_dir && unmatched[0] == 0) + if (!dir->is_dir && !unmatched_len) return !dir->exclude; return 0; @@ -350,8 +354,10 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir) { char *base = buf->buf + git_dir_len; + size_t len = strlen(base); + init_common_trie(); - if (trie_find(&common_trie, base, check_common, NULL) > 0) + if (trie_find(&common_trie, base, len, check_common, NULL) > 0) replace_dir(buf, git_dir_len, common_dir); } -- snap -- However, I think this is _really_ ugly and intrusive. The opposite of what my goals were. So I think I'll just bite the bullet and use a temporary copy if the argument ends in `.lock`. Ciao, Dscho ``` ## Johannes Schindelin via GitGitGadget, 2019-10-21 21:54 Subject: [PATCH v3 1/2] t1400: wrap setup code in test case Message-ID: URL: https://gitlist.dev/e/cf97c5182eb98cc0ae72f94d4331abcb4486ca83.1571694882.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Without this, you cannot use `--run=<...>` to skip that part, and a run with `--run=0` (which is a common way to determine the test case number corresponding to a given test case title). Signed-off-by: Johannes Schindelin --- t/t1400-update-ref.sh | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 1fbd940408..69a7f27311 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" ' test_cmp expect .git/logs/$m ' -git update-ref $m $D -cat >.git/logs/$m < 1117150320 -0500 -$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 -$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 -$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 -EOF +test_expect_success 'set up for querying the reflog' ' + git update-ref $m $D && + cat >.git/logs/$m <<-EOF + $Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500 + $C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 + $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 + $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 + $Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 + EOF +' ed="Thu, 26 May 2005 18:32:00 -0500" gd="Thu, 26 May 2005 18:33:00 -0500" -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-21 21:54 Subject: [PATCH v3 0/2] Handle git_path() with lock files correctly in worktrees Message-ID: URL: https://gitlist.dev/e/pull.401.v3.git.1571694882.gitgitgadget%40gmail.com In-Reply-To: ``` I stumbled over this during my recent work in Git GUI [https://github.com/gitgitgadget/git/pull/361] that was originally really only intended to use the correct hooks directory. It turns out that my fears that index.lock was mishandled were unfounded, hence this patch series has a lot lower priority for me than "OMG we must push this into v2.24.0!". Technically, the first patch is not needed (because I decided against adding a test to t1400 in the end, in favor of t1500), but it shouldn't hurt, either. Changes since v2: * Adjusted the commit message to really not talk about index.lock. * Instead of modifying the code inside trie_find() to special-case .lock, I now copy the string without the suffix .lock (if any) before handing it off to trie_find(). Changes since v1: * Clarified the commit message to state that index.lock is fine, it is logs/HEAD.lock that isn't. Johannes Schindelin (2): t1400: wrap setup code in test case git_path(): handle `.lock` files correctly path.c | 8 +++++++- t/t1400-update-ref.sh | 18 ++++++++++-------- t/t1500-rev-parse.sh | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 9 deletions(-) base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/401 Range-diff vs v2: 1: cf97c5182e = 1: cf97c5182e t1400: wrap setup code in test case 2: 93dba5a3a3 ! 2: 8b1f4f089a git_path(): handle `.lock` files correctly @@ -4,12 +4,12 @@ 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). However, the wrong path is returned for - `logs/HEAD.lock`. + 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 `index.lock` file is constructed from - `git_path("index")` by appending the `.lock` suffix. + 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 @@ -19,7 +19,8 @@ let's be safe rather than sorry. Side note: Git GUI _does_ ask for `index.lock`, but that is already - resolved correctly. + resolved correctly, due to `update_common_dir()` preferring to leave + unknown paths in the (worktree-specific) git directory. Signed-off-by: Johannes Schindelin @@ -27,23 +28,23 @@ --- a/path.c +++ b/path.c @@ - int result; - struct trie *child; - -- if (!*key) { -+ if (!*key || !strcmp(key, ".lock")) { - /* we have reached the end of the key */ - if (root->value && !root->len) - return fn(key, root->value, baton); -@@ - - /* Matched the entire compressed section */ - key += i; -- if (!*key) -+ if (!*key || !strcmp(key, ".lock")) - /* End of key */ - return fn(key, root->value, 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 --- a/t/t1500-rev-parse.sh -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-21 21:54 Subject: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: <8b1f4f089a6d4a2293507a1a77668c828598e84f.1571694882.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/8b1f4f089a6d4a2293507a1a77668c828598e84f.1571694882.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin 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 --- 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); + 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 && -- gitgitgadget ``` ## SZEDER Gábor, 2019-10-22 16:01 Subject: Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191022160105.GE4348@szeder.dev> URL: https://gitlist.dev/e/20191022160105.GE4348%40szeder.dev In-Reply-To: <8b1f4f089a6d4a2293507a1a77668c828598e84f.1571694882.git.gitgitgadget@gmail.com> ``` On Mon, Oct 21, 2019 at 09:54:42PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > 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 > --- > 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, 2019-10-23 02:12 Subject: Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/xmqqd0eojjll.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Johannes Schindelin writes: > However, I think this is _really_ ugly and intrusive. The opposite of > what my goals were. > > So I think I'll just bite the bullet and use a temporary copy if the > argument ends in `.lock`. Sounds like a quite sensible design decision to me. ``` ## Junio C Hamano, 2019-10-23 03:38 Subject: Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/xmqqmudsi10s.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20191022160105.GE4348@szeder.dev> ``` SZEDER Gábor 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, 2019-10-28 12:01 Subject: Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1910281225260.46%40tvgsbejvaqbjf.bet In-Reply-To: <20191022160105.GE4348@szeder.dev> ``` 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 > > > > 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 > > --- > > 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, 2019-10-28 12:32 Subject: Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: <20191028123214.GT4348@szeder.dev> URL: https://gitlist.dev/e/20191028123214.GT4348%40szeder.dev In-Reply-To: ``` 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 -- ``` ## Johannes Schindelin via GitGitGadget, 2019-10-28 12:57 Subject: [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Message-ID: URL: https://gitlist.dev/e/pull.401.v4.git.1572267438.gitgitgadget%40gmail.com In-Reply-To: ``` I stumbled over this during my recent work in Git GUI [https://github.com/gitgitgadget/git/pull/361] that was originally really only intended to use the correct hooks directory. It turns out that my fears that index.lock was mishandled were unfounded, hence this patch series has a lot lower priority for me than "OMG we must push this into v2.24.0!". Technically, the first patch is not needed (because I decided against adding a test to t1400 in the end, in favor of t1500), but it shouldn't hurt, either. I verified locally that this patch pair does not conflict with sg/dir-trie-fixes. Changes since v3: * Instead of duplicating the path when it has a .lock suffix, we now use strbuf manipulation to strip the suffix temporarily. * The test case in t1500 was scrapped in favor of a much simpler pair of test cases in t0060. Changes since v2: * Adjusted the commit message to really not talk about index.lock. * Instead of modifying the code inside trie_find() to special-case .lock, I now copy the string without the suffix .lock (if any) before handing it off to trie_find(). Changes since v1: * Clarified the commit message to state that index.lock is fine, it is logs/HEAD.lock that isn't. Johannes Schindelin (2): t1400: wrap setup code in test case git_path(): handle `.lock` files correctly path.c | 6 ++++++ t/t0060-path-utils.sh | 2 ++ t/t1400-update-ref.sh | 18 ++++++++++-------- 3 files changed, 18 insertions(+), 8 deletions(-) base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/401 Range-diff vs v3: 1: cf97c5182e = 1: cf97c5182e t1400: wrap setup code in test case 2: 8b1f4f089a ! 2: d98810875e git_path(): handle `.lock` files correctly @@ -28,46 +28,40 @@ --- a/path.c +++ b/path.c @@ - static void update_common_dir(struct strbuf *buf, int git_dir_len, + #include "path.h" + #include "packfile.h" + #include "object-store.h" ++#include "lockfile.h" + + static int get_st_mode_bits(const char *path, int *mode) + { +@@ 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); + char *base = buf->buf + git_dir_len; ++ 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); + -+ free(base2); ++ if (has_lock_suffix) ++ strbuf_addstr(buf, LOCK_SUFFIX); } void report_linked_checkout_garbage(void) - diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh - --- a/t/t1500-rev-parse.sh - +++ b/t/t1500-rev-parse.sh + diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh + --- a/t/t0060-path-utils.sh + +++ b/t/t0060-path-utils.sh @@ - 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 && + test_git_path GIT_OBJECT_DIRECTORY=foo objects2 .git/objects2 + test_expect_success 'setup common repository' 'git --git-dir=bar init' + test_git_path GIT_COMMON_DIR=bar index .git/index ++test_git_path GIT_COMMON_DIR=bar index.lock .git/index.lock + test_git_path GIT_COMMON_DIR=bar HEAD .git/HEAD + test_git_path GIT_COMMON_DIR=bar logs/HEAD .git/logs/HEAD ++test_git_path GIT_COMMON_DIR=bar logs/HEAD.lock .git/logs/HEAD.lock + test_git_path GIT_COMMON_DIR=bar logs/refs/bisect/foo .git/logs/refs/bisect/foo + test_git_path GIT_COMMON_DIR=bar logs/refs/bisec/foo bar/logs/refs/bisec/foo + test_git_path GIT_COMMON_DIR=bar logs/refs/bisec bar/logs/refs/bisec -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-28 12:57 Subject: [PATCH v4 1/2] t1400: wrap setup code in test case Message-ID: URL: https://gitlist.dev/e/cf97c5182eb98cc0ae72f94d4331abcb4486ca83.1572267438.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Without this, you cannot use `--run=<...>` to skip that part, and a run with `--run=0` (which is a common way to determine the test case number corresponding to a given test case title). Signed-off-by: Johannes Schindelin --- t/t1400-update-ref.sh | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 1fbd940408..69a7f27311 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" ' test_cmp expect .git/logs/$m ' -git update-ref $m $D -cat >.git/logs/$m < 1117150320 -0500 -$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 -$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 -$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 -EOF +test_expect_success 'set up for querying the reflog' ' + git update-ref $m $D && + cat >.git/logs/$m <<-EOF + $Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500 + $C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 + $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 + $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500 + $Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500 + EOF +' ed="Thu, 26 May 2005 18:32:00 -0500" gd="Thu, 26 May 2005 18:33:00 -0500" -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2019-10-28 12:57 Subject: [PATCH v4 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/d98810875ee4a085e84bf00836ce198a7d64d1a2.1572267438.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin 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 --- path.c | 6 ++++++ t/t0060-path-utils.sh | 2 ++ 2 files changed, 8 insertions(+) diff --git a/path.c b/path.c index e3da1f3c4e..198a867017 100644 --- a/path.c +++ b/path.c @@ -11,6 +11,7 @@ #include "path.h" #include "packfile.h" #include "object-store.h" +#include "lockfile.h" static int get_st_mode_bits(const char *path, int *mode) { @@ -350,9 +351,14 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir) { char *base = buf->buf + git_dir_len; + 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); } void report_linked_checkout_garbage(void) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index c7b53e494b..2aca8ccff9 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -285,8 +285,10 @@ test_git_path GIT_OBJECT_DIRECTORY=foo objects/foo foo/foo test_git_path GIT_OBJECT_DIRECTORY=foo objects2 .git/objects2 test_expect_success 'setup common repository' 'git --git-dir=bar init' test_git_path GIT_COMMON_DIR=bar index .git/index +test_git_path GIT_COMMON_DIR=bar index.lock .git/index.lock test_git_path GIT_COMMON_DIR=bar HEAD .git/HEAD test_git_path GIT_COMMON_DIR=bar logs/HEAD .git/logs/HEAD +test_git_path GIT_COMMON_DIR=bar logs/HEAD.lock .git/logs/HEAD.lock test_git_path GIT_COMMON_DIR=bar logs/refs/bisect/foo .git/logs/refs/bisect/foo test_git_path GIT_COMMON_DIR=bar logs/refs/bisec/foo bar/logs/refs/bisec/foo test_git_path GIT_COMMON_DIR=bar logs/refs/bisec bar/logs/refs/bisec -- gitgitgadget ``` ## Junio C Hamano, 2019-10-28 17:30 Subject: Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly Message-ID: URL: https://gitlist.dev/e/xmqqtv7s93rp.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Johannes Schindelin 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. ``` ## Junio C Hamano, 2019-10-29 03:39 Subject: Re: [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Message-ID: URL: https://gitlist.dev/e/xmqqy2x45if3.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > Changes since v3: > > * Instead of duplicating the path when it has a .lock suffix, we now use > strbuf manipulation to strip the suffix temporarily. > * The test case in t1500 was scrapped in favor of a much simpler pair of > test cases in t0060. Makes sense. Will replace. Thanks. ```