ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If It Exists #7796

hrai · 2020-02-22T05:00:21Z

Fixes #3954

Before

Merge commit message with quotes was failing the merge test "message text" test

After

The merge succeeds.

Test methodology

Added NUnit test

Test environment(s)

GIT version 2.22.0.windows.1
Windows 10

✒️ I contribute this code under The Developer Certificate of Origin.

…t Exists

mstv

LGTM, have not run

mstv · 2020-02-22T19:45:31Z

GitCommands/Git/GitCommandHelpers.cs

@@ -1024,6 +1026,11 @@ public static ArgumentString MergeBranchCmd(string branch, bool allowFastForward
            };
        }

+        private static string EscapeDoubleQuotes(string message)


Should rather be named EscapeQuotes.
Code block can be omitted, i.e. turn into => message?.Replace("\"", "\\\"");

RussKie

Trying to escape quotes and slashes is like opening a Pandora box of pain.
Instead I propose use --file option similar to how we run the commit command:

gitextensions/GitCommands/Git/GitModule.cs

Lines 2144 to 2156 in 85fde1c

    
           public ArgumentString CommitCmd(bool amend, bool signOff = false, string author = "", bool useExplicitCommitMessage = true, bool noVerify = false, bool gpgSign = false, string gpgKeyId = "") 
        
           { 
        
               return new GitArgumentBuilder("commit") 
        
               { 
        
                   { amend, "--amend" }, 
        
                   { noVerify, "--no-verify" }, 
        
                   { signOff, "--signoff" }, 
        
                   { !string.IsNullOrEmpty(author), $"--author=\"{author?.Trim().Trim('"')}\"" }, 
        
                   { gpgSign && string.IsNullOrWhiteSpace(gpgKeyId), "-S" }, 
        
                   { gpgSign && !string.IsNullOrWhiteSpace(gpgKeyId), $"-S{gpgKeyId}" }, 
        
                   { useExplicitCommitMessage, $"-F \"{Path.Combine(GetGitDirectory(), "COMMITMESSAGE")}\"" } 
        
               }; 
        
           }

hrai · 2020-02-25T01:20:11Z

@RussKie GetGitDirectory() needs RepositoryPath.

Is there a method or a property that I can use to get this value?

Also the algorithm will be:

Write merge message to .git/MERGE_MSG file
Use git merge -F command

RussKie · 2020-02-25T11:51:41Z

If you have an instance of a `GitModule`, you can get its repo path from `Working Dir`.

(cherry Picked From Commit 669936f)

hrai · 2020-02-29T03:34:16Z

@RussKie now the merge happens through MERGE_MSG file. I had to shuffle some methods around. Also updated the tests.

RussKie · 2020-02-29T12:34:21Z

GitCommands/Git/GitCommandHelpers.cs

+        public static string FormatCommitMessageFromTextBox(
+            string commitMessageText, bool usingCommitTemplate, bool ensureCommitMessageSecondLineEmpty)
+        {
+            if (commitMessageText == null)


What about commitMessageText == "" or commitMessageText == " "?

This is a pre-existing code. If replacing the above with IsNullOrWhitespace() is not going to affect the existing logic, I can push the change.

RussKie · 2020-02-29T12:35:31Z

GitCommands/Git/GitCommandHelpers.cs

+            var lineNumber = 1;
+            foreach (var line in commitMessageText.Split('\n'))
+            {
+                string nonNullLine = line ?? string.Empty;


How can line == null?

This is a preexisting method that I pulled from GitUI/CommandsDialogs/FormCommit.cs

RussKie · 2020-02-29T12:42:44Z

GitCommands/Git/GitCommandHelpers.cs

@@ -1007,30 +1008,23 @@ private static GitItemStatus GitItemStatusFromStatusCharacter(StagedStatus stage
            };
        }

-        public static ArgumentString MergeBranchCmd(string branch, bool allowFastForward, bool squash, bool noCommit, string strategy, bool allowUnrelatedHistories, string message, int? log)
+        public static ArgumentString MergeBranchCmd(string branch, bool allowFastForward, bool squash, bool noCommit, string strategy, bool allowUnrelatedHistories, string folderPath, int? log)


Please add tests for this method that at least cover the -F flag

All the test cases now accept a path instead of a message.

The response has -F flag.

gitextensions/UnitTests/GitCommands.Tests/Git/GitCommandHelpersTest.cs

Line 747 in 5935979

Assert.AreEqual(

RussKie · 2020-02-29T12:51:36Z

GitUI/CommandsDialogs/FormMergeBranch.cs

@@ -102,6 +109,21 @@ private void OkClick(object sender, EventArgs e)
            }
        }

+        private void WriteMessageToMergeFile(string mergeMessage)


Please expose it via a TestAccessor and add unit tests using ReferenceRepository

This is the method in FormCommit.cs that I used to write this method.

This method doesn't seem to have any tests. Neither is any of the invoking code covered by tests via the TestAccessor.

I'm afraid I don't have much idea about this test that you're proposing.

Hangjit Rai added 3 commits February 22, 2020 15:26

ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If I…

0872339

…t Exists

ESCAPE_COMMIT_MSG Added Test

ccad2da

ESCAPE_COMMIT_MSG Updated Contributor List

b5792b0

ghost assigned hrai Feb 22, 2020

mstv approved these changes Feb 22, 2020

View reviewed changes

mstv reviewed Feb 22, 2020

View reviewed changes

RussKie requested changes Feb 22, 2020

View reviewed changes

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 22, 2020

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 25, 2020

gerhardol mentioned this pull request Feb 25, 2020

Illegal character in path exception when opening clone dialog #7807

Closed

Hangjit Rai added 4 commits February 29, 2020 13:37

ESCAPE_COMMIT_MSG Cleanup

a04f3c9

ESCAPE_COMMIT_MSG Writing Merge Message To MERGE_MSG File

ca65d70

ESCAPE_COMMIT_MSG ESCAPE_COMMIT_MSG Using Git Directory

fdffb8f

(cherry Picked From Commit 669936f)

ESCAPE_COMMIT_MSG Updated Tests

5935979

RussKie reviewed Feb 29, 2020

View reviewed changes

Merge branch 'master' into escape_commit_msg

541a94d

hrai closed this Mar 20, 2020

hrai deleted the escape_commit_msg branch March 20, 2020 23:08

RussKie mentioned this pull request Apr 26, 2020

Fix 3954 quotes in merge commit message #8042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If It Exists #7796

ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If It Exists #7796

hrai commented Feb 22, 2020

mstv left a comment

mstv Feb 22, 2020

RussKie left a comment

hrai commented Feb 25, 2020 •

edited

RussKie commented Feb 25, 2020 via email •

edited by ghost

hrai commented Feb 29, 2020

RussKie Feb 29, 2020

hrai Mar 5, 2020

RussKie Feb 29, 2020

hrai Mar 5, 2020

RussKie Feb 29, 2020

hrai Mar 5, 2020

RussKie Feb 29, 2020

hrai Mar 5, 2020

	public ArgumentString CommitCmd(bool amend, bool signOff = false, string author = "", bool useExplicitCommitMessage = true, bool noVerify = false, bool gpgSign = false, string gpgKeyId = "")
	{
	return new GitArgumentBuilder("commit")
	{
	{ amend, "--amend" },
	{ noVerify, "--no-verify" },
	{ signOff, "--signoff" },
	{ !string.IsNullOrEmpty(author), $"--author=\"{author?.Trim().Trim('"')}\"" },
	{ gpgSign && string.IsNullOrWhiteSpace(gpgKeyId), "-S" },
	{ gpgSign && !string.IsNullOrWhiteSpace(gpgKeyId), $"-S{gpgKeyId}" },
	{ useExplicitCommitMessage, $"-F \"{Path.Combine(GetGitDirectory(), "COMMITMESSAGE")}\"" }
	};
	}

ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If It Exists #7796

ESCAPE_COMMIT_MSG Escaping Double Quotes In Merge Commit Message If It Exists #7796

Conversation

hrai commented Feb 22, 2020

Before

After

Test methodology

Test environment(s)

mstv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

hrai commented Feb 25, 2020 • edited

RussKie commented Feb 25, 2020 via email • edited by ghost

hrai commented Feb 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrai commented Feb 25, 2020 •

edited

RussKie commented Feb 25, 2020 via email •

edited by ghost