sequencer: make it clearer that commit descriptions are just comments #1923
+94
−88
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
|
I thought this was belaboring the point (and left it out of the commit message), but was slightly worried a reviewer might ask about an existing git-rebase-todo file at the time they upgraded git, so I'll just type it here: A leftover git-rebase-todo from an in-progress rebase at the time someone upgrades git poses no problem. As noted in the commit message, the only thing sequencer pays attention to is |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Reto wrote (reply to this): On Thu, May 15, 2025 at 02:20:27AM +0000, Elijah Newren via GitGitGadget wrote:
> Every once in a while, users report that editing the commit summaries
> in the todo list does not get reflected in the rebase operation,
> suggesting that users are (a) only using one-line commit messages, and
> (b) not understanding that the commit summaries are merely helpful
> comments to help them find the right hashes.
As with everything in git, you can learn that this is the case.
You could as well just add a explanatory comment to the footer.
> It may be difficult to correct users' poor commit messages, but we can
> at least try to make it clearer that the commit summaries are not
> directives of some sort by inserting a comment character. Hopefully
> that leads to them looking a little further and noticing the hints at
> the bottom to use 'reword' or 'edit' directives.
For fancy things/editors that recognize comments, this will *dim* the commit
messages, to light grey or such.
This is decidedly not what I'd like to happen at least. The commit messages
there are my primary way of navigating the commit, given that I'm not learning
the commit shas by hard ;)
While I appreciate the motivation, I don't think the comment string is a
good approach here.
Cheers,
Reto |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Reto
On 15/05/2025 05:42, Reto wrote:
> On Thu, May 15, 2025 at 02:20:27AM +0000, Elijah Newren via GitGitGadget wrote:
> >> It may be difficult to correct users' poor commit messages, but we can
>> at least try to make it clearer that the commit summaries are not
>> directives of some sort by inserting a comment character. Hopefully
>> that leads to them looking a little further and noticing the hints at
>> the bottom to use 'reword' or 'edit' directives.
> > For fancy things/editors that recognize comments, this will *dim* the commit
> messages, to light grey or such.
> This is decidedly not what I'd like to happen at least. The commit messages
> there are my primary way of navigating the commit, given that I'm not learning
> the commit shas by hard ;)
As the commit message makes clear we already insert a '#' between the parents and subject line of a merge command. We also append '# empty' to empty commits. If there are editors that erroneously treat a '#' anywhere in the line as a comment they're already dimming things they shouldn't. A line in the todo list is only a comment if it starts with core.commentString which defaults to '#'. Editors that unconditionally treat '#' as the start of a comment are buggy.
> While I appreciate the motivation, I don't think the comment string is a
> good approach here.
As the commit message points out strictly speaking we're not using the comment string we using a fixed single character.
Best Wishes
Phillip
|
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Sorry I forgot to Cc the original author in the message below.
On 15/05/2025 11:16, Phillip Wood wrote:
> Hi Reto
> > On 15/05/2025 05:42, Reto wrote:
>> On Thu, May 15, 2025 at 02:20:27AM +0000, Elijah Newren via >> GitGitGadget wrote:
>>
>>> It may be difficult to correct users' poor commit messages, but we can
>>> at least try to make it clearer that the commit summaries are not
>>> directives of some sort by inserting a comment character. Hopefully
>>> that leads to them looking a little further and noticing the hints at
>>> the bottom to use 'reword' or 'edit' directives.
>>
>> For fancy things/editors that recognize comments, this will *dim* the >> commit
>> messages, to light grey or such.
>> This is decidedly not what I'd like to happen at least. The commit >> messages
>> there are my primary way of navigating the commit, given that I'm not >> learning
>> the commit shas by hard ;)
> > As the commit message makes clear we already insert a '#' between the > parents and subject line of a merge command. We also append '# empty' to > empty commits. If there are editors that erroneously treat a '#' > anywhere in the line as a comment they're already dimming things they > shouldn't. A line in the todo list is only a comment if it starts with > core.commentString which defaults to '#'. Editors that unconditionally > treat '#' as the start of a comment are buggy.
> >> While I appreciate the motivation, I don't think the comment string is a
>> good approach here.
> > As the commit message points out strictly speaking we're not using the > comment string we using a fixed single character.
> > Best Wishes
> > Phillip
> |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Elijah
On 15/05/2025 03:20, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> > Every once in a while, users report that editing the commit summaries
> in the todo list does not get reflected in the rebase operation,
> suggesting that users are (a) only using one-line commit messages, and
> (b) not understanding that the commit summaries are merely helpful
> comments to help them find the right hashes.
> > It may be difficult to correct users' poor commit messages, but we can
> at least try to make it clearer that the commit summaries are not
> directives of some sort by inserting a comment character. Hopefully
> that leads to them looking a little further and noticing the hints at
> the bottom to use 'reword' or 'edit' directives.
> > Yes, this change may look funny at first since it hardcodes '#' rather
> than using comment_line_str. However:
> > * comment_line_str exists to allow disambiguation between lines in
> a commit message and lines that are instructions to users editing
> the commit message. No such disambiguation is needed for these
> comments that occur on the same line after existing directives
> * the exact "comment" character(s) on regular pick lines used aren't
> actually important; I could have used anything, including completely
> random variable length text for each line and it'd work because we
> ignore everything after 'pick' and the hash.
> * The whole point of this change is to signal to users that they
> should NOT be editing any part of the line after the hash (and if
> they do so, their edits will be ignored), while the whole point of
> comment_line_str is to allow highly flexible editing. So making
> it more general by using comment_line_str actually feels
> counterproductive.
> * The character for merge directives absolutely must be '#'; that
> has been deeply hardcoded for a long time (see below), and will
> break if some other comment character is used instead. In a
> desire to have pick and merge directives be similar, I use the
> same comment character for both.
> * Perhaps merge directives could be fixed to not be inflexible about
> the comment character used, if someone feels highly motivated, but
> I think that should be done in a separate follow-on patch.
> > Here are (some of?) the locations where '#' has already been hardcoded
> for a long time for merges:
> > 1) In check_label_or_ref_arg():
> case TODO_LABEL:
> /*
> * '#' is not a valid label as the merge command uses it to
> * separate merge parents from the commit subject.
> */
> > 2) In do_merge():
> > /*
> * For octopus merges, the arg starts with the list of revisions to be
> * merged. The list is optionally followed by '#' and the oneline.
> */
> merge_arg_len = oneline_offset = arg_len;
> for (p = arg; p - arg < arg_len; p += strspn(p, " \t\n")) {
> if (!*p)
> break;
> if (*p == '#' && (!p[1] || isspace(p[1]))) {
> > 3) In label_oid():
> > if ((buf->len == the_hash_algo->hexsz &&
> !get_oid_hex(label, &dummy)) ||
> (buf->len == 1 && *label == '#') ||
> hashmap_get_from_hash(&state->labels,
> strihash(label), label)) {
> /*
> * If the label already exists, or if the label is a
> * valid full OID, or the label is a '#' (which we use
> * as a separator between merge heads and oneline), we
> * append a dash and a number to make it unique.
> */
Thanks for the comprehensive commit message. I think this change is a very good idea. The code changes all look good to me. I've left one comment on the tests below though I should admit that I could feel my eyes glazing over by the time I got to the end of all the test changes. The test changes are pretty much all mechanical though so I'm reasonably confidant that the tests would fail if there was something wrong with the conversion.
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 47534f1062d..52a5e4c016b 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -813,7 +813,7 @@ test_expect_success 'git pull --rebase does not reapply old patches' '
> cd dst &&
> test_must_fail git pull --rebase &&
> cat .git/rebase-merge/done .git/rebase-merge/git-rebase-todo >work &&
> - grep -v -e \# -e ^$ work >patches &&
> + sed -e s/#.*// work | grep -v ^$ >patches &&
I think we could just use 'sed -n "!/^#/p work >patches &&" here
Thanks
Phillip |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <[email protected]> writes:
> suggesting that users are (a) only using one-line commit messages, and
> (b) not understanding that the commit summaries are merely helpful
> comments to help them find the right hashes.
>
> It may be difficult to correct users' poor commit messages, but we can
> at least try to make it clearer that the commit summaries are not
> directives of some sort by inserting a comment character. Hopefully
> that leads to them looking a little further and noticing the hints at
> the bottom to use 'reword' or 'edit' directives.
Cute and clever.
> Last commands done (2 commands done):
> - pick $COMMIT2 two_split
> - edit $COMMIT3 three_split
> + pick $COMMIT2 # two_split
> + edit $COMMIT3 # three_split
> Next command to do (1 remaining command):
> - pick $COMMIT4 four_split
> + pick $COMMIT4 # four_split
> (use "git rebase --edit-todo" to view and edit)
These show the intentions quite well. I think all places the
addition of '#' is done at the output layer, and the matching of
commits for the purpose of --autosquash should be done solely using
the internal representation without '#', this change should not
break anything, I would think, that we internally have.
Breaking custom todo-editors the users may have is a different
story, but if they are somehow matching against the first line of
the commit log message when there prominently is a short-but-unique
commit object name, they are simply crazy and not worth worrying
about, I would say ;-)
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
>> - grep -v -e \# -e ^$ work >patches &&
>> + sed -e s/#.*// work | grep -v ^$ >patches &&
>
> I think we could just use 'sed -n "!/^#/p work >patches &&" here
Or even "sed -e '/^#/d' -e '/^$/d'" ? |
Every once in a while, users report that editing the commit summaries
in the todo list does not get reflected in the rebase operation,
suggesting that users are (a) only using one-line commit messages, and
(b) not understanding that the commit summaries are merely helpful
comments to help them find the right hashes.
It may be difficult to correct users' poor commit messages, but we can
at least try to make it clearer that the commit summaries are not
directives of some sort by inserting a comment character. Hopefully
that leads to them looking a little further and noticing the hints at
the bottom to use 'reword' or 'edit' directives.
Yes, this change may look funny at first since it hardcodes '#' rather
than using comment_line_str. However:
* comment_line_str exists to allow disambiguation between lines in
a commit message and lines that are instructions to users editing
the commit message. No such disambiguation is needed for these
comments that occur on the same line after existing directives
* the exact "comment" character(s) on regular pick lines used aren't
actually important; I could have used anything, including completely
random variable length text for each line and it'd work because we
ignore everything after 'pick' and the hash.
* The whole point of this change is to signal to users that they
should NOT be editing any part of the line after the hash (and if
they do so, their edits will be ignored), while the whole point of
comment_line_str is to allow highly flexible editing. So making
it more general by using comment_line_str actually feels
counterproductive.
* The character for merge directives absolutely must be '#'; that
has been deeply hardcoded for a long time (see below), and will
break if some other comment character is used instead. In a
desire to have pick and merge directives be similar, I use the
same comment character for both.
* Perhaps merge directives could be fixed to not be inflexible about
the comment character used, if someone feels highly motivated, but
I think that should be done in a separate follow-on patch.
Here are (some of?) the locations where '#' has already been hardcoded
for a long time for merges:
1) In check_label_or_ref_arg():
case TODO_LABEL:
/*
* '#' is not a valid label as the merge command uses it to
* separate merge parents from the commit subject.
*/
2) In do_merge():
/*
* For octopus merges, the arg starts with the list of revisions to be
* merged. The list is optionally followed by '#' and the oneline.
*/
merge_arg_len = oneline_offset = arg_len;
for (p = arg; p - arg < arg_len; p += strspn(p, " \t\n")) {
if (!*p)
break;
if (*p == '#' && (!p[1] || isspace(p[1]))) {
3) In label_oid():
if ((buf->len == the_hash_algo->hexsz &&
!get_oid_hex(label, &dummy)) ||
(buf->len == 1 && *label == '#') ||
hashmap_get_from_hash(&state->labels,
strihash(label), label)) {
/*
* If the label already exists, or if the label is a
* valid full OID, or the label is a '#' (which we use
* as a separator between merge heads and oneline), we
* append a dash and a number to make it unique.
*/
Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
sequencer-todo-commit-msgs-are-comments
branch
from
e5f3d70 to
5d29020
Compare
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.
cc: Reto [email protected]
cc: Phillip Wood [email protected]