| Message ID | f23477c5a32e5d638310024194040146026972b8.1576574242.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: display advice hints when commit fails | expand |
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> 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
On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <heba.waly@gmail.com> > > 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 <heba.waly@gmail.com> > --- > 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
> From: Heba Waly <heba.waly@gmail.com> > > 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 <heba.waly@gmail.com> > --- > 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.)
Jonathan Tan <jonathantanmy@google.com> writes: >> From: Heba Waly <heba.waly@gmail.com> >> >> 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 <heba.waly@gmail.com> >> --- >> 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.
On Wed, Dec 18, 2019 at 11:45 AM Emily Shaffer <emilyshaffer@google.com> wrote: > > On Tue, Dec 17, 2019 at 09:17:22AM +0000, Heba Waly via GitGitGadget wrote: > > From: Heba Waly <heba.waly@gmail.com> > > > > 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 <heba.waly@gmail.com> > > --- > > 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
On Wed, Dec 18, 2019 at 4:13 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > From: Heba Waly <heba.waly@gmail.com> > > > > 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 <heba.waly@gmail.com> > > --- > > 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
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) 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