Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle git_path() with lock files correctly in worktrees #401

Closed
wants to merge 2 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 15, 2019

I stumbled over this during my recent work in Git GUI 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.

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 <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Oct 16, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 16, 2019

path.c Outdated Show resolved Hide resolved
@dscho
Copy link
Member Author

dscho commented Oct 17, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 17, 2019

path.c Outdated Show resolved Hide resolved
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2019

This branch is now known as js/git-path-head-dot-lock-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2019

This patch series was integrated into pu via git@5c3ead4.

@gitgitgadget gitgitgadget bot added the pu label Oct 18, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2019

This patch series was integrated into pu via git@48502d2.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 21, 2019

This patch series was integrated into pu via git@e9e2058.

@dscho
Copy link
Member Author

dscho commented Oct 21, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 21, 2019

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 22, 2019

This patch series was integrated into pu via git@38fe1fc.

path.c Outdated Show resolved Hide resolved
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>
@dscho
Copy link
Member Author

dscho commented Oct 28, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 28, 2019

@gitgitgadget

This comment has been minimized.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 29, 2019

This patch series was integrated into pu via git@af2a863.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 30, 2019

This patch series was integrated into pu via git@6fe6fec.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 2, 2019

This patch series was integrated into pu via git@7c3951c.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 4, 2019

This patch series was integrated into pu via git@74393e7.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 6, 2019

This patch series was integrated into pu via git@0fd1045.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 7, 2019

This patch series was integrated into pu via git@6e6457e.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 8, 2019

This patch series was integrated into pu via git@29765d6.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 10, 2019

This patch series was integrated into pu via git@dcbbf86.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 11, 2019

This patch series was integrated into pu via git@2b65dbb.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2019

This patch series was integrated into pu via git@befc330.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 19, 2019

This patch series was integrated into pu via git@29cd43a.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 19, 2019

This patch series was integrated into next via git@b406944.

@gitgitgadget gitgitgadget bot added the next label Nov 19, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Nov 20, 2019

This patch series was integrated into pu via git@48f9bda.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 21, 2019

This patch series was integrated into pu via git@1f121e3.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 22, 2019

This patch series was integrated into pu via git@80883c4.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 25, 2019

This patch series was integrated into pu via git@07e86b8.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 27, 2019

This patch series was integrated into pu via git@a126152.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 2, 2019

This patch series was integrated into pu via git@a2b0451.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 2, 2019

This patch series was integrated into next via git@a2b0451.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 2, 2019

This patch series was integrated into master via git@a2b0451.

@gitgitgadget gitgitgadget bot added the master label Dec 2, 2019
@gitgitgadget gitgitgadget bot closed this Dec 2, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Dec 2, 2019

Closed via a2b0451.

@dscho dscho deleted the lock-files-in-worktrees branch December 2, 2019 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant