Re: [PATCH 2/6] commit: add amend suboption to --fixup to create amend! commit
From: Junio C Hamano <hidden>
Date: 2021-02-17 19:51:00
Charvi Mendiratta [off-list ref] writes:
`git commit --fixup=amend:<commit>` will create an "amend!" commit. The resulting commit message subject will be "amend! ..." where "..." is the subject line of <commit> and the initial message body will be <commit>'s message. -m can be used to override the message body. The "amend!" commit when rebased with --autosquash will fixup the contents and replace the commit message of <commit> with the "amend!" commit's message body. Inorder to prevent rebase from creating commits with an empty message we refuse to create an "amend!" commit if commit message body is empty. Example usage: $ git commit --fixup=amend:HEAD $ git commit --fixup=amend:HEAD -m "clever commit message"
Sorry, but it is not so clear what these examples are trying to illustrate. The first one is to add a new commit to later amend the tip commit (It is a bit of mystery why the user does not do a more usual "git commit --amend" right there, though, and such a mystery may distract readers. If the commit were not at the tip, e.g. HEAD~3, it may be less distracting). The second one, even with s|HEAD|HEAD~3| is even less clear. Without the "-m", the resulting commit will have the subject that begins with !amend but the log message body is taken from the given commit, but with "-m", what happens? Does a single-liner 'clever commit message' _replace_ the log message of the named commit, resulting in an !amend commit that has no message from the original? Or does 'clever commit message' get _appended_ the log message? I think we can just remove the "example" from here and explain the feature well in the end-user facing documentation.
+ if (fixup_message) {
+ /*
+ * check if ':' occurs before '^' or '@', otherwise
+ * fixup_message is a commit reference.
+ */
Isn't it that you only intend to parse:
--fixup
--fixup=amend:<any string that names a commit>
--fixup=<any string that names a commit>
and later extend it to allow keywords other than "amend"?
I can understand that you are trying to avoid getting fooled by
things like
--fixup='HEAD^{/commit message with a colon : in it}'
but why special case only ^ and @? This feels brittle (note that I
said "things like", exactly because I do not know if any string that
can name a commit must have "@" or "^" appear before ":" if it is to
have ":" in anywhere, which is what this code assumes).
Instead, you can find the first colon, check for known keywords (or
a string that consists only of alnums to accomodate for future
enhancement), and treat any garbage that happens to have a colon
without the "keyword" as fixup_commit. I.e. something along this
line...
const char alphas[] = "abcde...xyz";
size_t kwd_len;
kwd_len = strspn(fixup_message, alphas);
if (kwd_len && fixup_message[kwd_len] == ':') {
/* found keyword? */
fixup_message[kwd_len] = '\0';
if (!strcmp("amend", fixup_message)) {
... do the amend:<commit> thing ...
#if in-next-step-when-you-add-support-for-reword
} else if (!strcmp("reword", fixup_message)) {
... do the reword:<commit> thing ...
#endif
} else {
die(_("unknown --fixup=%s:<commit>",
fixup_message));
}
} else {
/* the entire fixup_message is the commit */
}