{"thread":{"id":"55092","subject":"[PATCH] git-gui: remove lines starting with the comment character","startedAt":"2021-02-02T20:05:15Z","lastAt":"2021-02-03T21:43:31Z","messageCount":8,"participants":["Pratyush Yadav","Eric Sunshine","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"415953","messageId":"20210202200301.44282-1-me@yadavpratyush.com","threadId":"55092","inReplyTo":null,"subject":"[PATCH] git-gui: remove lines starting with the comment character","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-02-02T20:03:01Z","receivedAt":"2021-02-02T20:05:15Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"The comment character is specified by the config variable\n'core.commentchar'. Any lines starting with this character is considered\na comment and should not be included in the final commit message.\n\nTeach git-gui to filter out lines in the commit message that start with\nthe comment character. If the config is not set, '#' is taken as the\ndefault.\n\nSigned-off-by: Pratyush Yadav <me@yadavpratyush.com>\n---\n lib/commit.tcl | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/lib/commit.tcl b/lib/commit.tcl\nindex 11379f8..3c3035f 100644\n--- a/lib/commit.tcl\n+++ b/lib/commit.tcl\n@@ -209,6 +209,28 @@ You must stage at least 1 file before you can commit.\n \t#\n \tset msg [string trim [$ui_comm get 1.0 end]]\n \tregsub -all -line {[ \\t\\r]+$} $msg {} msg\n+\n+\t# Remove lines starting with the comment character.\n+\tset comment_char [get_config core.commentchar]\n+\tif {[string length $comment_char] > 1} {\n+\t\terror_popup [mc \"core.commitchar should only be one character.\"]\n+\t\tunlock_index\n+\t\treturn\n+\t}\n+\n+\tif {$comment_char eq {}} {\n+\t\tset comment_char \"#\"\n+\t}\n+\n+\t# If the comment character is not alphabetical, then we need to escape it\n+\t# with a backslash to make sure it is not interpreted as a special character\n+\t# in the regex.\n+\tif {![string is alpha $comment_char]} {\n+\t\tset comment_char \"\\\\$comment_char\"\n+\t}\n+\n+\tregsub -all -line \"$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n+\n \tif {$msg eq {}} {\n \t\terror_popup [mc \"Please supply a commit message.\n\n--\n2.30.0\n\n"},{"id":"415971","messageId":"CAPig+cT+n3=o9AVyK6FUQ=88aqdnoCfSAxmAO0Xe9S84a9RBqw@mail.gmail.com","threadId":"55092","inReplyTo":"20210202200301.44282-1-me@yadavpratyush.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-02T22:26:48Z","receivedAt":"2021-02-02T22:29:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 2, 2021 at 3:07 PM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> The comment character is specified by the config variable\n> 'core.commentchar'. Any lines starting with this character is considered\n> a comment and should not be included in the final commit message.\n>\n> Teach git-gui to filter out lines in the commit message that start with\n> the comment character. If the config is not set, '#' is taken as the\n> default.\n\nThanks. This shortcoming has bugged me for a long time. A few comments below...\n\n> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>\n> ---\n> diff --git a/lib/commit.tcl b/lib/commit.tcl\n> @@ -209,6 +209,28 @@ You must stage at least 1 file before you can commit.\n>         set msg [string trim [$ui_comm get 1.0 end]]\n>         regsub -all -line {[ \\t\\r]+$} $msg {} msg\n> +\n> +       # Remove lines starting with the comment character.\n> +       set comment_char [get_config core.commentchar]\n> +       if {[string length $comment_char] > 1} {\n> +               error_popup [mc \"core.commitchar should only be one character.\"]\n> +               unlock_index\n> +               return\n> +       }\n> +\n> +       if {$comment_char eq {}} {\n> +               set comment_char \"#\"\n> +       }\n> +\n> +       # If the comment character is not alphabetical, then we need to escape it\n> +       # with a backslash to make sure it is not interpreted as a special character\n> +       # in the regex.\n> +       if {![string is alpha $comment_char]} {\n> +               set comment_char \"\\\\$comment_char\"\n> +       }\n> +\n> +       regsub -all -line \"$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n\nThis regular expression is too loose. It will incorrectly change:\n\n    line one\n    line # two\n    # line three\n    line four\n\ninto:\n\n    line one\n    line\n    line four\n\nYou could fix it by anchoring the start of the match while being\ncareful not to lose the newline at the start of line. Perhaps like\nthis:\n\n    regsub -all -line \"(^|\\\\A)$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n\nHowever, an even better approach than doing this manipulation manually\nmight be to pass the commit message through `git stripspace\n--strip-comments` which will do the exact normalization that Git\nitself does. That way, you don't have to worry about weird corner\ncases. Also, using git-stripspace may allow you to get rid of the\nexisting `trim` and `regsub` which precede the new code you added.\n"},{"id":"416037","messageId":"20210203115453.5ahqba63kvxtdfyb@yadavpratyush.com","threadId":"55092","inReplyTo":"CAPig+cT+n3=o9AVyK6FUQ=88aqdnoCfSAxmAO0Xe9S84a9RBqw@mail.gmail.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-02-03T11:54:53Z","receivedAt":"2021-02-03T11:55:43Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 02/02/21 05:26PM, Eric Sunshine wrote:\n> On Tue, Feb 2, 2021 at 3:07 PM Pratyush Yadav <me@yadavpratyush.com> wrote:\n> > The comment character is specified by the config variable\n> > 'core.commentchar'. Any lines starting with this character is considered\n> > a comment and should not be included in the final commit message.\n> >\n> > Teach git-gui to filter out lines in the commit message that start with\n> > the comment character. If the config is not set, '#' is taken as the\n> > default.\n> \n> Thanks. This shortcoming has bugged me for a long time. A few comments below...\n> \n> > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>\n> > ---\n> > diff --git a/lib/commit.tcl b/lib/commit.tcl\n> > @@ -209,6 +209,28 @@ You must stage at least 1 file before you can commit.\n> >         set msg [string trim [$ui_comm get 1.0 end]]\n> >         regsub -all -line {[ \\t\\r]+$} $msg {} msg\n> > +\n> > +       # Remove lines starting with the comment character.\n> > +       set comment_char [get_config core.commentchar]\n> > +       if {[string length $comment_char] > 1} {\n> > +               error_popup [mc \"core.commitchar should only be one character.\"]\n> > +               unlock_index\n> > +               return\n> > +       }\n> > +\n> > +       if {$comment_char eq {}} {\n> > +               set comment_char \"#\"\n> > +       }\n> > +\n> > +       # If the comment character is not alphabetical, then we need to escape it\n> > +       # with a backslash to make sure it is not interpreted as a special character\n> > +       # in the regex.\n> > +       if {![string is alpha $comment_char]} {\n> > +               set comment_char \"\\\\$comment_char\"\n> > +       }\n> > +\n> > +       regsub -all -line \"$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n> \n> This regular expression is too loose. It will incorrectly change:\n> \n>     line one\n>     line # two\n>     # line three\n>     line four\n> \n> into:\n> \n>     line one\n>     line\n>     line four\n> \n> You could fix it by anchoring the start of the match while being\n> careful not to lose the newline at the start of line. Perhaps like\n> this:\n> \n>     regsub -all -line \"(^|\\\\A)$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n\nGood catch!\n\n> \n> However, an even better approach than doing this manipulation manually\n> might be to pass the commit message through `git stripspace\n> --strip-comments` which will do the exact normalization that Git\n> itself does. That way, you don't have to worry about weird corner\n> cases. Also, using git-stripspace may allow you to get rid of the\n> existing `trim` and `regsub` which precede the new code you added.\n\nThis is exactly what I was looking for when I was writing the patch but \ndidn't manage to find it. Will re-roll. Thanks.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"416069","messageId":"fdfe1661-463b-fc18-02cc-4c6e22ce0463@kdbg.org","threadId":"55092","inReplyTo":"20210202200301.44282-1-me@yadavpratyush.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-03T17:33:11Z","receivedAt":"2021-02-03T17:34:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.02.21 um 21:03 schrieb Pratyush Yadav:\n> The comment character is specified by the config variable\n> 'core.commentchar'. Any lines starting with this character is considered\n> a comment and should not be included in the final commit message.\n> \n> Teach git-gui to filter out lines in the commit message that start with\n> the comment character. If the config is not set, '#' is taken as the\n> default.\n\nThis is WRONG. Git GUI is that: a GUI, it's all about WYSIWYG. If you do\nnot give sufficient unambiguous visual clue to the user that certain\nlines will be ignored, you cannot ignore them.\n\nYou cannot just throw away what the user has typed into the edit box\nwithout warning. How would you make it possible to insert text that\nhappens to begin with the comment character of the day?\n\nPerhaps what you are really only interested in is to remove the list of\nconflicted files after a merge conflict? Then the correct way to proceed\nwould be to sanitize the contents of .git/MERGE_MSG before it is\ninserted into the edit box.\n\n> \n> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>\n> ---\n>  lib/commit.tcl | 22 ++++++++++++++++++++++\n>  1 file changed, 22 insertions(+)\n> \n> diff --git a/lib/commit.tcl b/lib/commit.tcl\n> index 11379f8..3c3035f 100644\n> --- a/lib/commit.tcl\n> +++ b/lib/commit.tcl\n> @@ -209,6 +209,28 @@ You must stage at least 1 file before you can commit.\n>  \t#\n>  \tset msg [string trim [$ui_comm get 1.0 end]]\n>  \tregsub -all -line {[ \\t\\r]+$} $msg {} msg\n> +\n> +\t# Remove lines starting with the comment character.\n> +\tset comment_char [get_config core.commentchar]\n> +\tif {[string length $comment_char] > 1} {\n> +\t\terror_popup [mc \"core.commitchar should only be one character.\"]\n> +\t\tunlock_index\n> +\t\treturn\n> +\t}\n> +\n> +\tif {$comment_char eq {}} {\n> +\t\tset comment_char \"#\"\n> +\t}\n> +\n> +\t# If the comment character is not alphabetical, then we need to escape it\n> +\t# with a backslash to make sure it is not interpreted as a special character\n> +\t# in the regex.\n> +\tif {![string is alpha $comment_char]} {\n> +\t\tset comment_char \"\\\\$comment_char\"\n> +\t}\n> +\n> +\tregsub -all -line \"$comment_char.*(\\\\n|\\\\Z)\" $msg {} msg\n> +\n>  \tif {$msg eq {}} {\n>  \t\terror_popup [mc \"Please supply a commit message.\n> \n\n-- Hannes\n"},{"id":"416071","messageId":"CAPig+cTQaPTNnGcd583B=xoVUR1qPb372Y_x9szROfMcA5h+tA@mail.gmail.com","threadId":"55092","inReplyTo":"fdfe1661-463b-fc18-02cc-4c6e22ce0463@kdbg.org","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-03T17:48:33Z","receivedAt":"2021-02-03T17:49:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 12:35 PM Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 02.02.21 um 21:03 schrieb Pratyush Yadav:\n> > The comment character is specified by the config variable\n> > 'core.commentchar'. Any lines starting with this character is considered\n> > a comment and should not be included in the final commit message.\n> >\n> > Teach git-gui to filter out lines in the commit message that start with\n> > the comment character. If the config is not set, '#' is taken as the\n> > default.\n>\n> This is WRONG. Git GUI is that: a GUI, it's all about WYSIWYG. If you do\n> not give sufficient unambiguous visual clue to the user that certain\n> lines will be ignored, you cannot ignore them.\n>\n> Perhaps what you are really only interested in is to remove the list of\n> conflicted files after a merge conflict? Then the correct way to proceed\n> would be to sanitize the contents of .git/MERGE_MSG before it is\n> inserted into the edit box.\n\nThis is indeed the case I run into which is annoying because the\ncommented-out list of conflicted files does not get removed when\ngit-gui performs the actual commit.\n\nHowever, although what you propose here seems superficially enticing,\nit doesn't mirror the behavior of git-commit itself when launching an\neditor, in which case the unsanitized file (containing the\ncommented-out conflicted file list) is loaded into the editor\nverbatim, and it is only sanitized when the edit session is finished.\nThe important difference is that extra text is added to the edit\nbuffer telling the user explicitly that \"lines beginning with '#' will\nbe ignored\".\n\nSo, perhaps one way forward is for Pratyush to emulate that behavior\nand insert some text into the edit box saying \"lines beginning with\n'#' will be ignored\", or add a label above or below the edit box\nstating the same. (Of course, the actual displayed comment-character\nshould be determined dynamically.)\n"},{"id":"416072","messageId":"CAPig+cQxASd8YNm+6E4XkiEoR__hAW8nrUbEkaBoJSicpq_rmA@mail.gmail.com","threadId":"55092","inReplyTo":"CAPig+cTQaPTNnGcd583B=xoVUR1qPb372Y_x9szROfMcA5h+tA@mail.gmail.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-03T17:58:10Z","receivedAt":"2021-02-03T18:00:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 12:48 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> So, perhaps one way forward is for Pratyush to emulate that behavior\n> and insert some text into the edit box saying \"lines beginning with\n> '#' will be ignored\", or add a label above or below the edit box\n> stating the same. (Of course, the actual displayed comment-character\n> should be determined dynamically.)\n\nEven more fancy would be to add a checkbox below the edit field which\nboth enables/disables the \"stripspace\" behavior and allows the user to\nspecify the comment-character. For instance:\n\n    [x] ignore lines beginning with [#]\n\nwhere [x] is the checkbox and [#] is a text field in which the user\ncan type the comment-character.\n\nFor convenience, the checkbox would be checked by default, and the\ncomment-character would default to the user's configured\ncomment-character or \"#\".\n"},{"id":"416087","messageId":"20210203203947.j2ut5p7nhrd6rcmh@yadavpratyush.com","threadId":"55092","inReplyTo":"CAPig+cTQaPTNnGcd583B=xoVUR1qPb372Y_x9szROfMcA5h+tA@mail.gmail.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-02-03T20:39:47Z","receivedAt":"2021-02-03T20:40:40Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 03/02/21 12:48PM, Eric Sunshine wrote:\n> On Wed, Feb 3, 2021 at 12:35 PM Johannes Sixt <j6t@kdbg.org> wrote:\n> > Am 02.02.21 um 21:03 schrieb Pratyush Yadav:\n> > > The comment character is specified by the config variable\n> > > 'core.commentchar'. Any lines starting with this character is considered\n> > > a comment and should not be included in the final commit message.\n> > >\n> > > Teach git-gui to filter out lines in the commit message that start with\n> > > the comment character. If the config is not set, '#' is taken as the\n> > > default.\n> >\n> > This is WRONG. Git GUI is that: a GUI, it's all about WYSIWYG. If you do\n> > not give sufficient unambiguous visual clue to the user that certain\n> > lines will be ignored, you cannot ignore them.\n> >\n> > Perhaps what you are really only interested in is to remove the list of\n> > conflicted files after a merge conflict? Then the correct way to proceed\n> > would be to sanitize the contents of .git/MERGE_MSG before it is\n> > inserted into the edit box.\n\nWhile this patch also fixes the merge conflict message case, it was in \nfact prompted by [0], which talks about comments in the commit message \ntemplate. This is likely useful information that we don't want to hide \nfrom the user.\n\n> \n> This is indeed the case I run into which is annoying because the\n> commented-out list of conflicted files does not get removed when\n> git-gui performs the actual commit.\n> \n> However, although what you propose here seems superficially enticing,\n> it doesn't mirror the behavior of git-commit itself when launching an\n> editor, in which case the unsanitized file (containing the\n> commented-out conflicted file list) is loaded into the editor\n> verbatim, and it is only sanitized when the edit session is finished.\n> The important difference is that extra text is added to the edit\n> buffer telling the user explicitly that \"lines beginning with '#' will\n> be ignored\".\n\nI agree. \n \n> So, perhaps one way forward is for Pratyush to emulate that behavior\n> and insert some text into the edit box saying \"lines beginning with\n> '#' will be ignored\", or add a label above or below the edit box\n> stating the same. (Of course, the actual displayed comment-character\n> should be determined dynamically.)\n\nThis would be a nice addition. Will see which approach looks best.\n\n[0] https://github.com/prati0100/git-gui/issues/51\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"416098","messageId":"ba8baa8c-d308-c7c9-b15c-4582cc41863e@kdbg.org","threadId":"55092","inReplyTo":"CAPig+cQxASd8YNm+6E4XkiEoR__hAW8nrUbEkaBoJSicpq_rmA@mail.gmail.com","subject":"Re: [PATCH] git-gui: remove lines starting with the comment character","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-02-03T21:42:45Z","receivedAt":"2021-02-03T21:43:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.02.21 um 18:58 schrieb Eric Sunshine:\n> On Wed, Feb 3, 2021 at 12:48 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> So, perhaps one way forward is for Pratyush to emulate that behavior\n>> and insert some text into the edit box saying \"lines beginning with\n>> '#' will be ignored\", or add a label above or below the edit box\n>> stating the same. (Of course, the actual displayed comment-character\n>> should be determined dynamically.)\n> \n> Even more fancy would be to add a checkbox below the edit field which\n> both enables/disables the \"stripspace\" behavior and allows the user to\n> specify the comment-character. For instance:\n> \n>     [x] ignore lines beginning with [#]\n> \n> where [x] is the checkbox and [#] is a text field in which the user\n> can type the comment-character.\n> \n> For convenience, the checkbox would be checked by default, and the\n> comment-character would default to the user's configured\n> comment-character or \"#\".\n\nWhile I'm not thrilled by this solution, it's probably the only sensible\nway forward. We would have to place the checkbox above the edit field,\nwhere there's still some space; otherwise, it takes away vertical space,\nand that I really prefer to use for the commit message and the patch text.\n\nI don't think, though, that we need an edit field to change the comment\ncharacter. It's a fairly stable setting, and as long as there's a\npreference for it and it's synchronized into the checkbox caption, it is\nfine, IMO.\n\n-- Hannes\n"}]}