Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grunt: update commitplease for compatibility with git commit -v #679

Closed
wants to merge 1 commit into from
Closed

Grunt: update commitplease for compatibility with git commit -v #679

wants to merge 1 commit into from

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Oct 6, 2014

commitplease had a bug before v1.9.0 where it didn't properly handle the verbose flag to git commit. This would lead to git diff for preview getting subjected to commit message validation, which could abort commits if it failed.

See jzaefferer/commitplease#19 (comment)

@leobalter
Copy link
Member

Looks good to me, before merging I would just change the commit title to:

  • Build: Update commitplease for compatibility with git commit -v

and insert a reference to the commitplease related issue.

@scottgonzalez
Copy link
Contributor

Generally the format would be "Build: Upgrade to commitplease 1.10.1" If you want to include a note about why, it would go in the long description.

commitplease had a bug before 1.9.0 where it didn't properly handle the
verbose flag `git commit -v`.

jzaefferer/commitplease#19 (comment)

This would lead to git diff for preview getting subjected to commit
message validation, which could abort commits if it failed.
@mislav
Copy link
Contributor Author

mislav commented Oct 6, 2014

Thanks for the feedback. Amended the commit now.

@jzaefferer
Copy link
Member

@mislav can you sign our CLA? http://contribute.jquery.org/CLA/

Depending on how fast my commitplease updates are done, I'd bump this directly to the new version. We need the signature for your other PR anyway.

@mislav
Copy link
Contributor Author

mislav commented Oct 7, 2014

Signed the CLA.

Let it just go on record that I don't like that commitplease is used in
this project. First, it has taken me by surprise that some git hooks were
installed without me knowing about it. I presume that commitplease does
this in post-install hooks for the npm package.

It breaks my git flow. I often make WIP commits that I intend to squash
later before pushing, either via git commit -m "WIP typo" or with git commit --fixup feature. These commits get aborted and then I'm forced to
invent a temporary "component" for the subject line.

But the worst offense, especially on the first commit before I'm made aware
that there are pre-commit hooks in the repo, is that my carefully
constructed commit message is discarded after aborting and the next git commit will start with a blank message. Pair all this with commitplease's
poor suppor for git commit -v (which is being worked on, thanks), and you
get a bad taste in your mouth as a first contributor. The commit message
validation doesn't tell you what you didn't wrong, it just aborts,
seemingly trashes your commit message (I guess you could still salvage it
manually from COMMIT_EDITMSG), says that you need a "component" and now
you need to inspect the project's history to learn what the component
should be. Then you still might have a chance to pick the wrong component,
as I did here by stating "Grunt" while is should have been "Build". The
whole practice of specifying a "component", as ridiculous as it already is
for a small library such as QUnit, is even more pointless when you remind
yourself that this whole practice of subject line metadata is a workaround
for a lazy reader of the commit log who can't be bothered to inspect the
diff of each commit or at least list use git log --name-only on the
command-line.

@scottgonzalez
Copy link
Contributor

this whole practice of subject line metadata is a workaround
for a lazy reader of the commit log who can't be bothered to inspect the
diff of each commit

That's actually not true. A large part of it is about automated changelogs across dozens of projects.

@jzaefferer
Copy link
Member

Thank you for the feedback!

It breaks my git flow. I often make WIP commits that I intend to squash
later before pushing, either via git commit -m "WIP typo" or with git commit --fixup feature. These commits get aborted and then I'm forced to
invent a temporary "component" for the subject line.

"[tmp]: " is often used in that case.

But the worst offense, especially on the first commit before I'm made aware
that there are pre-commit hooks in the repo, is that my carefully
constructed commit message is discarded after aborting and the next git commit will start with a blank message.

That's why the full commit message is dumped to the console for invalid commit messages, to recover what you previously typed. Its crude, but unfortunately the best we could come up with. You may have missed that since it also included the diff, which the next release of commitplease will address (assuming you use git 1.8.5+).

The commit message validation doesn't tell you what you didn't wrong, it just aborts,

It actually tells you what's wrong at the top, but that might also have scrolled away due to the diff output below.

says that you need a "component" and now you need to inspect the project's history to learn what the component should be.

This is being addressed with the extend component option. Once we configure that properly, it'll tell you what the accepted components are.

@mislav
Copy link
Contributor Author

mislav commented Oct 7, 2014

A large part of it is about automated changelogs across dozens of projects.

Sorry, I didn't know that. It would be nice if an automated system could infer the component from changed files, though. But I guess such heuristics would be difficult to define and brittle.

"[tmp]: " is often used in that case.

I've figured that out after trial-and-error, but it still disallows me to use git commit --fixup, since that prefixes the message with "Fixup!" and fails the validation. As a result, a core git feature is broken while developing for QUnit.

It actually tells you what's wrong at the top

It does, I've read the error message. But it doesn't help you fixing the issue. As a new contributor, I was not aware what "components" were, what are the options and what is the format of specifying the compontent. Is it in square brackets? Is it a word separated by a colon? By inspecting the log, I've quickly learned, but still.

A much friendlier technical solution for this would be to configure a commit.template for this project so that new contributors are guided by the sample commit message about specifying the correct compontent in the correct format. That way their first commit has a chance to not get aborted.

@jzaefferer
Copy link
Member

I didn't know about --fixup nor commit.template. The former could be supported by accepting commit messages starting with "fixup!" as valid. I'm not sure how we could use commit.template - it looks like every user needs to configure this locally. Providing a template without git automatically using it seems rather useless.

@mislav
Copy link
Contributor Author

mislav commented Oct 7, 2014

it looks like every user needs to configure this locally.

Just like hooks need to be configured locally per-repo as well. If you could set up git hooks, then you could also set up the template.

Here's an alternative to a template: a git hook that prepares the commit message before it opens in your text editor by listing the existing components:

#!/bin/bash
# .git/hooks/prepare-commit-msg
set -e

file="$1"
mode="$2"

list_components() {
  git log --no-merges --format='%s' | grep : | cut -d: -f1 | \
    sort | uniq -c | sort -rn | head -10 | sed 's/^.\{1,\}[[:digit:]] //'
}

if [ -z "$mode" ]; then
  c="$(git config core.commentchar || echo '#')"
  { echo "$c Please prefix the subject line with \"Component:\""
    echo -n "$c Example components: "
    list_components | xargs | sed 's/ /, /g'
    cat "$file"
  } > "${file}.new"
  mv "${file}.new" "$file"
fi

The effect is has on the default message when I git commit:

# Please prefix the subject line with "Component:"
# Example components: Build, qunit, Core, Tests, Test, QUnit, Grunt, Assert, Reporter, Readme

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
...

@Krinkle
Copy link
Member

Krinkle commented Oct 8, 2014

@mislav Very nice!

@jzaefferer jzaefferer closed this in 5ed001d Oct 8, 2014
@jzaefferer
Copy link
Member

Release commitplease 1.11.0 and updated this to use that version. Kept the rest of the commit message.

@mislav mislav deleted the commit-verbose branch October 10, 2014 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants