[1/2] git-gui: warn if the commit message contains lines longer than the set limit
diff mbox series

Message ID 7da71d89f9fa987eca2e25974e4cec382c146e44.1567627609.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • [1/2] git-gui: warn if the commit message contains lines longer than the set limit
Related show

Commit Message

Bert Wesarg Sept. 4, 2019, 8:10 p.m. UTC
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(-)

Comments

Eric Sunshine Sept. 4, 2019, 8:29 p.m. UTC | #1
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?
Bert Wesarg Sept. 4, 2019, 8:43 p.m. UTC | #2
On Wed, Sep 4, 2019 at 10:30 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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

>
> > 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?

How about a horizontal scrollbar? This indicates pretty conveniently
and in a standard visual way, that there is more text to the side ;-)


>
> > 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?

probably not. Does git expands tabs if it wrap lines (git shortlog's -w option)?
Pratyush Yadav Sept. 4, 2019, 10:48 p.m. UTC | #3
On 04/09/19 10:43PM, Bert Wesarg wrote:
[snip]
> > > 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.

Not true in my experience. Sure, no scrollbar appears, but the view of 
the textbox still scrolls when I type in a long line. I can go back to 
seeing the start of the line by moving with the arrow keys.

The point being, you _can_ see what you typed. Do you not get this 
behaviour in Windows (or whatever platform you use)? Maybe that is a 
Tcl/Tk problem?

> > 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?

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.
Johannes Sixt Sept. 5, 2019, 6:21 a.m. UTC | #4
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.

-- Hannes
Birger Skogeng Pedersen Sept. 5, 2019, 6:48 a.m. UTC | #5
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.

Birger
Bert Wesarg Sept. 5, 2019, 5:43 p.m. UTC | #6
On Thu, Sep 5, 2019 at 8:22 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> 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.

wrong memory bank, sorry. Though imagine you paste in some multi line
text, and one line is too long but the next not. In this case you wont
see the too long part anymore.

>
> > 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.

I already argued against an auto-hide scrollbar, which was also acked by ohers:

On 02.09.19 19:58, Bert Wesarg wrote:
> 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.

>
> -- Hannes
Bert Wesarg Sept. 5, 2019, 5:46 p.m. UTC | #7
On Thu, Sep 5, 2019 at 8:50 AM Birger Skogeng Pedersen
<birger.sp@gmail.com> wrote:
>
> 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.

Please exclude the first line, i.e., the subject. This should not be
wrapped at all.

I also imagine, that we should make it configurable, I'm unsure if I
would use this feature for my self.

Bert

>
> Birger
Birger Skogeng Pedersen Sept. 6, 2019, 2:08 p.m. UTC | #8
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
Philip Oakley Sept. 6, 2019, 8:07 p.m. UTC | #9
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?
>
>
>
> Birger
Philip
David Aguilar Sept. 10, 2019, 8:04 a.m. UTC | #10
On Fri, Sep 06, 2019 at 09:07:15PM +0100, Philip Oakley wrote:
> 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...)


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.
Johannes Sixt Sept. 10, 2019, 4:50 p.m. UTC | #11
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
Pratyush Yadav Sept. 10, 2019, 7:42 p.m. UTC | #12
On 10/09/19 01:04AM, David Aguilar wrote:
> 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).

I'm not sure how good this idea is. I feel like we should keep it simple 
and trust the user to know what they are doing. Two textboxes seems like 
a lot of clutter for not that much benefit. I've not seen many people 
complain about hitting the extra "enter". Plus, we get to keep our 
message buffer the same as when a user would use a plain text editor.

One idea I have for dealing with users wanting a longer subject line is 
to do something like git-format-patch's subject line is formatted (this 
could be an mbox thing, and not send-email specific). When the subject 
is too long, the subject is broken in multiple lines, but the first 
character on the next line is a space, so it signifies a continued 
subject.

Something like:
Subject: foo bar baz
 abcd  

Which gets converted to "foo bar baz abcd" in the commit message.

> 
> 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.

Yes, a paragraph reflow is what I had in mind. Without it editing 
already written lines is a pain.

> > > 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.

The aim is to have the commit message be exactly what the user sees. But 
I'm not sure how feasible that is considering the tools/APIs we have to 
work with.
 
> 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.

Nice catch! Didn't think of that.

> 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.

Sounds like a good idea, but I wonder how we can fit it into git-gui's 
current UI layout.
Pratyush Yadav Sept. 10, 2019, 8:34 p.m. UTC | #13
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.
Bert Wesarg Sept. 12, 2019, 7:13 p.m. UTC | #14
On Tue, Sep 10, 2019 at 10:35 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>
> 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.

No problem, will keep this patch as a branch. Just wanted to be pro-active.

Bert


>
> --
> Regards,
> Pratyush Yadav
Bert Wesarg Sept. 12, 2019, 7:26 p.m. UTC | #15
On Tue, Sep 10, 2019 at 6:50 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> 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
> '>='.

Thanks. As Pratyush wont pull this one, I separated it and keep it in
a branch in my fork:

https://github.com/bertwesarg/git-gui/tree/bw/warn-wide-commit-message

Bert

>
> > +         && [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

Patch
diff mbox series

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.
 	#