{"thread":{"id":"51479","subject":"[PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","startedAt":"2019-07-13T05:18:38Z","lastAt":"2019-07-18T02:33:10Z","messageCount":7,"participants":["Edmundo Carmona Antoranz","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"378916","messageId":"20190713051804.12893-1-eantoranz@gmail.com","threadId":"51479","inReplyTo":null,"subject":"[PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-13T05:18:04Z","receivedAt":"2019-07-13T05:18:38Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Using --squash made git stop regardless of conflicts so that the\nuser could finish the operation with a later call to git-commit.\n\nNow --squash allows for the operation to finish with the new revision\nif there are no conflicts (can still be controlled with --no-commit).\n\nOption -m can be used to defined the message for the revision instead\nof the default message that contains all squashed revisions info.\n\nSigned-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n---\n builtin/merge.c | 109 +++++++++++++++++++++++++-----------------------\n 1 file changed, 57 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex aad5a9504c..66fd57de02 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -64,6 +64,7 @@ static int option_edit = -1;\n static int allow_trivial = 1, have_message, verify_signatures;\n static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n+static struct strbuf squash_msg = STRBUF_INIT;\n static struct strategy **use_strategies;\n static size_t use_strategies_nr, use_strategies_alloc;\n static const char **xopts;\n@@ -390,39 +391,38 @@ static void finish_up_to_date(const char *msg)\n static void squash_message(struct commit *commit, struct commit_list *remoteheads)\n {\n \tstruct rev_info rev;\n-\tstruct strbuf out = STRBUF_INIT;\n \tstruct commit_list *j;\n \tstruct pretty_print_context ctx = {0};\n \n-\tprintf(_(\"Squash commit -- not updating HEAD\\n\"));\n-\n-\trepo_init_revisions(the_repository, &rev, NULL);\n-\trev.ignore_merges = 1;\n-\trev.commit_format = CMIT_FMT_MEDIUM;\n-\n-\tcommit->object.flags |= UNINTERESTING;\n-\tadd_pending_object(&rev, &commit->object, NULL);\n-\n-\tfor (j = remoteheads; j; j = j->next)\n-\t\tadd_pending_object(&rev, &j->item->object, NULL);\n-\n-\tsetup_revisions(0, NULL, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n-\t\tdie(_(\"revision walk setup failed\"));\n-\n-\tctx.abbrev = rev.abbrev;\n-\tctx.date_mode = rev.date_mode;\n-\tctx.fmt = rev.commit_format;\n-\n-\tstrbuf_addstr(&out, \"Squashed commit of the following:\\n\");\n-\twhile ((commit = get_revision(&rev)) != NULL) {\n-\t\tstrbuf_addch(&out, '\\n');\n-\t\tstrbuf_addf(&out, \"commit %s\\n\",\n-\t\t\toid_to_hex(&commit->object.oid));\n-\t\tpretty_print_commit(&ctx, commit, &out);\n+\tif (merge_msg.len)\n+\t\tsquash_msg = merge_msg;\n+\telse {\n+\t\trepo_init_revisions(the_repository, &rev, NULL);\n+\t\trev.ignore_merges = 1;\n+\t\trev.commit_format = CMIT_FMT_MEDIUM;\n+\n+\t\tcommit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&rev, &commit->object, NULL);\n+\n+\t\tfor (j = remoteheads; j; j = j->next)\n+\t\t\tadd_pending_object(&rev, &j->item->object, NULL);\n+\n+\t\tsetup_revisions(0, NULL, &rev, NULL);\n+\t\tif (prepare_revision_walk(&rev))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n+\n+\t\tctx.abbrev = rev.abbrev;\n+\t\tctx.date_mode = rev.date_mode;\n+\t\tctx.fmt = rev.commit_format;\n+\n+\t\tstrbuf_addstr(&squash_msg, \"Squashed commit of the following:\\n\");\n+\t\twhile ((commit = get_revision(&rev)) != NULL) {\n+\t\t\tstrbuf_addch(&squash_msg, '\\n');\n+\t\t\tstrbuf_addf(&squash_msg, \"commit %s\\n\",\n+\t\t\t\toid_to_hex(&commit->object.oid));\n+\t\t\tpretty_print_commit(&ctx, commit, &squash_msg);\n+\t\t}\n \t}\n-\twrite_file_buf(git_path_squash_msg(the_repository), out.buf, out.len);\n-\tstrbuf_release(&out);\n }\n \n static void finish(struct commit *head_commit,\n@@ -440,8 +440,11 @@ static void finish(struct commit *head_commit,\n \t\tstrbuf_addf(&reflog_message, \"%s: %s\",\n \t\t\tgetenv(\"GIT_REFLOG_ACTION\"), msg);\n \t}\n-\tif (squash) {\n+\tif (squash && !squash_msg.len) {\n \t\tsquash_message(head_commit, remoteheads);\n+\t\twrite_file_buf(git_path_squash_msg(the_repository), squash_msg.buf, squash_msg.len);\n+\t\tif (option_commit > 0)\n+\t\t\tprintf(_(\"Squash conflicts -- not updating HEAD\\n\"));\n \t} else {\n \t\tif (verbosity >= 0 && !merge_msg.len)\n \t\t\tprintf(_(\"No merge message -- not updating HEAD\\n\"));\n@@ -893,14 +896,23 @@ static int finish_automerge(struct commit *head,\n \tstruct object_id result_commit;\n \n \tfree_commit_list(common);\n-\tparents = remoteheads;\n-\tif (!head_subsumed || fast_forward == FF_NO)\n-\t\tcommit_list_insert(head, &parents);\n-\tprepare_to_commit(remoteheads);\n-\tif (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,\n-\t\t\t&result_commit, NULL, sign_commit))\n-\t\tdie(_(\"failed to write commit object\"));\n-\tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n+\tif (squash) {\n+\t\tsquash_message(head, remoteheads);\n+\t\tparents = commit_list_insert(head, &parents);\n+\t\tif (commit_tree(squash_msg.buf, squash_msg.len, result_tree, parents,\n+\t\t\t\t&result_commit, NULL, sign_commit))\n+\t\t\tdie(_(\"failed to write commit object on squash\"));\n+\t} else {\n+\t\tparents = remoteheads;\n+\t\tif (!head_subsumed || fast_forward == FF_NO)\n+\t\t\tcommit_list_insert(head, &parents);\n+\t\tprepare_to_commit(remoteheads);\n+\t\tif (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,\n+\t\t\t\t&result_commit, NULL, sign_commit))\n+\t\t\tdie(_(\"failed to write commit object\"));\n+\t}\n+\tstrbuf_addf(&buf, \"Merge made by the '%s' strategy\", wt_strategy);\n+\tstrbuf_addstr(&buf, squash ? \" (squashed).\" : \".\");\n \tfinish(head, remoteheads, &result_commit, buf.buf);\n \tstrbuf_release(&buf);\n \tremove_merge_branch_state(the_repository);\n@@ -1342,18 +1354,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (verbosity < 0)\n \t\tshow_diffstat = 0;\n \n-\tif (squash) {\n-\t\tif (fast_forward == FF_NO)\n-\t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n-\t\tif (option_commit > 0)\n-\t\t\tdie(_(\"You cannot combine --squash with --commit.\"));\n-\t\t/*\n-\t\t * squash can now silently disable option_commit - this is not\n-\t\t * a problem as it is only overriding the default, not a user\n-\t\t * supplied option.\n-\t\t */\n-\t\toption_commit = 0;\n-\t}\n+\tif (squash && fast_forward == FF_NO)\n+\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n \n \tif (option_commit < 0)\n \t\toption_commit = 1;\n@@ -1682,8 +1684,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\twrite_merge_state(remoteheads);\n \n \tif (merge_was_ok)\n-\t\tfprintf(stderr, _(\"Automatic merge went well; \"\n-\t\t\t\"stopped before committing as requested\\n\"));\n+\t\tif (!option_commit)\n+\t\t\tfprintf(stderr, _(\"Automatic merge went well; \"\n+\t\t\t\t\"stopped before committing as requested\\n\"));\n+\t\telse\n+\t\t\t;\n \telse\n \t\tret = suggest_conflicts();\n \n-- \n2.20.1\n\n"},{"id":"378917","messageId":"CAOc6etb_XFbQWDHg3YRNiskkntS0ro2MYgXCfp6oPv4LutQFGA@mail.gmail.com","threadId":"51479","inReplyTo":"20190713051804.12893-1-eantoranz@gmail.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-13T05:27:06Z","receivedAt":"2019-07-13T05:27:20Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> @@ -1342,18 +1354,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>         if (verbosity < 0)\n>                 show_diffstat = 0;\n>\n> -       if (squash) {\n> -               if (fast_forward == FF_NO)\n> -                       die(_(\"You cannot combine --squash with --no-ff.\"));\n> -               if (option_commit > 0)\n> -                       die(_(\"You cannot combine --squash with --commit.\"));\n> -               /*\n> -                * squash can now silently disable option_commit - this is not\n> -                * a problem as it is only overriding the default, not a user\n> -                * supplied option.\n> -                */\n> -               option_commit = 0;\n> -       }\n> +       if (squash && fast_forward == FF_NO)\n> +               die(_(\"You cannot combine --squash with --no-ff.\"));\n>\n>         if (option_commit < 0)\n>                 option_commit = 1;\n\nOne question that I have is if it makes sense to set option_commit to\n0 if the user didn't specify --commit when using --squash, so that the\ncurrent behavior of git is not broken. Like you run merge --squash,\ngit will stop as it currently does... but it would be possible to run\nwith --squash --commit so that the revision is created if there are no\nissues to take care of (currently impossible, you would see that\nmessage saying \"You cannot combine --squash with --commit.\").\n"},{"id":"378925","messageId":"CAOc6eta-jX93k6twcrJOeRt+JHtLk4mUs7YD_bG=Ggvw4thAZQ@mail.gmail.com","threadId":"51479","inReplyTo":"20190713051804.12893-1-eantoranz@gmail.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-14T07:15:22Z","receivedAt":"2019-07-14T07:15:36Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n>\n> Option -m can be used to defined the message for the revision instead\n> of the default message that contains all squashed revisions info.\n>\n\nI have noticed that just adding the support for -m in squash is more\ncomplex than this patch is reaching so I think I will break this patch\ninto two parts:\n- squash in a shot if there are no conflicts\n- support -m with squash\nDisregard this patch, please.\n"},{"id":"378933","messageId":"xmqqblxw5tod.fsf@gitster-ct.c.googlers.com","threadId":"51479","inReplyTo":"CAOc6etb_XFbQWDHg3YRNiskkntS0ro2MYgXCfp6oPv4LutQFGA@mail.gmail.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-14T18:59:14Z","receivedAt":"2019-07-14T18:59:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:\n\n> One question that I have is if it makes sense to set option_commit\n> to 0 if the user didn't specify --commit when using --squash, so\n> that the current behavior of git is not broken.  \n\nIf you mean that \"git merge --squash <other args but not\n--[no-]commit>\" should behave identically with or without your\npatch, then I think the answer is definitely yes.\n\n> Like you run merge --squash, git will stop as it currently\n> does... but it would be possible to run with --squash --commit so\n> that the revision is created if there are no issues to take care\n> of (currently impossible, you would see that message saying \"You\n> cannot combine --squash with --commit.\").\n\nThat is exactly a safe way to extend the system by adding a new mode\nof operation in a backward compatible fashion.  Good thinking.\n\n"},{"id":"379025","messageId":"xmqq5zo01qnv.fsf@gitster-ct.c.googlers.com","threadId":"51479","inReplyTo":"CAOc6eta-jX93k6twcrJOeRt+JHtLk4mUs7YD_bG=Ggvw4thAZQ@mail.gmail.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T18:07:00Z","receivedAt":"2019-07-17T18:07:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:\n\n> On Fri, Jul 12, 2019 at 11:18 PM Edmundo Carmona Antoranz\n> <eantoranz@gmail.com> wrote:\n>>\n>> Option -m can be used to defined the message for the revision instead\n>> of the default message that contains all squashed revisions info.\n>>\n>\n> I have noticed that just adding the support for -m in squash is more\n> complex than this patch is reaching so I think I will break this patch\n> into two parts:\n> - squash in a shot if there are no conflicts\n> - support -m with squash\n> Disregard this patch, please.\n\nSure.  I started skimming and then gave up after seeing that quite a\nlot of code has been shuffled around without much explanation (e.g.\nprinting of \"Squash commit -- not updating HEAD\" is gone from the\ncallee and now it is a responsibility of the caller), making it\nharder than necessary to see if there is any unintended behaviour\nchange when the new feature is not in use.  Whatever you are trying,\nit does look like the change deserves to be split into a smaller\npieces to become more manageable.\n\nThanks.\n\n\n"},{"id":"379031","messageId":"CAOc6etYM6DSDQ_H=eJs1xuGU9a83kTe2-vEy9+FEgHobT77_Eg@mail.gmail.com","threadId":"51479","inReplyTo":"xmqq5zo01qnv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-18T00:41:36Z","receivedAt":"2019-07-18T00:46:27Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Jul 17, 2019 at 12:07 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Sure.  I started skimming and then gave up after seeing that quite a\n> lot of code has been shuffled around without much explanation (e.g.\n> printing of \"Squash commit -- not updating HEAD\" is gone from the\n> callee and now it is a responsibility of the caller), making it\n> harder than necessary to see if there is any unintended behaviour\n> change when the new feature is not in use.  Whatever you are trying,\n> it does look like the change deserves to be split into a smaller\n> pieces to become more manageable.\n>\n> Thanks.\n>\n\nyw!\n\nI'm focusing on the squash --commit part only. I think I'm close to\ngetting the desired result and now I'm taking a close look at the unit\ntests and a question came up on two tests of t7600-merge.sh:\n\nmerge c0 with c1 (squash)\nmerge c0 with c1 (squash, ff-only)\n\nIn both cases it's a FF (right?) so no new revision is created. The\nunit tests are requiring that $GIT_DIR/squash_msg have some content:\n\nnot ok 20 - merge c0 with c1 (squash, ff-only)\n#\n#               git reset --hard c0 &&\n#               git merge --squash --ff-only c1 &&\n#               verify_merge file result.1 &&\n#               verify_head $c0 &&\n#               verify_no_mergehead &&\n#               test_cmp squash.1 .git/SQUASH_MSG\n\n\nnot ok 18 - merge c0 with c1 (squash)\n#\n#               git reset --hard c0 &&\n#               git merge --squash c1 &&\n#               verify_merge file result.1 &&\n#               verify_head $c0 &&\n#               verify_no_mergehead &&\n#               test_cmp squash.1 .git/SQUASH_MSG\n\n\nDoes it make sense to keep this file in those two situations?\n"},{"id":"379032","messageId":"CAOc6etY0tGNeekO7n5pqE_emtRytuSE77o1-fPetpzZPpkfMtA@mail.gmail.com","threadId":"51479","inReplyTo":"CAOc6etYM6DSDQ_H=eJs1xuGU9a83kTe2-vEy9+FEgHobT77_Eg@mail.gmail.com","subject":"Re: [PATCH v2] builtin/merge: allow --squash to commit if there are no conflicts","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-18T02:32:56Z","receivedAt":"2019-07-18T02:33:10Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Wed, Jul 17, 2019 at 6:41 PM Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n>\n>\n> Does it make sense to keep this file in those two situations?\n\nyes it does. disregard the question.\n"}]}