Support git commands run in submodules - draft

[sdarwin]

Hi,

So far, this pull request adds submodule support to currentBranch and resolveRef as a first step towards including the same function in all the other command as well (https://isomorphic-git.org/docs/en/alphabetic)

Enable the ability to run git commands within submodule directories: fixes https://github.com/isomorphic-git/isomorphic-git/issues/1647

About worktrees:

It would definitely be nice to parse both submodules and worktrees. The complexity with worktrees is that sometimes git needs to know the worktree's own git directory (.git/worktrees/my_worktree) and other times the commondir ( ../.. ) . It varies. If each git function used one or other of those choices, it would be possible to go through all supported commands and set the selection to either 1 or 2 (worktree dir or commondir). However there is a third category - when information is needed from both. In those cases, it would require fiddling with the internals of each command or adding more variables inside the functions. Not impossible. However with limited availability of developer time there is value in just doing an easier step - supporting submodules - which would be immediately useful. Nothing is blocking worktrees later, in the future.

I have tested this and it works, but please review, let me know. Then I could apply the code to other files.

[jcubic]

You've broken all tests that use the function you modified.

[sdarwin]

You are certainly right! This code was based on the example in the issue, where it showed this snippet:

const fs = require('fs')
const { promises: fsp } = fs
...  fsp.stat ...

Tests are failing on the stat command Cannot read properties of undefined (reading 'stat') .

When trying this locally, I do have require('fs') and it succeeds, but that is not in the tests.

I was debating adding a warning earlier, and so should include it now. I am usually programming in python or bash, not nodejs. But we need this feature, as end-users. The issue has been open multiple years, and it seems like fun to contribute.

Your collaboration is more than welcome. Do you have ideas? I will try to investigate the errors.

[jcubic]

fs inside the library is not the same as require('fs');. It's a wrapper over fs library for internal use.

If you want to contribute to a project, you first need to understand how it works, and if you modify something you need to test if it works. Not just the code snippet inside Node.js, but the library itself.

[jcubic]

This is the code for the fs used internally:

https://github.com/isomorphic-git/isomorphic-git/blob/main/src/models/FileSystem.js

[sdarwin]

Uploaded a new commit..

Test if it works. Not just the code snippet inside Node.js, but the library itself.

Agreed. Locally I have a copy of the library and then ran something like this when testing:

const git = require('isomorphic-git')
let ref = git.resolveRef({
  fs: require('fs'),
  dir: '/opt/github/gitorg/gitrepo',
  fullname: true,
  ref: 'HEAD'
})
ref.then(function(result) {
     console.log(result) 
  })

[sdarwin]

would be nice to have some unit test

Yes.
Not sure about my schedule though, have to see.

[sdarwin]

To compose a "superproject" that has "submodules" it's necessary to check out multiple git repos.
But they should all exist together in one directory.
In the tests I am attempting to checkout the main project, and then a submodule, and copy the submodule in. This works for a normal filesystem.
It's failing on the browser filesystem.
Any ideas?
I'm wondering if BrowserFS were upgraded to ZenFS and/or Nodejs was upgraded to version 24, there would be improved "copy" commands available.
An alternative to "copy" is "symlink" which supposedly ZenFS supports.

[sdarwin]

Migrating everything from BrowserFS to ZenFS is likely somewhat complicated. That is the latest and preferred filesystem so it should be done. Then 'symlink' would be available.
In the meantime I have applied the submodules test to only the standard filesystem. Tests are passing. CI should be passing as before.
It's possible to logically examine the code and attempt to determine "is this correct?".
Even if the browser is failing ( there is no reason to think that will fail ), adding submodule support in the cli is worthwhile.

[sdarwin]

There are three sets of tests to consider:

1. The existing tests, to confirm nothing was broken.
2. New tests, on a standard filesystem.
3. New tests, on a browser filesystem.

It's easy to forget about the first one, but in fact the existing tests will verify this PR doesn't break current functionality, which is quite important. What I would like to do is roll out the same update which has been applied to two files, onto all the other files. In the same way. Then merge. At least be sure nothing got broken. It's within the realm of possibility the new feature isn't quite right (on submodules). But probably it is. Do you agree? And better than previously.
Would this be acceptable?
How many tests exactly would you require? of any type

[jcubic]

Whatever you implemented needs to be tested that it's correct. It can be one test or multiple tests if there are multiple use case that your feature addresses.

[jcubic]

Also, can you rebase with the latest changes?

[sdarwin]

The plan forward is uncertain, since the idea would be to include those submodule updates to all commands from https://isomorphic-git.org/docs/en/alphabetic which is around 66 commands. So your position is that I must add 66 new tests also? It is possible. But laborious. And then, there are two main filesystems which are the browser filesystem and the regular filesystem. I am completely blocked at the moment in regards to the browser filesystem. And so, my last comment was proposing that new tests are not required for that.
And, to clarify, if you are saying I must compose 66 new tests.

[jcubic]

The file system is abstracted away. If one is working, it will work for the other.

Submodules are related to the file system, so you don't need to test every command from the docs. I think it will be fine to test how you use submodules IRL, you clone the repo and add submodule and commit and test if everything is ok. The test cloning with submodules.

In canonical git you can clone recursively or init submodules after the repo was cloned. I would just test what is allowed and document how to use submodules with the library.

[jcubic]

But if you add your feature to 66 commands, then all should be tested.

[sdarwin]

The file system is abstracted away. If one is working, it will work for the other.

Ok, then it's sufficient to test on the regular filesystem, and we will assume the browser filesystem is functional, although it is not explicitly tested. This is reasonable.

clone recursively

maybe not "clone recursively", we'll see, because that is getting into providing "full submodule support". I am focusing on a subset of that, hopefully a stand-alone, self-sufficient subset of submodule features. Namely "support git commands run in submodules". Assuming you already have submodules checked out, you want to be able to run certain commands like "git branch". Referring to the original Issue, the title is "work with submodules that have been checked out by other means".

Therefore maybe this will be very limited, and won't affect 66 commands. It will be the few commands that can be fixed by providing them the location of their git folder. Like currentbranch and resolveref. I haven't gone through how many other commands fit into this category. What is the total number... Perhaps not so many. But it adds a usable feature and doesn't prevent other related features from being added later on.

[sdarwin]

Removed the word "draft" from the description of the pull request. It's ready to test.

[sdarwin]

Hi @jcubic, could you please review this pull request when you have a chance

[jcubic]

Tests failed, and I don't see the longs anymore.

[sdarwin]

@jcubic , it appears the Github Actions didn't run on the latest commit.

"2 workflows awaiting approval. This workflow requires approval from a maintainer.
Learn more about approving workflows."

Can you trigger another test run?

Also, by the way, tests are expected to fail. Compare to this unrelated one:

https://github.com/isomorphic-git/isomorphic-git/actions/runs/16280340879

4 succeeded and 4 failed.

Edit:

I wasn't aware of "Azure Pipelines". Triggering another build now. Let's see.

Merely re-running those tests allowed them to pass. Green checkmark.

[jcubic]

Don't worry about the errors, only isomorphic-git-PR is important, this is an old pipeline from Azure.

I will need to review the code again.

[sdarwin]

"support submodules". Many end-users of our open-source software need this functionality.

Pinging @jcubic , the PR is ready to merge.

[jcubic]

I can't accept a PR where tests are disabled for browser.

[sdarwin]

@jcubic

Please go back in the conversation, when I was explaining how extremely difficult it was to set up the tests on the browser, because "copy" and "link" commands are missing. Everything must be updated to ZenFS in order to have improved access to such commands.

I wrote "I am completely blocked at the moment in regards to the browser filesystem. And so, my last comment was proposing that new tests are not required for that."

You replied "The file system is abstracted away. If one is working, it will work for the other."

"If one is working, it will work for the other" then tests on a regular filesystem demonstrate it's "working".

Then I wrote "Ok, then it's sufficient to test on the regular filesystem,"

You did not contradict that. You did not reply "no" or anything, to that statement.

create branches to start with feature. I don't understand what is the purpose of this.

Currently, github actions "Build" tests don't run on pushes. A "push" to a branch doesn't trigger a job. No job at all. For testing it's helpful to be able to push to a branch on your local fork. The ci.yml modification was to enable "Build" on some branches, to assist in local testing, for anyone who forks the codebase. If you prefer, I can remove it, let me know.

[jcubic]

The GitHub actions are not fully implemented and working. I think that we had problems with sharing secrets with forks.

Also note that your branch don't start with feature/. If you do this, GitHub action will stop running at all.

If the tests can run in the browser because of missing commands, it means that the feature will not run in the browser. I did not expect that you will call shell commands directly.

[sdarwin]

GitHub action will stop running

The new section in ci.yml:

  push:
    branches:
      - feature/** 

Is "additional". It "adds" yet another way to trigger a "Build". Without that part, no "Build" will run on a push. The current status quo is that no "Build" runs on a push. This is only increasing the number of Builds.

I will remove it, to remove any worries.

it means that the feature will not run in a browser

There are (at least) two parts of each test.

1. "Setting up the test bed". That involves copying files (manually). Copying files into place, so that the directory structure mimics what a user might have in their environment when they encounter submodules.

2. "Running the actual test". This part exercises the real code changes, which exist outside of the __tests__ folder, in the "api" folder and so on.

There would be a genuine problem if the "api" or "command" updates were using shell commands. That would mean shell commands are used during (step 2) "running the actual test", and that would be run in the real world by users. However, it's not the case. The "copying files" only exists in the __tests__ folder. To prepare the test.

Users execute files from the "command" and "api" directories. Those don't involve any "copy", or "shell", or anything.

What we need... is a proper, recursive, "cp -rp", that copies entire directories into place, perfectly, and maintains file permissions, and "just works". Or as a fallback, a symlink "ln -s" type command. Both the browser filesystem and the regular nodejs filesystem, both of those, seem to be lacking such features. After an upgrade to ZenFS, it may be "recursive copy" would be available to the browser filesystem. After an upgrade to nodejs 24 (more or less, not sure) those may be available to the regular filesystem. At the moment, we are stuck with nodejs 14 in ci.yml.
But to repeat... in spite of "recursive copy" being missing, not available, etc... That is only used in "setting up the test bed". In production usage, the end-user will have real submodules, already in place, and they need isomorphic-git to interact with the submodules. At that time, "copy" isn't need. It's not used. That's demonstrated by the absence of any "copy" type commands being added to the "api", "commands", or "utils" folders.

[jcubic]

So we can update Node if want. But we should support versions that are maintained. Node 14 reached end of life. So if the feature you need is in Node 20 that is still maintained or 22 we can update Node to those versions. Node 24 may be problematic, since not everyone will use it, and we want tests for Node that people use.

We can also introduce ZenFS just for this one test, there was an idea to migrate (since BrowserFS is not maintained anymore), but it looks like too much effort for a single person (or maybe just no one want to do that).

I'm not sure what did you change in last commit because you squashed or amended the commit, but the test failed. It's easier to track changes if you don't modify the history. The commits are squashed on merge, so it doesn't matter what you have, but it's easier to track and maintain.

[jcubic]

It looks like the test timeout because of the console.logs that slow down the tests.

[sdarwin]

It looks like the test timeout because of the console.logs that slow down the tests.

The failures were sporadic, before. I removed the console.log and "git push"ed again.

we can update Node if want.

I don't usually use AI, but just lately starting to. Asked "grok" : "Does the latest node.js support a recursive copy command? In what version was that feature added?"

Answer:

"The fs.cp and fs.cpSync methods are no longer experimental as of Node.js v22.3.0, meaning they are stable and suitable for production use."

There are some issues though.

1. The fs.cp in Nodejs 22 would apply to the "regular" filesystem, still not to the browser filesystem, right? For that we need ZenFS. I believe so.

2. I'm already pushing my limits of coding knowledge in nodejs with the current set of features. Actually I did briefly try to switch tests to ZenFS, but it didn't work, for me.

3. There is no compelling requirement to upgrade to nodejs 22 (for this PR specifically) because the "shell" command is enough, for the "regular filesystem". The compelling requirement is ZenFS for the browser filesystem. Although having both would be nice.

Action items:

• Maybe you could migrate a few existing tests to ZenFS, previous old tests, and check that into the repository, as a demonstration of using ZenFS. With further instructions: "See, just set this variable, as I did, and your tests can also switch to ZenFS".

• Nodejs 22 - That almost seems like a separate different pull request, involving multiple files. If you, or someone, would migrate this entire project to "node 22". Great. Please proceed. Switch to version 22. Or, we might not need it now, necessarily (as just mentioned above.)

All of these are "further and further enhancements" though. The current pull request doesn't break any previous tests. Previous tests weren't modified. And they still pass, without errors. That's an argument that what is here is enough.

Edit: I was briefly suggesting only Zen was needed, but maybe that is not right. for a greater chance of success all steps above ought to be done ( -> node 22).

[jcubic]

I don't do much coding for this project, but I can see if I can migrate one of the tests to ZenFS. If it's straight forward, maybe we can use AI to migrate all the tests.

Your tests are problematic, I did not expect that the test will be just disabled for browser like this. I'm not sure what I expected, I probably just didn't think about it. Your feature is nice to have, but I would prefer to have tests that don't have any exceptions like this.

[sdarwin]

I edited my last comment, which is to say: it's probably safer to also upgrade to node 22 or higher.

Right?

Then a question is: where does the nodejs version apply? where does the nodejs version have an effect? Only in __tests__, only in CI files, or is it project-wide.

Would that cause multiple package.json and package-lock.json files to change? Who is the right person to carry this out, in it's own pull request.