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

Automatic worktree detection #243

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

steinybot
Copy link
Contributor

@steinybot steinybot commented Jun 12, 2024

It's a pain submitting PRs to all the repositories that use sbt-git when I want to use a git worktree. This should eliminate the need for that.

Related issues:

@@ -0,0 +1,3 @@
$ exec ./init.sh
> reload plugins
Copy link
Contributor Author

@steinybot steinybot Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out how to run scripted sbt in a subdirectory so I use the meta build as a workaround. Is it possible to change the directory?

useConsoleForROGit := false,
gitReader := new DefaultReadableGit(baseDirectory.value, if (useConsoleForROGit.value) Some(new ConsoleGitReadableOnly(ConsoleGitRunner, file("."), sLog.value)) else None),
useConsoleForROGit := isGitLinkedRepo(baseDirectory.value),
gitReader := new DefaultReadableGit(baseDirectory.value, if (useConsoleForROGit.value) Some(new ConsoleGitReadableOnly(ConsoleGitRunner, baseDirectory.value, sLog.value)) else None),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentionally "." or was that an oversight? Without this it doesn't work when the meta build is the one with the worktree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely not an oversight, but apparently this was made in an attempt to work with worktree as well:
1fa3ca5

Copy link
Contributor Author

@steinybot steinybot Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. works when the main build has the worktree which is likely what the first implementation was solving for. During my testing I encountered the case where the meta build had the worktree and so . does not work. So I think baseDirectory.value is the correct approach as it should work for both cases. It is probably not a real problem anyway since I doubt anyone would ever set it up like this. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably not a real problem anyway since I doubt anyone would ever set it up like this. Do you agree?

I don't even know what a worktree is, so I'm like 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fair enough. They are pretty neat. Essentially they allow you to checkout a branch to a different directory without having to have a completely separate clone. So it is almost always going to be at the local root project base directory (or higher), just like a normal .git directory would be.

@steinybot
Copy link
Contributor Author

@SethTisue appologies for mentioning you directly but I noticed in #248 you were planning on doing a release soon. Any chance of getting this in too? It's a bit of a pain having to workaround it in each project that uses sbt-git.

@SethTisue
Copy link
Member

@eed3si9n @adpi2 would either of you like to take a look at this? I don't have any insight into Jason's questions above about the code tails.

if we don't hear back, I'd be happy to optimistically merge this anyway

@SethTisue
Copy link
Member

re: publishing, I've opened #253

@mkurz
Copy link
Member

mkurz commented Oct 7, 2024

If you haven't heard yet:

@SethTisue
Copy link
Member

@steinybot mind rebasing? also, does @mkurz's info affect this, or is just evidence for urgency?

@mkurz
Copy link
Member

mkurz commented Oct 16, 2024

fyi, the current Play main branch drops support for Java 11, so we just upgrade jgit ourselves to avoid all those workarounds in future (I think it will take years until sbt-git will drop Java 11...):

Tested locally and can confirm it works.

@@ -112,8 +112,8 @@ object SbtGit {
// Build settings.
import GitKeys._
def buildSettings = Seq(
useConsoleForROGit := false,
gitReader := new DefaultReadableGit(baseDirectory.value, if (useConsoleForROGit.value) Some(new ConsoleGitReadableOnly(ConsoleGitRunner, file("."), sLog.value)) else None),
useConsoleForROGit := isGitLinkedRepo(baseDirectory.value),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could check if jgit v7 is used and if so make useConsoleForROGit false, no matter if we are in a linked repo or not (because jgit 7 supports worktrees, read). Like:

    useConsoleForROGit := jGitVersionLtOrEq6 && isGitLinkedRepo(baseDirectory.value),

I looked into jgit but they do not publish the version in a variable nor method... So I don't see a way of doing this easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Maven Central we can find the latest jgit v6 and first v7 version published. The diff is quite big, most of the things we don't need to look at.
So I provide my own html rendered diff here from this command:

git diff v6.10.0.202406032230-r..v7.0.0.202409031743-r org.eclipse.jgit

You can see in v7 the class org.eclipse.jgit.lib.Constants added the GITDIR_FILE constant which is directly related to git worktree.

Would it be able to make use of reflection and check if the Contants class contains the field GITDIR_FILE and if so, we can treat the jgit version on the classpath as one that supports git worktree?
Like:

    useConsoleForROGit := !jGitWithWorktreeSupportDetected() && isGitLinkedRepo(baseDirectory.value),

and in the jGitWithWorktreeSupportDetected method put the reflection code?

What do you think?

Copy link
Member

@eed3si9n eed3si9n Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stepping back a bit, instead of making the automatic detection more complicated, would it make sense to make this more of a machine-wide setting that someone could always opt out of JGit (or opt into JGit)? Also in general, I don't really get the motivation behind JGit. Is the assumption that someone could be working on an opensource project but somehow do not have git installed on their PATH?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also sbt-pgp. In sbt/sbt-pgp#146, I defaulted to using command line gpg and I think the plugin became significantly more reliable because it relied less on the never-ending emulation of gpg via Bouncy Castle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants