{"thread":{"id":"55235","subject":"[PATCH v3 0/6][Outreachy] commit: Implementation of \"amend!\" commit","startedAt":"2021-03-01T08:48:50Z","lastAt":"2021-03-19T14:11:52Z","messageCount":95,"participants":["Charvi Mendiratta","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":3,"patchTotal":6},"messages":[{"id":"418009","messageId":"20210301084512.27170-1-charvi077@gmail.com","threadId":"55235","inReplyTo":null,"subject":"[PATCH v3 0/6][Outreachy] commit: Implementation of \"amend!\" commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:08Z","receivedAt":"2021-03-01T08:48:50Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"This patch series teaches `git commit --fixup` to create \"amend!\" commit\nas an alternative that works with `git rebase --autosquash`. It allows to\nfixup both the content and the commit message of the specified commit.\nHere we add two suboptions to the `--fixup`, first `amend` suboption that\ncreates an \"amend!\" commit. It takes the staged changes and also allows to\nedit the commit message of the commit we are fixing.\nExample usuage:\ngit commit --fixup=amend:\n\nSecondly, `reword` suboption that creates an empty \"amend!\" commit i.e it\nignores the staged changes and only allows to reword/edit the commit message\nof the commit we are fixing.\nExample usuage:\ngit commit --fixup=reword:\n\n** This work is rebased on the top of cm/rebase-i-updates.\n\nChanges from v2 :\n\n* doc/git-commit.c: reword and simplify the `--fixup=[amend|reword]`\n* error out \"foo\" in the output message when `--fixup=reword:` combined\n with pathspec i.e\n $ git commit --fixup=reword:HEAD~ -- foo\n (I am bit unsure about this as I have directly used the \"argv\" to get the\n first foo (file) passed. I am doubtful and not sure about, if we can\n use the `pathspec_from_file` in (builtin/commit.c) ? )\n* fixes the TRANSLATOR, commit message and other minor cleanup (v2 2/6)\n\nCharvi Mendiratta (6):\n sequencer: export subject_length()\n commit: add amend suboption to --fixup to create amend! commit\n commit: add a reword suboption to --fixup\n t7500: add tests for --fixup=[amend|reword] options\n t3437: use --fixup with options to create amend! commit\n doc/git-commit: add documentation for fixup=[amend|reword] options\n\n Documentation/git-commit.txt | 40 ++++++-\n Documentation/git-rebase.txt | 21 ++--\n builtin/commit.c | 125 ++++++++++++++++++--\n commit.c | 14 +++\n commit.h | 3 +\n sequencer.c | 14 ---\n t/t3437-rebase-fixup-options.sh | 30 +----\n t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++\n 8 files changed, 316 insertions(+), 66 deletions(-)\n\n--\n2.29.0.rc1\n\n"},{"id":"418010","messageId":"20210301084512.27170-3-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:10Z","receivedAt":"2021-03-01T08:49:20Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"`git commit --fixup=amend:` will create an \"amend!\" commit.\nThe resulting commit message subject will be \"amend! ...\" where\n\"...\" is the subject line of and the initial message\nbody will be 's message. -m can be used to override the\nmessage body.\n\nThe \"amend!\" commit when rebased with --autosquash will fixup the\ncontents and replace the commit message of with the\n\"amend!\" commit's message body.\n\nIn order to prevent rebase from creating commits with an empty\nmessage we refuse to create an \"amend!\" commit if commit message\nbody is empty.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: Charvi Mendiratta \n---\n builtin/commit.c | 97 +++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 88 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 505fe60956..200ef83cc0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -105,7 +105,8 @@ static const char *template_file;\n */\n static const char *author_message, *author_message_buffer;\n static char *edit_message, *use_message;\n-static char *fixup_message, *squash_message;\n+static char *fixup_message, *fixup_commit, *squash_message;\n+static const char *fixup_prefix;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n@@ -681,6 +682,21 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n \tcomment_line_char = *p;\n }\n\n+static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n+\t\t\t\t\t\t\t\t struct pretty_print_context *ctx) {\n+\t/*\n+\t * If we amend the 'amend!' commit then we don't want to\n+\t * duplicate the subject line.\n+\t */\n+\tconst char *format = NULL;\n+\tif (starts_with(sb->buf, \"amend! amend!\"))\n+\t\tformat = \"%b\";\n+\telse\n+\t\tformat = \"%B\";\n+\tformat_commit_message(commit, format, sb, ctx);\n+\treturn 0;\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct commit *current_head,\n \t\t\t struct wt_status *s,\n@@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t} else if (fixup_message) {\n \t\tstruct pretty_print_context ctx = {0};\n \t\tstruct commit *commit;\n-\t\tcommit = lookup_commit_reference_by_name(fixup_message);\n+\t\tchar *fmt = xstrfmt(\"%s! %%s\\n\\n\", fixup_prefix);\n+\t\tcommit = lookup_commit_reference_by_name(fixup_commit);\n \t\tif (!commit)\n-\t\t\tdie(_(\"could not lookup commit %s\"), fixup_message);\n+\t\t\tdie(_(\"could not lookup commit %s\"), fixup_commit);\n \t\tctx.output_encoding = get_commit_output_encoding();\n-\t\tformat_commit_message(commit, \"fixup! %s\\n\\n\",\n-\t\t\t\t &sb, &ctx);\n-\t\tif (have_option_m)\n-\t\t\tstrbuf_addbuf(&sb, &message);\n+\t\tformat_commit_message(commit, fmt, &sb, &ctx);\n+\t\tfree(fmt);\n \t\thook_arg1 = \"message\";\n+\n+\t\t/*\n+\t\t * Only `-m` commit message option is checked here, as\n+\t\t * it supports `--fixup` to append the commit message.\n+\t\t *\n+\t\t * The other commit message options `-c`/`-C`/`-F` are\n+\t\t * incompatible with all the forms of `--fixup` and\n+\t\t * have already errored out while parsing the `git commit`\n+\t\t * options.\n+\t\t */\n+\t\tif (have_option_m && !strcmp(fixup_prefix, \"fixup\"))\n+\t\t\tstrbuf_addbuf(&sb, &message);\n+\n+\t\tif (!strcmp(fixup_prefix, \"amend\")) {\n+\t\t\tif (have_option_m)\n+\t\t\t\tdie(_(\"cannot combine -m with --fixup:%s\"), fixup_message);\n+\t\t\telse\n+\t\t\t\tprepare_amend_commit(commit, &sb, &ctx);\n+\t\t}\n \t} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {\n \t\tsize_t merge_msg_start;\n\n@@ -1152,6 +1186,12 @@ static void finalize_deferred_config(struct wt_status *s)\n \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n }\n\n+/* returns the length of intial segment of alpha characters only */\n+static size_t get_alpha_len(char *fixup_message) {\n+\tconst char alphas[] = \"abcdefghijklmnopqrstuvwxyz\";\n+\treturn strspn(fixup_message, alphas);\n+}\n+\n static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\t\t const struct option *options,\n \t\t\t\t const char * const usage[],\n@@ -1170,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (force_author && renew_authorship)\n \t\tdie(_(\"Using both --reset-author and --author does not make sense\"));\n\n-\tif (logfile || have_option_m || use_message || fixup_message)\n+\tif (logfile || have_option_m || use_message)\n \t\tuse_editor = 0;\n \tif (0 <= edit_flag)\n \t\tuse_editor = edit_flag;\n@@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[],\n\n \tif (also + only + all + interactive > 1)\n \t\tdie(_(\"Only one of --include/--only/--all/--interactive/--patch can be used.\"));\n+\n+\tif (fixup_message) {\n+\t\t/*\n+\t\t * As `amend` suboption contains only alpha\n+\t\t * character. So check if first non alpha\n+\t\t * character in fixup_message is ':'.\n+\t\t */\n+\t\tsize_t len = get_alpha_len(fixup_message);\n+\t\tif (len && fixup_message[len] == ':') {\n+\t\t\tfixup_message[len++] = '\\0';\n+\t\t\tfixup_commit = fixup_message + len;\n+\t\t\tif (starts_with(\"amend\", fixup_message))\n+\t\t\t\tfixup_prefix = \"amend\";\n+\t\t\telse\n+\t\t\t\tdie(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n+\t\t} else {\n+\t\t\tfixup_commit = fixup_message;\n+\t\t\tfixup_prefix = \"fixup\";\n+\t\t\tuse_editor = 0;\n+\t\t}\n+\t}\n+\n \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n\n \thandle_untracked_files_arg(s);\n@@ -1504,7 +1566,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK('m', \"message\", &message, N_(\"message\"), N_(\"commit message\"), opt_parse_m),\n \t\tOPT_STRING('c', \"reedit-message\", &edit_message, N_(\"commit\"), N_(\"reuse and edit message from specified commit\")),\n \t\tOPT_STRING('C', \"reuse-message\", &use_message, N_(\"commit\"), N_(\"reuse message from specified commit\")),\n-\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"commit\"), N_(\"use autosquash formatted message to fixup specified commit\")),\n+\t\t/*\n+\t\t * TRANSLATORS: Leave \"[amend:]\" as-is, and\n+\t\t * only translate .\n+\t\t */\n+\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[amend:]commit\"), N_(\"use autosquash formatted message to fixup or amend specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n@@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n\n+\tif (fixup_message && starts_with(sb.buf, \"amend! \") &&\n+\t\t!allow_empty_message) {\n+\t\tstruct strbuf body = STRBUF_INIT;\n+\t\tsize_t len = subject_length(sb.buf);\n+\t\tstrbuf_addstr(&body, sb.buf + len);\n+\t\tif (message_is_empty(&body, cleanup_mode)) {\n+\t\t\trollback_index_files();\n+\t\t\tfprintf(stderr, _(\"Aborting commit due to empty commit message body.\\n\"));\n+\t\t\texit(1);\n+\t\t}\n+\t\tstrbuf_release(&body);\n+\t}\n+\n \tif (amend) {\n \t\tconst char *exclude_gpgsig[3] = { \"gpgsig\", \"gpgsig-sha256\", NULL };\n \t\textra = read_commit_extra_headers(current_head, exclude_gpgsig);\n--\n2.29.0.rc1\n\n"},{"id":"418011","messageId":"20210301084512.27170-2-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 1/6] sequencer: export subject_length()","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:09Z","receivedAt":"2021-03-01T08:49:20Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"This function can be used in other parts of git. Let's move the\nfunction to commit.c.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nSigned-off-by: Charvi Mendiratta \n---\n commit.c | 14 ++++++++++++++\n commit.h | 3 +++\n sequencer.c | 14 --------------\n 3 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex bab8d5ab07..41cc72c4de 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)\n \treturn eol - p;\n }\n \n+size_t subject_length(const char *body)\n+{\n+\tconst char *p = body;\n+\twhile (*p) {\n+\t\tconst char *next = skip_blank_lines(p);\n+\t\tif (next != p)\n+\t\t\tbreak;\n+\t\tp = strchrnul(p, '\\n');\n+\t\tif (*p)\n+\t\t\tp++;\n+\t}\n+\treturn p - body;\n+}\n+\n struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)\n {\n \tstruct commit_list *new_list = xmalloc(sizeof(struct commit_list));\ndiff --git a/commit.h b/commit.h\nindex f4e7b0158e..12ff6bc641 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\n \n+/* Return length of the commit subject from commit log message. */\n+size_t subject_length(const char *body);\n+\n struct commit_list *commit_list_insert(struct commit *item,\n \t\t\t\t\tstruct commit_list **list);\n int commit_list_contains(struct commit *item,\ndiff --git a/sequencer.c b/sequencer.c\nindex abc6d5cdfd..3fac4ba62f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1724,20 +1724,6 @@ enum todo_item_flags {\n \tTODO_EDIT_FIXUP_MSG = (1 << 2),\n };\n \n-static size_t subject_length(const char *body)\n-{\n-\tconst char *p = body;\n-\twhile (*p) {\n-\t\tconst char *next = skip_blank_lines(p);\n-\t\tif (next != p)\n-\t\t\tbreak;\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n-\t}\n-\treturn p - body;\n-}\n-\n static const char first_commit_msg_str[] = N_(\"This is the 1st commit message:\");\n static const char nth_commit_msg_fmt[] = N_(\"This is the commit message #%d:\");\n static const char skip_first_commit_msg_str[] = N_(\"The 1st commit message will be skipped:\");\n-- \n2.29.0.rc1\n\n"},{"id":"418012","messageId":"20210301084512.27170-6-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 5/6] t3437: use --fixup with options to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:13Z","receivedAt":"2021-03-01T08:50:10Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"We taught `git commit --fixup` to create \"amend!\" commit. Let's also\nupdate the tests and use it to setup the rebase tests.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nSigned-off-by: Charvi Mendiratta \n---\n t/t3437-rebase-fixup-options.sh | 30 +++---------------------------\n 1 file changed, 3 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex a5a20354e3..d0bdc7ed02 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -72,40 +72,16 @@ test_expect_success 'setup' '\n \tgit commit --fixup=HEAD -a &&\n \tgit tag B1 &&\n \ttest_tick &&\n-\tgit commit --allow-empty -F - <<-EOF &&\n-\tamend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 1\" git commit --fixup=reword:B &&\n \ttest_tick &&\n-\tgit commit --allow-empty -F - <<-EOF &&\n-\tamend! amend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\t$EMPTY\n-\tedited 2\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 2\" git commit --fixup=reword:HEAD &&\n \techo B2 >B &&\n \ttest_tick &&\n \tFAKE_COMMIT_AMEND=\"edited squash\" git commit --squash=HEAD -a &&\n \tgit tag B2 &&\n \techo B3 >B &&\n \ttest_tick &&\n-\tgit commit -a -F - <<-EOF &&\n-\tamend! amend! amend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\t$EMPTY\n-\tedited 2\n-\t$EMPTY\n-\tedited 3\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 3\" git commit -a --fixup=amend:HEAD^ &&\n \tgit tag B3 &&\n \n \tGIT_AUTHOR_NAME=\"Rebase Author\" &&\n-- \n2.29.0.rc1\n\n"},{"id":"418013","messageId":"20210301084512.27170-5-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:12Z","receivedAt":"2021-03-01T08:50:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"Mentored-by: Christian Couder \nMentored-by: Phillip Wood \nSigned-off-by: Charvi Mendiratta \n---\n t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++\n 1 file changed, 135 insertions(+)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 6d19ece05d..e62b7fbc1a 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'\n \n . ./test-lib.sh\n \n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n commit_msg_is () {\n \texpect=commit_msg_is.expect\n \tactual=commit_msg_is.actual\n@@ -279,6 +281,139 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+get_commit_msg () {\n+\trev=\"$1\" &&\n+\tgit log -1 --pretty=format:\"%B\" \"$rev\"\n+}\n+\n+test_expect_success 'commit --fixup=amend: creates amend! commit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success '--fixup=reword: does not commit staged changes' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited\" \\\n+\t\t\tgit commit --fixup=reword:HEAD~\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&\n+\ttest_cmp_rev HEAD@{1} HEAD^ &&\n+\ttest_expect_code 1 git diff --cached --exit-code &&\n+\tgit cat-file blob :foo >actual &&\n+\ttest_cmp foo actual\n+'\n+\n+test_expect_success '--fixup=reword: error out with -m option' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine -m with --fixup:reword\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -m \"reword commit message\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--fixup=amend: error out with -m option' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine -m with --fixup:amend\" >expect &&\n+\ttest_must_fail git commit --fixup=amend:HEAD~ -m \"amend commit message\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! amend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited 1\n+\n+\tedited 2\n+\tEOF\n+\techo \"reword new commit message\" >actual &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited 1\" \\\n+\t\t\tgit commit --fixup=reword:HEAD~ &&\n+\t\tFAKE_COMMIT_AMEND=\"edited 2\" \\\n+\t\t\tgit commit --fixup=reword:HEAD\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"Aborting commit due to empty commit message body.\" >expected &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_COMMIT_MESSAGE=\"amend! target message subject line\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~ 2>actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=\"amend! target message subject line\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~ --allow-empty-message &&\n+\t\tget_commit_msg HEAD >actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_fixup_reword_opt () {\n+\ttest_expect_success C_LOCALE_OUTPUT \"--fixup=reword: incompatible with $1\" \"\n+\t\techo 'fatal: cannot combine reword option of --fixup with $1' >expect &&\n+\t\ttest_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&\n+\t\ttest_cmp expect actual\n+\t\"\n+}\n+\n+for opt in --all --include --only --interactive --patch\n+do\n+\ttest_fixup_reword_opt $opt\n+done\n+\n+test_expect_success '--fixup=reword: give error with pathsec' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine reword option of --fixup with path foo\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--fixup=reword: -F give error message' '\n+\techo \"fatal: Only one of -c/-C/-F/--fixup can be used.\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'commit --squash works with -F' '\n \tcommit_for_rebase_autosquash_setup &&\n-- \n2.29.0.rc1\n\n"},{"id":"418014","messageId":"20210301084512.27170-4-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 3/6] commit: add a reword suboption to --fixup","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:11Z","receivedAt":"2021-03-01T08:50:46Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"`git commit --fixup=reword:` creates an empty \"amend!\" commit\nthat will reword without changing its contents when it is\nrebased with --autosquash.\n\nApart from ignoring staged changes it works similarly to\n`--fixup=amend:`.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: Charvi Mendiratta \n---\n builtin/commit.c | 44 ++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 36 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 200ef83cc0..a4d18d96df 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1186,6 +1186,27 @@ static void finalize_deferred_config(struct wt_status *s)\n \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n }\n\n+static void check_fixup_reword_options(int argc, const char *argv[]) {\n+\tif (whence != FROM_COMMIT) {\n+\t\tif (whence == FROM_MERGE)\n+\t\t\tdie(_(\"You are in the middle of a merge -- cannot reword.\"));\n+\t\telse if (is_from_cherry_pick(whence))\n+\t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot reword.\"));\n+\t}\n+\tif (argc)\n+\t\tdie(_(\"cannot combine reword option of --fixup with path %s\"), *argv);\n+\tif (patch_interactive)\n+\t\tdie(_(\"cannot combine reword option of --fixup with --patch\"));\n+\tif (interactive)\n+\t\tdie(_(\"cannot combine reword option of --fixup with --interactive\"));\n+\tif (all)\n+\t\tdie(_(\"cannot combine reword option of --fixup with --all\"));\n+\tif (also)\n+\t\tdie(_(\"cannot combine reword option of --fixup with --include\"));\n+\tif (only)\n+\t\tdie(_(\"cannot combine reword option of --fixup with --only\"));\n+}\n+\n /* returns the length of intial segment of alpha characters only */\n static size_t get_alpha_len(char *fixup_message) {\n \tconst char alphas[] = \"abcdefghijklmnopqrstuvwxyz\";\n@@ -1270,18 +1291,25 @@ static int parse_and_validate_options(int argc, const char *argv[],\n\n \tif (fixup_message) {\n \t\t/*\n-\t\t * As `amend` suboption contains only alpha\n-\t\t * character. So check if first non alpha\n-\t\t * character in fixup_message is ':'.\n+\t\t * As `amend`/`reword` suboptions contains only alpha\n+\t\t * characters. So check if first non alpha character\n+\t\t * in fixup_message is ':'.\n \t\t */\n \t\tsize_t len = get_alpha_len(fixup_message);\n \t\tif (len && fixup_message[len] == ':') {\n \t\t\tfixup_message[len++] = '\\0';\n \t\t\tfixup_commit = fixup_message + len;\n-\t\t\tif (starts_with(\"amend\", fixup_message))\n+\t\t\tif (starts_with(\"amend\", fixup_message) ||\n+\t\t\t starts_with(\"reword\", fixup_message)) {\n \t\t\t\tfixup_prefix = \"amend\";\n-\t\t\telse\n+\t\t\t\tif (*fixup_message == 'r') {\n+\t\t\t\t\tcheck_fixup_reword_options(argc, argv);\n+\t\t\t\t\tallow_empty = 1;\n+\t\t\t\t\tonly = 1;\n+\t\t\t\t}\n+\t\t\t} else {\n \t\t\t\tdie(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n+\t\t\t}\n \t\t} else {\n \t\t\tfixup_commit = fixup_message;\n \t\t\tfixup_prefix = \"fixup\";\n@@ -1567,10 +1595,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING('c', \"reedit-message\", &edit_message, N_(\"commit\"), N_(\"reuse and edit message from specified commit\")),\n \t\tOPT_STRING('C', \"reuse-message\", &use_message, N_(\"commit\"), N_(\"reuse message from specified commit\")),\n \t\t/*\n-\t\t * TRANSLATORS: Leave \"[amend:]\" as-is, and\n-\t\t * only translate .\n+\t\t * TRANSLATORS: Leave \"[(amend|reword):]\" as-is,\n+\t\t * and only translate .\n \t\t */\n-\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[amend:]commit\"), N_(\"use autosquash formatted message to fixup or amend specified commit\")),\n+\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n--\n2.29.0.rc1\n\n"},{"id":"418015","messageId":"20210301084512.27170-7-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-01T08:45:14Z","receivedAt":"2021-03-01T08:51:13Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"Mentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: Charvi Mendiratta \n---\n Documentation/git-commit.txt | 40 ++++++++++++++++++++++++++++++------\n Documentation/git-rebase.txt | 21 ++++++++++---------\n 2 files changed, 45 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 17150fa7ea..722f8f0a6d 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n-\t [--dry-run] [(-c | -C | --fixup | --squash) ]\n+\t [--dry-run] [(-c | -C | --squash) | --fixup [(amend|reword):])]\n \t [-F | -m ] [--reset-author] [--allow-empty]\n \t [--allow-empty-message] [--no-verify] [-e] [--author=]\n \t [--date=] [--cleanup=] [--[no-]status]\n@@ -86,11 +86,39 @@ OPTIONS\n \tLike '-C', but with `-c` the editor is invoked, so that\n \tthe user can further edit the commit message.\n \n---fixup=::\n-\tConstruct a commit message for use with `rebase --autosquash`.\n-\tThe commit message will be the subject line from the specified\n-\tcommit with a prefix of \"fixup! \". See linkgit:git-rebase[1]\n-\tfor details.\n+--fixup=[(amend|reword):]::\n+\tWithout `amend:` or `reword:`, create a `fixup!` commit where\n+\tthe commit message will be the subject line from the specified\n+\tcommit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n+\tis further used with `git rebase --autosquash` to fixup the\n+\tcontent of the specified commit.\n++\n+The `--fixup=amend:` form creates an \"amend!\" commit to\n+fixup both the content and the commit log message of the specified\n+commit. The resulting \"amend!\" commit's commit message subject\n+will be the subject line from the specified commit with a prefix of\n+\"amend!'\" and the message body will be commit log message of the\n+specified commit. It also invokes an editor seeded with the log\n+message of the \"amend!\" commit to allow to edit further. And it\n+refuses to create \"amend!\" commit if it's commit message body is\n+empty unless used with the `--allow-empty-message` option. \"amend!\"\n+commit when rebased with `--autosquash` will fixup the contents and\n+replace the commit message of the specified commit with the \"amend!\"\n+commit's message body.\n++\n+The `--fixup=reword:` form creates an \"amend!\" commit similar\n+to `--fixup=amend:` creates, but it records the same tree as\n+`HEAD`, i.e. it does not take any staged changes and only allows to\n+fixup the commit message of the specified commit. It will reword the\n+specified commit when it is rebased with `--autosquash`.\n++\n+The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with\n+other options to add to the commit log message i.e it is incompatible\n+with `-m`/`-F`/`-c`/`-C` options.\n++\n+Also, after fixing the commit using `--fixup`, with or without option\n+and rebased with `--autosquash`, the authorship of the original commit\n+remains unchanged. See linkgit:git-rebase[1] for details.\n \n --squash=::\n \tConstruct a commit message for use with `rebase --autosquash`.\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8bfa5a9272..ffea76e53b 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.\n \n --autosquash::\n --no-autosquash::\n-\tWhen the commit log message begins with \"squash! ...\" (or\n-\t\"fixup! ...\"), and there is already a commit in the todo list that\n-\tmatches the same `...`, automatically modify the todo list of rebase\n-\t-i so that the commit marked for squashing comes right after the\n-\tcommit to be modified, and change the action of the moved commit\n-\tfrom `pick` to `squash` (or `fixup`). A commit matches the `...` if\n-\tthe commit subject matches, or if the `...` refers to the commit's\n-\thash. As a fall-back, partial matches of the commit subject work,\n-\ttoo. The recommended way to create fixup/squash commits is by using\n-\tthe `--fixup`/`--squash` options of linkgit:git-commit[1].\n+\tWhen the commit log message begins with \"squash! ...\" (or \"fixup! ...\"\n+\tor \"amend! ...\"), and there is already a commit in the todo list that\n+\tmatches the same `...`, automatically modify the todo list of\n+\t`rebase -i`, so that the commit marked for squashing comes right after\n+\tthe commit to be modified, and change the action of the moved commit\n+\tfrom `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit\n+\tmatches the `...` if the commit subject matches, or if the `...` refers\n+\tto the commit's hash. As a fall-back, partial matches of the commit\n+\tsubject work, too. The recommended way to create fixup/squash/amend\n+\tcommits is by using the `--fixup=[amend|reword]`/`--squash` options of\n+\tlinkgit:git-commit[1].\n +\n If the `--autosquash` option is enabled by default using the\n configuration variable `rebase.autoSquash`, this option can be\n-- \n2.29.0.rc1\n\n"},{"id":"418053","messageId":"xmqqlfb6radq.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"20210301084512.27170-3-charvi077@gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T18:34:25Z","receivedAt":"2021-03-01T18:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n> +\t\t\t\t\t\t\t\t struct pretty_print_context *ctx) {\n\nWhy does this need to be overly indented? Are you using some funny\ntab width settings? In this project, a tab stop is 8-spaces wide.\n\n> +\t\t/*\n> +\t\t * Only `-m` commit message option is checked here, as\n> +\t\t * it supports `--fixup` to append the commit message.\n\nAs it is OK to use \"-m\" with the plain vanilla \"--fixup\", an earlier\ncheck did not reject the combination, but now we look at what kind\nof fixup it is, and error out if it is \"--fixup=amend:\". OK.\n\n> +\t\t * The other commit message options `-c`/`-C`/`-F` are\n> +\t\t * incompatible with all the forms of `--fixup` and\n> +\t\t * have already errored out while parsing the `git commit`\n> +\t\t * options.\n> +\t\t */\n\n\n"},{"id":"418054","messageId":"xmqqh7lura1j.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"20210301084512.27170-4-charvi077@gmail.com","subject":"Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T18:41:44Z","receivedAt":"2021-03-01T18:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> `git commit --fixup=reword:` creates an empty \"amend!\" commit\n> that will reword without changing its contents when it is\n> rebased with --autosquash.\n>\n> Apart from ignoring staged changes it works similarly to\n> `--fixup=amend:`.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Phillip Wood \n> Helped-by: Junio C Hamano \n> Signed-off-by: Charvi Mendiratta \n> ---\n> builtin/commit.c | 44 ++++++++++++++++++++++++++++++++++++--------\n> 1 file changed, 36 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 200ef83cc0..a4d18d96df 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1186,6 +1186,27 @@ static void finalize_deferred_config(struct wt_status *s)\n> \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n> }\n>\n> +static void check_fixup_reword_options(int argc, const char *argv[]) {\n> +\tif (whence != FROM_COMMIT) {\n> +\t\tif (whence == FROM_MERGE)\n> +\t\t\tdie(_(\"You are in the middle of a merge -- cannot reword.\"));\n> +\t\telse if (is_from_cherry_pick(whence))\n> +\t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot reword.\"));\n> +\t}\n> +\tif (argc)\n> +\t\tdie(_(\"cannot combine reword option of --fixup with path %s\"), *argv);\n\nI think our convention is to quote '%s' with a single-quote pair.\nSee other error messages.\n\ncommit.c:\t\t\tdie_errno(_(\"could not read '%s'\"), templat...\ncommit.c:\t\tdie_errno(_(\"could not open '%s'\"), git_path_commit...\ncommit.c:\tdie(_(\"--author '%s' is not 'Name ' and matches no e...\ncommit.c:\t\tdie(_(\"Invalid ignored mode '%s'\"), ignored_arg);\ncommit.c:\t\tdie(_(\"Invalid untracked files mode '%s'\"), untrack...\n...\n"},{"id":"418055","messageId":"xmqqczwir9wx.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"20210301084512.27170-7-charvi077@gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T18:44:30Z","receivedAt":"2021-03-01T18:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> +--fixup=[(amend|reword):]::\n> +\tWithout `amend:` or `reword:`, create a `fixup!` commit where\n> +\tthe commit message will be the subject line from the specified\n> +\tcommit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n\nWhat's the single quote in \"fixup!'\"??? We also have an \"amend!'\"\nbelow.\n\n> +\tis further used with `git rebase --autosquash` to fixup the\n> +\tcontent of the specified commit.\n> ++\n> +The `--fixup=amend:` form creates an \"amend!\" commit to\n> +fixup both the content and the commit log message of the specified\n> +commit. The resulting \"amend!\" commit's commit message subject\n> +will be the subject line from the specified commit with a prefix of\n> +\"amend!'\" and the message body will be commit log message of the\n> +specified commit. It also invokes an editor seeded with the log\n> ...\n> +The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with\n> +other options to add to the commit log message i.e it is incompatible\n\n\"i.e.\"\n\n> +with `-m`/`-F`/`-c`/`-C` options.\n> ++\n"},{"id":"418056","messageId":"xmqq8s76r9v1.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"Re: [PATCH v3 0/6][Outreachy] commit: Implementation of \"amend!\" commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T18:45:38Z","receivedAt":"2021-03-01T18:50:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> This patch series teaches `git commit --fixup` to create \"amend!\" commit\n> as an alternative that works with `git rebase --autosquash`. It allows to\n> fixup both the content and the commit message of the specified commit.\n> Here we add two suboptions to the `--fixup`, first `amend` suboption that\n> creates an \"amend!\" commit. It takes the staged changes and also allows to\n> edit the commit message of the commit we are fixing.\n> Example usuage:\n> git commit --fixup=amend:\n\nI only had a few minor nits and I think with them fixed the series\nbecomes as good as it would get in the shorter term to be ready for\n'next'. We're almost there.\n\nThanks.\n\n"},{"id":"418067","messageId":"CAPig+cSaZ+i+2P0x67BiHLxAGZaggNFK=dHxLJkmOfY8uafS5g@mail.gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-2-charvi077@gmail.com","subject":"Re: [PATCH v3 1/6] sequencer: export subject_length()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-01T20:25:23Z","receivedAt":"2021-03-01T20:29:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n> This function can be used in other parts of git. Let's move the\n> function to commit.c.\n>\n> Signed-off-by: Charvi Mendiratta \n> ---\n> diff --git a/commit.h b/commit.h\n> @@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n> +/* Return length of the commit subject from commit log message. */\n> +size_t subject_length(const char *body);\n\nNow that this function is public, is the name too generic? Most other\nfunctions in this header have \"commit\" in the name. So,\ncommit_subject_length() might be one possibility (assuming the current\nname is too generic).\n"},{"id":"418081","messageId":"CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-3-charvi077@gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-01T22:15:05Z","receivedAt":"2021-03-01T22:21:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n> `git commit --fixup=amend:` will create an \"amend!\" commit.\n> The resulting commit message subject will be \"amend! ...\" where\n> \"...\" is the subject line of and the initial message\n> body will be 's message. -m can be used to override the\n> message body.\n>\n> The \"amend!\" commit when rebased with --autosquash will fixup the\n> contents and replace the commit message of with the\n> \"amend!\" commit's message body.\n>\n> In order to prevent rebase from creating commits with an empty\n> message we refuse to create an \"amend!\" commit if commit message\n> body is empty.\n>\n> Signed-off-by: Charvi Mendiratta \n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -105,7 +105,8 @@ static const char *template_file;\n> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n> + struct pretty_print_context *ctx) {\n> + /*\n> + * If we amend the 'amend!' commit then we don't want to\n> + * duplicate the subject line.\n> + */\n> + const char *format = NULL;\n> + if (starts_with(sb->buf, \"amend! amend!\"))\n\nIs the content of the incoming strbuf created mechanically so that we\nknow that there will only ever be one space between the two \"amend!\"\nliterals? If not, then this starts_with() check feels fragile.\n(Compare with the code in sequencer.c which checks for this sort of\nduplication but is tolerant of one or more spaces, not just a single\nspace.)\n\n> + format = \"%b\";\n> + else\n> + format = \"%B\";\n\nIt's subjective and minor, but this could be expressed more compactly as:\n\n const char *fmt = starts_with(...) ? \"%b\" : \"%B\";\n\nAlso, no need to initialize `format` to NULL since it gets assigned in\nall code paths.\n\nNot worth a re-roll.\n\n> @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> + if (!strcmp(fixup_prefix, \"amend\")) {\n> + if (have_option_m)\n> + die(_(\"cannot combine -m with --fixup:%s\"), fixup_message);\n> + else\n> + prepare_amend_commit(commit, &sb, &ctx);\n> + }\n\nThis is minor, but the way this is written, the error case and the\nnormal case appear to have the same significance, whereas, if you\nwrite it like this:\n\n if (!strcmp(...)) {\n if (have_option_m)\n die(...);\n prepare_amend_commit(...);\n }\n\nthen it's easier to see that you're checking for and getting an error\ncase out of the way early, which allows the reader to concentrate\nwithout distraction on the normal case. As a minor benefit, you also\nget to eliminate an indentation level for the normal case, which could\nbe important if more code is added to that case.\n\nNot worth a re-roll.\n\n> @@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> + if (fixup_message) {\n> + /*\n> + * As `amend` suboption contains only alpha\n> + * character. So check if first non alpha\n> + * character in fixup_message is ':'.\n> + */\n> + size_t len = get_alpha_len(fixup_message);\n> + if (len && fixup_message[len] == ':') {\n> + fixup_message[len++] = '\\0';\n> + fixup_commit = fixup_message + len;\n> + if (starts_with(\"amend\", fixup_message))\n> + fixup_prefix = \"amend\";\n> + else\n> + die(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n\nI haven't read ahead in the series yet, but I presume you're making\nthis code extra generic because you plan to support additional `fixup`\noptions (such as `reword`), but I wonder if the cognitive overhead is\nwarranted or you could get by with something simpler, such as:\n\n if (skip_prefix(msg, \"amend:\", &arg) ||\n skip_prefix(msg, \"reword:\", &arg)) {\n ...\n }\n\nAlso, am I misreading when I think that the use of starts_with() could\nbe replaced with a simple strcmp() since you've already inserted a\n'\\0' immediately after the final alphabetic character?\n\nNot necessarily worth a re-roll.\n\n> @@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> + if (message_is_empty(&body, cleanup_mode)) {\n> + rollback_index_files();\n> + fprintf(stderr, _(\"Aborting commit due to empty commit message body.\\n\"));\n> + exit(1);\n\nI was wondering why you are capitalizing the error message (these days\nwe don't) and using exit() instead of die(), but I see that you're\nmirroring existing practice in this function. Okay.\n"},{"id":"418085","messageId":"xmqqlfb6mrnx.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T22:32:18Z","receivedAt":"2021-03-01T23:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + if (len && fixup_message[len] == ':') {\n>> + fixup_message[len++] = '\\0';\n>> + fixup_commit = fixup_message + len;\n>> + if (starts_with(\"amend\", fixup_message))\n>> + fixup_prefix = \"amend\";\n>> + else\n>> + die(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n>\n> I haven't read ahead in the series yet, but I presume you're making\n> this code extra generic because you plan to support additional `fixup`\n> options (such as `reword`), but I wonder if the cognitive overhead is\n> warranted or you could get by with something simpler, such as:\n>\n> if (skip_prefix(msg, \"amend:\", &arg) ||\n> skip_prefix(msg, \"reword:\", &arg)) {\n> ...\n> }\n\nYou still need to compute \"len\" because you'd want to tell between\n--fixup=\"HEAD^{/^area: string}\" and --fixup=bogus:HEAD (the latter\nwould want to say \"no such variant 'bogus' for --fixup\", but the\ncolon in the former is not the end of the name of variant.\n\nSo, skip_prefix() would not buy us much, I guess.\n\nBut the use of starts_with() in the original patch is bogus, I\nthink. fixup_message[] by the time the comparison is made is\nNULL terminated at where the colon was originally, so we should be\ndoing !strcmp() to reject \"--fixup=amendo:HEAD~2\" with \"no, 'amendo'\nis not a valid variant name for --fixup option\".\n\n> Also, am I misreading when I think that the use of starts_with() could\n> be replaced with a simple strcmp() since you've already inserted a\n> '\\0' immediately after the final alphabetic character?\n\nCorrect.\n\n"},{"id":"418086","messageId":"CAPig+cQR2ibdc1CQdwWKdk=mZM4FyaMsfw2Mpgk7Rdc-1R4PRQ@mail.gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-4-charvi077@gmail.com","subject":"Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-01T22:36:44Z","receivedAt":"2021-03-01T23:34:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n> `git commit --fixup=reword:` creates an empty \"amend!\" commit\n> that will reword without changing its contents when it is\n> rebased with --autosquash.\n>\n> Apart from ignoring staged changes it works similarly to\n> `--fixup=amend:`.\n>\n> Signed-off-by: Charvi Mendiratta \n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1186,6 +1186,27 @@ static void finalize_deferred_config(struct wt_status *s)\n> +static void check_fixup_reword_options(int argc, const char *argv[]) {\n> + if (whence != FROM_COMMIT) {\n> + if (whence == FROM_MERGE)\n> + die(_(\"You are in the middle of a merge -- cannot reword.\"));\n> + else if (is_from_cherry_pick(whence))\n> + die(_(\"You are in the middle of a cherry-pick -- cannot reword.\"));\n> + }\n> + if (argc)\n> + die(_(\"cannot combine reword option of --fixup with path %s\"), *argv);\n> + if (patch_interactive)\n> + die(_(\"cannot combine reword option of --fixup with --patch\"));\n> + if (interactive)\n> + die(_(\"cannot combine reword option of --fixup with --interactive\"));\n> + if (all)\n> + die(_(\"cannot combine reword option of --fixup with --all\"));\n> + if (also)\n> + die(_(\"cannot combine reword option of --fixup with --include\"));\n> + if (only)\n> + die(_(\"cannot combine reword option of --fixup with --only\"));\n> +}\n\nOr, more concisely:\n\n if (argc)\n die(_(\"--fixup mutually exclusive with path '%s'), ...);\n if (patch_interactive || interactive || all || also || only)\n die(_(\"--fixup mutually exclusive with\n--patch/--interactive/--all/--include/--only);\n\nThe mix of two different error message styles (capitalized with\nfull-stop vs. lowercase no-full-stop) is a bit jarring, but minor.\n\nNot worth re-roll.\n"},{"id":"418090","messageId":"CAPig+cQ9wBLdKqmgjX4eOT6mvukmHXCDicTo5Ud+4VTkzxcXrg@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqlfb6mrnx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-01T22:47:38Z","receivedAt":"2021-03-01T23:35:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > if (skip_prefix(msg, \"amend:\", &arg) ||\n> > skip_prefix(msg, \"reword:\", &arg)) {\n> > ...\n> > }\n>\n> You still need to compute \"len\" because you'd want to tell between\n> --fixup=\"HEAD^{/^area: string}\" and --fixup=bogus:HEAD (the latter\n> would want to say \"no such variant 'bogus' for --fixup\", but the\n> colon in the former is not the end of the name of variant.\n\nI see what you mean. I vaguely recall quickly scanning over that\nearlier discussion about \":\" being otherwise legitimate when embedded\nin the argument as you demonstrate, but didn't think about it when\nreading this code. Perhaps the comment which this code adds:\n\n * As `amend` suboption contains only alpha\n * character. So check if first non alpha\n * character in fixup_message is ':'.\n\ncould be extended a bit to mention that briefly since, without it, the\nsignificance of \"alpha-only characters followed by colon\" is not\nimmediately obvious.\n"},{"id":"418098","messageId":"CAPig+cTVrcWm8pJvnkP4gnWE6B8SKHENjvbAR7Do0ury-ArnaA@mail.gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-5-charvi077@gmail.com","subject":"Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-02T05:43:10Z","receivedAt":"2021-03-02T15:28:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n> t7500: add tests for --fixup=[amend|reword] options\n\nIt's usually preferable for tests and documentation updates to be\nbundled along with the patch which makes a particular change[1] rather\nthan waiting until the very end of the series and adding tests and\ndocumentation covering all the changes made by patches earlier in the\nseries. As a reviewer, it is much harder to tell if the late-added\ntests and documentation updates are comprehensive since it's difficult\nto keep in mind all the changes made by earlier patches.\n\nWhen reading earlier patches in this series, I questioned whether or\nnot certain features of each patch were going to be covered by tests\nor documentation updates, but I couldn't tell because those updates\nweren't made at the same time as the change about which I was reading.\nFor instance, when reading the implementation of `--fixup:reword`, I\nwas wondering if the documentation was going to be updated to mention\nthat it would ignore changes staged in the index and leave the index\nuntouched, and I wondered if and hoped that tests would be added to\nverify that the index was indeed left untouched. Over the course of\nmany patches, it can be difficult to keep track of all the accumulated\nquestions, which makes it onerous to review the final patches adding\nthe tests and documentation updates enmasse.\n\nI'm not necessarily suggesting that you re-roll merely to incorporate\nthe tests and documentation updates into the patches to which they\nbelong, but it's something to keep in mind for future submissions.\n\nFOOTNOTES\n\n[1]: Once in a while a patch introducing a change is so large on its\nown that it may make sense to split tests and documentation updates\nout to their own patches which immediately follow the patch to which\nthey apply, but that's different from delaying _all_ tests and\ndocumentation updates and plopping them at the end of the series all\ncrammed together.\n"},{"id":"418102","messageId":"CAPig+cRvwvT7QrO0-aLZX-2vsBPJSq6WO-O7g5A0OjDMNAYmCQ@mail.gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-7-charvi077@gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-02T06:39:01Z","receivedAt":"2021-03-02T15:28:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta wrote:\n> Signed-off-by: Charvi Mendiratta \n> ---\n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> @@ -9,7 +9,7 @@ SYNOPSIS\n> + [--dry-run] [(-c | -C | --squash) | --fixup [(amend|reword):])]\n> @@ -86,11 +86,39 @@ OPTIONS\n> ---fixup=::\n> +--fixup=[(amend|reword):]::\n\nAlthough technically correct, I can't help but wonder if we can be\nmore friendly to readers by rephrasing this as:\n\n --fixup=::\n --fixup=amend:::\n --fixup=reword:::\n\nwhich is probably a lot easier to take in and understand at a glance.\nSame comment applies to the synopsis.\n\nNot necessarily worth a re-roll.\n\n> + Without `amend:` or `reword:`, create a `fixup!` commit where\n> + the commit message will be the subject line from the specified\n> + commit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n> + is further used with `git rebase --autosquash` to fixup the\n> + content of the specified commit.\n\nI think it becomes important at this point to make it more clear that\n_only_ the content of gets changed by the \"fixup!\" commit,\nand that the log message of is untouched.\n\n> +The `--fixup=amend:` form creates an \"amend!\" commit to\n> +fixup both the content and the commit log message of the specified\n> +commit. The resulting \"amend!\" commit's commit message subject\n> +will be the subject line from the specified commit with a prefix of\n> +\"amend!'\" and the message body will be commit log message of the\n> +specified commit. It also invokes an editor seeded with the log\n> +message of the \"amend!\" commit to allow to edit further. And it\n> +refuses to create \"amend!\" commit if it's commit message body is\n> +empty unless used with the `--allow-empty-message` option. \"amend!\"\n> +commit when rebased with `--autosquash` will fixup the contents and\n> +replace the commit message of the specified commit with the \"amend!\"\n> +commit's message body.\n\nI had to read this several times to understand what it is trying to\nsay. I believe that part of the problem is that the bulk of the\ndescription goes into great detail describing bits and behaviors which\nmake no sense without understanding what an \"amend!\" commit actually\ndoes, which isn't explained until the very last sentence. So, I think\nthe entire description needs to be flipped on its head. In particular,\nit should start by saying \"create a new commit which both fixes up the\ncontent of and replaces 's log message\", and only\nthen dive into the details.\n\nIn fact, what I just wrote suggests a larger problem with the\ndescription of `--fixup` overall. There is no high-level explanation\nof what a \"fixup\" (or \"amend\" or \"reword\") is; it just dives right\ninto the minutiae without providing the reader with sufficient context\nto understand any of it. Only a reader who is already familiar with\ninteractive rebase is likely to grok what is being said here. So,\nextending the thought I expressed above, it would be helpful for the\ndescription of `--fixup=[amend:|reword:]` to start by first explaining\nwhat a \"fixup\" is, followed by simple descriptions of \"amend\" and\n\"reword\" (building upon \"fixup\"), and followed finally by details of\neach. Very roughly, something like this:\n\n Creates a new commit which \"fixes up\" when applied with\n `git rebase --autosquash`.\n\n A \"fixup\" commit changes the content of but leaves its\n log message untouched.\n\n An \"amend\" commit is like \"fixup\" but also replaces the log\n message of with the log message of the \"amend\" commit.\n\n A \"reword\" commit replaces the log message of with its\n own log message but makes no changes to the content.\n\nAnd then dive into the details of each variation.\n\n> +The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with\n> +other options to add to the commit log message i.e it is incompatible\n> +with `-m`/`-F`/`-c`/`-C` options.\n\nI suppose it doesn't hurt, but I wonder if it's really necessary to\ndocument this considering that the user will learn soon enough upon\ntrying invalid combinations.\n\n> +Also, after fixing the commit using `--fixup`, with or without option\n> +and rebased with `--autosquash`, the authorship of the original commit\n> +remains unchanged. See linkgit:git-rebase[1] for details.\n\nGood.\n\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> @@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.\n> --autosquash::\n> --no-autosquash::\n> + When the commit log message begins with \"squash! ...\" (or \"fixup! ...\"\n> + or \"amend! ...\"), and there is already a commit in the todo list that\n\nShould this also be mentioning `reword!`?\n\n> + matches the same `...`, automatically modify the todo list of\n> + `rebase -i`, so that the commit marked for squashing comes right after\n> + the commit to be modified, and change the action of the moved commit\n> + from `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit\n\nIt's becoming difficult to know which of the \"foo!\" prefixes get\ntransformed into which sequencer command since there is no longer a\none-to-one correspondence between \"foo!\" prefixes and sequencer\ncommands as there was when only \"squash!\" and \"fixup!\" existed. The\nreader should be told what sequencer command(s) \"amend!\" and \"reword!\"\nbecome.\n\n> + matches the `...` if the commit subject matches, or if the `...` refers\n> + to the commit's hash. As a fall-back, partial matches of the commit\n> + subject work, too. The recommended way to create fixup/squash/amend\n> + commits is by using the `--fixup=[amend|reword]`/`--squash` options of\n> + linkgit:git-commit[1].\n\nAt this point, it may be beneficial to write these out long-form to\nmake it easier on the reader; something along the lines of:\n\n ... the `--fixup`, `--fixup:amend:`, `--fixup:reword:`, and\n `--squash` options of ...\n"},{"id":"418126","messageId":"xmqqv9a8lpm7.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPig+cSaZ+i+2P0x67BiHLxAGZaggNFK=dHxLJkmOfY8uafS5g@mail.gmail.com","subject":"Re: [PATCH v3 1/6] sequencer: export subject_length()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-03T06:26:24Z","receivedAt":"2021-03-03T06:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n>> This function can be used in other parts of git. Let's move the\n>> function to commit.c.\n>>\n>> Signed-off-by: Charvi Mendiratta \n>> ---\n>> diff --git a/commit.h b/commit.h\n>> @@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n>> +/* Return length of the commit subject from commit log message. */\n>> +size_t subject_length(const char *body);\n>\n> Now that this function is public, is the name too generic? Most other\n> functions in this header have \"commit\" in the name. So,\n> commit_subject_length() might be one possibility (assuming the current\n> name is too generic).\n\nThanks for being a careful reviewer, as always.\n"},{"id":"418128","messageId":"xmqqr1kwlpim.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPig+cTVrcWm8pJvnkP4gnWE6B8SKHENjvbAR7Do0ury-ArnaA@mail.gmail.com","subject":"Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-03T06:28:33Z","receivedAt":"2021-03-03T06:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n>> t7500: add tests for --fixup=[amend|reword] options\n>\n> It's usually preferable for tests and documentation updates to be\n> bundled along with the patch which makes a particular change[1] rather\n> than waiting until the very end of the series and adding tests and\n> documentation covering all the changes made by patches earlier in the\n> series. As a reviewer, it is much harder to tell if the late-added\n> tests and documentation updates are comprehensive since it's difficult\n> to keep in mind all the changes made by earlier patches.\n>\n> When reading earlier patches in this series, I questioned whether or\n> not certain features of each patch were going to be covered by tests\n> or documentation updates, but I couldn't tell because those updates\n> weren't made at the same time as the change about which I was reading.\n> For instance, when reading the implementation of `--fixup:reword`, I\n> was wondering if the documentation was going to be updated to mention\n> that it would ignore changes staged in the index and leave the index\n> untouched, and I wondered if and hoped that tests would be added to\n> verify that the index was indeed left untouched. Over the course of\n> many patches, it can be difficult to keep track of all the accumulated\n> questions, which makes it onerous to review the final patches adding\n> the tests and documentation updates enmasse.\n>\n> I'm not necessarily suggesting that you re-roll merely to incorporate\n> the tests and documentation updates into the patches to which they\n> belong, but it's something to keep in mind for future submissions.\n>\n> FOOTNOTES\n>\n> [1]: Once in a while a patch introducing a change is so large on its\n> own that it may make sense to split tests and documentation updates\n> out to their own patches which immediately follow the patch to which\n> they apply, but that's different from delaying _all_ tests and\n> documentation updates and plopping them at the end of the series all\n> crammed together.\n\nA good piece of advice. Thanks.\n\n"},{"id":"418133","messageId":"CAPSFM5ds3743XcBQg2k6TkQKFhxyefAEG_+RMM5OAA+0odyyAA@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqlfb6radq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:32:47Z","receivedAt":"2021-03-04T00:22:40Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 00:04, Junio C Hamano wrote:\n>\n> Charvi Mendiratta writes:\n>\n> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n> > + struct pretty_print_context *ctx) {\n>\n> Why does this need to be overly indented? Are you using some funny\n> tab width settings? In this project, a tab stop is 8-spaces wide.\n>\n\nOops, I myself didn't expect it after cross-check in the vscode\neditor. I will fix this.\n"},{"id":"418136","messageId":"CAPSFM5cgwX+NfsJ7Rib9xk0RGhTo4C35s2vTRG_Qxny=mwPk0Q@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqh7lura1j.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:33:02Z","receivedAt":"2021-03-04T00:22:41Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 00:11, Junio C Hamano wrote:\n>\n[...]\n> > + if (argc)\n> > + die(_(\"cannot combine reword option of --fixup with path %s\"), *argv);\n>\n> I think our convention is to quote '%s' with a single-quote pair.\n> See other error messages.\n>\n> commit.c: die_errno(_(\"could not read '%s'\"), templat...\n> commit.c: die_errno(_(\"could not open '%s'\"), git_path_commit...\n> commit.c: die(_(\"--author '%s' is not 'Name ' and matches no e...\n> commit.c: die(_(\"Invalid ignored mode '%s'\"), ignored_arg);\n> commit.c: die(_(\"Invalid untracked files mode '%s'\"), untrack...\n> ...\n\nI admit I forgot to add it. I will fix it.\n"},{"id":"418137","messageId":"CAPSFM5eskkpwB0gN-nr3KBeH31A+K3LRh50daHZ-xhR9EaQpug@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cTVrcWm8pJvnkP4gnWE6B8SKHENjvbAR7Do0ury-ArnaA@mail.gmail.com","subject":"Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:43:04Z","receivedAt":"2021-03-04T00:22:43Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 11:13, Eric Sunshine wrote:\n>\n> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta wrote:\n> > t7500: add tests for --fixup=[amend|reword] options\n>\n> It's usually preferable for tests and documentation updates to be\n> bundled along with the patch which makes a particular change[1] rather\n> than waiting until the very end of the series and adding tests and\n> documentation covering all the changes made by patches earlier in the\n> series. As a reviewer, it is much harder to tell if the late-added\n> tests and documentation updates are comprehensive since it's difficult\n> to keep in mind all the changes made by earlier patches.\n>\n> When reading earlier patches in this series, I questioned whether or\n> not certain features of each patch were going to be covered by tests\n> or documentation updates, but I couldn't tell because those updates\n> weren't made at the same time as the change about which I was reading.\n> For instance, when reading the implementation of `--fixup:reword`, I\n> was wondering if the documentation was going to be updated to mention\n> that it would ignore changes staged in the index and leave the index\n> untouched, and I wondered if and hoped that tests would be added to\n> verify that the index was indeed left untouched. Over the course of\n> many patches, it can be difficult to keep track of all the accumulated\n> questions, which makes it onerous to review the final patches adding\n> the tests and documentation updates enmasse.\n>\n> I'm not necessarily suggesting that you re-roll merely to incorporate\n> the tests and documentation updates into the patches to which they\n> belong, but it's something to keep in mind for future submissions.\n>\n> FOOTNOTES\n>\n> [1]: Once in a while a patch introducing a change is so large on its\n> own that it may make sense to split tests and documentation updates\n> out to their own patches which immediately follow the patch to which\n> they apply, but that's different from delaying _all_ tests and\n> documentation updates and plopping them at the end of the series all\n> crammed together.\n\nNoted. Thanks for guiding, I will take care of it.\n"},{"id":"418138","messageId":"CAPSFM5feuZ99qM1M9sxeX3hvbvcu3OXHhcQWhJZc1_PZJHH8tQ@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqczwir9wx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:33:15Z","receivedAt":"2021-03-04T00:22:46Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 00:14, Junio C Hamano wrote:\n>\n> Charvi Mendiratta writes:\n>\n> > +--fixup=[(amend|reword):]::\n> > + Without `amend:` or `reword:`, create a `fixup!` commit where\n> > + the commit message will be the subject line from the specified\n> > + commit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n>\n> What's the single quote in \"fixup!'\"??? We also have an \"amend!'\"\n> below.\n\nI think I mistook it from the previous discussion and used a single\nquote instead of space, as earlier it was \"fixup \" and \"amend \" which\nwas technically correct. But I will fix it and replace it with\n\"fixup!\" and \"amend!\".\n"},{"id":"418140","messageId":"CAPSFM5c3xHRYVxgwVSh888yBxp01PvsEnRsxOjsJtXzjgeJ7Ng@mail.gmail.com","threadId":"55235","inReplyTo":"xmqq8s76r9v1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 0/6][Outreachy] commit: Implementation of \"amend!\" commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:33:51Z","receivedAt":"2021-03-04T00:22:54Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"> I only had a few minor nits and I think with them fixed the series\n> becomes as good as it would get in the shorter term to be ready for\n> 'next'. We're almost there.\n>\n\nThanks for pointing out the mistakes, I will fix them and update in\nthe next version.\n\nThanks and Regards,\nCharvi\n"},{"id":"418145","messageId":"CAPSFM5eosfQofhZjPz_TbchrSqj07EnQPKU0MFvgG=LtsiKVPg@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cQ9wBLdKqmgjX4eOT6mvukmHXCDicTo5Ud+4VTkzxcXrg@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:42:15Z","receivedAt":"2021-03-04T00:22:57Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 04:17, Eric Sunshine wrote:\n>\n> On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> > > if (skip_prefix(msg, \"amend:\", &arg) ||\n> > > skip_prefix(msg, \"reword:\", &arg)) {\n> > > ...\n> > > }\n> >\n> > You still need to compute \"len\" because you'd want to tell between\n> > --fixup=\"HEAD^{/^area: string}\" and --fixup=bogus:HEAD (the latter\n> > would want to say \"no such variant 'bogus' for --fixup\", but the\n> > colon in the former is not the end of the name of variant.\n>\n> I see what you mean. I vaguely recall quickly scanning over that\n> earlier discussion about \":\" being otherwise legitimate when embedded\n> in the argument as you demonstrate, but didn't think about it when\n> reading this code. Perhaps the comment which this code adds:\n>\n> * As `amend` suboption contains only alpha\n> * character. So check if first non alpha\n> * character in fixup_message is ':'.\n>\n> could be extended a bit to mention that briefly since, without it, the\n> significance of \"alpha-only characters followed by colon\" is not\n> immediately obvious.\n\nOkay, I will reword it and make it more clear regarding the main\nreason for choosing this way.\n"},{"id":"418151","messageId":"CAPig+cSJCSLBDumaXvnkdSBTATs62h9EfgtgJu0O7R_PJEumCg@mail.gmail.com","threadId":"55235","inReplyTo":"CAPSFM5dm7vD8pszCG+JKNb7Hu6Zp48wKXaj9ayxe6yaf6swePA@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-03T07:46:17Z","receivedAt":"2021-03-04T00:23:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta wrote:\n> On Tue, 2 Mar 2021 at 03:45, Eric Sunshine wrote:\n> > > + if (starts_with(sb->buf, \"amend! amend!\"))\n> >\n> > Is the content of the incoming strbuf created mechanically so that we\n> > know that there will only ever be one space between the two \"amend!\"\n> > literals? If not, then this starts_with() check feels fragile.\n>\n> Yes, so for preparing each \"amend!\" commit we add prefix \"amend! '' to\n> the subject of the specific commit. And further if we amend the\n> \"amend!\" commit then this above code is checked before creating a\n> \"amend! amend!\" commit for the user. So I think maybe we don't need to\n> check for multiple spaces ?\n\nOkay, if this is guaranteed to be created mechanically, then what you\nhave should work, though it may be a good idea to add an in-code\ncomment stating the reason it is okay to expect just the single space.\n\nThe alternative would be to avoid having \"amend! amend!\" in the first\nplace. I didn't trace through the code carefully so I don't know if it\nis possible, but would it make sense for the caller(s) to check before\nadding a second \"amend!\", thus eliminating the need to do so here?\n(Perhaps I'm misunderstanding, but the above code almost feels like a\ncase of \"whoops, we did something undesirable, so let's undo it.\".)\n"},{"id":"418150","messageId":"CAPig+cRiiQyavaMGzgBkXOoGFPhMBC7GbpB61ziFMrckReFbcQ@mail.gmail.com","threadId":"55235","inReplyTo":"CAPSFM5c1zR6yz=gATGxih0wL-W18AWgCHQhL_SPno5SeTzGQGg@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-03T08:18:25Z","receivedAt":"2021-03-04T00:23:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 3, 2021 at 2:44 AM Charvi Mendiratta wrote:\n> On Tue, 2 Mar 2021 at 12:09, Eric Sunshine wrote:\n> > On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta wrote:\n> > > Signed-off-by: Charvi Mendiratta \n> > > + Without `amend:` or `reword:`, create a `fixup!` commit where\n> > > + the commit message will be the subject line from the specified\n> > > + commit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n> > > + is further used with `git rebase --autosquash` to fixup the\n> > > + content of the specified commit.\n\nBy the way, now that you explained in the other thread that \"short\"\nprefix-matching of \"amend\" and \"reword\" are allowed, I realize that\nthe documentation doesn't mention it (or at least I don't remember\nreading it).\n\n(Nevertheless, I still feel uncomfortable about supporting short\nprefix-matching in the initial implementation without any evidence\nthat users will demand it, since we can't change that decision once\nit's in the hands of users.)\n\n> > > + When the commit log message begins with \"squash! ...\" (or \"fixup! ...\"\n> > > + or \"amend! ...\"), and there is already a commit in the todo list that\n> >\n> > Should this also be mentioning `reword!`?\n>\n> No, as both `amend` and `reword` suboptions create \"amend!\" commit\n> only. I think it seems a bit confusing but I will try another attempt\n> to reword the document.\n\nHmm, I see. So \"reword!\" is really just an \"amend!\" with only commit\nmessage but no patch content. That makes perfect sense from an\nimplementation standpoint, but it makes me wonder if it would be\neasier for users to understand if it created a \"reword!\" commit which\nwould be recognized as an alias of \"amend!\". (But maybe that's getting\ntoo confusing, and my musing should be ignored.)\n\nThis also answers an unasked question I had regarding the duplicate\n\"amend! amend!\" check. I was wondering why it wasn't also checking for\n\"reword! reword!\".\n"},{"id":"418152","messageId":"CAPSFM5fmK_3rFfTbD7o3qCcYYeMXoX7K-HhGorXDPcFseBEDzQ@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqlfb6mrnx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:41:19Z","receivedAt":"2021-03-04T00:23:08Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 04:02, Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> >> + if (len && fixup_message[len] == ':') {\n> >> + fixup_message[len++] = '\\0';\n> >> + fixup_commit = fixup_message + len;\n> >> + if (starts_with(\"amend\", fixup_message))\n> >> + fixup_prefix = \"amend\";\n> >> + else\n> >> + die(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n> >\n> > I haven't read ahead in the series yet, but I presume you're making\n> > this code extra generic because you plan to support additional `fixup`\n> > options (such as `reword`), but I wonder if the cognitive overhead is\n> > warranted or you could get by with something simpler, such as:\n> >\n> > if (skip_prefix(msg, \"amend:\", &arg) ||\n> > skip_prefix(msg, \"reword:\", &arg)) {\n> > ...\n> > }\n>\n> You still need to compute \"len\" because you'd want to tell between\n> --fixup=\"HEAD^{/^area: string}\" and --fixup=bogus:HEAD (the latter\n> would want to say \"no such variant 'bogus' for --fixup\", but the\n> colon in the former is not the end of the name of variant.\n>\n> So, skip_prefix() would not buy us much, I guess.\n>\n\nYes, I also agree.\n\n> But the use of starts_with() in the original patch is bogus, I\n> think. fixup_message[] by the time the comparison is made is\n> NULL terminated at where the colon was originally, so we should be\n> doing !strcmp() to reject \"--fixup=amendo:HEAD~2\" with \"no, 'amendo'\n> is not a valid variant name for --fixup option\".\n>\n\nI am not sure about this because we used the starts_with() so that it can\nsupport the _any_ prefix of `amend` or `reword` i.e to make all below\nlike combinations possible :\n--fixup=a:HEAD~2\n--fixup=am:HEAD~2\n\nSo, I am not sure if we need to replace it with !strcmp and work for\nthe specified prefix only ?\n\n> > Also, am I misreading when I think that the use of starts_with() could\n> > be replaced with a simple strcmp() since you've already inserted a\n> > '\\0' immediately after the final alphabetic character?\n>\n> Correct.\n>\n\nSame reason for using the starts_with() applies for this part also\nafter inserting the '\\0'. So, I think we can keep starts_with() ?\n"},{"id":"418156","messageId":"CAPSFM5dAdD2GYdrTx3fN2eoca63YZxFq76Qi7ftX4892v4BiUA@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cQR2ibdc1CQdwWKdk=mZM4FyaMsfw2Mpgk7Rdc-1R4PRQ@mail.gmail.com","subject":"Re: [PATCH v3 3/6] commit: add a reword suboption to --fixup","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:41:36Z","receivedAt":"2021-03-04T00:23:08Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 04:06, Eric Sunshine wrote:\n>\n\n> > + if (argc)\n> > + die(_(\"cannot combine reword option of --fixup with path %s\"), *argv);\n> > + if (patch_interactive)\n> > + die(_(\"cannot combine reword option of --fixup with --patch\"));\n> > + if (interactive)\n> > + die(_(\"cannot combine reword option of --fixup with --interactive\"));\n> > + if (all)\n> > + die(_(\"cannot combine reword option of --fixup with --all\"));\n> > + if (also)\n> > + die(_(\"cannot combine reword option of --fixup with --include\"));\n> > + if (only)\n> > + die(_(\"cannot combine reword option of --fixup with --only\"));\n> > +}\n>\n> Or, more concisely:\n>\n> if (argc)\n> die(_(\"--fixup mutually exclusive with path '%s'), ...);\n> if (patch_interactive || interactive || all || also || only)\n> die(_(\"--fixup mutually exclusive with\n> --patch/--interactive/--all/--include/--only);\n>\n\nGot it ! Its seems more clear, I will fix it.\n\n> The mix of two different error message styles (capitalized with\n> full-stop vs. lowercase no-full-stop) is a bit jarring, but minor.\n>\n\nOkay, I will fix it.\n"},{"id":"418158","messageId":"CAPSFM5eqspjRbQa-i+W5q+NkggaSuBhYVqu6uQrU1KBMxh1Ceg@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cSaZ+i+2P0x67BiHLxAGZaggNFK=dHxLJkmOfY8uafS5g@mail.gmail.com","subject":"Re: [PATCH v3 1/6] sequencer: export subject_length()","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:35:49Z","receivedAt":"2021-03-04T00:23:09Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 01:55, Eric Sunshine wrote:\n\n> Now that this function is public, is the name too generic? Most other\n> functions in this header have \"commit\" in the name. So,\n> commit_subject_length() might be one possibility (assuming the current\n> name is too generic).\n\nAgree, I will change it and update the patch. Thanks for this fix.\n"},{"id":"418159","messageId":"CAPSFM5dm7vD8pszCG+JKNb7Hu6Zp48wKXaj9ayxe6yaf6swePA@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:37:38Z","receivedAt":"2021-03-04T00:23:09Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 03:45, Eric Sunshine wrote:\n\n> > @@ -105,7 +105,8 @@ static const char *template_file;\n> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n> > + struct pretty_print_context *ctx) {\n> > + /*\n> > + * If we amend the 'amend!' commit then we don't want to\n> > + * duplicate the subject line.\n> > + */\n> > + const char *format = NULL;\n> > + if (starts_with(sb->buf, \"amend! amend!\"))\n>\n> Is the content of the incoming strbuf created mechanically so that we\n> know that there will only ever be one space between the two \"amend!\"\n> literals? If not, then this starts_with() check feels fragile.\n> (Compare with the code in sequencer.c which checks for this sort of\n> duplication but is tolerant of one or more spaces, not just a single\n> space.)\n>\n\nYes, so for preparing each \"amend!\" commit we add prefix \"amend! '' to\nthe subject of the specific commit. And further if we amend the\n\"amend!\" commit then this above code is checked before creating a\n\"amend! amend!\" commit for the user. So I think maybe we don't need to\ncheck for multiple spaces ?\n\n> > + format = \"%b\";\n> > + else\n> > + format = \"%B\";\n>\n> It's subjective and minor, but this could be expressed more compactly as:\n>\n> const char *fmt = starts_with(...) ? \"%b\" : \"%B\";\n>\n> Also, no need to initialize `format` to NULL since it gets assigned in\n> all code paths.\n>\n> Not worth a re-roll.\n>\n\nI agree, I will fix it.\n\n> > @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> > + if (!strcmp(fixup_prefix, \"amend\")) {\n> > + if (have_option_m)\n> > + die(_(\"cannot combine -m with --fixup:%s\"), fixup_message);\n> > + else\n> > + prepare_amend_commit(commit, &sb, &ctx);\n> > + }\n>\n> This is minor, but the way this is written, the error case and the\n> normal case appear to have the same significance, whereas, if you\n> write it like this:\n>\n> if (!strcmp(...)) {\n> if (have_option_m)\n> die(...);\n> prepare_amend_commit(...);\n> }\n>\n> then it's easier to see that you're checking for and getting an error\n> case out of the way early, which allows the reader to concentrate\n> without distraction on the normal case. As a minor benefit, you also\n> get to eliminate an indentation level for the normal case, which could\n> be important if more code is added to that case.\n>\n> Not worth a re-roll.\n>\n\nThanks for pointing this out, I will remove the unnecessary `else`.\n"},{"id":"418160","messageId":"CAPig+cQcjUzXpW1OBCiEC3Sr13uyeesqK49KWmgnY1iHNnS2Rw@mail.gmail.com","threadId":"55235","inReplyTo":"CAPSFM5fmK_3rFfTbD7o3qCcYYeMXoX7K-HhGorXDPcFseBEDzQ@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-03T07:57:41Z","receivedAt":"2021-03-04T00:23:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 3, 2021 at 2:41 AM Charvi Mendiratta wrote:\n> On Tue, 2 Mar 2021 at 04:02, Junio C Hamano wrote:\n> > But the use of starts_with() in the original patch is bogus, I\n> > think. fixup_message[] by the time the comparison is made is\n> > NULL terminated at where the colon was originally, so we should be\n> > doing !strcmp() to reject \"--fixup=amendo:HEAD~2\" with \"no, 'amendo'\n> > is not a valid variant name for --fixup option\".\n>\n> I am not sure about this because we used the starts_with() so that it can\n> support the _any_ prefix of `amend` or `reword` i.e to make all below\n> like combinations possible :\n> --fixup=a:HEAD~2\n> --fixup=am:HEAD~2\n>\n> So, I am not sure if we need to replace it with !strcmp and work for\n> the specified prefix only ?\n\nHmm, I see. I didn't follow whatever discussion led to the decision to\nuse this sort of prefix matching, but I have to wonder if it is a good\nidea. Was the idea that it behave similarly to sequencer commands in\n`git rebase --interactive` which are often abbreviated to a single\nletter? I personally would feel much more comfortable requiring a\nfull-word match for `amend` and `reword` at initial implementation.\nThat leaves the door open to later loosening it to do prefix-matching\nif enough people request such a feature, whereas starting with\nprefix-matching closes that door since we can never later tighten it\nto require full words.\n\nAnyhow, if the decision is to keep this behavior, then it almost\ncertainly deserves an in-code comment explaining the sort of\nprefix-matching it's doing since it's otherwise too easy for readers\nto be fooled as Junio and I were by not noticing that you had reversed\nthe arguments to starts_with().\n"},{"id":"418161","messageId":"CAPSFM5c1zR6yz=gATGxih0wL-W18AWgCHQhL_SPno5SeTzGQGg@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cRvwvT7QrO0-aLZX-2vsBPJSq6WO-O7g5A0OjDMNAYmCQ@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T07:43:55Z","receivedAt":"2021-03-04T00:23:27Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Tue, 2 Mar 2021 at 12:09, Eric Sunshine wrote:\n>\n> On Mon, Mar 1, 2021 at 3:52 AM Charvi Mendiratta wrote:\n> > Signed-off-by: Charvi Mendiratta \n> > ---\n> > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> > @@ -9,7 +9,7 @@ SYNOPSIS\n> > + [--dry-run] [(-c | -C | --squash) | --fixup [(amend|reword):])]\n> > @@ -86,11 +86,39 @@ OPTIONS\n> > ---fixup=::\n> > +--fixup=[(amend|reword):]::\n>\n> Although technically correct, I can't help but wonder if we can be\n> more friendly to readers by rephrasing this as:\n>\n> --fixup=::\n> --fixup=amend:::\n> --fixup=reword:::\n>\n> which is probably a lot easier to take in and understand at a glance.\n> Same comment applies to the synopsis.\n>\n> Not necessarily worth a re-roll.\n>\n> > + Without `amend:` or `reword:`, create a `fixup!` commit where\n> > + the commit message will be the subject line from the specified\n> > + commit with a prefix of \"fixup!'\". The resulting \"fixup!\" commit\n> > + is further used with `git rebase --autosquash` to fixup the\n> > + content of the specified commit.\n>\n> I think it becomes important at this point to make it more clear that\n> _only_ the content of gets changed by the \"fixup!\" commit,\n> and that the log message of is untouched.\n>\n> > +The `--fixup=amend:` form creates an \"amend!\" commit to\n> > +fixup both the content and the commit log message of the specified\n> > +commit. The resulting \"amend!\" commit's commit message subject\n> > +will be the subject line from the specified commit with a prefix of\n> > +\"amend!'\" and the message body will be commit log message of the\n> > +specified commit. It also invokes an editor seeded with the log\n> > +message of the \"amend!\" commit to allow to edit further. And it\n> > +refuses to create \"amend!\" commit if it's commit message body is\n> > +empty unless used with the `--allow-empty-message` option. \"amend!\"\n> > +commit when rebased with `--autosquash` will fixup the contents and\n> > +replace the commit message of the specified commit with the \"amend!\"\n> > +commit's message body.\n>\n> I had to read this several times to understand what it is trying to\n> say. I believe that part of the problem is that the bulk of the\n> description goes into great detail describing bits and behaviors which\n> make no sense without understanding what an \"amend!\" commit actually\n> does, which isn't explained until the very last sentence. So, I think\n> the entire description needs to be flipped on its head. In particular,\n> it should start by saying \"create a new commit which both fixes up the\n> content of and replaces 's log message\", and only\n> then dive into the details.\n>\n> In fact, what I just wrote suggests a larger problem with the\n> description of `--fixup` overall. There is no high-level explanation\n> of what a \"fixup\" (or \"amend\" or \"reword\") is; it just dives right\n> into the minutiae without providing the reader with sufficient context\n> to understand any of it. Only a reader who is already familiar with\n> interactive rebase is likely to grok what is being said here. So,\n> extending the thought I expressed above, it would be helpful for the\n> description of `--fixup=[amend:|reword:]` to start by first explaining\n> what a \"fixup\" is, followed by simple descriptions of \"amend\" and\n> \"reword\" (building upon \"fixup\"), and followed finally by details of\n> each. Very roughly, something like this:\n>\n> Creates a new commit which \"fixes up\" when applied with\n> `git rebase --autosquash`.\n>\n> A \"fixup\" commit changes the content of but leaves its\n> log message untouched.\n>\n> An \"amend\" commit is like \"fixup\" but also replaces the log\n> message of with the log message of the \"amend\" commit.\n>\n> A \"reword\" commit replaces the log message of with its\n> own log message but makes no changes to the content.\n>\n> And then dive into the details of each variation.\n>\n\nAgree, thanks for pointing this out in detail. I will rewrite the doc\nin the above suggested way and update in the next version.\n\n> > +The `--fixup=amend:` and `--fixup=reword:` forms cannot be used with\n> > +other options to add to the commit log message i.e it is incompatible\n> > +with `-m`/`-F`/`-c`/`-C` options.\n>\n> I suppose it doesn't hurt, but I wonder if it's really necessary to\n> document this considering that the user will learn soon enough upon\n> trying invalid combinations.\n>\n\nNot necessary, but I thought that users must know that `-m` is\notherwise supported with plain `--fixup` and not with the `amend` and\n`reword` suboptions. So, I think to reword it and add with the above.\n\n> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> > @@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.\n> > --autosquash::\n> > --no-autosquash::\n> > + When the commit log message begins with \"squash! ...\" (or \"fixup! ...\"\n> > + or \"amend! ...\"), and there is already a commit in the todo list that\n>\n> Should this also be mentioning `reword!`?\n\nNo, as both `amend` and `reword` suboptions create \"amend!\" commit\nonly. I think it seems a bit confusing but I will try another attempt\nto reword the document.\n\n>\n> > + matches the same `...`, automatically modify the todo list of\n> > + `rebase -i`, so that the commit marked for squashing comes right after\n> > + the commit to be modified, and change the action of the moved commit\n> > + from `pick` to `squash` (or `fixup` or `fixup -C`) respectively. A commit\n>\n> It's becoming difficult to know which of the \"foo!\" prefixes get\n> transformed into which sequencer command since there is no longer a\n> one-to-one correspondence between \"foo!\" prefixes and sequencer\n> commands as there was when only \"squash!\" and \"fixup!\" existed. The\n> reader should be told what sequencer command(s) \"amend!\" and \"reword!\"\n> become.\n>\n\nOkay, I will change it and explain it in more detail.\n\n> > + matches the `...` if the commit subject matches, or if the `...` refers\n> > + to the commit's hash. As a fall-back, partial matches of the commit\n> > + subject work, too. The recommended way to create fixup/squash/amend\n> > + commits is by using the `--fixup=[amend|reword]`/`--squash` options of\n> > + linkgit:git-commit[1].\n>\n> At this point, it may be beneficial to write these out long-form to\n> make it easier on the reader; something along the lines of:\n>\n> ... the `--fixup`, `--fixup:amend:`, `--fixup:reword:`, and\n> `--squash` options of ...\n\nAgree, I will fix it.\n\n\nThanks for all the detailed reviews and suggestions. I will update all\nthe changes in the next revision.\n\nThanks and Regards,\nCharvi\n"},{"id":"418169","messageId":"CAPSFM5cECFPk180G3aOdqnZ09FogwsBzC_WwO4OFutsjnTXQ7w@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cSJCSLBDumaXvnkdSBTATs62h9EfgtgJu0O7R_PJEumCg@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T19:21:23Z","receivedAt":"2021-03-04T00:23:43Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"Hi Eric,\n\nOn Wed, 3 Mar 2021 at 13:16, Eric Sunshine wrote:\n>\n> On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta wrote:\n> > On Tue, 2 Mar 2021 at 03:45, Eric Sunshine wrote:\n> > > > + if (starts_with(sb->buf, \"amend! amend!\"))\n> > >\n> > > Is the content of the incoming strbuf created mechanically so that we\n> > > know that there will only ever be one space between the two \"amend!\"\n> > > literals? If not, then this starts_with() check feels fragile.\n> >\n> > Yes, so for preparing each \"amend!\" commit we add prefix \"amend! '' to\n> > the subject of the specific commit. And further if we amend the\n> > \"amend!\" commit then this above code is checked before creating a\n> > \"amend! amend!\" commit for the user. So I think maybe we don't need to\n> > check for multiple spaces ?\n>\n> Okay, if this is guaranteed to be created mechanically, then what you\n> have should work, though it may be a good idea to add an in-code\n> comment stating the reason it is okay to expect just the single space.\n>\n> The alternative would be to avoid having \"amend! amend!\" in the first\n> place.\n\nAgree. I think we can do this...\n\n> I didn't trace through the code carefully so I don't know if it\n> is possible, but would it make sense for the caller(s) to check before\n> adding a second \"amend!\", thus eliminating the need to do so here?\n> (Perhaps I'm misunderstanding, but the above code almost feels like a\n> case of \"whoops, we did something undesirable, so let's undo it.\".)\n\nI looked into it and got another alternative, to extend the same\nprepare_amend_commit() function and replace the check condition of\nif (starts_with(sb->buf, \"amend! amend!\")) with the code as below :\n\nconst char *buffer = get_commit_buffer(commit, NULL);\nconst char *subject;\nfind_commit_subject(buffer, &subject);\nif (starts_with(subject, \"amend!\"))\nconst char *fmt = starts_with(subject, \"amend!\") ? \"%b\" : \"%B\";\nformat_commit_message(commit, fmt, sb, ctx);\nunuse_commit_buffer(commit, buffer);\n\nSo, now it checks the commit subject here only. Otherwise as you have\nsuggested above to check before adding a second \"amend!\", I think that\ncan result in confusion as currently both \"fixup!\" and \"amend!\"\ncommits (commit's subject) are prepared by same code and further for\n\"amend!\" commit as we write a commit message body also so we used\nprepare_amend_commit() to do that stuff.\n"},{"id":"418170","messageId":"CAPSFM5eAdfAD+TeLn8f4+1x38g_6r7T=DuyPJf2f-828pwqgyQ@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cQcjUzXpW1OBCiEC3Sr13uyeesqK49KWmgnY1iHNnS2Rw@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T19:21:57Z","receivedAt":"2021-03-04T00:23:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Wed, 3 Mar 2021 at 13:27, Eric Sunshine wrote:\n>\n[...]\n> > I am not sure about this because we used the starts_with() so that it can\n> > support the _any_ prefix of `amend` or `reword` i.e to make all below\n> > like combinations possible :\n> > --fixup=a:HEAD~2\n> > --fixup=am:HEAD~2\n> >\n> > So, I am not sure if we need to replace it with !strcmp and work for\n> > the specified prefix only ?\n>\n> Hmm, I see. I didn't follow whatever discussion led to the decision to\n> use this sort of prefix matching, but I have to wonder if it is a good\n> idea. Was the idea that it behave similarly to sequencer commands in\n> `git rebase --interactive` which are often abbreviated to a single\n> letter?\n\nYes, this is also true. Also, same is discussed as here:\nhttps://lore.kernel.org/git/CAPSFM5cEnex1xaBy5ia_xNFDNzt5_Y=W-6TB9d9yW_AiPAKxDg@mail.gmail.com/\n\n>I personally would feel much more comfortable requiring a\n> full-word match for `amend` and `reword` at initial implementation.\n> That leaves the door open to later loosening it to do prefix-matching\n> if enough people request such a feature, whereas starting with\n> prefix-matching closes that door since we can never later tighten it\n> to require full words.\n>\n> Anyhow, if the decision is to keep this behavior, then it almost\n> certainly deserves an in-code comment explaining the sort of\n> prefix-matching it's doing since it's otherwise too easy for readers\n> to be fooled as Junio and I were by not noticing that you had reversed\n> the arguments to starts_with().\n\nOkay, I will add the comments to it .\n"},{"id":"418171","messageId":"CAPSFM5eF1c5--8nqiDQ0ZLwz7cnNxx_BahPHGCvqbnrYKkCf9A@mail.gmail.com","threadId":"55235","inReplyTo":"CAPig+cRiiQyavaMGzgBkXOoGFPhMBC7GbpB61ziFMrckReFbcQ@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-03T19:22:44Z","receivedAt":"2021-03-04T00:23:45Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Wed, 3 Mar 2021 at 13:48, Eric Sunshine wrote:\n>\n[...]\n> By the way, now that you explained in the other thread that \"short\"\n> prefix-matching of \"amend\" and \"reword\" are allowed, I realize that\n> the documentation doesn't mention it (or at least I don't remember\n> reading it).\n>\n\nYes, I admit it was not included. I will add it too.\n\n> (Nevertheless, I still feel uncomfortable about supporting short\n> prefix-matching in the initial implementation without any evidence\n> that users will demand it, since we can't change that decision once\n> it's in the hands of users.)\n>\n\nI am not sure about strong evidence but I tried to keep the major\npoints discussed earlier, as mentioned in the previous thread. Also I\nthink otherwise the short prefix will ease out / shorten the command\nto prepare the \"amend!\" and also mirrors the commands in interactive\nrebase.\n\n> > > > + When the commit log message begins with \"squash! ...\" (or \"fixup! ...\"\n> > > > + or \"amend! ...\"), and there is already a commit in the todo list that\n> > >\n> > > Should this also be mentioning `reword!`?\n> >\n> > No, as both `amend` and `reword` suboptions create \"amend!\" commit\n> > only. I think it seems a bit confusing but I will try another attempt\n> > to reword the document.\n>\n> Hmm, I see. So \"reword!\" is really just an \"amend!\" with only commit\n> message but no patch content. That makes perfect sense from an\n> implementation standpoint, but it makes me wonder if it would be\n> easier for users to understand if it created a \"reword!\" commit which\n> would be recognized as an alias of \"amend!\". (But maybe that's getting\n> too confusing, and my musing should be ignored.)\n>\n\nYes, we didn't choose to make \"reword!\" commit because if we do so\nthen again it would be expected to implicitly change 'pick' command to\n'reword' in sequencer/ rebase to-do list when combined with 'git\nrebase -- autosquash'. But here we are changing 'pick' to ' fixup -C'\nto fulfill the working. So, we decided to create a variant of\n'--fixup' and serve it as \"amend!\" commit.\n\n> This also answers an unasked question I had regarding the duplicate\n> \"amend! amend!\" check. I was wondering why it wasn't also checking for\n> \"reword! reword!\".\n\nYes, it's true.\n\nThanks for the reveiws, I will add the above mentioned changes too.\n\nThanks and Regards,\nCharvi\n"},{"id":"418189","messageId":"xmqqh7lrg2g0.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPig+cQcjUzXpW1OBCiEC3Sr13uyeesqK49KWmgnY1iHNnS2Rw@mail.gmail.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T00:58:07Z","receivedAt":"2021-03-04T01:09:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Hmm, I see. I didn't follow whatever discussion led to the decision to\n> use this sort of prefix matching, but I have to wonder if it is a good\n> idea.\n\nMeaning --fixup=a: and --fixup=amend: do the same\nthing, until somebody invents --fixup=another: and makes the\nprefix 'a' no longer unique? I tend to agree that, especially with\ncommand line completion support with modern shells, such a prefix\nmatching would not be necessary. \n\n> Was the idea that it behave similarly to sequencer commands in\n> `git rebase --interactive` which are often abbreviated to a single\n> letter? I personally would feel much more comfortable requiring a\n> full-word match for `amend` and `reword` at initial implementation.\n\nMe too.\n"},{"id":"418197","messageId":"xmqqczwfg23t.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPig+cRiiQyavaMGzgBkXOoGFPhMBC7GbpB61ziFMrckReFbcQ@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T01:05:26Z","receivedAt":"2021-03-04T01:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> No, as both `amend` and `reword` suboptions create \"amend!\" commit\n>> only. I think it seems a bit confusing but I will try another attempt\n>> to reword the document.\n>\n> Hmm, I see. So \"reword!\" is really just an \"amend!\" with only commit\n> message but no patch content. That makes perfect sense from an\n> implementation standpoint, but it makes me wonder if it would be\n> easier for users to understand if it created a \"reword!\" commit which\n> would be recognized as an alias of \"amend!\". (But maybe that's getting\n> too confusing, and my musing should be ignored.)\n\nPerhaps related, perhaps not, but I wonder if we really need --fixup=amend\nand --fixup=reword to begin with. The \"amend\" variant,\n\n $ git commit --fixup=amend: ... other args ...\n\nis about shaping the index with \"other args\" and recording the\nresulting tree with the log message taken from , marked\nwith the \"amend!\" prefix. The --fixup=reword: variant\nis a mere special case of it where the recorded tree is made of the\nindex in the same way as a partial commit with pathspec that matches\nno paths, i.e. If you have --fixup=amend, you can do\n\n $ git commit --fixup=amend: --only\n\nand you do not need --fixup=reword: at all, no?\n\n\n"},{"id":"418223","messageId":"CAPSFM5cM4fdyWXD33PkT2bH6kM+3ixkxgAnhjUVYFtjUHgwU5g@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqczwfg23t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-04T09:00:34Z","receivedAt":"2021-03-04T09:02:21Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Thu, 4 Mar 2021 at 06:35, Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> > Hmm, I see. So \"reword!\" is really just an \"amend!\" with only commit\n> > message but no patch content. That makes perfect sense from an\n> > implementation standpoint, but it makes me wonder if it would be\n> > easier for users to understand if it created a \"reword!\" commit which\n> > would be recognized as an alias of \"amend!\". (But maybe that's getting\n> > too confusing, and my musing should be ignored.)\n>\n> Perhaps related, perhaps not, but I wonder if we really need --fixup=amend\n> and --fixup=reword to begin with. The \"amend\" variant,\n>\n> $ git commit --fixup=amend: ... other args ...\n>\n> is about shaping the index with \"other args\" and recording the\n> resulting tree with the log message taken from , marked\n> with the \"amend!\" prefix. The --fixup=reword: variant\n> is a mere special case of it where the recorded tree is made of the\n> index in the same way as a partial commit with pathspec that matches\n> no paths, i.e. If you have --fixup=amend, you can do\n>\n> $ git commit --fixup=amend: --only\n>\n> and you do not need --fixup=reword: at all, no?\n>\n\nMaybe as an alternative User interface, we can remove the\n`--fixup=reword:`.\n\nBut for this patch, as we have kept separate suboption\n`--fixup=reword:` , so if now we do\n`--fixup=amend: --only` then it will return the error as\nbelow :\nfatal: No paths with --include/--only does not make sense.\nSo, `amend` works only if staged changes are present otherwise to\nchange only the commit message `reword` option is there.\n\nI agree we can change to the above UI but still I wonder which one is\nmore friendly ? Also, I think we need to add complete `--allow-empty\n--only` to mirror the working of `reword` so this may result in a lot\nof typing and hard to remember.\n\nThanks and Regards,\nCharvi\n"},{"id":"418224","messageId":"CAPSFM5dXDdi9_RF7nK+JTgPUUfvY4j3AMuWVoqaY=3yhCBwyWA@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqh7lrg2g0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-04T09:01:52Z","receivedAt":"2021-03-04T09:03:25Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Thu, 4 Mar 2021 at 06:28, Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> > Hmm, I see. I didn't follow whatever discussion led to the decision to\n> > use this sort of prefix matching, but I have to wonder if it is a good\n> > idea.\n>\n> Meaning --fixup=a: and --fixup=amend: do the same\n> thing, until somebody invents --fixup=another: and makes the\n> prefix 'a' no longer unique? I tend to agree that, especially with\n> command line completion support with modern shells, such a prefix\n> matching would not be necessary.\n>\n\nOkay, so for now I think (as suggested), let's remove it and directly\nuse \"!strcmp()\" only instead of \"starts_with()\". I agree that if we\nkeep prefix matching or not, it will not matter much.\n\nThanks and Regards,\nCharvi\n"},{"id":"418265","messageId":"xmqqpn0ed0m2.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPSFM5cM4fdyWXD33PkT2bH6kM+3ixkxgAnhjUVYFtjUHgwU5g@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T22:18:13Z","receivedAt":"2021-03-04T22:18:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n>> no paths, i.e. If you have --fixup=amend, you can do\n>>\n>> $ git commit --fixup=amend: --only\n>>\n>> and you do not need --fixup=reword: at all, no?\n>>\n>\n> Maybe as an alternative User interface, we can remove the\n> `--fixup=reword:`.\n>\n> But for this patch, as we have kept separate suboption\n> `--fixup=reword:` , so if now we do\n> `--fixup=amend: --only` then it will return the error as\n> below :\n> fatal: No paths with --include/--only does not make sense.\n\nYes, but it is something we can easily fix, just like we made\n\"--only\" without any pathname to work with \"--amend\" (or with\n\"--allow-empty\").\n\nThe reason I brought it up was not because \"--fixup=reword\" is not\nneeded as a short-hand for \"--only --fixup=amend\" (but thinking\nabout it again, I do not think it is so bad), but primarily in\nresponse to \"would it be easier for users if we had reword! insn in\naddition to amend! verb in the todo file?\" that was raised earlier\nin the thread. If we position \"--fixup=reword\" as a short-hand\nand/or a syntax sugar for \"--fixup=amend\" and advertise it as such\nsufficiently to educate users, it would be easier for users to\nunderstand why they both result in \"amend!\".\n"},{"id":"418299","messageId":"CAPSFM5dM4NMeGqEG7hFLzyhJskqcrNtNqL9=MUCw9SEYYaFLoQ@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqpn0ed0m2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-05T06:14:51Z","receivedAt":"2021-03-05T06:15:06Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Fri, 5 Mar 2021 at 03:48, Junio C Hamano wrote:\n>\n> Charvi Mendiratta writes:\n>\n> >> no paths, i.e. If you have --fixup=amend, you can do\n> >>\n> >> $ git commit --fixup=amend: --only\n> >>\n> >> and you do not need --fixup=reword: at all, no?\n> >>\n> >\n> > Maybe as an alternative User interface, we can remove the\n> > `--fixup=reword:`.\n> >\n> > But for this patch, as we have kept separate suboption\n> > `--fixup=reword:` , so if now we do\n> > `--fixup=amend: --only` then it will return the error as\n> > below :\n> > fatal: No paths with --include/--only does not make sense.\n>\n> Yes, but it is something we can easily fix, just like we made\n> \"--only\" without any pathname to work with \"--amend\" (or with\n> \"--allow-empty\").\n>\n\nAgree.\n\n> The reason I brought it up was not because \"--fixup=reword\" is not\n> needed as a short-hand for \"--only --fixup=amend\" (but thinking\n> about it again, I do not think it is so bad), but primarily in\n> response to \"would it be easier for users if we had reword! insn in\n> addition to amend! verb in the todo file?\" that was raised earlier\n> in the thread. If we position \"--fixup=reword\" as a short-hand\n> and/or a syntax sugar for \"--fixup=amend\" and advertise it as such\n> sufficiently to educate users, it would be easier for users to\n> understand why they both result in \"amend!\".\n\nOkay, so now if it's Ok to keep the short-hand \"--fixup=reword\" ? then\nI think making the documentation more clear would be sufficient to\nserve it to the users ?\nOtherwise if the votes are more that the user will get confused as\nboth results in \"amend!\", then should we change this patch to \"--only\n--fixup=amend\" ?\n"},{"id":"418331","messageId":"xmqq4khpbgqk.fsf@gitster.c.googlers.com","threadId":"55235","inReplyTo":"CAPSFM5dM4NMeGqEG7hFLzyhJskqcrNtNqL9=MUCw9SEYYaFLoQ@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-05T18:25:07Z","receivedAt":"2021-03-05T18:25:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n>> The reason I brought it up was not because \"--fixup=reword\" is not\n>> needed as a short-hand for \"--only --fixup=amend\" (but thinking\n>> about it again, I do not think it is so bad), but primarily in\n>> response to \"would it be easier for users if we had reword! insn in\n>> addition to amend! verb in the todo file?\" that was raised earlier\n>> in the thread. If we position \"--fixup=reword\" as a short-hand\n>> and/or a syntax sugar for \"--fixup=amend\" and advertise it as such\n>> sufficiently to educate users, it would be easier for users to\n>> understand why they both result in \"amend!\".\n>\n> Okay, so now if it's Ok to keep the short-hand \"--fixup=reword\" ? then\n> I think making the documentation more clear would be sufficient to\n> serve it to the users ?\n\nIt would be good \n\n (1) to keep \"--fixup=reword:\"\n\n (2) to keep \"amend!\" but not introduce \"reword!\" insn\n\n (3) document \"--fixup=reword:\" can be thought of as a mere\n special-case short-hand for \"--fixup=amend: --only\",\n and\n\n (4) make sure \"fixup=amend: --only\" is usable as a\n replacement for \"--fixup=reword:\".\n\nbut if we are not doing (3) and (4), then it would also be OK to\n\n (1) to keep \"--fixup=reword:\"\n\n (2) to keep \"amend!\" and introduce \"reword!\" insn\n\nI would think.\n"},{"id":"418361","messageId":"CAPSFM5dhxm9kuzyXj6wF7s3BoDNzCmZHpaDFhvOBVB1QWbM25w@mail.gmail.com","threadId":"55235","inReplyTo":"xmqq4khpbgqk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-06T04:13:10Z","receivedAt":"2021-03-06T04:25:01Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Fri, 5 Mar 2021 at 23:55, Junio C Hamano wrote:\n>\n> Charvi Mendiratta writes:\n>\n> >> The reason I brought it up was not because \"--fixup=reword\" is not\n> >> needed as a short-hand for \"--only --fixup=amend\" (but thinking\n> >> about it again, I do not think it is so bad), but primarily in\n> >> response to \"would it be easier for users if we had reword! insn in\n> >> addition to amend! verb in the todo file?\" that was raised earlier\n> >> in the thread. If we position \"--fixup=reword\" as a short-hand\n> >> and/or a syntax sugar for \"--fixup=amend\" and advertise it as such\n> >> sufficiently to educate users, it would be easier for users to\n> >> understand why they both result in \"amend!\".\n> >\n> > Okay, so now if it's Ok to keep the short-hand \"--fixup=reword\" ? then\n> > I think making the documentation more clear would be sufficient to\n> > serve it to the users ?\n>\n> It would be good\n>\n> (1) to keep \"--fixup=reword:\"\n>\n> (2) to keep \"amend!\" but not introduce \"reword!\" insn\n>\n> (3) document \"--fixup=reword:\" can be thought of as a mere\n> special-case short-hand for \"--fixup=amend: --only\",\n> and\n>\n> (4) make sure \"fixup=amend: --only\" is usable as a\n> replacement for \"--fixup=reword:\".\n>\n\nOkay, I agree that this method is more clear ...\n\n> but if we are not doing (3) and (4), then it would also be OK to\n>\n> (1) to keep \"--fixup=reword:\"\n>\n> (2) to keep \"amend!\" and introduce \"reword!\" insn\n>\n\n... than this one and will update the patch in the above (former) suggested way.\n\n\nThanks for suggestions and detailed explanation.\n\nThanks and Regards,\nCharvi\n"},{"id":"418362","messageId":"CAPig+cQ-MaASh4pq1GyvGpFyQRUyw2RHg9HnPTAWe7fvaHOfxA@mail.gmail.com","threadId":"55235","inReplyTo":"CAPSFM5dhxm9kuzyXj6wF7s3BoDNzCmZHpaDFhvOBVB1QWbM25w@mail.gmail.com","subject":"Re: [PATCH v3 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-06T06:11:47Z","receivedAt":"2021-03-06T06:12:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 5, 2021 at 11:13 PM Charvi Mendiratta wrote:\n> On Fri, 5 Mar 2021 at 23:55, Junio C Hamano wrote:\n> > (1) to keep \"--fixup=reword:\"\n> >\n> > (2) to keep \"amend!\" but not introduce \"reword!\" insn\n> >\n> > (3) document \"--fixup=reword:\" can be thought of as a mere\n> > special-case short-hand for \"--fixup=amend: --only\",\n> > and\n> >\n> > (4) make sure \"fixup=amend: --only\" is usable as a\n> > replacement for \"--fixup=reword:\".\n>\n> Okay, I agree that this method is more clear ...\n\nThis works for me too, especially the bit about improving the\ndocumentation to be more clear that --fixup=reword: is a special-case\n(or syntactic sugar) for --fixup=amend:.\n\nMy confusion all along was thinking that --fixup=amend: and\n--fixup=reword: resulted in distinct \"amend!\" and \"reword!\" prefixes.\nI don't know whether that confusion was due to me not reading the\ncommit messages or documentation carefully enough, or because the\nbehavior wasn't clearly documented or easily understood. (I did have\nto re-read the documentation patch multiple times in an attempt to\nunderstand what it was saying, so perhaps I can blame that. ;-) At any\nrate, it will be good if we can get it clearly documented.\n\n> > but if we are not doing (3) and (4), then it would also be OK to\n> >\n> > (1) to keep \"--fixup=reword:\"\n> >\n> > (2) to keep \"amend!\" and introduce \"reword!\" insn\n>\n> ... than this one and will update the patch in the above (former) suggested way.\n\nThis option would likely be less desirable since it could confuse\npeople into thinking that \"reword!\" would become \"reword\" in the\nsequencer instruction sheet -- which isn't the case at all -- it\nbecomes \"fixup -c\" (or -C, I can't remember).\n"},{"id":"418726","messageId":"20210310194306.32565-1-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 0/6][Outreachy] commit: Implementation of \"amend!\" commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:01Z","receivedAt":"2021-03-10T19:44:39Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"This patch series teaches `git commit --fixup` to create \"amend!\" commit\nas an alternative that works with `git rebase --autosquash`. It allows to\nfixup both the content and the commit message of the specified commit.\nHere we add two suboptions to the `--fixup`, first `amend` suboption that\ncreates an \"amend!\" commit. It takes the staged changes and also allows to\nedit the commit message of the commit we are fixing.\nExample usuage:\ngit commit --fixup=amend:\n\nSecondly, `reword` suboption that creates an empty \"amend!\" commit i.e it\nignores the staged changes and only allows to reword/edit the commit message\nof the commit we are fixing. `--fixup=reword:` is a short-hand of\n`--fixup=amend: --only`.\nExample usuage:\ngit commit --fixup=reword:\n\n** This work is rebased on the top of cm/rebase-i-updates.\n\nChanges from v3 :\n(Thanks to Junio C Hamano and Eric Sunshine for the reviews and suggesting the\nimprovements required in the previous patches )\n\n* Now `--fixup=reword:` aliases `--fixup=amend: --only`. Added the respective\ntest and updated documentation.\n* Modified prepared_amend_commit() in commit.c.\n* Renamed the subject_length() to commit_subject_length() [v3 - 1/6]\n* Modified the --fixup=[amend|reword] documentation\n* Also, updated the commit messages and some other minor cleanup.\n\nCharvi Mendiratta (6):\n sequencer: export and rename subject_length()\n commit: add amend suboption to --fixup to create amend! commit\n commit: add a reword suboption to --fixup\n t7500: add tests for --fixup=[amend|reword] options\n t3437: use --fixup with options to create amend! commit\n doc/git-commit: add documentation for fixup=[amend|reword] options\n\n Documentation/git-commit.txt | 40 ++++++-\n Documentation/git-rebase.txt | 21 ++--\n builtin/commit.c | 118 +++++++++++++++++--\n commit.c | 14 +++\n commit.h | 3 +\n sequencer.c | 16 +--\n t/t3437-rebase-fixup-options.sh | 30 +----\n t/t7500-commit-template-squash-signoff.sh | 135 ++++++++++++++++++++++\n 8 files changed, 309 insertions(+), 68 deletions(-)\n\n--\n2.29.0.rc1\n\n"},{"id":"418727","messageId":"20210310194306.32565-3-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:06Z","receivedAt":"2021-03-10T19:45:15Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"`git commit --fixup=amend:` will create an \"amend!\" commit.\nThe resulting commit message subject will be \"amend! ...\" where\n\"...\" is the subject line of and the initial message\nbody will be 's message.\n\nThe \"amend!\" commit when rebased with --autosquash will fixup the\ncontents and replace the commit message of with the\n\"amend!\" commit's message body.\n\nIn order to prevent rebase from creating commits with an empty\nmessage we refuse to create an \"amend!\" commit if commit message\nbody is empty.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Charvi Mendiratta \n---\n builtin/commit.c | 107 ++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 97 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 505fe60956..05594fa8ab 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -105,7 +105,8 @@ static const char *template_file;\n */\n static const char *author_message, *author_message_buffer;\n static char *edit_message, *use_message;\n-static char *fixup_message, *squash_message;\n+static char *fixup_message, *fixup_commit, *squash_message;\n+static const char *fixup_prefix;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n@@ -357,7 +358,8 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n \t}\n \n-\tif (!pathspec.nr && (also || (only && !amend && !allow_empty)))\n+\tif (!pathspec.nr && (also || (only && !allow_empty &&\n+\t (!amend || (fixup_message && strcmp(fixup_prefix, \"amend\"))))))\n \t\tdie(_(\"No paths with --include/--only does not make sense.\"));\n \n \tif (read_cache_preload(&pathspec) < 0)\n@@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n \tcomment_line_char = *p;\n }\n \n+static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n+\t\t\t\tstruct pretty_print_context *ctx) {\n+\n+\tconst char *buffer, *subject, *fmt;\n+\n+\tbuffer = get_commit_buffer(commit, NULL);\n+\tfind_commit_subject(buffer, &subject);\n+\t/*\n+\t * If we amend the 'amend!' commit then we don't want to\n+\t * duplicate the subject line.\n+\t */\n+\tfmt = starts_with(subject, \"amend!\") ? \"%b\" : \"%B\";\n+\tformat_commit_message(commit, fmt, sb, ctx);\n+\tunuse_commit_buffer(commit, buffer);\n+\treturn 0;\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct commit *current_head,\n \t\t\t struct wt_status *s,\n@@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t} else if (fixup_message) {\n \t\tstruct pretty_print_context ctx = {0};\n \t\tstruct commit *commit;\n-\t\tcommit = lookup_commit_reference_by_name(fixup_message);\n+\t\tchar *fmt = xstrfmt(\"%s! %%s\\n\\n\", fixup_prefix);\n+\t\tcommit = lookup_commit_reference_by_name(fixup_commit);\n \t\tif (!commit)\n-\t\t\tdie(_(\"could not lookup commit %s\"), fixup_message);\n+\t\t\tdie(_(\"could not lookup commit %s\"), fixup_commit);\n \t\tctx.output_encoding = get_commit_output_encoding();\n-\t\tformat_commit_message(commit, \"fixup! %s\\n\\n\",\n-\t\t\t\t &sb, &ctx);\n-\t\tif (have_option_m)\n-\t\t\tstrbuf_addbuf(&sb, &message);\n+\t\tformat_commit_message(commit, fmt, &sb, &ctx);\n+\t\tfree(fmt);\n \t\thook_arg1 = \"message\";\n+\n+\t\t/*\n+\t\t * Only `-m` commit message option is checked here, as\n+\t\t * it supports `--fixup` to append the commit message.\n+\t\t *\n+\t\t * The other commit message options `-c`/`-C`/`-F` are\n+\t\t * incompatible with all the forms of `--fixup` and\n+\t\t * have already errored out while parsing the `git commit`\n+\t\t * options.\n+\t\t */\n+\t\tif (have_option_m && !strcmp(fixup_prefix, \"fixup\"))\n+\t\t\tstrbuf_addbuf(&sb, &message);\n+\n+\t\tif (!strcmp(fixup_prefix, \"amend\")) {\n+\t\t\tif (have_option_m)\n+\t\t\t\tdie(_(\"cannot combine -m with --fixup:%s\"), fixup_message);\n+\t\t\tprepare_amend_commit(commit, &sb, &ctx);\n+\t\t}\n \t} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {\n \t\tsize_t merge_msg_start;\n \n@@ -1152,6 +1188,12 @@ static void finalize_deferred_config(struct wt_status *s)\n \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n }\n \n+/* returns the length of intial segment of alpha characters only */\n+static size_t get_alpha_len(char *fixup_message) {\n+\tconst char alphas[] = \"abcdefghijklmnopqrstuvwxyz\";\n+\treturn strspn(fixup_message, alphas);\n+}\n+\n static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\t\t const struct option *options,\n \t\t\t\t const char * const usage[],\n@@ -1170,7 +1212,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (force_author && renew_authorship)\n \t\tdie(_(\"Using both --reset-author and --author does not make sense\"));\n \n-\tif (logfile || have_option_m || use_message || fixup_message)\n+\tif (logfile || have_option_m || use_message)\n \t\tuse_editor = 0;\n \tif (0 <= edit_flag)\n \t\tuse_editor = edit_flag;\n@@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \n \tif (also + only + all + interactive > 1)\n \t\tdie(_(\"Only one of --include/--only/--all/--interactive/--patch can be used.\"));\n+\n+\tif (fixup_message) {\n+\t\t/*\n+\t\t * To check if fixup_message that contains ':' is a commit\n+\t\t * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n+\t\t * a suboption of `--fixup`.\n+\t\t *\n+\t\t * As `amend` suboption contains only alpha character.\n+\t\t * So check if first non alpha character in fixup_message\n+\t\t * is ':'.\n+\t\t */\n+\t\tsize_t len = get_alpha_len(fixup_message);\n+\t\tif (len && fixup_message[len] == ':') {\n+\t\t\tfixup_message[len++] = '\\0';\n+\t\t\tfixup_commit = fixup_message + len;\n+\t\t\tif (!strcmp(\"amend\", fixup_message)) {\n+\t\t\t\tfixup_prefix = \"amend\";\n+\t\t\t\tallow_empty = 1;\n+\t\t\t} else {\n+\t\t\t\tdie(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n+\t\t\t}\n+\t\t} else {\n+\t\t\tfixup_commit = fixup_message;\n+\t\t\tfixup_prefix = \"fixup\";\n+\t\t\tuse_editor = 0;\n+\t\t}\n+\t}\n+\n \tcleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);\n \n \thandle_untracked_files_arg(s);\n@@ -1504,7 +1574,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK('m', \"message\", &message, N_(\"message\"), N_(\"commit message\"), opt_parse_m),\n \t\tOPT_STRING('c', \"reedit-message\", &edit_message, N_(\"commit\"), N_(\"reuse and edit message from specified commit\")),\n \t\tOPT_STRING('C', \"reuse-message\", &use_message, N_(\"commit\"), N_(\"reuse message from specified commit\")),\n-\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"commit\"), N_(\"use autosquash formatted message to fixup specified commit\")),\n+\t\t/*\n+\t\t * TRANSLATORS: Leave \"[amend:]\" as-is, and\n+\t\t * only translate .\n+\t\t */\n+\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[amend:]commit\"), N_(\"use autosquash formatted message to fixup or amend specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n@@ -1663,6 +1737,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n \n+\tif (fixup_message && starts_with(sb.buf, \"amend! \") &&\n+\t !allow_empty_message) {\n+\t\tstruct strbuf body = STRBUF_INIT;\n+\t\tsize_t len = commit_subject_length(sb.buf);\n+\t\tstrbuf_addstr(&body, sb.buf + len);\n+\t\tif (message_is_empty(&body, cleanup_mode)) {\n+\t\t\trollback_index_files();\n+\t\t\tfprintf(stderr, _(\"Aborting commit due to empty commit message body.\\n\"));\n+\t\t\texit(1);\n+\t\t}\n+\t\tstrbuf_release(&body);\n+\t}\n+\n \tif (amend) {\n \t\tconst char *exclude_gpgsig[3] = { \"gpgsig\", \"gpgsig-sha256\", NULL };\n \t\textra = read_commit_extra_headers(current_head, exclude_gpgsig);\n-- \n2.29.0.rc1\n\n"},{"id":"418728","messageId":"20210310194306.32565-2-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 1/6] sequencer: export and rename subject_length()","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:04Z","receivedAt":"2021-03-10T19:45:16Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"This function can be used in other parts of git. Let's move the\nfunction to commit.c and also rename it to make the name of the\nfunction more generic.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Eric Sunshine \nSigned-off-by: Charvi Mendiratta \n---\n commit.c | 14 ++++++++++++++\n commit.h | 3 +++\n sequencer.c | 16 +---------------\n 3 files changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex bab8d5ab07..199c7e274c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -535,6 +535,20 @@ int find_commit_subject(const char *commit_buffer, const char **subject)\n \treturn eol - p;\n }\n \n+size_t commit_subject_length(const char *body)\n+{\n+\tconst char *p = body;\n+\twhile (*p) {\n+\t\tconst char *next = skip_blank_lines(p);\n+\t\tif (next != p)\n+\t\t\tbreak;\n+\t\tp = strchrnul(p, '\\n');\n+\t\tif (*p)\n+\t\t\tp++;\n+\t}\n+\treturn p - body;\n+}\n+\n struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list_p)\n {\n \tstruct commit_list *new_list = xmalloc(sizeof(struct commit_list));\ndiff --git a/commit.h b/commit.h\nindex f4e7b0158e..e65d9b3e75 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -165,6 +165,9 @@ const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\n \n+/* Return length of the commit subject from commit log message. */\n+size_t commit_subject_length(const char *body);\n+\n struct commit_list *commit_list_insert(struct commit *item,\n \t\t\t\t\tstruct commit_list **list);\n int commit_list_contains(struct commit *item,\ndiff --git a/sequencer.c b/sequencer.c\nindex abc6d5cdfd..da7d54c7ce 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1724,20 +1724,6 @@ enum todo_item_flags {\n \tTODO_EDIT_FIXUP_MSG = (1 << 2),\n };\n \n-static size_t subject_length(const char *body)\n-{\n-\tconst char *p = body;\n-\twhile (*p) {\n-\t\tconst char *next = skip_blank_lines(p);\n-\t\tif (next != p)\n-\t\t\tbreak;\n-\t\tp = strchrnul(p, '\\n');\n-\t\tif (*p)\n-\t\t\tp++;\n-\t}\n-\treturn p - body;\n-}\n-\n static const char first_commit_msg_str[] = N_(\"This is the 1st commit message:\");\n static const char nth_commit_msg_fmt[] = N_(\"This is the commit message #%d:\");\n static const char skip_first_commit_msg_str[] = N_(\"The 1st commit message will be skipped:\");\n@@ -1861,7 +1847,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \tif (starts_with(body, \"amend!\") ||\n \t ((command == TODO_SQUASH || seen_squash(opts)) &&\n \t (starts_with(body, \"squash!\") || starts_with(body, \"fixup!\"))))\n-\t\tcommented_len = subject_length(body);\n+\t\tcommented_len = commit_subject_length(body);\n \n \tstrbuf_addf(buf, \"\\n%c \", comment_line_char);\n \tstrbuf_addf(buf, _(nth_commit_msg_fmt),\n-- \n2.29.0.rc1\n\n"},{"id":"418729","messageId":"20210310194306.32565-4-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 3/6] commit: add a reword suboption to --fixup","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:07Z","receivedAt":"2021-03-10T19:45:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"`git commit --fixup=reword:` aliases\n`--fixup=amend: --only`, where it creates an empty \"amend!\"\ncommit that will reword without changing its contents when\nit is rebased with `--autosquash`.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Charvi Mendiratta \n---\n builtin/commit.c | 32 +++++++++++++++++++++++++-------\n 1 file changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 05594fa8ab..d8cec75888 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1188,6 +1188,19 @@ static void finalize_deferred_config(struct wt_status *s)\n \t\ts->ahead_behind_flags = AHEAD_BEHIND_FULL;\n }\n \n+static void check_fixup_reword_options(int argc, const char *argv[]) {\n+\tif (whence != FROM_COMMIT) {\n+\t\tif (whence == FROM_MERGE)\n+\t\t\tdie(_(\"You are in the middle of a merge -- cannot reword.\"));\n+\t\telse if (is_from_cherry_pick(whence))\n+\t\t\tdie(_(\"You are in the middle of a cherry-pick -- cannot reword.\"));\n+\t}\n+\tif (argc)\n+\t\tdie(_(\"cannot combine reword option of --fixup with path '%s'\"), *argv);\n+\tif (patch_interactive || interactive || all || also || only)\n+\t\tdie(_(\"reword option of --fixup is mutually exclusive with --patch/--interactive/--all/--include/--only\"));\n+}\n+\n /* returns the length of intial segment of alpha characters only */\n static size_t get_alpha_len(char *fixup_message) {\n \tconst char alphas[] = \"abcdefghijklmnopqrstuvwxyz\";\n@@ -1276,17 +1289,22 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\t * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n \t\t * a suboption of `--fixup`.\n \t\t *\n-\t\t * As `amend` suboption contains only alpha character.\n-\t\t * So check if first non alpha character in fixup_message\n-\t\t * is ':'.\n+\t\t * As `amend`/`reword` suboptions contains only alpha\n+\t\t * characters. So check if first non alpha character\n+\t\t * in fixup_message is ':'.\n \t\t */\n \t\tsize_t len = get_alpha_len(fixup_message);\n \t\tif (len && fixup_message[len] == ':') {\n \t\t\tfixup_message[len++] = '\\0';\n \t\t\tfixup_commit = fixup_message + len;\n-\t\t\tif (!strcmp(\"amend\", fixup_message)) {\n+\t\t\tif (!strcmp(\"amend\", fixup_message) ||\n+\t\t\t !strcmp(\"reword\", fixup_message)) {\n \t\t\t\tfixup_prefix = \"amend\";\n \t\t\t\tallow_empty = 1;\n+\t\t\t\tif (*fixup_message == 'r') {\n+\t\t\t\t\tcheck_fixup_reword_options(argc, argv);\n+\t\t\t\t\tonly = 1;\n+\t\t\t\t}\n \t\t\t} else {\n \t\t\t\tdie(_(\"unknown option: --fixup=%s:%s\"), fixup_message, fixup_commit);\n \t\t\t}\n@@ -1575,10 +1593,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING('c', \"reedit-message\", &edit_message, N_(\"commit\"), N_(\"reuse and edit message from specified commit\")),\n \t\tOPT_STRING('C', \"reuse-message\", &use_message, N_(\"commit\"), N_(\"reuse message from specified commit\")),\n \t\t/*\n-\t\t * TRANSLATORS: Leave \"[amend:]\" as-is, and\n-\t\t * only translate .\n+\t\t * TRANSLATORS: Leave \"[(amend|reword):]\" as-is,\n+\t\t * and only translate .\n \t\t */\n-\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[amend:]commit\"), N_(\"use autosquash formatted message to fixup or amend specified commit\")),\n+\t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n-- \n2.29.0.rc1\n\n"},{"id":"418730","messageId":"20210310194306.32565-7-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:10Z","receivedAt":"2021-03-10T19:45:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"Mentored-by: Christian Couder \nMentored-by: Phillip Wood \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Charvi Mendiratta \n---\n Documentation/git-commit.txt | 41 ++++++++++++++++++++++++++++++------\n Documentation/git-rebase.txt | 21 +++++++++---------\n 2 files changed, 46 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 17150fa7ea..2de8255bdb 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git commit' [-a | --interactive | --patch] [-s] [-v] [-u] [--amend]\n-\t [--dry-run] [(-c | -C | --fixup | --squash) ]\n+\t [--dry-run] [(-c | -C | --squash) | --fixup [(amend|reword):])]\n \t [-F | -m ] [--reset-author] [--allow-empty]\n \t [--allow-empty-message] [--no-verify] [-e] [--author=]\n \t [--date=] [--cleanup=] [--[no-]status]\n@@ -86,11 +86,40 @@ OPTIONS\n \tLike '-C', but with `-c` the editor is invoked, so that\n \tthe user can further edit the commit message.\n \n---fixup=::\n-\tConstruct a commit message for use with `rebase --autosquash`.\n-\tThe commit message will be the subject line from the specified\n-\tcommit with a prefix of \"fixup! \". See linkgit:git-rebase[1]\n-\tfor details.\n+--fixup=[(amend|reword):]::\n+\tConstruct a new commit for use with `rebase --autosquash`,\n+\twhich fixes the specified commit. The plain form\n+\t`--fixup=` creates a \"fixup!\" commit, that allows\n+\tto fixup only the content of the specified commit and leave\n+\tit's commit log message untouched. When used with `amend:`\n+\tor `reword:`, it creates \"amend!\" commit that is like \"fixup!\"\n+\tcommit but it allows to fixup both the content and the commit\n+\tlog message of the specified commit. The commit log message of\n+\tthe specified commit is fixed implicitly by replacing it with\n+\tthe \"amend!\" commit's message body upon `rebase --autosquash`.\n++\n+The resulting \"fixup!\" commit message will be the subject line\n+from the specified commit with a prefix of \"fixup!\". Can be used\n+with additional commit message option `-m`.\n++\n+The `--fixup=amend:` form creates an \"amend!\" commit where\n+its commit message subject will be the subject line from the\n+specified commit with a prefix of \"amend!\" and the message body\n+will be commit log message of the specified commit. It also invokes\n+an editor seeded with the log message of the \"amend!\" commit to\n+allow to edit further. It refuses to create \"amend!\" commit if it's\n+commit message body is empty unless used with the\n+`--allow-empty-message` option.\n++\n+The `--fixup=reword:` aliases `--fixup=amend: --only`\n+and it also creates an \"amend!\" commit, but here it records the same\n+tree as `HEAD`, i.e. it does not take any staged changes and only allows\n+to fixup the commit message of the specified commit. It will reword the\n+specified commit when it is rebased with `--autosquash`.\n++\n+Also, after fixing the commit using `--fixup`, with or without option\n+and rebased with `--autosquash`, the authorship of the original commit\n+remains unchanged. See linkgit:git-rebase[1] for details.\n \n --squash=::\n \tConstruct a commit message for use with `rebase --autosquash`.\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8bfa5a9272..f08ae27e2a 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -593,16 +593,17 @@ See also INCOMPATIBLE OPTIONS below.\n \n --autosquash::\n --no-autosquash::\n-\tWhen the commit log message begins with \"squash! ...\" (or\n-\t\"fixup! ...\"), and there is already a commit in the todo list that\n-\tmatches the same `...`, automatically modify the todo list of rebase\n-\t-i so that the commit marked for squashing comes right after the\n-\tcommit to be modified, and change the action of the moved commit\n-\tfrom `pick` to `squash` (or `fixup`). A commit matches the `...` if\n-\tthe commit subject matches, or if the `...` refers to the commit's\n-\thash. As a fall-back, partial matches of the commit subject work,\n-\ttoo. The recommended way to create fixup/squash commits is by using\n-\tthe `--fixup`/`--squash` options of linkgit:git-commit[1].\n+\tWhen the commit log message begins with \"squash! ...\" or \"fixup! ...\"\n+\tor \"amend! ...\", and there is already a commit in the todo list that\n+\tmatches the same `...`, automatically modify the todo list of\n+\t`rebase -i`, so that the commit marked for squashing comes right after\n+\tthe commit to be modified, and change the action of the moved commit\n+\tfrom `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit\n+\tmatches the `...` if the commit subject matches, or if the `...` refers\n+\tto the commit's hash. As a fall-back, partial matches of the commit\n+\tsubject work, too. The recommended way to create fixup/amend/squash\n+\tcommits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`\n+\tand `--squash` options respectively of linkgit:git-commit[1].\n +\n If the `--autosquash` option is enabled by default using the\n configuration variable `rebase.autoSquash`, this option can be\n-- \n2.29.0.rc1\n\n"},{"id":"418731","messageId":"20210310194306.32565-6-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 5/6] t3437: use --fixup with options to create amend! commit","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:09Z","receivedAt":"2021-03-10T19:45:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"We taught `git commit --fixup` to create \"amend!\" commit. Let's also\nupdate the tests and use it to setup the rebase tests.\n\nMentored-by: Christian Couder \nMentored-by: Phillip Wood \nSigned-off-by: Charvi Mendiratta \n---\n t/t3437-rebase-fixup-options.sh | 30 +++---------------------------\n 1 file changed, 3 insertions(+), 27 deletions(-)\n\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex a5a20354e3..d0bdc7ed02 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -72,40 +72,16 @@ test_expect_success 'setup' '\n \tgit commit --fixup=HEAD -a &&\n \tgit tag B1 &&\n \ttest_tick &&\n-\tgit commit --allow-empty -F - <<-EOF &&\n-\tamend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 1\" git commit --fixup=reword:B &&\n \ttest_tick &&\n-\tgit commit --allow-empty -F - <<-EOF &&\n-\tamend! amend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\t$EMPTY\n-\tedited 2\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 2\" git commit --fixup=reword:HEAD &&\n \techo B2 >B &&\n \ttest_tick &&\n \tFAKE_COMMIT_AMEND=\"edited squash\" git commit --squash=HEAD -a &&\n \tgit tag B2 &&\n \techo B3 >B &&\n \ttest_tick &&\n-\tgit commit -a -F - <<-EOF &&\n-\tamend! amend! amend! B\n-\t$EMPTY\n-\tB\n-\t$EMPTY\n-\tedited 1\n-\t$EMPTY\n-\tedited 2\n-\t$EMPTY\n-\tedited 3\n-\tEOF\n+\tFAKE_COMMIT_AMEND=\"edited 3\" git commit -a --fixup=amend:HEAD^ &&\n \tgit tag B3 &&\n \n \tGIT_AUTHOR_NAME=\"Rebase Author\" &&\n-- \n2.29.0.rc1\n\n"},{"id":"418732","messageId":"20210310194306.32565-5-charvi077@gmail.com","threadId":"55235","inReplyTo":"20210301084512.27170-1-charvi077@gmail.com","subject":"[PATCH v4 4/6] t7500: add tests for --fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-10T19:43:08Z","receivedAt":"2021-03-10T19:45:44Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"Mentored-by: Christian Couder \nMentored-by: Phillip Wood \nSigned-off-by: Charvi Mendiratta \n---\n t/t7500-commit-template-squash-signoff.sh | 159 ++++++++++++++++++++++\n 1 file changed, 159 insertions(+)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 6d19ece05d..05e43a31d3 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -9,6 +9,8 @@ Tests for template, signoff, squash and -F functions.'\n \n . ./test-lib.sh\n \n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n+\n commit_msg_is () {\n \texpect=commit_msg_is.expect\n \tactual=commit_msg_is.actual\n@@ -279,6 +281,163 @@ test_expect_success 'commit --fixup -m\"something\" -m\"extra\"' '\n \n extra\"\n '\n+get_commit_msg () {\n+\trev=\"$1\" &&\n+\tgit log -1 --pretty=format:\"%B\" \"$rev\"\n+}\n+\n+test_expect_success 'commit --fixup=amend: creates amend! commit' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success '--fixup=amend: --only ignores staged changes' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~ --only\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&\n+\ttest_cmp_rev HEAD@{1} HEAD^ &&\n+\ttest_expect_code 1 git diff --cached --exit-code &&\n+\tgit cat-file blob :foo >actual &&\n+\ttest_cmp foo actual\n+'\n+\n+test_expect_success '--fixup=reword: ignores staged changes' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited\" \\\n+\t\t\tgit commit --fixup=reword:HEAD~\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_cmp_rev HEAD@{1}^{tree} HEAD^{tree} &&\n+\ttest_cmp_rev HEAD@{1} HEAD^ &&\n+\ttest_expect_code 1 git diff --cached --exit-code &&\n+\tgit cat-file blob :foo >actual &&\n+\ttest_cmp foo actual\n+'\n+\n+test_expect_success '--fixup=reword: error out with -m option' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine -m with --fixup:reword\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -m \"reword commit message\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--fixup=amend: error out with -m option' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine -m with --fixup:amend\" >expect &&\n+\ttest_must_fail git commit --fixup=amend:HEAD~ -m \"amend commit message\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'consecutive amend! commits remove amend! line from commit msg body' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! amend! $(git log -1 --format=%s HEAD~)\n+\n+\t$(get_commit_msg HEAD~)\n+\n+\tedited 1\n+\n+\tedited 2\n+\tEOF\n+\techo \"reword new commit message\" >actual &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_AMEND=\"edited 1\" \\\n+\t\t\tgit commit --fixup=reword:HEAD~ &&\n+\t\tFAKE_COMMIT_AMEND=\"edited 2\" \\\n+\t\t\tgit commit --fixup=reword:HEAD\n+\t) &&\n+\tget_commit_msg HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'deny to create amend! commit if its commit msg body is empty' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"Aborting commit due to empty commit message body.\" >expected &&\n+\t(\n+\t\tset_fake_editor &&\n+\t\ttest_must_fail env FAKE_COMMIT_MESSAGE=\"amend! target message subject line\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~ 2>actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\tcat >expected <<-EOF &&\n+\tamend! $(git log -1 --format=%s HEAD~)\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_COMMIT_MESSAGE=\"amend! target message subject line\" \\\n+\t\t\tgit commit --fixup=amend:HEAD~ --allow-empty-message &&\n+\t\tget_commit_msg HEAD >actual\n+\t) &&\n+\ttest_cmp expected actual\n+'\n+\n+test_fixup_reword_opt () {\n+\ttest_expect_success C_LOCALE_OUTPUT \"--fixup=reword: incompatible with $1\" \"\n+\t\techo 'fatal: reword option of --fixup is mutually exclusive with'\\\n+\t\t\t'--patch/--interactive/--all/--include/--only' >expect &&\n+\t\ttest_must_fail git commit --fixup=reword:HEAD~ $1 2>actual &&\n+\t\ttest_cmp expect actual\n+\t\"\n+}\n+\n+for opt in --all --include --only --interactive --patch\n+do\n+\ttest_fixup_reword_opt $opt\n+done\n+\n+test_expect_success '--fixup=reword: give error with pathsec' '\n+\tcommit_for_rebase_autosquash_setup &&\n+\techo \"fatal: cannot combine reword option of --fixup with path '\\''foo'\\''\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -- foo 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--fixup=reword: -F give error message' '\n+\techo \"fatal: Only one of -c/-C/-F/--fixup can be used.\" >expect &&\n+\ttest_must_fail git commit --fixup=reword:HEAD~ -F msg 2>actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'commit --squash works with -F' '\n \tcommit_for_rebase_autosquash_setup &&\n-- \n2.29.0.rc1\n\n"},{"id":"418762","messageId":"xmqqwnueh6ps.fsf@gitster.g","threadId":"55235","inReplyTo":"20210310194306.32565-7-charvi077@gmail.com","subject":"Re: [PATCH v4 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-11T00:30:55Z","receivedAt":"2021-03-11T00:31:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> ---fixup=::\n> -\tConstruct a commit message for use with `rebase --autosquash`.\n> -\tThe commit message will be the subject line from the specified\n> -\tcommit with a prefix of \"fixup! \". See linkgit:git-rebase[1]\n> -\tfor details.\n\nOverall, the text in this round reads much better than the previous\nround.\n\n> +--fixup=[(amend|reword):]::\n> +\tConstruct a new commit for use with `rebase --autosquash`,\n> +\twhich fixes the specified commit. The plain form\n> +\t`--fixup=` creates a \"fixup!\" commit, that allows\n> +\tto fixup only the content of the specified commit and leave\n> +\tit's commit log message untouched. When used with `amend:`\n> +\tor `reword:`, it creates \"amend!\" commit that is like \"fixup!\"\n> +\tcommit but it allows to fixup both the content and the commit\n> +\tlog message of the specified commit. The commit log message of\n> +\tthe specified commit is fixed implicitly by replacing it with\n> +\tthe \"amend!\" commit's message body upon `rebase --autosquash`.\n\nRewriting\n\n \"is fixed implicitly by replacing it with\" -> \"is replaced by\"\n\nmay make it easier to follow.\n\nThanks.\n"},{"id":"418763","messageId":"xmqqpn06h6os.fsf@gitster.g","threadId":"55235","inReplyTo":"20210310194306.32565-4-charvi077@gmail.com","subject":"Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-11T00:31:31Z","receivedAt":"2021-03-11T00:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> \t\t * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n> \t\t * a suboption of `--fixup`.\n> \t\t *\n> -\t\t * As `amend` suboption contains only alpha character.\n> -\t\t * So check if first non alpha character in fixup_message\n> -\t\t * is ':'.\n> +\t\t * As `amend`/`reword` suboptions contains only alpha\n> +\t\t * characters. So check if first non alpha character\n> +\t\t * in fixup_message is ':'.\n\nSorry, but I cannot quite follow the logic. \n\n\tWe limit --fixup's suboptions to only alpha characters. If\n\tthe first character after a len of alpha is colon, then the\n\tpart before the colon may be a known suboption name like\n\t`amend` or `reword`, or a misspelt suboption name.\n\n\tOtherwise, we are dealing with --fixup= that happens\n\tto have a colon in object name.\n\nperhaps?\n"},{"id":"418821","messageId":"CAPSFM5dqRbuoCvEFcHQPSQt+EK1_pBVQ--f42jR+GNqCFd_SOg@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqpn06h6os.fsf@gitster.g","subject":"Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-11T04:01:59Z","receivedAt":"2021-03-11T04:02:54Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Thu, 11 Mar 2021 at 06:01, Junio C Hamano wrote:\n>\n> Charvi Mendiratta writes:\n>\n> > * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n> > * a suboption of `--fixup`.\n> > *\n> > - * As `amend` suboption contains only alpha character.\n> > - * So check if first non alpha character in fixup_message\n> > - * is ':'.\n> > + * As `amend`/`reword` suboptions contains only alpha\n> > + * characters. So check if first non alpha character\n> > + * in fixup_message is ':'.\n>\n> Sorry, but I cannot quite follow the logic.\n>\n> We limit --fixup's suboptions to only alpha characters. If\n> the first character after a len of alpha is colon, then the\n> part before the colon may be a known suboption name like\n> `amend` or `reword`, or a misspelt suboption name.\n>\n> Otherwise, we are dealing with --fixup= that happens\n> to have a colon in object name.\n>\n> perhaps?\n\nYes, Agree. Here I just intend to mention the special case\n\"--fixup=HEAD^{/^area: string}\" because of which we chose the method\nto check if first non alpha char is ':' instead of directly checking\nthe suboption like (skip_prefix(msg, \"amend:\", &arg). So maybe we can\nreword it like\n\n- To check if fixup_message that contains ':' is a commit\n- reference for example: --fixup=\"HEAD^{/^area: string}\" or\n- a suboption of `--fixup`.\n+ fixup_message could be a commit reference for example:\n+ --fixup=\"HEAD^{/^area:string}\" or a suboption of `--fixup`.\n+\n+ As `amend` ...\n\nThanks and Regards,\nCharvi\n"},{"id":"418822","messageId":"CAPSFM5c7ULGxThtFjPam33wMzK3kprds97zwJ63stc_Ct24nmQ@mail.gmail.com","threadId":"55235","inReplyTo":"xmqqwnueh6ps.fsf@gitster.g","subject":"Re: [PATCH v4 6/6] doc/git-commit: add documentation for fixup=[amend|reword] options","fromName":"Charvi Mendiratta","fromEmail":"charvi077@gmail.com","sentAt":"2021-03-11T04:02:13Z","receivedAt":"2021-03-11T04:03:26Z","isPatch":true,"sender":{"key":"charvi077@gmail.com","avatar":"https://avatars.githubusercontent.com/u/32788121?v=4"},"body":"On Thu, 11 Mar 2021 at 06:01, Junio C Hamano wrote:\n\n> > +--fixup=[(amend|reword):]::\n> > + Construct a new commit for use with `rebase --autosquash`,\n> > + which fixes the specified commit. The plain form\n> > + `--fixup=` creates a \"fixup!\" commit, that allows\n> > + to fixup only the content of the specified commit and leave\n> > + it's commit log message untouched. When used with `amend:`\n> > + or `reword:`, it creates \"amend!\" commit that is like \"fixup!\"\n> > + commit but it allows to fixup both the content and the commit\n> > + log message of the specified commit. The commit log message of\n> > + the specified commit is fixed implicitly by replacing it with\n> > + the \"amend!\" commit's message body upon `rebase --autosquash`.\n>\n> Rewriting\n>\n> \"is fixed implicitly by replacing it with\" -> \"is replaced by\"\n>\n> may make it easier to follow.\n>\n\nOkay I will replace it.\n\nThanks and Regards,\nCharvi\n"},{"id":"418827","messageId":"xmqqa6rafdyg.fsf@gitster.g","threadId":"55235","inReplyTo":"CAPSFM5dqRbuoCvEFcHQPSQt+EK1_pBVQ--f42jR+GNqCFd_SOg@mail.gmail.com","subject":"Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-11T05:37:27Z","receivedAt":"2021-03-11T05:38:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charvi Mendiratta writes:\n\n> On Thu, 11 Mar 2021 at 06:01, Junio C Hamano wrote:\n>>\n>> Charvi Mendiratta writes:\n>>\n>> > * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n>> > * a suboption of `--fixup`.\n>> > *\n>> > - * As `amend` suboption contains only alpha character.\n>> > - * So check if first non alpha character in fixup_message\n>> > - * is ':'.\n>> > + * As `amend`/`reword` suboptions contains only alpha\n>> > + * characters. So check if first non alpha character\n>> > + * in fixup_message is ':'.\n>>\n>> Sorry, but I cannot quite follow the logic.\n>>\n>> We limit --fixup's suboptions to only alpha characters. If\n>> the first character after a len of alpha is colon, then the\n>> part before the colon may be a known suboption name like\n>> `amend` or `reword`, or a misspelt suboption name.\n>>\n>> Otherwise, we are dealing with --fixup= that happens\n>> to have a colon in object name.\n>>\n>> perhaps?\n>\n> Yes, Agree. Here I just intend to mention the special case\n> \"--fixup=HEAD^{/^area: string}\" because of which we chose the method\n> to check if first non alpha char is ':' instead of directly checking\n> the suboption like (skip_prefix(msg, \"amend:\", &arg). So maybe we can\n> reword it like\n>\n> - To check if fixup_message that contains ':' is a commit\n> - reference for example: --fixup=\"HEAD^{/^area: string}\" or\n> - a suboption of `--fixup`.\n> + fixup_message could be a commit reference for example:\n> + --fixup=\"HEAD^{/^area:string}\" or a suboption of `--fixup`.\n> +\n> + As `amend` ...\n\nMy suggestion primarily started a reaction to that \"As `amend`...\"\nwhich was not gramatically complete sentence, and I ended up\nrewriting everything after \"As `amend`...\"\n\nBut re-reading what is in the paragraph before, I tend to think that\nit places too much stress on 'colon' and should be removed.\n\nThe comment is about what is being parsed, so\n\n We limit --fixup's suboptions to only alpha characters. If\n the first character after a run of alpha is colon, then the\n part before the colon may be a known suboption name like\n `amend` or `reword`, or a misspelt suboption name. In\n either case, we treat it as --fixup=:\n\n Otherwise, we are dealing with --fixup=.\n\nwould be good. The code, when it decides it is not in the\n--fixup=: form but it is --fixup=, does not\neven care about a colon, so there is no need to mention colon in the\n\"Otherwise\" part.\n"},{"id":"418830","messageId":"CAPig+cRxZFV7DDtnYOxRqEYU4PwXK8gb2HSehDyGorqdgsmoNQ@mail.gmail.com","threadId":"55235","inReplyTo":"20210310194306.32565-3-charvi077@gmail.com","subject":"Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-11T06:25:44Z","receivedAt":"2021-03-11T06:26:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 10, 2021 at 2:44 PM Charvi Mendiratta wrote:\n> `git commit --fixup=amend:` will create an \"amend!\" commit.\n> The resulting commit message subject will be \"amend! ...\" where\n> \"...\" is the subject line of and the initial message\n> body will be 's message.\n>\n> The \"amend!\" commit when rebased with --autosquash will fixup the\n> contents and replace the commit message of with the\n> \"amend!\" commit's message body.\n> [...]\n> Signed-off-by: Charvi Mendiratta \n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb)\n> +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,\n> + struct pretty_print_context *ctx) {\n> +\n> + const char *buffer, *subject, *fmt;\n\nTwo style nits:\n\n* opening curly brace of function goes on its own line\n\n* we don't normally have a blank line at the top of the function body\npreceding the declarations\n\nSo:\n\n static int prepare_amend_commit(...)\n {\n const char *buffer, *subject, *fmt;\n\n> + buffer = get_commit_buffer(commit, NULL);\n> + find_commit_subject(buffer, &subject);\n> + /*\n> + * If we amend the 'amend!' commit then we don't want to\n> + * duplicate the subject line.\n> + */\n> + fmt = starts_with(subject, \"amend!\") ? \"%b\" : \"%B\";\n> + format_commit_message(commit, fmt, sb, ctx);\n> + unuse_commit_buffer(commit, buffer);\n> + return 0;\n> +}\n\nWhat is the significance of this function's return value? At least in\nthis patch, the single caller of this function ignores the return\nvalue, which suggests that the function need not return any value.\nWill a later patch add other possible return values to indicate an\nerror or something?\n\n> @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> + char *fmt = xstrfmt(\"%s! %%s\\n\\n\", fixup_prefix);\n> + commit = lookup_commit_reference_by_name(fixup_commit);\n> if (!commit)\n> + die(_(\"could not lookup commit %s\"), fixup_commit);\n> ctx.output_encoding = get_commit_output_encoding();\n> + format_commit_message(commit, fmt, &sb, &ctx);\n> + free(fmt);\n\nNit: it would reduce the cognitive load slightly if `fmt` is prepared\njust before it is used rather than being prepared at the top of the\nblock:\n\n fmt = xstrfmt(\"%s! %%s\\n\\n\", fixup_prefix);\n format_commit_message(commit, fmt, &sb, &ctx);\n free(fmt);\n\nSubjective and not at all worth a re-roll.\n\n> @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[],\n> + if (fixup_message) {\n> + /*\n> + * To check if fixup_message that contains ':' is a commit\n> + * reference for example: --fixup=\"HEAD^{/^area: string}\" or\n> + * a suboption of `--fixup`.\n> + *\n> + * As `amend` suboption contains only alpha character.\n> + * So check if first non alpha character in fixup_message\n> + * is ':'.\n> + */\n\nI have a tough time figuring out what this comment is trying to say,\nand I don't think I would have understood it if Junio had not already\nexplained earlier in this thread why this code is as complex as it is\n(rather than using, say, skip_prefix()). Perhaps the entire comment\ncan be replaced with this:\n\n Extract