feat: add jujutsu walker #601
Conversation
delafthi
changed the title
delafthi
force-pushed
the
push-rtxoonqmxkxy
branch
2 times, most recently
from
2dd1b91 to
60b2c48
Compare
|
Jujutsu tests in |
delafthi
force-pushed
the
push-rtxoonqmxkxy
branch
7 times, most recently
from
56904ba to
84f58f0
Compare
- added jujutsu module similar to the git module, which provides the `IsInsideWorktree` function - added jujutsu walker, with the following differences to the git walker - the list command does not update the index. thus, new files are not listed, the user has to add them to the index first by running a `jj` command - added jujutsu walker test - added jujutsu walker root test - adapted config and docs
delafthi
force-pushed
the
push-rtxoonqmxkxy
branch
from
84f58f0 to
c6d51bc
Compare
There was a problem hiding this comment.
The approach here looks good to me.
In the future, it could make sense to create some abstraction over git and jj, but IMO that's not warranted here. Copying and tweaking is good, and the tests you're adding are super valuable if we ever feel the need to do a future refactor.
| @@ -329,6 +330,21 @@ func determineTreeRoot(v *viper.Viper, cfg *Config, logger *log.Logger) error { | |||
| } | |||
| } | |||
|
|
|||
| // attempt to resolve with jujutsu | |||
There was a problem hiding this comment.
If walk == auto, do we first attempt to resolve with git, and then attempt to resolve with jujutsu? That feels silly to me: we should not keep searching after we find a tree route. In other words, guard this with cfg.TreeRoot == "" (as we do below).
I suspect there's a flatter refactor of all this that looks less like the current case with a big default block and is instead:
- if no tree root, search with tree root file
- if (still) no tree root, search with tree root cmd
- if (still) no tree root (and walk is auto or git), search with git
- if (still) no tree root (and walk is auto or jujutsu), search with jujutsu
- if (still) no tree root, use the directory containing the config file
If you do opt to make this refactor, please split it into a separate initial commit that we could merge first.
| cmd.Dir = path | ||
|
|
||
| err := cmd.Run() | ||
| if err != nil { |
There was a problem hiding this comment.
This must error out of jj is not installed, right? What happens? Is the user still able to use treefmt?
| jjCmd = exec.Command("jj", "file", "track", "--config", "signing.backend=none", ".") | ||
| as.NoError(jjCmd.Run(), "failed to add everything to the index") | ||
|
|
||
| // update jujutsu's index (disable signing for testing) |
There was a problem hiding this comment.
(disable signing for testing)
This is pretty repetitive. Can we instead create a config file for the test?
| as.NoError(jjCmd.Run(), "failed to init jujutsu repository") | ||
|
|
||
| // run before adding anything to the index | ||
| // we shouldn't pick up untracked files, because here the jujutsu walker differs from the git walker |
There was a problem hiding this comment.
I have no knowledge of jj. I can't tell if this comment is telling me something fundamentally different about jj, or if it's telling me something about treefmt's jj walker implementation. Could you rephrase this to make it clear?
| }), | ||
| ) | ||
|
|
||
| // try with a path not in the git index |
There was a problem hiding this comment.
git index
Is this the right term with jj?
| reader, err := walk.NewJujutsuReader(tempDir, "", &statz) | ||
| as.NoError(err) | ||
|
|
||
| files := make([]*walk.File, 33) |
There was a problem hiding this comment.
nit: The number 33 looks pretty magical here. Is there some way to get rid of it? If not, could you add some explanation?
| @@ -208,7 +209,10 @@ func NewReader( | |||
| // for now, we keep it simple and try git first, filesystem second | |||
There was a problem hiding this comment.
This comment is no longer accurate.
| nextLine := func() (string, error) { | ||
| line := j.scanner.Text() | ||
|
|
||
| if len(line) == 0 || line[0] != '"' { |
There was a problem hiding this comment.
line[0] != '"'
This needs some explanation. What are we skipping? Is this really safe? Is there a more structured (json?) output we could consume instead?
This PR introduces a new Jujutsu walker to numtide/treefmt, modeled after the existing Git walker but tailored to Jujutsu repositories. Key differences include:
jjcommands.This addition enables proper handling of Jujutsu ignore rules without requiring co-located Git repos, improving integration and file traversal accuracy for Jujutsu users.
I lack Go experience and primarily copied, and adapted the existing Git walker by adjusting commands. I would welcome guidance or collaboration to develop a more idiomatic and robust implementation.