| Message ID | 20210310194306.32565-4-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Charvi Mendiratta <charvi077@gmail.com> writes: > * reference for example: --fixup="HEAD^{/^area: string}" or > * a suboption of `--fixup`. > * > - * As `amend` suboption contains only alpha character. > - * So check if first non alpha character in fixup_message > - * is ':'. > + * As `amend`/`reword` suboptions contains only alpha > + * characters. So check if first non alpha character > + * in fixup_message is ':'. Sorry, but I cannot quite follow the logic. We limit --fixup's suboptions to only alpha characters. If the first character after a len of alpha is colon, then the part before the colon may be a known suboption name like `amend` or `reword`, or a misspelt suboption name. Otherwise, we are dealing with --fixup=<commit> that happens to have a colon in <commit> object name. perhaps?
On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote: > > Charvi Mendiratta <charvi077@gmail.com> writes: > > > * reference for example: --fixup="HEAD^{/^area: string}" or > > * a suboption of `--fixup`. > > * > > - * As `amend` suboption contains only alpha character. > > - * So check if first non alpha character in fixup_message > > - * is ':'. > > + * As `amend`/`reword` suboptions contains only alpha > > + * characters. So check if first non alpha character > > + * in fixup_message is ':'. > > Sorry, but I cannot quite follow the logic. > > We limit --fixup's suboptions to only alpha characters. If > the first character after a len of alpha is colon, then the > part before the colon may be a known suboption name like > `amend` or `reword`, or a misspelt suboption name. > > Otherwise, we are dealing with --fixup=<commit> that happens > to have a colon in <commit> object name. > > perhaps? Yes, Agree. Here I just intend to mention the special case "--fixup=HEAD^{/^area: string}" because of which we chose the method to check if first non alpha char is ':' instead of directly checking the suboption like (skip_prefix(msg, "amend:", &arg). So maybe we can reword it like - To check if fixup_message that contains ':' is a commit - reference for example: --fixup="HEAD^{/^area: string}" or - a suboption of `--fixup`. + fixup_message could be a commit reference for example: + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`. + + As `amend` ... Thanks and Regards, Charvi
Charvi Mendiratta <charvi077@gmail.com> writes: > On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote: >> >> Charvi Mendiratta <charvi077@gmail.com> writes: >> >> > * reference for example: --fixup="HEAD^{/^area: string}" or >> > * a suboption of `--fixup`. >> > * >> > - * As `amend` suboption contains only alpha character. >> > - * So check if first non alpha character in fixup_message >> > - * is ':'. >> > + * As `amend`/`reword` suboptions contains only alpha >> > + * characters. So check if first non alpha character >> > + * in fixup_message is ':'. >> >> Sorry, but I cannot quite follow the logic. >> >> We limit --fixup's suboptions to only alpha characters. If >> the first character after a len of alpha is colon, then the >> part before the colon may be a known suboption name like >> `amend` or `reword`, or a misspelt suboption name. >> >> Otherwise, we are dealing with --fixup=<commit> that happens >> to have a colon in <commit> object name. >> >> perhaps? > > Yes, Agree. Here I just intend to mention the special case > "--fixup=HEAD^{/^area: string}" because of which we chose the method > to check if first non alpha char is ':' instead of directly checking > the suboption like (skip_prefix(msg, "amend:", &arg). So maybe we can > reword it like > > - To check if fixup_message that contains ':' is a commit > - reference for example: --fixup="HEAD^{/^area: string}" or > - a suboption of `--fixup`. > + fixup_message could be a commit reference for example: > + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`. > + > + As `amend` ... My suggestion primarily started a reaction to that "As `amend`..." which was not gramatically complete sentence, and I ended up rewriting everything after "As `amend`..." But re-reading what is in the paragraph before, I tend to think that it places too much stress on 'colon' and should be removed. The comment is about what is being parsed, so We limit --fixup's suboptions to only alpha characters. If the first character after a run of alpha is colon, then the part before the colon may be a known suboption name like `amend` or `reword`, or a misspelt suboption name. In either case, we treat it as --fixup=<suboption>:<arg> Otherwise, we are dealing with --fixup=<commit>. would be good. The code, when it decides it is not in the --fixup=<suboption>:<arg> form but it is --fixup=<commit>, does not even care about a colon, so there is no need to mention colon in the "Otherwise" part.
On Thu, Mar 11, 2021 at 12:37 AM Junio C Hamano <gitster@pobox.com> wrote: > My suggestion primarily started a reaction to that "As `amend`..." > which was not gramatically complete sentence, and I ended up > rewriting everything after "As `amend`..." > > But re-reading what is in the paragraph before, I tend to think that > it places too much stress on 'colon' and should be removed. > > The comment is about what is being parsed, so > > We limit --fixup's suboptions to only alpha characters. If > the first character after a run of alpha is colon, then the > part before the colon may be a known suboption name like > `amend` or `reword`, or a misspelt suboption name. In > either case, we treat it as --fixup=<suboption>:<arg> > > Otherwise, we are dealing with --fixup=<commit>. I suggested a different way to rewrite this comment[1] in my review of patch [2/6] before I was aware that you two were discussing rewrites here in [3/6]. [1]: https://lore.kernel.org/git/CAPig+cRxZFV7DDtnYOxRqEYU4PwXK8gb2HSehDyGorqdgsmoNQ@mail.gmail.com/
On Thu, 11 Mar 2021 at 11:07, Junio C Hamano <gitster@pobox.com> wrote: > [...] > > - To check if fixup_message that contains ':' is a commit > > - reference for example: --fixup="HEAD^{/^area: string}" or > > - a suboption of `--fixup`. > > + fixup_message could be a commit reference for example: > > + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`. > > + > > + As `amend` ... > > My suggestion primarily started a reaction to that "As `amend`..." > which was not gramatically complete sentence, and I ended up > rewriting everything after "As `amend`..." > Oops, I must have written a complete comment. I will take care of it. > But re-reading what is in the paragraph before, I tend to think that > it places too much stress on 'colon' and should be removed. > > The comment is about what is being parsed, so > > We limit --fixup's suboptions to only alpha characters. If > the first character after a run of alpha is colon, then the > part before the colon may be a known suboption name like > `amend` or `reword`, or a misspelt suboption name. In > either case, we treat it as --fixup=<suboption>:<arg> > > Otherwise, we are dealing with --fixup=<commit>. > > would be good. The code, when it decides it is not in the > --fixup=<suboption>:<arg> form but it is --fixup=<commit>, does not > even care about a colon, so there is no need to mention colon in the > "Otherwise" part. Okay, I will replace it. Thanks for helping out and guiding. Thanks and Regards, Charvi
diff --git a/builtin/commit.c b/builtin/commit.c index 05594fa8ab..d8cec75888 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1188,6 +1188,19 @@ static void finalize_deferred_config(struct wt_status *s) s->ahead_behind_flags = AHEAD_BEHIND_FULL; } +static void check_fixup_reword_options(int argc, const char *argv[]) { + if (whence != FROM_COMMIT) { + if (whence == FROM_MERGE) + die(_("You are in the middle of a merge -- cannot reword.")); + else if (is_from_cherry_pick(whence)) + die(_("You are in the middle of a cherry-pick -- cannot reword.")); + } + if (argc) + die(_("cannot combine reword option of --fixup with path '%s'"), *argv); + if (patch_interactive || interactive || all || also || only) + die(_("reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only")); +} + /* returns the length of intial segment of alpha characters only */ static size_t get_alpha_len(char *fixup_message) { const char alphas[] = "abcdefghijklmnopqrstuvwxyz"; @@ -1276,17 +1289,22 @@ static int parse_and_validate_options(int argc, const char *argv[], * reference for example: --fixup="HEAD^{/^area: string}" or * a suboption of `--fixup`. * - * As `amend` suboption contains only alpha character. - * So check if first non alpha character in fixup_message - * is ':'. + * As `amend`/`reword` suboptions contains only alpha + * characters. So check if first non alpha character + * in fixup_message is ':'. */ size_t len = get_alpha_len(fixup_message); if (len && fixup_message[len] == ':') { fixup_message[len++] = '\0'; fixup_commit = fixup_message + len; - if (!strcmp("amend", fixup_message)) { + if (!strcmp("amend", fixup_message) || + !strcmp("reword", fixup_message)) { fixup_prefix = "amend"; allow_empty = 1; + if (*fixup_message == 'r') { + check_fixup_reword_options(argc, argv); + only = 1; + } } else { die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit); } @@ -1575,10 +1593,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")), OPT_STRING('C', "reuse-message", &use_message, N_("commit"), N_("reuse message from specified commit")), /* - * TRANSLATORS: Leave "[amend:]" as-is, and - * only translate <commit>. + * TRANSLATORS: Leave "[(amend|reword):]" as-is, + * and only translate <commit>. */ - OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")), + OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")), OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
`git commit --fixup=reword:<commit>` aliases `--fixup=amend:<commit> --only`, where it creates an empty "amend!" commit that will reword <commit> without changing its contents when it is rebased with `--autosquash`. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- builtin/commit.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)