Bug 29127 – commit-log-editor: Abort commit on unchanged commit messages

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Jakob Petsovits]

Normally, when I submit an empty commit message to my VCS, it would abort (or at least ask me to abort) the commit.
When commit-log-editor runs, it prefills the message, thus it's not empty anymore, thus the commit doesn't abort if I choose to back off and leave it as is. That's kinda annoying.

The patch below makes commit-log-editor keep a backup of the original file (that is passed to $EDITOR) and after $EDITOR exits, compares the user's one with the original one so that it can abort when nothing changed.

Improves commit workflows considerably! Please review, thanks!

[Jakob Petsovits]

Created attachment 39354 [details]
commit-log-editor: Abort commit on unchanged commit messages

[Jakob Petsovits]

Comment on attachment 39354 [details]
commit-log-editor: Abort commit on unchanged commit messages

Whoops, forgot the ChangeLog. Let me reroll.

[Jakob Petsovits]

Created attachment 39358 [details]
Abort commits when a "no description or bug URL" OOPS is given.

Alright, here's a better one that actually works with messages being copied from the ChangeLogs (as is normally the case). Only aborts on description/URL OOPSes. Still does the job to abort for empty commit messages, both when it's pre-filled-in from the ChangeLogs and when it's generated from the Git index.

[Darin Adler]

Comment on attachment 39358 [details]
Abort commits when a "no description or bug URL" OOPS is given.

This seems too specific. We should abort if any OOPS is present.

[Jakob Petsovits]

Well, we shouldn't abort if the reviewer OOPS is present, as that would break Git workflows. I also think it would be worthwhile to (preliminarily) commit local Git patches that come without tests, they can still be squashed into the commit later on.

All I'm looking for is really a way to abort the commit process when I already invoked git commit, and the description OOPS seems the best indicator that the committer does not want to commit in the first place. I would defer the other OOPSes to the committer's best judgement; for WebKit SVN, patches with an OOPS wouldn't be r+'d in the first place.

[Timothy Hatcher]

You can already about a commit in Git or SVN by clearing the commit log and saving. The commands wont commit an emily log and will abort.

I agree we shouldn't prevent committing with OOPS in the commit, since that prevents using Git to commit locally. It also seems silly to prevent svn commits with OOPS on the client side, since we already have a check on the server side.

[Timothy Hatcher]

I meant: You can already *abort* a commit… the commands wont commit an *empty* log and will abort.

[Jakob Petsovits]

It's true that I can abort the commit by manually clearing the lines, I noticed that when skimming through the code.

However, what I was asking for is aborting on an *unchanged* commit message, that is, exiting the editor without any modifications being made. The approach that the patch takes seems to me like a good compromise between achieving that goal and not diaturbing existing workflows or possibilities.

Note that checking the commit message for OOPSes is just an implementation detail, but not the original intent of this patch.

[Timothy Hatcher]

I don't think that is a good change to make. What if you have a review already (in person) and you put the reviewer in the ChangeLog before commiting even once (assuming git), so there is never a need to edit the commit log.

[Jakob Petsovits]

The current (second) patch doesn't deal with review OOPSes so there's no difference inn behaviour to the current state of commit-log-editor.

But fair enough; it might not be all too common to commit stuff locally without changing the actual CommitLog file and only adding it before the actual submission to upstream (which is the use case that motivated my patch in the first place). I understand if that is not enough reason to warrant the patch being submitted upstream. (Still hoping for a change of mind, though!)

[Adam Roben (:aroben)]

I agree with Tim in comment 9. As to what Jakob said in comment 1:

> When commit-log-editor runs, it prefills the message, thus it's not empty
> anymore, thus the commit doesn't abort if I choose to back off and leave it as
> is. That's kinda annoying.

The same situation occurs after a merge commit, or a conflicted cherry-pick. (I'm assuming you're talking about git.) So I don't think this problem is specific to commit-log-editor. The solution for commit-log-editor is the same as for those other situations: just delete the prefilled commit message.

[David Levin]

Comment on attachment 39358 [details]
Abort commits when a "no description or bug URL" OOPS is given.

It looks like at least two reviewers find this change objectionable and afaik no other reviewers find this problem bad enough to counter that.Also, it looks like there are no reviewers who find this issue to be so much of a problem to argue with them about it.

I suppose if you want to pursue this further, you could attempt to gather support on webkit-dev for this change.

Given that there doesn't seem to be a way forward for this right now, I'm r- to move this out of the review queue.

[Jakob Petsovits]

I think this is not going anywhere, so let's rest this case and stick with what we've got. Resolved wontfix.