Conversation
Co-authored-by: Paweł Smyrek <56868128+psmyrek@users.noreply.github.com>
There was a problem hiding this comment.
Below, you can find my environment.
mrgit.json
{
"packages": "external/",
"$rootRepository": "ckeditor/ckeditor5",
"dependencies": {
"ckeditor5-dev": "ckeditor/ckeditor5-dev@latest"
},
"presets": {
"stable": {
"$rootRepository": "ckeditor/ckeditor5#stable"
},
"release": {
"$rootRepository": "ckeditor/ckeditor5#release"
},
"dev": {
"ckeditor5-dev": "ckeditor/ckeditor5-dev"
}
}
}package.json
{
}Executing the mrgit sync command ends with the following results.
# ckeditor5 [ROOT REPOSITORY] 1/2 (50%)
fatal: not a git repository (or any of the parent directories): .git
# ckeditor5-dev 2/2 (100%)
Package "ckeditor5-dev" was not found. Cloning...
Cloning into '/home/pomek/Projects/tmp/workspace-ckeditor5/external/ckeditor5-dev'...
remote: Enumerating objects: 37011, done.
remote: Counting objects: 100% (6290/6290), done.
remote: Compressing objects: 100% (1805/1805), done.
remote: Total 37011 (delta 4426), reused 5981 (delta 4296), pack-reused 30721
Receiving objects: 100% (37011/37011), 7.40 MiB | 1.67 MiB/s, done.
Resolving deltas: 100% (27181/27181), done.
2 packages have been processed.
Execution time: 7s702ms.
❗❗❗ The command failed to execute in 1 repository:
- ckeditor5 [ROOT REPOSITORY]
Hence, we need to improve the sync command. It can bootstrap or sync (checkout + pull) changes. I would focus on the first task. Could we try to clone the repository to a non-empty directory? (example). If it doesn't work, I propose to display a warning that the root repository must be cloned manually, and do not process it.
|
Due to the fact that commands are executed in parallel for each repository, cloning the root repository if it's not there already proves problematic. Additionally, in case of missing root repository, I've decided to prevent the script from executing entirely. Initially, I've implemented behaviour that printed warning about missing root repository, but let the script to clone and work on nested repositories. However, this puts user in awkward situation where he now has to clone his repository into a non-empty directory. While this is possible, I think that it would be easier for the user to simply clone the root repository into an empty directory, and then execute |
lib/index.js
Outdated
|
|
||
| const mainPkgJsonPath = path.resolve( cwd, 'package.json' ); | ||
|
|
||
| if ( !fs.existsSync( mainPkgJsonPath ) ) { |
There was a problem hiding this comment.
The occurrence of the package.json file does not say "you are in the .git" repo. It can be true in 99% of cases, but I would check if the .git/ directory is available. The program should finish if it does not exist if the $rootRepository is specified in the configuration.
lib/index.js
Outdated
| const repositoryResolver = require( toolOptions.resolverPath ); | ||
| const forkPool = createForkPool( CHILD_PROCESS_PATH ); | ||
|
|
||
| if ( !options.skipRoot && toolOptions.$rootRepository && !fs.existsSync( path.join( cwd, '.git' ) ) ) { |
There was a problem hiding this comment.
For future reference, please extract such complex conditions to a smaller util. It's PITA to understand it without going deeper.
The goal is to figure out if mrgit should break the process. So, we have a name for the util.
| if ( !options.skipRoot && toolOptions.$rootRepository && !fs.existsSync( path.join( cwd, '.git' ) ) ) { | |
| if ( shouldBreakProcess( toolOptions ) ) { |
And somewhere below:
function shouldBreakProcess( toolOptions ) {
if ( options.skipRoot ) {
return false;
}
if ( !toolOptions.$rootRepository ) {
return false;
}
if ( fs.existsSync( path.join( cwd, '.git' ) ) ) {
return false;
}
return true;
}Code style depends on the developer, but all conditions are a bit more readable than it was before.
4 participants
Suggested merge commit message (convention)
Feature: Added support for executing commands in the root repository. Closes #160.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.