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

Fix/top level #784

Merged
merged 5 commits into from
Oct 21, 2019
Merged

Conversation

huanghai21
Copy link
Contributor

do NOT specify the .git to be a directory in case of the git submodule not able to

work properly

Description

After the top level codes refactor here:
6a6a8b0 | 2019-07-07 [Cedric van Putten] refactor: rewrite top level to typescript (#679)

The commitlint can't work properly with the git submodule situation. The root cause is that in the submodule situation, the .git can be a file, not a directory.

Motivation and Context

When we try to use commitlint in a submodule, the commitlint will pick the message from the .git/COMMIT_EDITMSG, but actually we hope it can pick the message from the .git/modules/${submoduleName}/COMMIT_EDITMSG.

This issue is closed but I think it is the most related one, so I still paste it here:
#448

Types of changes

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@escapedcat escapedcat requested a review from byCedric September 11, 2019 09:01
@byCedric
Copy link
Member

Ok, so the issue is that commitlint doesn't work with submodules, because the path of COMMIT_EDITMSG is relative to a submodule path? I think we need to add additional tests if we want to support this. I also think that this fix doesn't fix that same issue, here is why.

Commitlint searches for the latest commit using the /.git/COMMIT_EDITMSG path, right? But when a submodule is used, it's in a whole different folder; /.git/modules/<submodule>/COMMIT_EDITMSG. That means /.git is still a folder, but the location of COMMIT_EDITMSG is in a different place than Commitlint would expect. If we want to support submodules, I think we need to open a new issue and try to come up with a solution for the "moved COMMIT_EDITMSG". Or do you think I'm missing something here?

(cc @escapedcat)

@byCedric
Copy link
Member

@huanghai21 I'm refactoring @comitlint/read to typescript, and I noticed some interesting settings for you. Could you try running Commitlint with the following flags from the root of your git project (the one that includes the submodules)?

$ commitlint --edit=".git/modules/<submodule?/COMMIT_EDITMSG"

This should point Commitlint to the commit edit msg from the submodule 😄

@huanghai21
Copy link
Contributor Author

Hi, @byCedric
Thanks for your response.

Here is more detail want to share with you.

  1. About your suggestion commitlint --edit=".git/modules/<submodule?/COMMIT_EDITMSG"
    This one definitely can work but it is not suite for our situation. In our super repo, there are 30+ submodules, and still growing. So we do not want to add so many duplicated scripts in the package.json file.

  2. With the git submodule workflow. We need figure out 2 concepts. one is the super-repo(parent), another is the sub-repo(child). The only connection between them is a commit-id(or a hash-id), but still, they are 2 independent repo.
    So if we run git cm -m "test" in the super-repo folder, the commit message will be written into the superFolder/.git/COMMIT_EDITMSG file and the commit will get into the super-repo;
    and if we run git cm -m "test" in the sub-repo folder, the commit message will be written into the superFolder/.git/modules/subRepoName/COMMIT_EDITMSG file and the commit will get into the sub-repo.
    The real work situation for us is: If we had some changes for a sub repo, we will get inside the subFolder, do the modification > git cm > git push, just like what we did for a normal git repo. The difference is that there is a .git file(NOT a directory) in the subFolder, and inside this .git file contains the real git directory location: gitdir: ../.git/modules/subRepoName.

One more thing: we can downgrade the commitlint to "7.6.1" and it worked for our situation. But still, do hope you can fix this and publish a new version to the npm registry.

Thank you.

@byCedric
Copy link
Member

byCedric commented Sep 16, 2019

Hi @huanghai21, thanks for your detailed explanation! This helps a lot trying to come up with something here 😄

As for 7.6.1, this one is looking for both (.git) files and directories I think. It's definitely not good if you are forced to use an older version of commitlint. Also took a look at the git specification, and this exact use case came up.

Note: Also you can have a plain text file .git at the root of your working tree, containing gitdir: to point at the real directory that has the repository. This mechanism is often used for a working tree of a submodule checkout, to allow you in the containing superproject to git checkout a branch that does not have the submodule. The checkout has to remove the entire submodule working tree, without losing the submodule repository.

So one way I'm thinking of right now is this:

  1. check for both .git files and directories.
  2. if file: read, parse and return gitdir (if gitdir is not parsable, go to 3)
  3. if directory: return
  4. nothing is found

I think this is also something @marionebl and @escapedcat should take a look at.

Cheers,
Cedric

@huanghai21
Copy link
Contributor Author

Hi @byCedric

Today I spent some time on this and found more information.
Before the refactor of the top-level, we do not specify the 'type'(file | directory) in the option, and it can identify .git(file & directory) correctly, but now, we specify the type to be 'direcotry', so we can only identify the .git directory. So at first, I just try to remove the 'type' from the option, but it still not work cause the 'type' has a default value which is 'file'.
This change is came from the locate-path
The old version without 'type' could identify both file and directory, but now, it can only identify one of them.
To fix this in our top-level code, I will add one more commit and hope you can help me to do the code review.

Thanks

@byCedric
Copy link
Member

byCedric commented Oct 9, 2019

Awesome! Yeah, that's similar to what I had in mind 😄

I'm still a bit concerned about the spec though, it states that if it's a file, it contains a gitdir: . So this is how I interpret that: "If .git is a file, the actual directory is written in that file prefixed with "gitdir:". That means that we should parse that one right? Or do you think that's not what the spec says?

@huanghai21
Copy link
Contributor Author

Hi @byCedric
Correct.
And you can check the logic inside of the @commitlint/read module,
From line62-64

const gitFile = await sander.readFile(dotgitPath, {encoding: 'utf-8'});
const relativeGitPath = gitFile.replace('gitdir: ', '').replace('\n', '');
editFilePath = path.resolve(top, relativeGitPath, 'COMMIT_EDITMSG');

It explains how to locate the COMMIT_EDITMSG file path while the .git is a file.

@byCedric
Copy link
Member

Oh wouw! I totally missed that one! Yeah, let's get this merged then 😄

@marionebl marionebl merged commit 9d09693 into conventional-changelog:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants