patch, 2 partsgit-gui: warn if the commit message contains lines longer than the set limit

>
> On Wed, Sep 4, 2019 at 4:10 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> > While the commit message widget has a configurable fixed width, it
> > nevertheless allows to write commit messages which exceed this limit.
> > Though it does not show this content because there is not scrollbar for
> > this widget. No it is.
>
> "No it is" what?

> There seems to be a bug in at least up to Tcl/Tk 8.6.8, which does not
> update the horizontal scrollbar if one removes the whole content at once.
> 
> Suggested-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  git-gui.sh | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index a491085..fa9c0d2 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -3363,14 +3363,20 @@ ttext $ui_comm -background white -foreground black \
>  	-relief sunken \
>  	-width $repo_config(gui.commitmsgwidth) -height 9 -wrap none \
>  	-font font_diff \
> +	-xscrollcommand {.vpane.lower.commarea.buffer.frame.sbx set} \
>  	-yscrollcommand {.vpane.lower.commarea.buffer.frame.sby set}
> +${NS}::scrollbar .vpane.lower.commarea.buffer.frame.sbx \
> +	-orient horizontal \
> +	-command [list $ui_comm xview]
>  ${NS}::scrollbar .vpane.lower.commarea.buffer.frame.sby \
> +	-orient vertical \
>  	-command [list $ui_comm yview]
>  
> +pack .vpane.lower.commarea.buffer.frame.sbx -side bottom -fill x
>  pack .vpane.lower.commarea.buffer.frame.sby -side right -fill y
> -pack $ui_comm -side left -fill y
> +pack $ui_comm -side left -fill both -expand 1

