Unblock special commit message

[mstv]

Fixes #7104
Fixes #6993

Proposed changes

• add button to enable editing the special commit message of fixup! and squash! commits
• add/adapt hotkeys for buttons
• allow Amend commit' for any CommitKind
• reset CommitKind after commiting without closing FormCommit
• slightly off topic: add more test cases for empty lines in commit messages (addendum to Retain newlines in commit message #6877)

Screenshots

Before

After

(different message font due to different font settings vs. portable build)

(Intermediate with ToolStripItemButton)

Before

After

Test methodology

• manual tests

Test environment(s)

• Git Extensions 3.3.0
• Build 79dee30
• Git 2.23.0.windows.1
• Microsoft Windows NT 10.0.18362.0
• .NET Framework 4.8.3815.0
• DPI 96dpi (no scaling)

---

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

[pmiossec]

I like the way the 'preview' button is placed in the screenshots of#7039.

And I think it could be interesting to doi it also for 'Edit special commit message'.

What do you think?

[codecov]

Codecov Report

Merging #7128 into master will increase coverage by 0.01%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master    #7128      +/-   ##
==========================================
+ Coverage   47.31%   47.32%   +0.01%     
==========================================
  Files         746      746              
  Lines       54475    54486      +11     
  Branches     7140     7140              
==========================================
+ Hits        25775    25788      +13     
- Misses      27279    27283       +4     
+ Partials     1421     1415       -6

Flag	Coverage Δ
#production	36.47% <56.66%> (+0.02%)	⬆️
#tests	97.62% <ø> (ø)	⬆️

[mstv]

https://codecov.io/gh/gitextensions/gitextensions/pull/7128/changes does not contain any touched file but many unrelated?!?

[mstv]

I like the way the 'preview' button is placed in the screenshots of#7039.

pro:

• a little more related to the TextBox

con:

• The bottom right corner could be too far away, especially if the FormCommit is maximized.
• I don't like overlays in general because some day, they will hide some text.

[mstv]

Added hotkeys also for the Reset buttons, @mrexodia.

[mrexodia]

Cool, thanks!

[RussKie]

• add/adapt hotkeys for buttons

I propose this is submitted as its own PR, as it will get merged quickly.

• slightly off topic: add more test cases for empty lines in commit messages (addendum to Retain newlines in commit message #6877)

So as this.

• allow Amend commit' for any CommitKind

Could you please explain why this is a good change?

• add button to enable editing the special commit message of fixup! and squash! commits
  

I do not disagree with the change, but I am not fond of the proposed implementation.
There is no immediate connection (visual or otherwise) between the textbox and the button.

I think we provide a better user experience by rendering a link label on top of the textbox (z-order wise), e.g. something like the following:

The text on the link is up for a discussion (it could be "cancel fixup"?).

[mstv]

allow Amend commit' for any CommitKind

#7104 (comment): "It's a totally valid scenario to turn the latest commit into a fixup! commit."

I think we provide a better user experience by rendering a link label on top of the textbox (z-order wise)

Understood. Can it be activated using a shortcut key?

The text on the link is up for a discussion (it could be "cancel fixup"?).

I'd prefer "Modify this special commit message", perhaps with an addition like "(The warrenty ends here.)". (I know we don't give warrenties and should never do.)
It is not cancelling the fixup: The message will not be changed by clicking. Just CommitKind is switched to Normal in order to store the manually entered commit message on (temporary) close of FormCommit.

[RussKie]

Understood. Can it be activated using a shortcut key?

I would think it can.
https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.label.usemnemonic?view=netframework-4.8#System_Windows_Forms_Label_UseMnemonic

I'd prefer "Modify this special commit message", perhaps with an addition like "(The warrenty ends here.)".

We can have a tooltip that warns users (e.g. "do it at your own risk" kind of thing)

[mstv]

Fiddled around with [Link]Label without success. As I explained above, we need a button-like control, not a label which controls only the focus - the documentation (which I read too late) states:
"Pressing ALT + the mnemonic character sets the focus to the control that follows the Label in the tab order."
BTW: The VS 2019 Designer does not understand FormCommit completely. And it did not allow to place [Link]Label in the area of the message editor.

[RussKie]

I'll try to find some time and fiddle with it

…

On Tue, 17 Sep 2019 at 01:20, Michael Seibt ***@***.***> wrote: Fiddled around with [Link]Label without success. As I explained above, we need a button-like control, not a label which controls only the focus - the documentation (which I read too late) states: "Pressing ALT + the mnemonic character sets the focus to the control that follows the Label in the tab order." BTW: The VS 2019 Designer does not understand FormCommit completely. And it did not allow to place [Link]Label in the area of the message editor. [image: grafik] <https://user-images.githubusercontent.com/36601201/64997288-8ceb1000-d8e0-11e9-9b3d-b4f8e6a87690.png> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7128?email_source=notifications&email_token=ABBTEXRATB425NV2JL2Q6NDQKABEFA5CNFSM4IWJHD62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD62WDMY#issuecomment-531980723>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBTEXS7P4OWUM7QHPBPB5LQKABEFANCNFSM4IWJHD6Q> .

[mstv]

I turned the ToolStripItemButton into a Label, into a LinkLabel and finally into a Button in dedicated commits.
I think this is a good compromise:

TODO:

• make ToolTip translatable:
  The TranslationApp seems to ignore ToolTips. Must I really create a TranslationString by hand?
• Does it look OK with a dark theme?
• squash

[gerhardol]

Does it look OK with a dark theme?

It is simple to test with high contrast dark theme (nothing looks OK, But the button should be visible.
To install a real dark system the only simple way is to use WindowsBlinds. Paid app, but have a trial.
#6651 (comment)

[mdonatas]

Food for thought. From UX perspective it seems weird to make a text-area read only and then place a button to make it editable... for what reason exactly? There is no explanation and adding an explanation would only dig this hole deeper.
Git users know that there is nothing special with fixup! and squash! they are just guidelines for interactive rebase to automate reordering rows.
If someone is going to do interactive rebase they are supposedly git savvy enough to know what they are doing and then we really just need a "heads up" UX signalling that this is a commit message with some special meaning. So something like this seems to hit the right balance:

#KeepItSimple :)

[RussKie]

I like a different perspective, thanks @mdonatas.

Will a commit be treated as a fixup if the text changed though?

[gerhardol]

Will a commit be treated as a fixup if the text changed though?

The subject after fixup must match.
If you change it is something else, which is what the issue is about.

As the button is implemented, I prefer the guidence it provides but I do not have a strong preference.

[mdonatas]

The subject must match. But that's the same if you are using git from the shell. If the user didn't know that subjects must match, this would be a good learning experience when the lines wouldn't get reordered in an interactive rebase. After all no harm is done at all.

offtopic
IMO there are way too many guard rails on by default already. For instance git embraces commit graph modifications with rebase and amending, there is even a saying rebase makes it look as if you did everything perfect the first time but GE scares you with dialogs about history rewrite which is one of the pillar features of git. And sadly very few go to change these settings.
I recommend GE to everyone because it exposes git features in GUI form without interpretations or restrictions as closely as possible meaning GE embraces git as opposed to crippling it. I am voting with both hands for continuing to embrace git as one of the GE pillar features. 🕊

[pmiossec]

GE scares you with dialogs about history rewrite which is one of the pillar features of git.

The perfect thing would have been to display this message only when the commit has been already pushed.

At least we could update the warning message to explain that.

[RussKie]

The perfect thing would have been to display this message only when the commit has been already pushed.

How do you see it implemented?

[RussKie]

The subject must match.

If subject must match to the dot, it is not quite unreasonable to make the textbox readonly to indicate it to the user that it isn't something they should be changing.
However to allow the use-case where a user may change their mind and do another type of commit (i.e. not a fixup), we need to make the textbox editable.

If we leave the textbox editable for the special types of commits (i.e. fixup) our users may be left confused, change text and then wonder why they didn't get a fixup commit they wanted.

I'm speaking from first-hand experience where I had/have to teach engineers (including smart senior level devs and architects) about these git commands, and a lot of time I need(ed) to hold their hands with git. The same way others held my hand when I was learning git.

[mrexodia]

With regards to detecting if something is pushed, I guess you can check where origin/branch is and only warn if the rebase is in the past of origin/branch. The way me (and my team) are using git is that we rebase and force push feature branches until everything is clean so the message is useless anyway (similar to the force push warning). For that the concept of “protected” branches could be explored, but generally the server is enforcing it so it’s probably not worth it.

…

On Thu, 19 Sep 2019 at 22:44, Igor Velikorossov ***@***.***> wrote: The subject must match. If subject must match to the dot, it is not quite unreasonable to make the textbox readonly to indicate it to the user that it isn't something they should be changing. However to allow the use-case where a user may change their mind and do another type of commit (i.e. not a fixup), we need to make the textbox editable. If we leave the textbox editable for the special types of commits (i.e. fixup) our users may be left confused, change text and then wonder why they didn't get a fixup commit they wanted. I'm speaking from first-hand experience where I had/have to teach engineers (including smart senior level devs and architects) about these git commands, and a lot of time I need(ed) to hold their hands with git. The same way others held my hand when I was learning git. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7128?email_source=notifications&email_token=AASYFGOMOHRNPHO24GGLRKDQKPQCBA5CNFSM4IWJHD62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7EYVAY#issuecomment-533301891>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASYFGNALX54IZLVNUKCD23QKPQCBANCNFSM4IWJHD6Q> .

[RussKie]

Hmm... I vaguely remember we had issues with translations for tooltips, but I don't remember whether it was resolved. I speculate but I don't think we set tooltips in designer files.

…

On Fri, 20 Sep 2019 at 00:55, Michael Seibt ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In GitUI/Translation/English.xlf <#7128 (comment)> : > @@ -3568,6 +3568,10 @@ You can unset the template: <source>Interactive Add</source> <target /> </trans-unit> + <trans-unit id="modifyCommitMessageButton.Text"> + <source>Modify this special commit m&amp;essage</source> + <target /> + </trans-unit> The TranslationApp seems to ignore ToolTips. Must I really create a TranslationString by hand? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7128?email_source=notifications&email_token=ABBTEXQLXF73OEXNPFGBDY3QKPYN7A5CNFSM4IWJHD62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFLD4DA#pullrequestreview-290864652>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBTEXXZ6MQVRGJ3KNHEEELQKPYN7ANCNFSM4IWJHD6Q> .

[pmiossec]

I also don't remember but perhaps #3745 could help. Especially, #3745 (comment)

[mstv]

Added tooltip translation and squashed.

[RussKie]

LGTM

[ghost]

Hello @mstv!

Because this pull request has the status: auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[mstv]

Sorry for merging too early, I expected that the msftbot was configured to wait for at least one approval.
@RussKie, can the msftbot's default behavior be configured? Or must I always type something like
merge 1 day after approval by @RussKie or after two approvals

[RussKie]

Yes, you need to write something like "@msftbot merge if @desired_user_name approves"
Have a look at bot's guide

[mstv]

Have a look at bot's guide

I had; and I had the hope the instantly handled "status: auto merge" could be tamed - by the repo admin at least.
When you used it, it wrote something like you had reset its configuration. That's why I thought the default behavior could be configured.
Perhaps it can be added to the PR template.