Commit message not throwing error while commit from git gui

[krishna63]

Analysis: cli.js is looking for the commit message from git/COMMIT_EDITMSG file irrespective of mode of committing the code (GIT BASH or GIT GUI or GITHUB) GIT GUI will be storing the commit message in .git/GITGUI_EDITMSG file and we are reading from COMMIT_EDITMSG file always for the commit message which is causing the issue.

Solution: Checking for the existance of GITGUI_EDITMSG file, if it is there then we are taking the commit message from that file else we are reading the message from COMMIT_EDITMSG. As GITGUI_EDITMSG will be created and deleted only when we are commiting from GIT GUI.

[codecov-io]

Codecov Report

Merging #79 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #79   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         126    126           
=====================================
  Hits          126    126

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e3a81f8...da128ae. Read the comment docs.

[krishna63]

@kentcdodds : Have a look and validate, merge ASAP, since we have dependency in our project

[krishna63]

@Garbee : we cannot loop through array of files because, if we pick the first value of the array and that file exist then again when the user is commit from gui it fails.

ex var filesArray = ['COMMIT_EDITMSG', 'GITGUI_EDITMSG', 'SOMEOTHERUI_EDITMSG']

so the COMMIT_EDITMSG will store the value while commit thru cli whereas commit msg is stored in GITGUI_EDIMSG when commiting from gui. so if we look for the first value file it will always exist and never get the current git commit message incase he is making thru gui.

[spirosikmd]

Hi @krishna63! As the tool supports passing a file path for the file that contains the commit message, would it work for you passing GITGUI_EDITMSG as an argument to commitmsg hook in package.json? For example,

{
  "scripts": {
    "commitmsg": "opt --in commitmsg --exec \"node ./lib/cli.js GITGUI_EDITMSG\"",
  }
}

assuming that git gui generates GITGUI_EDITMSG in the root project directory and that you are using husky's commitmsg hook. In this way, we won't need to hardcode another filename format.

What do you think? Am I making sense?

[krishna63]

@spirosikmd : I agree :+1: , I feel still some changes are required mentioned below

Checks:

1. We need to check whether the mentioned/passed file exist or not
2. While replacing we cannot hard code the value.

Above code checks are need for following reasons: Case 1 : When user commiting from cli. GITGUI_EDITMSG file does not exist

• As we are passing GITGUI_EDITMSG from package.json, if it does not exist then we have to pick the commit message from COMMIT_EDITMSG file so that it validate the commit message.
• if the file(GITGUI_EDITMSG) does not exist meaning the commit is made from CLI .

Case 2 : User commiting from GITGUI. COMMIT_EDITMSG file exist and GITGUI_EDITMSG file also exist

• GITGUI_EDITMSG file will exist and it picks the commit message and validate accordingly.
• But while replacing commitMsgFileOrText it will not find the hard coded string and the incorrect message will not be logged
• if the file(GITGUI_EDITMSG) exist meaning the commit is made from GIT GUI.

Code changes will be as follows @lineNo: 21

var commitMsgFileOrText = getGitFolder();

@lineNo: 51~56 filename = ( fs.existsSync(commitMsgFileOrText + process.argv[2]) ? filename : 'COMMIT_EDITMSG' ); commitMsgFileOrText += '/' + filename;

Doing this way we are not hard coding any new files in the cli.js. Kindly let me know your thoughts

[spirosikmd]

This seems like it will work! Please update the PR!

I would however name var gitFolder = getGitFolder();. Also to avoid any hoisting issues with commitMsgFileOrText variable, I would pass filename to validate function, so in the end we could call it like validate(msg, isFile, filename);.

[krishna63]

@spirosikmd : I have made the required changes, please validate

[krishna63]

@spirosikmd : Could review this and process the PR further

[Garbee]

Sorry was away most of the day. LGTM at this point FWIW.