[Outreachy] commit: display advice hints when commit fails #495
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] |
|
The cover letter on list is formatted really weirdly, @dscho any idea what happened? https://lore.kernel.org/git/[email protected]/ Where did these ====== come from? |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
builtin/commit.c
Outdated
| @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, | |||
| */ | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <[email protected]> writes:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2db2ad0de4..4439666465 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> */
> if (!committable && whence != FROM_MERGE && !allow_empty &&
> !(amend && is_a_merge(current_head))) {
> + s->hints = advice_status_hints;
> s->display_comment_prefix = old_display_comment_prefix;
> run_status(stdout, index_file, prefix, 0, s);
> if (amend)
It almost tempts me to say why this is not done inside run_status(),
which has other callers, but I think the answer is because we do not
want these hints when we are actually committing (iow, the ongoing
commit must be aborted before the user can actually say "git add"
etc. that are suggested).
So the change makes sense to me.
Will queue.
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 46a5cd4b73..3d76e8ebbd 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
> )
> '
>
> +test_expect_success 'commit without staging files fails and displays hints' '
> + echo "initial" >>file &&
> + git add file &&
> + git commit -m initial &&
> + echo "changes" >>file &&
> + test_must_fail git commit -m initial >actual &&
> + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> +'
> +
> test_done
builtin/commit.c
Outdated
| @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, | |||
| */ | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <[email protected]>
>
> Display hints to the user when trying to commit without staging the modified
> files first (when advice.statusHints is set to true). Change the output of the
> unsuccessful commit from e.g:
>
> # [...]
> # Changes not staged for commit:
> # modified: builtin/commit.c
> #
> # no changes added to commit
>
> to:
>
> # [...]
> # Changes not staged for commit:
> # (use "git add <file>..." to update what will be committed)
> # (use "git checkout -- <file>..." to discard changes in working directory)
> #
> # modified: ../builtin/commit.c
> #
> # no changes added to commit (use "git add" and/or "git commit -a")
>
> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
> 2013-09-12) the intent was to disable status hints when writing to
> COMMIT_EDITMSG, but in fact the implementation disabled status messages in
> more locations, e.g in case the commit wasn't successful, status hints
> will still be disabled and no hints will be displayed to the user although
> advice.statusHints is set to true.
>
> Signed-off-by: Heba Waly <[email protected]>
> ---
> builtin/commit.c | 1 +
> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2db2ad0de4..4439666465 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> */
> if (!committable && whence != FROM_MERGE && !allow_empty &&
> !(amend && is_a_merge(current_head))) {
> + s->hints = advice_status_hints;
Hm. This looks like it turns hints back on specifically for this case,
but might not fix other places where ea9882bfc4 turned them off.
I think the intent of that commit was to not put hints into the editor,
so does it make sense to instead wrap this guy:
/*
* Most hints are counter-productive when the commit has
* already started.
*/
s->hints = 0;
in "if (use_editor)"?
I didn't try it on my end. Maybe it won't help much, because we think
we're going to use the editor right up until we realize it's not
committable?
I wonder which other cases that commit got rid of hints for by accident.
- Emily
> s->display_comment_prefix = old_display_comment_prefix;
> run_status(stdout, index_file, prefix, 0, s);
> if (amend)
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 46a5cd4b73..3d76e8ebbd 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
> )
> '
>
> +test_expect_success 'commit without staging files fails and displays hints' '
> + echo "initial" >>file &&
> + git add file &&
> + git commit -m initial &&
> + echo "changes" >>file &&
> + test_must_fail git commit -m initial >actual &&
> + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> +'
> +
> test_done
> --
> gitgitgadget
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Wed, Dec 18, 2019 at 11:45 AM Emily Shaffer <[email protected]> wrote:
>
> On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <[email protected]>
> >
> > Display hints to the user when trying to commit without staging the modified
> > files first (when advice.statusHints is set to true). Change the output of the
> > unsuccessful commit from e.g:
> >
> > # [...]
> > # Changes not staged for commit:
> > # modified: builtin/commit.c
> > #
> > # no changes added to commit
> >
> > to:
> >
> > # [...]
> > # Changes not staged for commit:
> > # (use "git add <file>..." to update what will be committed)
> > # (use "git checkout -- <file>..." to discard changes in working directory)
> > #
> > # modified: ../builtin/commit.c
> > #
> > # no changes added to commit (use "git add" and/or "git commit -a")
> >
> > In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
> > 2013-09-12) the intent was to disable status hints when writing to
> > COMMIT_EDITMSG, but in fact the implementation disabled status messages in
> > more locations, e.g in case the commit wasn't successful, status hints
> > will still be disabled and no hints will be displayed to the user although
> > advice.statusHints is set to true.
> >
> > Signed-off-by: Heba Waly <[email protected]>
> > ---
> > builtin/commit.c | 1 +
> > t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 2db2ad0de4..4439666465 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> > */
> > if (!committable && whence != FROM_MERGE && !allow_empty &&
> > !(amend && is_a_merge(current_head))) {
> > + s->hints = advice_status_hints;
>
> Hm. This looks like it turns hints back on specifically for this case,
> but might not fix other places where ea9882bfc4 turned them off.
>
> I think the intent of that commit was to not put hints into the editor,
> so does it make sense to instead wrap this guy:
>
> /*
> * Most hints are counter-productive when the commit has
> * already started.
> */
> s->hints = 0;
>
> in "if (use_editor)"?
>
That's a good idea, I tried it and it seems to be working fine.
> I didn't try it on my end. Maybe it won't help much, because we think
> we're going to use the editor right up until we realize it's not
> committable?
>
> I wonder which other cases that commit got rid of hints for by accident.
>
The number of cases is quite overwhelming because of all the options that
can be passed to the commit command, but hopefully after wrapping it in
an if condition as you suggested we'll be more certain of the affected cases.
Will send an update shortly.
> - Emily
>
> > s->display_comment_prefix = old_display_comment_prefix;
> > run_status(stdout, index_file, prefix, 0, s);
> > if (amend)
> > diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> > index 46a5cd4b73..3d76e8ebbd 100755
> > --- a/t/t7500-commit-template-squash-signoff.sh
> > +++ b/t/t7500-commit-template-squash-signoff.sh
> > @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
> > )
> > '
> >
> > +test_expect_success 'commit without staging files fails and displays hints' '
> > + echo "initial" >>file &&
> > + git add file &&
> > + git commit -m initial &&
> > + echo "changes" >>file &&
> > + test_must_fail git commit -m initial >actual &&
> > + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> > +'
> > +
> > test_done
> > --
> > gitgitgadget
Thanks,
Heba
|
Yes, I believe that's because the '#' character in the commit message was
translated to a header (h1 tag) in markdown syntax.
I replaced it by dots in the PR description for future updates.
Heba
…On Wed, Dec 18, 2019 at 11:33 AM Emily Shaffer ***@***.***> wrote:
The cover letter on list is formatted really weirdly, @dscho
<https://github.com/dscho> any idea what happened?
***@***.***/
Where did these ====== come from?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#495?email_source=notifications&email_token=AALXYBEBUIQ3XDR5JCUN2ULQZFHRZA5CNFSM4J3WWEVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEF4BA#issuecomment-566779396>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALXYBCJOMHZ7WDBWDFSNSTQZFHRZANCNFSM4J3WWEVA>
.
|
builtin/commit.c
Outdated
| @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, | |||
| */ | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
> From: Heba Waly <[email protected]>
>
> Display hints to the user when trying to commit without staging the modified
> files first (when advice.statusHints is set to true). Change the output of the
> unsuccessful commit from e.g:
Wrap your commit messages at 72 characters.
> # [...]
> # Changes not staged for commit:
> # modified: builtin/commit.c
> #
> # no changes added to commit
>
> to:
>
> # [...]
> # Changes not staged for commit:
> # (use "git add <file>..." to update what will be committed)
> # (use "git checkout -- <file>..." to discard changes in working directory)
> #
> # modified: ../builtin/commit.c
For tidiness, can this line also be "builtin/commit.c" (that is, without
the "../" at the beginning) to match what's before "to:"?
> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
> 2013-09-12) the intent was to disable status hints when writing to
> COMMIT_EDITMSG, but in fact the implementation disabled status messages in
> more locations, e.g in case the commit wasn't successful, status hints
> will still be disabled and no hints will be displayed to the user although
> advice.statusHints is set to true.
>
> Signed-off-by: Heba Waly <[email protected]>
> ---
> builtin/commit.c | 1 +
> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
I wondered if there was a better place to put the test, but I couldn't
find one, so this is fine.
> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> */
> if (!committable && whence != FROM_MERGE && !allow_empty &&
> !(amend && is_a_merge(current_head))) {
> + s->hints = advice_status_hints;
> s->display_comment_prefix = old_display_comment_prefix;
> run_status(stdout, index_file, prefix, 0, s);
> if (amend)
I checked that this undoing of "s->hints = 0" is safe, because s is no
longer used in this function nor in the calling function cmd_commit()
(which is the one that declared s locally).
Still probably worth a comment, though. For example:
This status is to be printed to stdout, so hints will be useful to the
user. Reset s->hints to what the user configured.
The corresponding comment on "s->hints = 0" might need to be tweaked,
too, but I can't think of anything at the moment.
> +test_expect_success 'commit without staging files fails and displays hints' '
> + echo "initial" >>file &&
> + git add file &&
> + git commit -m initial &&
> + echo "changes" >>file &&
> + test_must_fail git commit -m initial >actual &&
Use another commit message for this, since this is no longer "initial".
(Maybe "after initial" or something like that.)
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jonathan Tan <[email protected]> writes:
>> From: Heba Waly <[email protected]>
>>
>> Display hints to the user when trying to commit without staging the modified
>> files first (when advice.statusHints is set to true). Change the output of the
>> unsuccessful commit from e.g:
>
> Wrap your commit messages at 72 characters.
>
>> # [...]
>> # Changes not staged for commit:
>> # modified: builtin/commit.c
>> #
>> # no changes added to commit
>>
>> to:
>>
>> # [...]
>> # Changes not staged for commit:
>> # (use "git add <file>..." to update what will be committed)
>> # (use "git checkout -- <file>..." to discard changes in working directory)
>> #
>> # modified: ../builtin/commit.c
>
> For tidiness, can this line also be "builtin/commit.c" (that is, without
> the "../" at the beginning) to match what's before "to:"?
>
>> In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
>> 2013-09-12) the intent was to disable status hints when writing to
>> COMMIT_EDITMSG, but in fact the implementation disabled status messages in
>> more locations, e.g in case the commit wasn't successful, status hints
>> will still be disabled and no hints will be displayed to the user although
>> advice.statusHints is set to true.
>>
>> Signed-off-by: Heba Waly <[email protected]>
>> ---
>> builtin/commit.c | 1 +
>> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
>
> I wondered if there was a better place to put the test, but I couldn't
> find one, so this is fine.
>
>> @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>> */
>> if (!committable && whence != FROM_MERGE && !allow_empty &&
>> !(amend && is_a_merge(current_head))) {
>> + s->hints = advice_status_hints;
>> s->display_comment_prefix = old_display_comment_prefix;
>> run_status(stdout, index_file, prefix, 0, s);
>> if (amend)
>
> I checked that this undoing of "s->hints = 0" is safe, because s is no
> longer used in this function nor in the calling function cmd_commit()
> (which is the one that declared s locally).
>
> Still probably worth a comment, though. For example:
>
> This status is to be printed to stdout, so hints will be useful to the
> user. Reset s->hints to what the user configured.
>
> The corresponding comment on "s->hints = 0" might need to be tweaked,
> too, but I can't think of anything at the moment.
>
>> +test_expect_success 'commit without staging files fails and displays hints' '
>> + echo "initial" >>file &&
>> + git add file &&
>> + git commit -m initial &&
>> + echo "changes" >>file &&
>> + test_must_fail git commit -m initial >actual &&
>
> Use another commit message for this, since this is no longer "initial".
> (Maybe "after initial" or something like that.)
Thanks for a careful review.
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Wed, Dec 18, 2019 at 4:13 PM Jonathan Tan <[email protected]> wrote:
>
> > From: Heba Waly <[email protected]>
> >
> > Display hints to the user when trying to commit without staging the modified
> > files first (when advice.statusHints is set to true). Change the output of the
> > unsuccessful commit from e.g:
>
> Wrap your commit messages at 72 characters.
>
OK
> > # [...]
> > # Changes not staged for commit:
> > # modified: builtin/commit.c
> > #
> > # no changes added to commit
> >
> > to:
> >
> > # [...]
> > # Changes not staged for commit:
> > # (use "git add <file>..." to update what will be committed)
> > # (use "git checkout -- <file>..." to discard changes in working directory)
> > #
> > # modified: ../builtin/commit.c
>
> For tidiness, can this line also be "builtin/commit.c" (that is, without
> the "../" at the beginning) to match what's before "to:"?
>
Sure.
> > In ea9882bfc4 (commit: disable status hints when writing to COMMIT_EDITMSG,
> > 2013-09-12) the intent was to disable status hints when writing to
> > COMMIT_EDITMSG, but in fact the implementation disabled status messages in
> > more locations, e.g in case the commit wasn't successful, status hints
> > will still be disabled and no hints will be displayed to the user although
> > advice.statusHints is set to true.
> >
> > Signed-off-by: Heba Waly <[email protected]>
> > ---
> > builtin/commit.c | 1 +
> > t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
>
> I wondered if there was a better place to put the test, but I couldn't
> find one, so this is fine.
>
> > @@ -961,6 +961,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> > */
> > if (!committable && whence != FROM_MERGE && !allow_empty &&
> > !(amend && is_a_merge(current_head))) {
> > + s->hints = advice_status_hints;
> > s->display_comment_prefix = old_display_comment_prefix;
> > run_status(stdout, index_file, prefix, 0, s);
> > if (amend)
>
> I checked that this undoing of "s->hints = 0" is safe, because s is no
> longer used in this function nor in the calling function cmd_commit()
> (which is the one that declared s locally).
>
> Still probably worth a comment, though. For example:
>
> This status is to be printed to stdout, so hints will be useful to the
> user. Reset s->hints to what the user configured.
>
Ok.
> The corresponding comment on "s->hints = 0" might need to be tweaked,
> too, but I can't think of anything at the moment.
>
> > +test_expect_success 'commit without staging files fails and displays hints' '
> > + echo "initial" >>file &&
> > + git add file &&
> > + git commit -m initial &&
> > + echo "changes" >>file &&
> > + test_must_fail git commit -m initial >actual &&
>
> Use another commit message for this, since this is no longer "initial".
> (Maybe "after initial" or something like that.)
Makes sense.
Thank you Jonathan.
Heba
Heba
@HebaWaly I fear that this message did not reach Emily, as GitGitGadget only mirrors mails from the Git mailing list to the respective PR, but not the other way round. Please send this as a reply by following the instructions in the "reply to this" link at the top of the comment to which you would like to reply to. [EDIT: whoops, I now realize that you are responding not to a mail, but really to an honest-to-deity PR comment. I apologize for the noise.] |
|
This patch series was integrated into pu via git@ea2ce4e. |
Display hints to the user when trying to commit without staging the modified files first (when advice.statusHints is set to true). Change the output of the unsuccessful commit from e.g: # [...] # Changes not staged for commit: # modified: builtin/commit.c # # no changes added to commit to: # [...] # Changes not staged for commit: # (use "git add <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working directory) # # modified: /builtin/commit.c # # no changes added to commit (use "git add" and/or "git commit -a") In ea9882b (commit: disable status hints when writing to COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints when writing to COMMIT_EDITMSG, but in fact the implementation disabled status messages in more locations, e.g in case the commit wasn't successful, status hints will still be disabled and no hints will be displayed to the user although advice.statusHints is set to true. Signed-off-by: Heba Waly <[email protected]>
HebaWaly
force-pushed
the
hints-for-unsuccessful-commit
branch
from
f23477c to
ebec237
Compare
|
/submit |
|
Submitted as [email protected] |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Emily Shaffer wrote (reply to this): |
| @@ -811,12 +811,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix, | |||
| old_display_comment_prefix = s->display_comment_prefix; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <[email protected]> writes:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e48c1fd90a..868c0d7819 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -811,12 +811,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> old_display_comment_prefix = s->display_comment_prefix;
> s->display_comment_prefix = 1;
>
> - /*
> - * Most hints are counter-productive when the commit has
> - * already started.
> - */
> - s->hints = 0;
> -
Hmm.
> @@ -837,6 +831,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> int saved_color_setting;
> struct ident_split ci, ai;
>
> + /*
> + * Most hints are counter-productive when displayed in
> + * the commit message editor.
> + */
> + s->hints = 0;
> +
We no longer drop s->hints when we are not using editor and not
including status (i.e. the "else" side) because these lines are
moved inside "if". As this change is not about that "no editor"
side, I am not 100% convinced that this is a good change.
> @@ -912,6 +912,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> saved_color_setting = s->use_color;
> s->use_color = 0;
> committable = run_status(s->fp, index_file, prefix, 1, s);
> + if(!committable)
Style: SP between "if" and "(".
> + /*
> + Status is to be printed to stdout, so hints will be useful to the
> + user. Reset s->hints to what the user configured
> + */
> + s->hints = advice_status_hints;
The "if" side has been changed to flip s->hints to the configured
advice hints value when !committable here. The "else" side
(i.e. when we are not using editor and not including status) does
not do anything to s->hints after finding out if committable after
this change. Because "s->hints = 0" was moved to "if" with this
patch, the "else" side no longer drops s->hints at all.
So the final run_status() called when the attempt to commit is
rejected will feed s->hints that is not cleared with this change.
Is that intended? Is the updated behaviour checked with a test?
> s->use_color = saved_color_setting;
> string_list_clear(&s->change, 1);
> } else {
This fix was about "we do not want to unconditionally drop the
advice messages when we reject the attempt to commit and show the
output like 'git status'", wasn't it? The earlier single-liner fix
in v1 that flips s->hints just before calling run_status() before
rejecting the attempt to commit was a lot easier to reason about, as
the fix was very focused and to the point. Why are we seeing this
many (seemingly unrelated) changes?
Puzzled...
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <[email protected]> writes:
> This fix was about "we do not want to unconditionally drop the
> advice messages when we reject the attempt to commit and show the
> output like 'git status'", wasn't it? The earlier single-liner fix
> in v1 that flips s->hints just before calling run_status() before
> rejecting the attempt to commit was a lot easier to reason about, as
> the fix was very focused and to the point. Why are we seeing this
> many (seemingly unrelated) changes?
In any case, here is what I tentatively have in my tree (with heavy
rewrite to the proposed log message).
-- >8 --
From: Heba Waly <[email protected]>
Date: Tue, 17 Dec 2019 09:17:22 +0000
Subject: [PATCH] commit: honor advice.statusHints when rejecting an empty
commit
In ea9882bfc4 (commit: disable status hints when writing to
COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints
when writing to COMMIT_EDITMSG, because giving the hints in the "git
status" like output in the commit message template are too late to
be useful (they say things like "'git add' to stage", but that is
only possible after aborting the current "git commit" session).
But there is one case that the hints can be useful: When the current
attempt to commit is rejected because no change is recorded in the
index. The message is given and "git commit" errors out, so the
hints can immediately be followed by the user. Teach the codepath
to honor the configuration variable.
Signed-off-by: Heba Waly <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
---
builtin/commit.c | 1 +
t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/builtin/commit.c b/builtin/commit.c
index e588bc6ad3..0078faf117 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -944,6 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
*/
if (!committable && whence != FROM_MERGE && !allow_empty &&
!(amend && is_a_merge(current_head))) {
+ s->hints = advice_status_hints;
s->display_comment_prefix = old_display_comment_prefix;
run_status(stdout, index_file, prefix, 0, s);
if (amend)
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 46a5cd4b73..a8179e4074 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
)
'
+test_expect_success 'commit without staging files fails and displays hints' '
+ echo "initial" >>file &&
+ git add file &&
+ git commit -m initial &&
+ echo "changes" >>file &&
+ test_must_fail git commit -m update >actual &&
+ test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
+'
+
test_done
--
2.24.1-732-ga9f9d4909c
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Thu, Dec 19, 2019 at 2:22 PM Junio C Hamano <[email protected]> wrote:
> In any case, here is what I tentatively have in my tree (with heavy
> rewrite to the proposed log message).
>
> +test_expect_success 'commit without staging files fails and displays hints' '
> + echo "initial" >>file &&
The use of '>>' here rather than '>' feels wrong, especially when
"initial" is used for both the file body and the commit message,
causing a reader of the test to wonder if this test somehow depends
upon earlier tests.
> + git add file &&
> + git commit -m initial &&
> + echo "changes" >>file &&
> + test_must_fail git commit -m update >actual &&
> + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> +'
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <[email protected]> writes:
> On Thu, Dec 19, 2019 at 2:22 PM Junio C Hamano <[email protected]> wrote:
>> In any case, here is what I tentatively have in my tree (with heavy
>> rewrite to the proposed log message).
>>
>> +test_expect_success 'commit without staging files fails and displays hints' '
>> + echo "initial" >>file &&
>
> The use of '>>' here rather than '>' feels wrong, especially when
> "initial" is used for both the file body and the commit message,
> causing a reader of the test to wonder if this test somehow depends
> upon earlier tests.
Yeah, makes sense. This was verbatim from v1 but I think starting
the file from scratch like you suggest makes it clearer what is
going on.
>
>> + git add file &&
>> + git commit -m initial &&
>> + echo "changes" >>file &&
>> + test_must_fail git commit -m update >actual &&
>> + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
>> +'
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Thu, Dec 19, 2019 at 11:22:38AM -0800, Junio C Hamano wrote:
> Junio C Hamano <[email protected]> writes:
>
> > This fix was about "we do not want to unconditionally drop the
> > advice messages when we reject the attempt to commit and show the
> > output like 'git status'", wasn't it? The earlier single-liner fix
> > in v1 that flips s->hints just before calling run_status() before
> > rejecting the attempt to commit was a lot easier to reason about, as
> > the fix was very focused and to the point. Why are we seeing this
> > many (seemingly unrelated) changes?
>
> In any case, here is what I tentatively have in my tree (with heavy
> rewrite to the proposed log message).
Hm. I'm surprised to see this feedback come in the form of a local
change when making the topic branch, rather than in a reply to the v1
patch. What's the reasoning? (Or is this scissors patch intended to be
the feedback?)
I ask because out of all of us, it seems the Outreachy interns can
benefit the most from advice on how and why to write their commit
messages - that is, part of the point of an internship is to learn best
practices and cultural norms in addition to coding practice. (Plus, I
find being asked to rewrite a commit message tends to force me to
understand my own change even better than before.)
I'll go ahead and look through the changes to the commit message so I
can learn what you're looking for too :)
>
> -- >8 --
> From: Heba Waly <[email protected]>
> Date: Tue, 17 Dec 2019 09:17:22 +0000
> Subject: [PATCH] commit: honor advice.statusHints when rejecting an empty
> commit
>
> In ea9882bfc4 (commit: disable status hints when writing to
> COMMIT_EDITMSG, 2013-09-12) the intent was to disable status hints
> when writing to COMMIT_EDITMSG, because giving the hints in the "git
> status" like output in the commit message template are too late to
> be useful (they say things like "'git add' to stage", but that is
> only possible after aborting the current "git commit" session).
More context on why the previous change was made - "by the time the
editor was open, it was too late to apply hints anyways". Sure.
>
> But there is one case that the hints can be useful: When the current
> attempt to commit is rejected because no change is recorded in the
> index. The message is given and "git commit" errors out, so the
> hints can immediately be followed by the user. Teach the codepath
> to honor the configuration variable.
Expanding the "but" to supply the specific story this commit touches,
including "what happens instead" and "how are we gonna fix it".
And the copy-paste of the output before and the output now is different.
For me, I don't particularly see why we'd want to be rid of it - it sort
of feels like "a picture is worth a thousand words" to include the
actual use case in the commit message. Is there style guidance
suggesting not to do that that I missed?
- Emily
>
> Signed-off-by: Heba Waly <[email protected]>
> Signed-off-by: Junio C Hamano <[email protected]>
> ---
> builtin/commit.c | 1 +
> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e588bc6ad3..0078faf117 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -944,6 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> */
> if (!committable && whence != FROM_MERGE && !allow_empty &&
> !(amend && is_a_merge(current_head))) {
> + s->hints = advice_status_hints;
> s->display_comment_prefix = old_display_comment_prefix;
> run_status(stdout, index_file, prefix, 0, s);
> if (amend)
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 46a5cd4b73..a8179e4074 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -382,4 +382,13 @@ test_expect_success 'check commit with unstaged rename and copy' '
> )
> '
>
> +test_expect_success 'commit without staging files fails and displays hints' '
> + echo "initial" >>file &&
> + git add file &&
> + git commit -m initial &&
> + echo "changes" >>file &&
> + test_must_fail git commit -m update >actual &&
> + test_i18ngrep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
> +'
> +
> test_done
> --
> 2.24.1-732-ga9f9d4909c
>
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Emily Shaffer <[email protected]> writes:
> Hm. I'm surprised to see this feedback come in the form of a local
> change when making the topic branch, rather than in a reply to the v1
> patch. What's the reasoning? (Or is this scissors patch intended to be
> the feedback?)
You haven't seen a suggestion in the form of counter-proposal?
> I ask because out of all of us, it seems the Outreachy interns can
> benefit the most from advice on how and why to write their commit
> messages - that is, part of the point of an internship is to learn best
> practices and cultural norms in addition to coding practice. (Plus, I
> find being asked to rewrite a commit message tends to force me to
> understand my own change even better than before.)
It's something Mentors can help doing (I do not necessarily have
time for that myself), and you're welcome to use the "tenatively
queued" version as an example.
> I'll go ahead and look through the changes to the commit message so I
> can learn what you're looking for too :)
Nice.
One thing you missed in your review of the "tentatively queued"
version is the reversal of the order of presentation. Instead of
starting with "I decided to do this" without explanation, give the
picture of status quo to set the stage, explain what issue exists in
the current behaviour, and then describe what approach was chosen to
solve the issue.
> For me, I don't particularly see why we'd want to be rid of it - it sort
> of feels like "a picture is worth a thousand words" to include the
> actual use case in the commit message.
Output coming from commands and/or options that are used only in a
bit more advanced workflow and the ones that are rarely seen, I do
agree that showing example is a good way to illustrate exactly what
you are talking about.
On the other hand, for behaviour of basic local commands like "git
add", "git commit", "git diff", ..., I do not necessarily agree, as
these should be obvious and clear to all the intended audiences,
which would be "anybody who has used Git for say more than two
weeks.
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Fri, Dec 20, 2019 at 10:34:28AM -0800, Junio C Hamano wrote:
> Emily Shaffer <[email protected]> writes:
>
> > Hm. I'm surprised to see this feedback come in the form of a local
> > change when making the topic branch, rather than in a reply to the v1
> > patch. What's the reasoning? (Or is this scissors patch intended to be
> > the feedback?)
>
> You haven't seen a suggestion in the form of counter-proposal?
I actually have only seen the scissors-patch as a "yes, and" in
practice. I think this is a sign I should be doing more reviews ;)
>
> > I ask because out of all of us, it seems the Outreachy interns can
> > benefit the most from advice on how and why to write their commit
> > messages - that is, part of the point of an internship is to learn best
> > practices and cultural norms in addition to coding practice. (Plus, I
> > find being asked to rewrite a commit message tends to force me to
> > understand my own change even better than before.)
>
> It's something Mentors can help doing (I do not necessarily have
> time for that myself), and you're welcome to use the "tenatively
> queued" version as an example.
>
> > I'll go ahead and look through the changes to the commit message so I
> > can learn what you're looking for too :)
>
> Nice.
>
> One thing you missed in your review of the "tentatively queued"
> version is the reversal of the order of presentation. Instead of
> starting with "I decided to do this" without explanation, give the
> picture of status quo to set the stage, explain what issue exists in
> the current behaviour, and then describe what approach was chosen to
> solve the issue.
Thanks for explaining this - that's a good point for me to take home.
>
> > For me, I don't particularly see why we'd want to be rid of it - it sort
> > of feels like "a picture is worth a thousand words" to include the
> > actual use case in the commit message.
>
> Output coming from commands and/or options that are used only in a
> bit more advanced workflow and the ones that are rarely seen, I do
> agree that showing example is a good way to illustrate exactly what
> you are talking about.
>
> On the other hand, for behaviour of basic local commands like "git
> add", "git commit", "git diff", ..., I do not necessarily agree, as
> these should be obvious and clear to all the intended audiences,
> which would be "anybody who has used Git for say more than two
> weeks.
Hm, I see. Thanks for clarifying.
- Emily
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
> > This fix was about "we do not want to unconditionally drop the
> > advice messages when we reject the attempt to commit and show the
> > output like 'git status'", wasn't it? The earlier single-liner fix
> > in v1 that flips s->hints just before calling run_status() before
> > rejecting the attempt to commit was a lot easier to reason about, as
> > the fix was very focused and to the point. Why are we seeing this
> > many (seemingly unrelated) changes?
>
> In any case, here is what I tentatively have in my tree (with heavy
> rewrite to the proposed log message).
Junio, what are your plans over what you have in your tree? If you'd
like to hear Heba's opinion on it, then she can chime in; if you'd like
a review, then I think it's good to go in.
I think the main area of discussion is whether we should go with Heba's
attempt to address Emily's comment [1]:
> I think the intent of that commit was to not put hints into the editor,
> so does it make sense to instead wrap this guy:
>
> /*
> * Most hints are counter-productive when the commit has
> * already started.
> */
> s->hints = 0;
>
> in "if (use_editor)"?
>
> I didn't try it on my end. Maybe it won't help much, because we think
> we're going to use the editor right up until we realize it's not
> committable?
And I think the answer to that is "s" is used throughout the function in
various ways (in particular, used to print statuses both to stdout and
to the message template) so any wrapping or corralling of scope would
just make things more complicated. In particular, the way Heba did it in
v2 is more unclear - at the time of setting s->hints = 0, it's done
within a "if (use_editor && include_status)" block, but (as far as I can
tell) the commit message template might also be used when there is no
editor - for example, as input to a hook. And more importantly, when
s->hints is reset to the config, we don't know at that point that the
next status is going to stdout. So I think it's better just to use the
v1 way.
The second area of discussion I see is in the commit message. Commit
messages have to balance brevity and comprehensiveness, and this can be
a subjective matter, but I think Junio's strikes a good balance.
[1] https://lore.kernel.org/git/[email protected]/
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jonathan Tan <[email protected]> writes:
>> In any case, here is what I tentatively have in my tree (with heavy
>> rewrite to the proposed log message).
>
> Junio, what are your plans over what you have in your tree? If you'd
> like to hear Heba's opinion on it, then she can chime in; if you'd like
> a review, then I think it's good to go in.
On hold until anything like those happens ;-)
A random reviewer mentioning something on a patch (either in a
line-by-line critique form or "how about doing it this way instead"
counterproposal form) without getting followed up by others
(including the original author) is a stall review thread, and it
does not change the equation if the random reviewer happens to be me.
>> I didn't try it on my end. Maybe it won't help much, because we think
>> we're going to use the editor right up until we realize it's not
>> committable?
>
> And I think the answer to that is "s" is used throughout the function in
> various ways (in particular, used to print statuses both to stdout and
> to the message template) so any wrapping or corralling of scope would
> just make things more complicated. In particular, the way Heba did it in
> v2 is more unclear - at the time of setting s->hints = 0, it's done
You mean "less clear" (just double checking if I got the negation right)?
> within a "if (use_editor && include_status)" block, but (as far as I can
> tell) the commit message template might also be used when there is no
> editor - for example, as input to a hook. And more importantly, when
> s->hints is reset to the config, we don't know at that point that the
> next status is going to stdout. So I think it's better just to use the
> v1 way.
Yeah, thanks for going back to compare v1 and v2, and I agree with
your assessment.
> The second area of discussion I see is in the commit message. Commit
> messages have to balance brevity and comprehensiveness, and this can be
> a subjective matter, but I think Junio's strikes a good balance.
As one side of the comparison is my own, I won't be a good judge on
this, but yes I tried to strick a good balance as much as possible.
I think I've merged it to 'next' yesterday, but it does not mean
that much as we are in -rc and it is not such an urgent "oops we
broke it in this cycle, let's fix it" issue. If we see a v3 that
improves it, I do not mind at all reverting what I merged to 'next'
and use the updated one instead (either way, it will be in 'master'
during the next cycle at the earliest).
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
> > Junio, what are your plans over what you have in your tree? If you'd
> > like to hear Heba's opinion on it, then she can chime in; if you'd like
> > a review, then I think it's good to go in.
>
> On hold until anything like those happens ;-)
>
> A random reviewer mentioning something on a patch (either in a
> line-by-line critique form or "how about doing it this way instead"
> counterproposal form) without getting followed up by others
> (including the original author) is a stall review thread, and it
> does not change the equation if the random reviewer happens to be me.
OK :-)
> > And I think the answer to that is "s" is used throughout the function in
> > various ways (in particular, used to print statuses both to stdout and
> > to the message template) so any wrapping or corralling of scope would
> > just make things more complicated. In particular, the way Heba did it in
> > v2 is more unclear - at the time of setting s->hints = 0, it's done
>
> You mean "less clear" (just double checking if I got the negation right)?
Yes, less clear - v2 is less clear than v1.
> I think I've merged it to 'next' yesterday, but it does not mean
> that much as we are in -rc and it is not such an urgent "oops we
> broke it in this cycle, let's fix it" issue. If we see a v3 that
> improves it, I do not mind at all reverting what I merged to 'next'
> and use the updated one instead (either way, it will be in 'master'
> during the next cycle at the earliest).
Sounds good - thanks.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Heba Waly wrote (reply to this): |
|
On the Git mailing list, Heba Waly wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Display hints to the user when trying to commit without staging the modified
files first (when advice.statusHints is set to true). Change the output of the
unsuccessful commit from e.g:
. [...]
. Changes not staged for commit:
. modified: builtin/commit.c
.
. no changes added to commit
to:
. [...]
. Changes not staged for commit:
. (use "git add ..." to update what will be committed)
. (use "git checkout -- ..." to discard changes in working directory)
.
. modified: ../builtin/commit.c
.
. no changes added to commit (use "git add" and/or "git commit -a")
In ea9882b (commit: disable status hints when writing to COMMIT_EDITMSG,
2013-09-12) the intent was to disable status hints when writing to
COMMIT_EDITMSG, but in fact the implementation disabled status messages in
more locations, e.g in case the commit wasn't successful, status hints
will still be disabled and no hints will be displayed to the user although
advice.statusHints is set to true.
Signed-off-by: Heba Waly [email protected]