Add support for determining default branch when calling hub pull-request form worktree

[csaska]

Fixes #2484 .

When hub pull-request is called from a worktree, hub does not correctly find the default branch for the repo.

This code is a bit of a hack; it attempts to check for worktree in the path pointed to by $GIT_DIR/.git of the worktree.

This should work for the default cause; however, I am not sure if it will work in all scenarios, specifically when users decide to set git config extensions.worktreeConfig true

[csaska]

On second thought, perhaps these changes should be added to this function
https://github.com/github/hub/blob/b0db79dbf3988dd25c0053abc39a77fc92c46bfc/git/git.go#L26

[csaska]

Perhaps the fix is as simple as changing --git-dir to --git-common-diras long as the ref being used is a shared ref i.e. under the .git/ref dir

https://github.com/github/hub/blob/b0db79dbf3988dd25c0053abc39a77fc92c46bfc/git/git.go#L31

[mislav]

Thank you! I wonder if this is safe to apply across the board, or whether it might have unwanted consequences 🤔

[mislav]

What is the earliest git version where --git-common-dir was introduced?

[csaska]

Looks like it was first introduced in https://git-scm.com/docs/git-rev-parse/2.5.1.

[csaska]

Based on these two pages, I'm inclined to say that it will always work, even if the path is relative.

https://git-scm.com/docs/git

GIT_COMMON_DIR

If this variable is set to a path, non-worktree files that are normally in $GIT_DIR will be taken from this path instead. Worktree-specific files such as HEAD or index are taken from $GIT_DIR

commondir

If this file exists, $GIT_COMMON_DIR (see git[1]) will be set to the path specified in this file if it is not explicitly set. If the specified path is relative, it is relative to $GIT_DIR. The repository with commondir is incomplete without the repository pointed by "commondir"

[csaska]

If you're setting GIT_COMMON_DIR, then based on the docs, it seems like you should be expecting this dir to be used for all non-worktree files. I guess your question though is concerned with what other code is using Dir()? So, what other hub features rely on GIT_DIR, and do they ever expect to use worktree-specific files?

[mislav]

Requiring git 2.5 all of the sudden is a dealbreaker, since thus far we've supported git 1.7+

[csaska]

Are you against something like?

dirCmd := gitCmd("rev-parse", "-q", "--git-dir")
parseCmd.Stderr = nil
gitDir, err := parseCmd.Output()

parseCmd := gitCmd("rev-parse", "--is-inside-worktree")
parseCmd.Stderr = nil
isWorktree, err := parseCmd.Output()

if (isWorkTree) {
     // try and use GIT_DIR/../../
     gitDir = gitDir + "/../../"

     // check if path exists, otherwise fallback to regular behavior
}

edit:

actually, I don't think it's a problem that this option isn't in earlier versions of git. I think git rev-parse fails silently if an option doesn't exist. So we could check if git rev-parse --git-common-dir returns a valid path, otherwise fallback

Alternatively, we could do git rev-parse --local-env-vars, see if GIT_COMMON_DIR exists and use it if it does, otherwise fallback

I will work on both and test them on git 1.7+. Stay tuned.

[csaska]

Okay, so git worktrees aren't supported in [email protected]. Therefore, any git command attempted from a worktree in [email protected] will fail.

So, changing this code to use git rev-parse --git-common-dir instead of git rev-parse --git-dir will not change the behavior in any git release that does not include worktree support.

worktrees were introduced in [email protected].

[csaska]

Okay so now looking at the first release of git with worktrees.

Based on these results, I would say there is no issue in changing git rev-parse --git-dir to git rev-parse --git-common-dir. If you are in a worktree prior to [email protected], your rev-parse command will fail anyway.

@mislav do you have any other concerns?

[mislav]

Based on these results, I would say there is no issue in changing git rev-parse --git-dir to git rev-parse --git-common-dir. If you are in a worktree prior to [email protected], your rev-parse command will fail anyway.

That's a good point; thank you for looking into this in such depth!

I'm wondering what about when you're not in a worktree, but in a regular git working directory, and you issue git rev-parse --git-common-dir with an old git version. Wouldn't it fail because --git-common-dir flag is not recognized there? If so, should we perhaps run git rev-parse --git-dir as fallback?

[csaska]

lol yes you're right...I got so hung up on worktrees that I forgot about the nomal scenario.

[csaska]

@mislav Thanks for the responses! I added this function. Thought being that this perhaps isn't the last time a hub command has different behavior in a worktree.

https://github.com/github/hub/blob/479ed29d9253c88f4da78caf8c9061a6da760528/git/git.go#L82

In Dir(), if the function returns an error, we fall back to the original behavior of the code

[mislav]

I finally looked into this in depth and realized that the main problem is that we're trying to figure out the correct .git directory to manually read the .git/refs/remotes/<remote>/HEAD file from. This PR just feels like we're making an already complicated approach even more complicated.

What if we just use git commands to read the branch instead? Then we don't have to implement any manual reading of files under the .git directory, and we can have git resolve everything correctly in worktrees.

I've opened this exploration in #2489, let me know what you think

[csaska]

This PR just feels like we're making an already complicated approach even more complicated.

I completely agree.