>
> On 04/09/19 10:10PM, Bert Wesarg wrote:
> > While the commit message widget has a configurable fixed width, it
> > nevertheless allows to write commit messages which exceed this limit.
> > Though it does not show this content because there is not scrollbar for
>
> Like we discussed before, it does show the content, you just have to
> scroll by keyboard, and can't scroll by mouse. So maybe reword this?
>
> > this widget. No it is.
>
> I pulled from your GitHub since you seem to have fixed this typo there.
>
> > There seems to be a bug in at least up to Tcl/Tk 8.6.8, which does not
> > update the horizontal scrollbar if one removes the whole content at once.
> >
> > Suggested-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> >  git-gui.sh | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index a491085..fa9c0d2 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -3363,14 +3363,20 @@ ttext $ui_comm -background white -foreground black \
> >       -relief sunken \
> >       -width $repo_config(gui.commitmsgwidth) -height 9 -wrap none \
> >       -font font_diff \
> > +     -xscrollcommand {.vpane.lower.commarea.buffer.frame.sbx set} \
> >       -yscrollcommand {.vpane.lower.commarea.buffer.frame.sby set}
> > +${NS}::scrollbar .vpane.lower.commarea.buffer.frame.sbx \
> > +     -orient horizontal \
> > +     -command [list $ui_comm xview]
> >  ${NS}::scrollbar .vpane.lower.commarea.buffer.frame.sby \
> > +     -orient vertical \
> >       -command [list $ui_comm yview]
> >
> > +pack .vpane.lower.commarea.buffer.frame.sbx -side bottom -fill x
> >  pack .vpane.lower.commarea.buffer.frame.sby -side right -fill y
> > -pack $ui_comm -side left -fill y
> > +pack $ui_comm -side left -fill both -expand 1
>
> Dropping this change does not seem to make a difference. The commit
> message buffer expands on resize even without it. Can you please explain
> why you did this?
>
> >  pack .vpane.lower.commarea.buffer.header -side top -fill x
> > -pack .vpane.lower.commarea.buffer.frame -side left -fill y
> > +pack .vpane.lower.commarea.buffer.frame -side bottom -fill both -expand 1
>
> I'm not too familiar with pack, but why did you change the side from
> left to bottom? I tested by changing it back to left and didn't notice
> any difference.
>
> >  pack .vpane.lower.commarea.buffer -side left -fill y
> >
> >  # -- Commit Message Buffer Context Menu
>
> Other than these couple of minor things, the patch LGTM. Thanks.

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
> diff --git a/lib/commit.tcl b/lib/commit.tcl
> @@ -215,6 +215,16 @@ A good commit message has the following format:
> +       if {[tcl::mathfunc::max {*}[lmap x [split $msg "\n"] {string length $x}]] >= $repo_config(gui.commitmsgwidth) \

>
> On Wed, Sep 4, 2019 at 4:10 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> > The commit message widget does not wrap the next and has a configurable
>
> s/next/text/

>
> > fixed width to avoid creating too wide commit messages. Though this was
> > only enforced in the GUI. Now we also check the commit message at commit
> > time for long lines and ask the author for confirmation if it exceeds the
> > configured line length.
>
> Hmm, more confirmation dialogs tend to mean more annoyance for users,
> especially considering that the line length limit is a
> project-specific _policy_ (so this has the potential to annoy a lot of
> people), and also because there often are legitimate reasons for
> exceeding the limit (such as pasting in URLs).

>
> As an alternative to a confirmation dialog, how about instead adding a
> _warning_ message (perhaps with red text) on the window itself
> alongside to the commit message field (below or above it or
> something)? Is that something that could be triggered by a text widget
> callback?

>
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> > diff --git a/lib/commit.tcl b/lib/commit.tcl
> > @@ -215,6 +215,16 @@ A good commit message has the following format:
> > +       if {[tcl::mathfunc::max {*}[lmap x [split $msg "\n"] {string length $x}]] >= $repo_config(gui.commitmsgwidth) \
>
> Does this take TABs into account?

> > > fixed width to avoid creating too wide commit messages. Though 
> > > this was
> > > only enforced in the GUI. Now we also check the commit message at commit
> > > time for long lines and ask the author for confirmation if it exceeds the
> > > configured line length.
> >
> > Hmm, more confirmation dialogs tend to mean more annoyance for users,
> > especially considering that the line length limit is a
> > project-specific _policy_ (so this has the potential to annoy a lot of
> > people), and also because there often are legitimate reasons for
> > exceeding the limit (such as pasting in URLs).
> 
> these people did not saw the entered text anyway. they would have
> needed to change the option (default to 75 characters) to see what
> they have typed. which could have been garbage to begin with.

> > As an alternative to a confirmation dialog, how about instead adding 
> > a
> > _warning_ message (perhaps with red text) on the window itself
> > alongside to the commit message field (below or above it or
> > something)? Is that something that could be triggered by a text widget
> > callback?

>
> On Thu, Sep 5, 2019 at 12:48 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > I'll chime in with what I think would be a great solution: auto word
> > wrapping. This way, the users can set the text width, and not have to
> > worry about manually formatting it. Long "words" like URLs would still
> > get to be on one line, and we avoid showing annoying dialogs.
>
> +1. IMO git-gui really needs automatic word wrapping for the commit messages.

> Hi Bert,
>
>
> We should probably distinguish between what is wrapped in git-gui
> (i.e. purely visual), and what is actually wrapped in the commit
> message.
> I believe the former is referred to as "soft wrap", while the latter
> is "hard wrap".
>
>
> On Thu, Sep 5, 2019 at 7:46 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> Please exclude the first line, i.e., the subject. This should not be
>> wrapped at all.
> I think all lines should be soft wrapped. Scrolling sideways is just
> not something I'd want to do in the gui.
>
> How about we soft wrap all lines (in gui). But when the commit is
> created, the actual hard wrap (newline characters) happens only on
> lines other than the first one?

> But then again, the user might get frustrated when the resulting
> commit message looks different than what it appeared in git-gui.
>
> Honestly I'd prefer just wrap the first line as well. If the user gets
> frustrated that the first line gets wrapped there are two options:
> - Refrain from writing such a long line
> - Disable word wrapping (it should be configurable, like you said)

>
> Thoughts?
>
>
>
> Birger

> Hi Birger,
> 
> On 06/09/2019 15:08, Birger Skogeng Pedersen wrote:
> > Hi Bert,
> > 
> > 
> > We should probably distinguish between what is wrapped in git-gui
> > (i.e. purely visual), and what is actually wrapped in the commit
> > message.
> > I believe the former is referred to as "soft wrap", while the latter
> > is "hard wrap".
> > 
> > 
> > On Thu, Sep 5, 2019 at 7:46 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> > > Please exclude the first line, i.e., the subject. This should not be
> > > wrapped at all.
> > I think all lines should be soft wrapped. Scrolling sideways is just
> > not something I'd want to do in the gui.
> > 
> > How about we soft wrap all lines (in gui). But when the commit is
> > created, the actual hard wrap (newline characters) happens only on
> > lines other than the first one?
> 
> Not sure if I parsed this correctly, but I'd want a WYSIWYG approach that if
> we wrap on the display it will be wrapped (newline char) in the commit. It
> sounded as if you were proposing a soft wrap visually, but not doing the
> same for the commit.
> 
> Personally, I've had both feelings with the gui. I like that the 'hard'
> visual char limit is there that encourages me to wrap my messages.
> But at the same time if I'm typing on a flow then it's annoying that there
> wasn't any auto wrap.
> 
> The other problem is if one is amending a commit and I need to add a few
> word mid paragraph, the manual re-flowing and manual wrapping can be
> annoying.
> 
> I suspect there is a moderately happy medium between the two, perhaps with
> an autowrap key (per paragraph) being available.
> 
> I also had it in my head that some parts of Git do allow more than a single
> line headers, or at least can cope with a run-on second line before the
> blank line that flags the start of the message proper. (I may be wrong...)

> > But then again, the user might get frustrated when the resulting
> > commit message looks different than what it appeared in git-gui.
> > 
> > Honestly I'd prefer just wrap the first line as well. If the user gets
> > frustrated that the first line gets wrapped there are two options:
> > - Refrain from writing such a long line
> > - Disable word wrapping (it should be configurable, like you said)
> Configurable wrapping point - yes, would be nice (a feeling of control, that
> I'd probably never change ;-).
> > 
> > Thoughts?

> On Fri, Sep 06, 2019 at 09:07:15PM +0100, Philip Oakley wrote:
> > 
> > Not sure if I parsed this correctly, but I'd want a WYSIWYG approach that if
> > we wrap on the display it will be wrapped (newline char) in the commit. It
> > sounded as if you were proposing a soft wrap visually, but not doing the
> > same for the commit.
> > 
> > Personally, I've had both feelings with the gui. I like that the 'hard'
> > visual char limit is there that encourages me to wrap my messages.
> > But at the same time if I'm typing on a flow then it's annoying that there
> > wasn't any auto wrap.
> > 
> > The other problem is if one is amending a commit and I need to add a few
> > word mid paragraph, the manual re-flowing and manual wrapping can be
> > annoying.
> > 
> > I suspect there is a moderately happy medium between the two, perhaps with
> > an autowrap key (per paragraph) being available.
> > 
> > I also had it in my head that some parts of Git do allow more than a single
> > line headers, or at least can cope with a run-on second line before the
> > blank line that flags the start of the message proper. (I may be wrong...)
> 
> 
> Correct, you're thinking about the subject line -- it has that
> multi-line behavior.
> 
> In git-cola we use a separate LineEdit for the "Commit summary" line,
> which has improved the commit messages users write and is good because
> the user doesn't need to worry about the "blank line between subject and
> body" Git commit message convention.
> 
> When the user presses "enter" in the subject line it jumps to the
> "Extended description" TextEdit, so it still "feels" like a single
> widget.  Likewise, up-arrow from the first line of the Extended
> description also jumps to the summary LineEdit.  We also have a "Ctrl-L"
> global hotkey to jump back to the subject line (same hotkey as Web
> browsers for the "hot" action of focusing the URL).

> 
> Using two widgets is a bigger rework for git-gui, but should be
> considered for future enhancement.
> 
> We also deal with the "Amend a commit" issue through the word-wrap
> approach.  It's actually a paragraph reflow (not a line-by-line wrap) so
> adding words is fine and easy and does the natural expected thing --
> when amending it'll still feel like a long flow line and when committed
> it'll wrap with newlines at the configured column.

> > > But then again, the user might get frustrated when the resulting
> > > commit message looks different than what it appeared in git-gui.
> > > 
> > > Honestly I'd prefer just wrap the first line as well. If the user gets
> > > frustrated that the first line gets wrapped there are two options:
> > > - Refrain from writing such a long line
> > > - Disable word wrapping (it should be configurable, like you said)
> > Configurable wrapping point - yes, would be nice (a feeling of control, that
> > I'd probably never change ;-).
> > > 
> > > Thoughts?
> 
> 
> Those are the same controls git-cola has.
> - Commit message word-wrap can be turned on/off.  Defaults to on.
> - The line length is configurable.  Defaults to 72.
> 
> Regarding, "the user might get frusted when the resulting commit message
> looks different" -- I don't think that's really a concern in practice
> but that might be we have a dedicated LineEdit for the subject line.

> For git-gui, it seems fine since Git already has conventions about how
> the subject line gets wrapped, so tools will still see the wrapped
> subject line as a logical "single line".
> 
> The "Extended description" commit message editor is WYSWIYG, but if the
> widget is smaller than the configured value then we still wrap the
> committed message using the configured value, which is what the user
> would likely expect even though the visual wrapping is smaller.
> 
> We also special-case trailers like "Signed-off-by:" and other common
> trailers since user names can get long, and users sometimes use things
> like "See-also:" and paste a long URL that we don't want to wrap.

> Lastly, we have a convenient session-only checkbox to temporarily 
> disable
> wrapping for a commit that does not persist across restarts.  The idea
> is that sometimes you might use the GUI for a one-off commit where you
> want to disable the wrapping for whatever reason, but don't want to
> change your configuration.

>
> Am 04.09.19 um 22:43 schrieb Bert Wesarg:
> > these people did not saw the entered text anyway. they would have
> > needed to change the option (default to 75 characters) to see what
> > they have typed. which could have been garbage to begin with.
>
> Huh? When I type overly long line, all text scrolls out of view on the
> left side. So I definitely _can_ see the long text.

>
> > How about a horizontal scrollbar? This indicates pretty conveniently
> > and in a standard visual way, that there is more text to the side ;-)
>
> The scrollbar is an option, of course, but I dislike somewhat that it
> takes away screen space if it is permanently visible.

> On Mon, Sep 2, 2019 at 7:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> True, and that reasoning would justify hiding scrollbar when not
>> necessary to gain vertical space.  Can we arrange the scrollbar to
>> appear only when needed?
>
> up to now, git-gui does not hide any scrollbars, if they are not
> needed. IMHO, I would keep it that way, as I don't like the flicker
> when it appears and disappears. Imagine you are typing in the bottom
> line and than you typed too much. The scrollbar appears, your input
> line jumps one up line (or is hidden behind the scrollbar), than you
> remove the too long line, the scrollbar disappears again and your
> input line jumps down again.

> The commit message widget does not wrap the next and has a configurable
> fixed width to avoid creating too wide commit messages. Though this was
> only enforced in the GUI. Now we also check the commit message at commit
> time for long lines and ask the author for confirmation if it exceeds the
> configured line length.
> 
> Needs Tcl 8.6 because of `lmap`.
> 
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  git-gui.sh     |  4 ++--
>  lib/commit.tcl | 10 ++++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 5bc21b8..a491085 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -31,8 +31,8 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA}]
>  ##
>  ## Tcl/Tk sanity check
>  
> -if {[catch {package require Tcl 8.4} err]
> - || [catch {package require Tk  8.4} err]
> +if {[catch {package require Tcl 8.6} err]
> + || [catch {package require Tk  8.6} err]
>  } {
>  	catch {wm withdraw .}
>  	tk_messageBox \
> diff --git a/lib/commit.tcl b/lib/commit.tcl
> index 83620b7..fa9760b 100644
> --- a/lib/commit.tcl
> +++ b/lib/commit.tcl
> @@ -215,6 +215,16 @@ A good commit message has the following format:
>  		unlock_index
>  		return
>  	}
> +	if {[tcl::mathfunc::max {*}[lmap x [split $msg "\n"] {string length $x}]] >= $repo_config(gui.commitmsgwidth) \

> +	    && [ask_popup "Commit message contains lines longer than $repo_config(gui.commitmsgwidth) characters.
> +
> +You may change this limit in the options.
> +
> +Continue to commit?
> +"] ne yes} {
> +		unlock_index
> +		return
> +	}
>  
>  	# -- Build the message file.
>  	#
> 

>
> Am 04.09.19 um 22:10 schrieb Bert Wesarg:
> > The commit message widget does not wrap the next and has a configurable
> > fixed width to avoid creating too wide commit messages. Though this was
> > only enforced in the GUI. Now we also check the commit message at commit
> > time for long lines and ask the author for confirmation if it exceeds the
> > configured line length.
> >
> > Needs Tcl 8.6 because of `lmap`.
> >
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> >  git-gui.sh     |  4 ++--
> >  lib/commit.tcl | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index 5bc21b8..a491085 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -31,8 +31,8 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA}]
> >  ##
> >  ## Tcl/Tk sanity check
> >
> > -if {[catch {package require Tcl 8.4} err]
> > - || [catch {package require Tk  8.4} err]
> > +if {[catch {package require Tcl 8.6} err]
> > + || [catch {package require Tk  8.6} err]
> >  } {
> >       catch {wm withdraw .}
> >       tk_messageBox \
> > diff --git a/lib/commit.tcl b/lib/commit.tcl
> > index 83620b7..fa9760b 100644
> > --- a/lib/commit.tcl
> > +++ b/lib/commit.tcl
> > @@ -215,6 +215,16 @@ A good commit message has the following format:
> >               unlock_index
> >               return
> >       }
> > +     if {[tcl::mathfunc::max {*}[lmap x [split $msg "\n"] {string length $x}]] >= $repo_config(gui.commitmsgwidth) \
>
> This has an off-by-one error: When I fill the edit box to the limit, but
> not beyond it, I get the warning popup. I guess this should be '>', not
> '>='.

>
> > +         && [ask_popup "Commit message contains lines longer than $repo_config(gui.commitmsgwidth) characters.
> > +
> > +You may change this limit in the options.
> > +
> > +Continue to commit?
> > +"] ne yes} {
> > +             unlock_index
> > +             return
> > +     }
> >
> >       # -- Build the message file.
> >       #
> >
>
> -- Hannes

> The commit message widget does not wrap the next and has a configurable
> fixed width to avoid creating too wide commit messages. Though this was
> only enforced in the GUI. Now we also check the commit message at commit
> time for long lines and ask the author for confirmation if it exceeds the
> configured line length.

>
> On 04/09/19 10:10PM, Bert Wesarg wrote:
> > The commit message widget does not wrap the next and has a configurable
> > fixed width to avoid creating too wide commit messages. Though this was
> > only enforced in the GUI. Now we also check the commit message at commit
> > time for long lines and ask the author for confirmation if it exceeds the
> > configured line length.
>
> Thanks for the patch, but I'm not a big fan of this. I agree with Eric
> that more dialogs are annoying. Even as a temporary change until we have
> word wrap support, I don't like it. I hope you don't mind if I only take
> the scrollbar change and drop this.