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

Improve git root detection logic #195

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jesseleite
Copy link

@jesseleite jesseleite commented Nov 3, 2024

Firstly, a quick thank you, for all your awesome work on sesh! It's a game changer for me ❤️

The problem:

The sesh README says that sesh root assumes existence of a .bare folder...

Note: This will only work if you are in a git worktree or git repository. For now, git worktrees expect a .bare folder.

Not everyone uses git worktrees this way. For example, many prefer the conventional git clone --bare [repo] project, which leaves you with a messy project folder, but this is still very common nonetheless.

I on the other hand, prefer to run git clone --bare [repo] project/.git, which stores all of the git meta in .git folder, but still creates a bare repo. This leaves me with a really clean project folder when using worktrees...

CleanShot 2024-11-03 at 13 12 18

The .bare convention is cool too, but again would require different pathing logic for each situation.

A possible solution:

My proposition is to use the output of git worktree list, since git knows where the main worktree / bare repo lives...

CleanShot 2024-11-03 at 13 13 09

^ As you can see, the main worktree is always listed first, no matter how you roll with git worktrees (whether that be using .git, or .bare, or doing it the messier default way.

For example, this is what you see when running git worktree list...

CleanShot 2024-11-03 at 13 16 30

This PR uses git worktree list to grab that first field, for the basis of sesh root and also for the namer package used when naming sessions.

I was able to remove all --git-common-dir, --show-toplevel, and .bare checks, because those all seem to be handled well with the first field returned by git worktree list; It seems to be super consistent across all situations 🎉

Todo:

  • Proof of concept
    • Using git worktree list to find root from within git worktree repos
      • Using .bare convention
      • Using .git convention
      • With standard worktree (git config/refs/etc. files at root)
    • Using git worktree list to find root from non-worktree repo subdirs
  • Get working with sesh root command
  • Get working with namer package
  • Add test coverage

@jesseleite
Copy link
Author

jesseleite commented Nov 3, 2024

Also, as a side effect of the new implementation, the issue described in #192 seems to be solved! 🎉

CleanShot 2024-11-03 at 14 17 16

Tmux + gum example using the new sesh root functionality...

CleanShot 2024-11-03 at 14 21 31

@joshmedeski
Copy link
Owner

Gotta fix tests, I'll DM you and we can do some pair programming to fix it.

@joshmedeski joshmedeski self-requested a review November 5, 2024 22:21
@jesseleite jesseleite marked this pull request as ready for review November 7, 2024 02:53
@jesseleite jesseleite changed the title Improve sesh root logic Improve git root logic Nov 7, 2024
@jesseleite jesseleite changed the title Improve git root logic Improve git root detection logic Nov 7, 2024
@joshmedeski
Copy link
Owner

When I choose a git worktree I got this result:

_bare/Users/joshmedeski/c/sesh/v1

But expected this::

sesh/v1

@joshmedeski
Copy link
Owner

Furthermore, when I call sesh root I'm getting the following output:

~/c/sesh/.bare

However, I need this to be the root so I can properly leverage the root search feature:

~/c/sesh

@joshmedeski joshmedeski removed their request for review December 20, 2024 18:45
@joshmedeski joshmedeski marked this pull request as draft January 7, 2025 04:44
@joshmedeski
Copy link
Owner

I moved this to a draft for now, let me know if you want someone else to take over or have some time soon to revisit this work and finish it up, thanks!

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.

2 participants