{"thread":{"id":"41987","subject":"[PATCH v4 0/4] Add --base option to git-format-patch to record base tree info","startedAt":"2016-04-11T02:47:49Z","lastAt":"2016-04-14T16:23:17Z","messageCount":11,"participants":["Xiaolong Ye","Junio C Hamano","Ye Xiaolong"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"283121","messageId":"1460342873-28900-1-git-send-email-xiaolong.ye@intel.com","threadId":"41987","inReplyTo":null,"subject":"[PATCH v4 0/4] Add --base option to git-format-patch to record base tree info","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-11T02:47:49Z","receivedAt":"2016-04-11T02:47:49Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"V4 mainly addresses Junio's comments on V3, Changes include:\n\n - Polish up the documentation to make output files git-format-patch.1 and\n   git-format-patch.html more sensible.\n\n - Add error handling when base commit is not ancestor of revision list\n   specified in cmdline.\n\n - Specify topo order to do the traverse work, and show the base tree info\n   block in a more natural sequence.\n \n - If --base=auto is set and there is more than one best merge base, instead\n   of picking up a random one, it will error out for they may be complicated\n   situation such as criss-cross merges.\n\n - Add tests for the --base option and format.base configuration.\n\n - Fix a segfault error due to bases structure hasn't been initialized when\n   --base option is not set, Thanks for Ramsay's report.\n\nXiaolong Ye (4):\n  patch-ids: make commit_patch_id() a public helper function\n  format-patch: add '--base' option to record base tree info\n  format-patch: introduce --base=auto option\n  format-patch: introduce format.base configuration\n\n Documentation/git-format-patch.txt |  64 +++++++++++++++++++\n builtin/log.c                      | 123 +++++++++++++++++++++++++++++++++++++\n patch-ids.c                        |   2 +-\n patch-ids.h                        |   2 +\n t/t4014-format-patch.sh            |  48 +++++++++++++++\n 5 files changed, 238 insertions(+), 1 deletion(-)\n\n-- \n2.8.1.120.g24d6b3f\n\nbase-commit: 7b0d47b3b6b5b64e02a5aa06b0452cadcdb18355\n"},{"id":"283122","messageId":"1460342873-28900-2-git-send-email-xiaolong.ye@intel.com","threadId":"41987","inReplyTo":"1460342873-28900-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v4 1/4] patch-ids: make commit_patch_id() a public helper function","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-11T02:47:50Z","receivedAt":"2016-04-11T02:47:50Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Make commit_patch_id() available to other builtins.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n patch-ids.c | 2 +-\n patch-ids.h | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex b7b3e5a..a4d0016 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -4,7 +4,7 @@\n #include \"sha1-lookup.h\"\n #include \"patch-ids.h\"\n \n-static int commit_patch_id(struct commit *commit, struct diff_options *options,\n+int commit_patch_id(struct commit *commit, struct diff_options *options,\n \t\t    unsigned char *sha1)\n {\n \tif (commit->parents)\ndiff --git a/patch-ids.h b/patch-ids.h\nindex c8c7ca1..eeb56b3 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -13,6 +13,8 @@ struct patch_ids {\n \tstruct patch_id_bucket *patches;\n };\n \n+int commit_patch_id(struct commit *commit, struct diff_options *options,\n+\t\t    unsigned char *sha1);\n int init_patch_ids(struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);\n-- \n2.8.1.120.g24d6b3f\n"},{"id":"283123","messageId":"1460342873-28900-3-git-send-email-xiaolong.ye@intel.com","threadId":"41987","inReplyTo":"1460342873-28900-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v4 2/4] format-patch: add '--base' option to record base tree info","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-11T02:47:51Z","receivedAt":"2016-04-11T02:47:51Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Maintainers or third party testers may want to know the exact base tree\nthe patch series applies to. Teach git format-patch a '--base' option\nto record the base tree info and append it at the end of the_first_\nmessage(either the cover letter or the first patch in the series).\n\nThe base tree info consists of the \"base commit\", which is a well-known\ncommit that is part of the stable part of the project history everybody\nelse works off of, and zero or more \"prerequisite patches\", which are\nwell-known patches in flight that is not yet part of the \"base commit\"\nthat need to be applied on top of \"base commit\" in topological order\nbefore the patches can be applied.\n\nThe \"base commit\" is shown as \"base-commit: \" followed by the 40-hex of\nthe commit object name.  A \"prerequisite patch\" is shown as\n\"prerequisite-patch-id: \" followed by the 40-hex \"patch id\", which is a\nsum of SHA-1 of the file diffs associated with a patch, with whitespace\nand line numbers ignored, it's reasonably stable and unique.\n\nFor example, we have history where base is Z, with three prerequisites\nX-Y-Z, before the patch series A-B-C, i.e.\n\n\tP---X---Y---Z---A---B---C\n\nWe could say \"git format-patch --base=P -3 C\"(or variants thereof, e.g.\nwith \"--cover-letter\" of using \"Z..C\" instead of \"-3 C\" to specify the\nrange), then we could get base tree information block showing at the\nend of _first_ message as below:\n\n\tbase-commit: P\n\tprerequisite-patch-id: X\n\tprerequisite-patch-id: Y\n\tprerequisite-patch-id: Z\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Wu Fengguang <fengguang.wu@intel.com>\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n Documentation/git-format-patch.txt | 56 +++++++++++++++++++++++\n builtin/log.c                      | 92 ++++++++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh            | 15 +++++++\n 3 files changed, 163 insertions(+)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6821441..2a4c293 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -265,6 +265,11 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n   Output an all-zero hash in each patch's From header instead\n   of the hash of the commit.\n \n+--base=<commit>::\n+\tRecord the base tree information to identify the state the\n+\tpatch series applies to.  See the BASE TREE INFORMATION section\n+\tbelow for details.\n+\n --root::\n \tTreat the revision argument as a <revision range>, even if it\n \tis just a single commit (that would normally be treated as a\n@@ -520,6 +525,57 @@ This should help you to submit patches inline using KMail.\n 5. Back in the compose window: add whatever other text you wish to the\n    message, complete the addressing and subject fields, and press send.\n \n+BASE TREE INFORMATION\n+---------------------\n+\n+The base tree information block is used for maintainers or third party\n+testers to know the exact state the patch series applies to. It consists\n+of the 'base commit', which is a well-known commit that is part of the\n+stable part of the project history everybody else works off of, and zero\n+or more 'prerequisite patches', which are well-known patches in flight\n+that is not yet part of the 'base commit' that need to be applied on top\n+of 'base commit' in topological order before the patches can be applied.\n+\n+The 'base commit' is shown as \"base-commit: \" followed by the 40-hex of\n+the commit object name.  A 'prerequisite patch' is shown as\n+\"prerequisite-patch-id: \" followed by the 40-hex 'patch id', which is a\n+sum of SHA-1 of the file diffs associated with a patch, with whitespace\n+and line numbers ignored, it's reasonably stable and unique.\n+\n+For example, the patch submitter has a commit history of this shape:\n+\n+................................................\n+---P---X---Y---Z---A---B---C\n+................................................\n+\n+where 'P' is the well-known public commit (e.g. one in Linus's tree),\n+'X', 'Y', 'Z' are prerequisite patches in flight, and 'A', 'B', 'C'\n+are the work being sent out, the submitter could say `git format-patch\n+--base=P -3 C` (or variants thereof, e.g. with `--cover` or using\n+`Z..C` instead of `-3 C` to specify the range), and the identifiers\n+for P, X, Y, Z are appended at the end of the _first_ message (either\n+the cover letter or the first patch in the series). Then we could get\n+base tree information block showing at the end of _first_ message as\n+below:\n+\n+------------\n+base-commit: P\n+prerequisite-patch-id: X\n+prerequisite-patch-id: Y\n+prerequisite-patch-id: Z\n+------------\n+\n+For non-linear topology, such as\n+\n+................................................\n+---P---X---A---M---C\n+    \\         /\n+     Y---Z---B\n+................................................\n+\n+The submitter could also use `git format-patch --base=P -3 C` to generate\n+patches for A, B and C, and the identifiers for P, X, Y, Z are appended\n+at the end of the _first_ message.\n \n EXAMPLES\n --------\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9430b80..73bc36d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1191,6 +1191,85 @@ static int from_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+struct base_tree_info {\n+\tstruct object_id base_commit;\n+\tint nr_patch_id, alloc_patch_id;\n+\tstruct object_id *patch_id;\n+};\n+\n+static void prepare_bases(struct base_tree_info *bases,\n+\t\t\t  const char *base_commit,\n+\t\t\t  struct commit **list,\n+\t\t\t  int total)\n+{\n+\tstruct commit *base = NULL, *commit;\n+\tstruct rev_info revs;\n+\tstruct diff_options diffopt;\n+\tunsigned char sha1[20];\n+\tint i;\n+\n+\tdiff_setup(&diffopt);\n+\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n+\tdiff_setup_done(&diffopt);\n+\n+\tbase = lookup_commit_reference_by_name(base_commit);\n+\tif (!base)\n+\t\tdie(_(\"Unknown commit %s\"), base_commit);\n+\toidcpy(&bases->base_commit, &base->object.oid);\n+\n+\tinit_revisions(&revs, NULL);\n+\trevs.max_parents = 1;\n+\trevs.topo_order = 1;\n+\tfor (i = 0; i < total; i++) {\n+\t\tif (!in_merge_bases(base, list[i]) || base == list[i])\n+\t\t\tdie(_(\"base commit should be the ancestor of revision list\"));\n+\t\tlist[i]->object.flags &= ~UNINTERESTING;\n+\t\tadd_pending_object(&revs, &list[i]->object, \"rev_list\");\n+\t\tlist[i]->util = (void *)1;\n+\t}\n+\tbase->object.flags |= UNINTERESTING;\n+\tadd_pending_object(&revs, &base->object, \"base\");\n+\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(_(\"revision walk setup failed\"));\n+\t/*\n+\t * Traverse the prerequisite commits list,\n+\t * get the patch ids and stuff them in bases structure.\n+\t */\n+\twhile ((commit = get_revision(&revs)) != NULL) {\n+\t\tstruct object_id *patch_id;\n+\t\tif (commit->util)\n+\t\t\tcontinue;\n+\t\tif (commit_patch_id(commit, &diffopt, sha1))\n+\t\t\tdie(_(\"cannot get patch id\"));\n+\t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n+\t\tpatch_id = bases->patch_id + bases->nr_patch_id;\n+\t\thashcpy(patch_id->hash, sha1);\n+\t\tbases->nr_patch_id++;\n+\t}\n+}\n+\n+static void print_bases(struct base_tree_info *bases)\n+{\n+\tint i;\n+\n+\t/* Only do this once, either for the cover or for the first one */\n+\tif (is_null_oid(&bases->base_commit))\n+\t\treturn;\n+\n+\t/* Show the base commit */\n+\tprintf(\"base-commit: %s\\n\", oid_to_hex(&bases->base_commit));\n+\n+\t/* Show the prerequisite patches */\n+\tfor (i = bases->nr_patch_id - 1; i >= 0; i--)\n+\t\tprintf(\"prerequisite-patch-id: %s\\n\", oid_to_hex(&bases->patch_id[i]));\n+\n+\tfree(bases->patch_id);\n+\tbases->nr_patch_id = 0;\n+\tbases->alloc_patch_id = 0;\n+\toidclr(&bases->base_commit);\n+}\n+\n int cmd_format_patch(int argc, const char **argv, const char *prefix)\n {\n \tstruct commit *commit;\n@@ -1215,6 +1294,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint reroll_count = -1;\n \tchar *branch_name = NULL;\n \tchar *from = NULL;\n+\tchar *base_commit = NULL;\n+\tstruct base_tree_info bases;\n+\n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n \t\t\t    N_(\"use [PATCH n/m] even with a single patch\"),\n@@ -1277,6 +1359,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t    PARSE_OPT_OPTARG, thread_callback },\n \t\tOPT_STRING(0, \"signature\", &signature, N_(\"signature\"),\n \t\t\t    N_(\"add a signature\")),\n+\t\tOPT_STRING(0, \"base\", &base_commit, N_(\"base-commit\"),\n+\t\t\t   N_(\"add prerequisite tree info to the patch series\")),\n \t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n \t\t\t\tN_(\"add a signature from a file\")),\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n@@ -1513,6 +1597,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tsignature = strbuf_detach(&buf, NULL);\n \t}\n \n+\tmemset(&bases, 0, sizeof(bases));\n+\tif (base_commit) {\n+\t\treset_revision_walk();\n+\t\tprepare_bases(&bases, base_commit, list, nr);\n+\t}\n+\n \tif (in_reply_to || thread || cover_letter)\n \t\trev.ref_message_ids = xcalloc(1, sizeof(struct string_list));\n \tif (in_reply_to) {\n@@ -1526,6 +1616,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, \"cover\");\n \t\tmake_cover_letter(&rev, use_stdout,\n \t\t\t\t  origin, nr, list, branch_name, quiet);\n+\t\tprint_bases(&bases);\n \t\ttotal++;\n \t\tstart_number--;\n \t}\n@@ -1591,6 +1682,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t\t       rev.mime_boundary);\n \t\t\telse\n \t\t\t\tprint_signature();\n+\t\t\tprint_bases(&bases);\n \t\t}\n \t\tif (!use_stdout)\n \t\t\tfclose(stdout);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex eed2981..a6ce727 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1460,4 +1460,19 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \ttest_path_is_dir patchset\n '\n \n+test_expect_success 'format-patch --base' '\n+\tgit checkout side &&\n+\tgit format-patch --stdout --base=HEAD~~~ -1 >patch &&\n+\tgrep -e \"^base-commit:\" -A3 patch >actual &&\n+\techo \"base-commit: $(git rev-parse HEAD~~~)\" >expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~~ | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~ | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'format-patch --base error handling' '\n+\t! git format-patch --base=HEAD~ -2 &&\n+\t! git format-patch --base=HEAD~ -3\n+'\n+\n test_done\n-- \n2.8.1.120.g24d6b3f\n"},{"id":"283125","messageId":"1460342873-28900-4-git-send-email-xiaolong.ye@intel.com","threadId":"41987","inReplyTo":"1460342873-28900-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v4 3/4] format-patch: introduce --base=auto option","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-11T02:47:52Z","receivedAt":"2016-04-11T02:47:52Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Introduce --base=auto to record the base commit info automatically, the\nbase_commit will be the merge base of tip commit of the upstream branch\nand revision-range specified in cmdline.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Wu Fengguang <fengguang.wu@intel.com>\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n Documentation/git-format-patch.txt |  4 ++++\n builtin/log.c                      | 32 ++++++++++++++++++++++++++++----\n t/t4014-format-patch.sh            | 14 ++++++++++++++\n 3 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 2a4c293..8283eea 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -577,6 +577,10 @@ The submitter could also use `git format-patch --base=P -3 C` to generate\n patches for A, B and C, and the identifiers for P, X, Y, Z are appended\n at the end of the _first_ message.\n \n+If set `--base=auto` in cmdline, it will track base commit automatically,\n+the base commit will be the merge base of tip commit of the remote-tracking\n+branch and revision-range specified in cmdline.\n+\n EXAMPLES\n --------\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 73bc36d..510a427 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1205,6 +1205,9 @@ static void prepare_bases(struct base_tree_info *bases,\n \tstruct commit *base = NULL, *commit;\n \tstruct rev_info revs;\n \tstruct diff_options diffopt;\n+\tstruct branch *curr_branch;\n+\tstruct commit_list *base_list;\n+\tconst char *upstream;\n \tunsigned char sha1[20];\n \tint i;\n \n@@ -1212,10 +1215,31 @@ static void prepare_bases(struct base_tree_info *bases,\n \tDIFF_OPT_SET(&diffopt, RECURSIVE);\n \tdiff_setup_done(&diffopt);\n \n-\tbase = lookup_commit_reference_by_name(base_commit);\n-\tif (!base)\n-\t\tdie(_(\"Unknown commit %s\"), base_commit);\n-\toidcpy(&bases->base_commit, &base->object.oid);\n+\tif (!strcmp(base_commit, \"auto\")) {\n+\t\tcurr_branch = branch_get(NULL);\n+\t\tupstream = branch_get_upstream(curr_branch, NULL);\n+\t\tif (upstream) {\n+\t\t\tif (get_sha1(upstream, sha1))\n+\t\t\t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), upstream);\n+\t\t\tcommit = lookup_commit_or_die(sha1, \"upstream base\");\n+\t\t\tbase_list = get_merge_bases_many(commit, total, list);\n+\t\t\t/* There should be one and only one merge base. */\n+\t\t\tif (!base_list || base_list->next)\n+\t\t\t\tdie(_(\"Could not find exact merge base.\"));\n+\t\t\tbase = base_list->item;\n+\t\t\tfree_commit_list(base_list);\n+\t\t\toidcpy(&bases->base_commit, &base->object.oid);\n+\t\t} else {\n+\t\t\tdie(_(\"Failed to get upstream, if you want to record base commit automatically,\\n\"\n+\t\t\t      \"please use git branch --set-upstream-to to track a remote branch.\\n\"\n+\t\t\t      \"Or you could specify base commit by --base=<base-commit-id> manually.\"));\n+\t\t}\n+\t} else {\n+\t\tbase = lookup_commit_reference_by_name(base_commit);\n+\t\tif (!base)\n+\t\t\tdie(_(\"Unknown commit %s\"), base_commit);\n+\t\toidcpy(&bases->base_commit, &base->object.oid);\n+\t}\n \n \tinit_revisions(&revs, NULL);\n \trevs.max_parents = 1;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex a6ce727..4603915 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1475,4 +1475,18 @@ test_expect_success 'format-patch --base error handling' '\n \t! git format-patch --base=HEAD~ -3\n '\n \n+test_expect_success 'format-patch --base=auto' '\n+\tgit checkout -b new master &&\n+\tgit branch --set-upstream-to=master &&\n+\techo \"A\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"New change #A\" &&\n+\techo \"B\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"New change #B\" &&\n+\tgit format-patch --stdout --base=auto -2 >patch &&\n+\tgrep -e \"^base-commit:\" patch >actual &&\n+\techo \"base-commit: $(git rev-parse master)\" >expected &&\n+\ttest_cmp expected actual\n+'\n test_done\n-- \n2.8.1.120.g24d6b3f\n"},{"id":"283124","messageId":"1460342873-28900-5-git-send-email-xiaolong.ye@intel.com","threadId":"41987","inReplyTo":"1460342873-28900-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v4 4/4] format-patch: introduce format.base configuration","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-11T02:47:53Z","receivedAt":"2016-04-11T02:47:53Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"We can set format.base=auto to record the base commit info automatically,\nit is equivalent to set --base=auto in cmdline.\n\nThe format.base has lower priority than command line option, so if user\nset format.base=auto and pass the command line option in the meantime,\nbase_commit will be the one passed to command line option.\n\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n Documentation/git-format-patch.txt |  4 ++++\n builtin/log.c                      | 21 ++++++++++++++-------\n t/t4014-format-patch.sh            | 19 +++++++++++++++++++\n 3 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 8283eea..738121d 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -581,6 +581,10 @@ If set `--base=auto` in cmdline, it will track base commit automatically,\n the base commit will be the merge base of tip commit of the remote-tracking\n branch and revision-range specified in cmdline.\n \n+If 'format.base=auto' is set in configuration file, it is equivalent\n+to set '--base=auto' in cmdline.\n+\n+\n EXAMPLES\n --------\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 510a427..489434a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -705,6 +705,7 @@ static int do_signoff;\n static const char *signature = git_version_string;\n static const char *signature_file;\n static int config_cover_letter;\n+static int config_base_commit;\n static const char *config_output_directory;\n \n enum {\n@@ -786,6 +787,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"format.outputdirectory\"))\n \t\treturn git_config_string(&config_output_directory, var, value);\n+\tif (!strcmp(var, \"format.base\")){\n+\t\tif (value && !strcasecmp(value, \"auto\")) {\n+\t\t\tconfig_base_commit = 1;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info *bases,\n \tDIFF_OPT_SET(&diffopt, RECURSIVE);\n \tdiff_setup_done(&diffopt);\n \n-\tif (!strcmp(base_commit, \"auto\")) {\n+\tif (base_commit && strcmp(base_commit, \"auto\")) {\n+\t\tbase = lookup_commit_reference_by_name(base_commit);\n+\t\tif (!base)\n+\t\t\tdie(_(\"Unknown commit %s\"), base_commit);\n+\t\toidcpy(&bases->base_commit, &base->object.oid);\n+\t} else if ((base_commit && !strcmp(base_commit, \"auto\")) || config_base_commit) {\n \t\tcurr_branch = branch_get(NULL);\n \t\tupstream = branch_get_upstream(curr_branch, NULL);\n \t\tif (upstream) {\n@@ -1234,11 +1246,6 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\t\t      \"please use git branch --set-upstream-to to track a remote branch.\\n\"\n \t\t\t      \"Or you could specify base commit by --base=<base-commit-id> manually.\"));\n \t\t}\n-\t} else {\n-\t\tbase = lookup_commit_reference_by_name(base_commit);\n-\t\tif (!base)\n-\t\t\tdie(_(\"Unknown commit %s\"), base_commit);\n-\t\toidcpy(&bases->base_commit, &base->object.oid);\n \t}\n \n \tinit_revisions(&revs, NULL);\n@@ -1622,7 +1629,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tmemset(&bases, 0, sizeof(bases));\n-\tif (base_commit) {\n+\tif (base_commit || config_base_commit) {\n \t\treset_revision_walk();\n \t\tprepare_bases(&bases, base_commit, list, nr);\n \t}\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 4603915..6005b7c 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1489,4 +1489,23 @@ test_expect_success 'format-patch --base=auto' '\n \techo \"base-commit: $(git rev-parse master)\" >expected &&\n \ttest_cmp expected actual\n '\n+\n+test_expect_success 'format-patch format.base option' '\n+\ttest_when_finished \"git config --unset format.base\" &&\n+\tgit config format.base auto &&\n+\tgit format-patch --stdout -1 >patch &&\n+\tgrep -e \"^base-commit:\" patch >actual &&\n+\techo \"base-commit: $(git rev-parse master)\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'format-patch --base overrides format.base' '\n+\ttest_when_finished \"git config --unset format.base\" &&\n+\tgit config format.base auto &&\n+\tgit format-patch --stdout --base=HEAD~ -1 >patch &&\n+\tgrep -e \"^base-commit:\" patch >actual &&\n+\techo \"base-commit: $(git rev-parse HEAD~)\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.8.1.120.g24d6b3f\n"},{"id":"283211","messageId":"xmqq7fg2r6fi.fsf@gitster.mtv.corp.google.com","threadId":"41987","inReplyTo":"1460342873-28900-3-git-send-email-xiaolong.ye@intel.com","subject":"Re: [PATCH v4 2/4] format-patch: add '--base' option to record base tree info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T19:08:33Z","receivedAt":"2016-04-12T19:08:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xiaolong Ye <xiaolong.ye@intel.com> writes:\n\n> Maintainers or third party testers may want to know the exact base tree\n> the patch series applies to. Teach git format-patch a '--base' option\n> to record the base tree info and append it at the end of the_first_\n\nIt probably was a good idea to add stress during the discussion to\ncompare various possibilities, but there no longer is a need to\nitalicise \"first\" like this, I think.\n\n> message(either the cover letter or the first patch in the series).\n\nPlease have space before \"(\" (also found elsewhere in this message)\nto make this readable.\n\n>\n> The base tree info consists of the \"base commit\", which is a well-known\n> commit that is part of the stable part of the project history everybody\n> else works off of, and zero or more \"prerequisite patches\", which are\n> well-known patches in flight that is not yet part of the \"base commit\"\n> that need to be applied on top of \"base commit\" in topological order\n> before the patches can be applied.\n>\n> The \"base commit\" is shown as \"base-commit: \" followed by the 40-hex of\n> the commit object name.  A \"prerequisite patch\" is shown as\n> \"prerequisite-patch-id: \" followed by the 40-hex \"patch id\", which is a\n> sum of SHA-1 of the file diffs associated with a patch, with whitespace\n> and line numbers ignored, it's reasonably stable and unique.\n\nLet's be more helpful to end users.  They do not need to know the\nexact formula, especially when there is a command to generate or\ncheck the id for themselves:\n\n    \"patch id\", which can be obtained by passing the patch through the\n    \"git patch-id --stable\" command\n\nor something?  \n\n> For example, we have history where base is Z, with three prerequisites\n> X-Y-Z, before the patch series A-B-C, i.e.\n\nbase is Z???\n\n\tImagine that on top of the public commit P, you applied\n\twell-known patches X, Y and Z from somebody else, and then\n\tbuilt your three-patch series A, B, C.\n\nperhaps?\n\n>\n> \tP---X---Y---Z---A---B---C\n>\n> We could say \"git format-patch --base=P -3 C\"(or variants thereof, e.g.\n> with \"--cover-letter\" of using \"Z..C\" instead of \"-3 C\" to specify the\n> range), then we could get base tree information block showing at the\n> end of _first_ message as below:\n\nAgain, if \"first\" is _so_ important to stress, it probably is worth\nsaying that by \"first\" you mean either patch 1/n or patch 0/n when\nthe cover letter exists.\n\nAlso \"could\" may have made sense while we were having discussion on\npossible design of the hypothetical feature, but with the patch\napplied, the feature becomes a reality, so you can and should stop\nliving in the hypothetical world and do s/could/can/ the above.\n\n\tWith \"git format-patch --base=P -3 C\" (or variants...), the\n\tbase tree information block is shown at the end of the first\n\tmessage the command outputs (either the first patch, or the\n\tcover letter), like this:\n\nperhaps?\n\nI assume that the patch to the documentation has the same text I\ncommented on the above, so I won't repeat my comments to them.\n\n> \tbase-commit: P\n> \tprerequisite-patch-id: X\n> \tprerequisite-patch-id: Y\n> \tprerequisite-patch-id: Z\n>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Helped-by: Wu Fengguang <fengguang.wu@intel.com>\n> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n> ---\n>  Documentation/git-format-patch.txt | 56 +++++++++++++++++++++++\n>  builtin/log.c                      | 92 ++++++++++++++++++++++++++++++++++++++\n>  t/t4014-format-patch.sh            | 15 +++++++\n>  3 files changed, 163 insertions(+)\n> ...\n> +static void prepare_bases(struct base_tree_info *bases,\n> +\t\t\t  const char *base_commit,\n> +\t\t\t  struct commit **list,\n> +\t\t\t  int total)\n> +{\n> +\tstruct commit *base = NULL, *commit;\n> +\tstruct rev_info revs;\n> +\tstruct diff_options diffopt;\n> +\tunsigned char sha1[20];\n> +\tint i;\n> +\n> +\tdiff_setup(&diffopt);\n> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n> +\tdiff_setup_done(&diffopt);\n> +\n> +\tbase = lookup_commit_reference_by_name(base_commit);\n> +\tif (!base)\n> +\t\tdie(_(\"Unknown commit %s\"), base_commit);\n> +\toidcpy(&bases->base_commit, &base->object.oid);\n> +\n> +\tinit_revisions(&revs, NULL);\n> +\trevs.max_parents = 1;\n> +\trevs.topo_order = 1;\n> +\tfor (i = 0; i < total; i++) {\n> +\t\tif (!in_merge_bases(base, list[i]) || base == list[i])\n> +\t\t\tdie(_(\"base commit should be the ancestor of revision list\"));\n\nThis check looks overly expensive, but I do not think of a more\nefficient way to do this, given that \"All the commits from our\nseries must reach the specified base\" is what you seem to want.\n\nMy understanding is that if base=P is given and you are doing\n\"format-patch Z..C\" in this picture:\n\n    Q---P---Z---B---*---C\n     \\             /\n      .-----------A\n\nyour list would become A, B and C, and you want to detect that P is\nnot an ancestor of A.  merge_bases_many() computes a wrong thing for\nthis use case, and you'd need to go one-by-one.\n\nUnless there is some clever trick to take advantage of the previous\ntraversal you made in order to find out A, B and C are the commits\nthat are part of your series somehow.\n\nAnybody with clever ideas?\n"},{"id":"283213","messageId":"xmqq37qqr4ms.fsf@gitster.mtv.corp.google.com","threadId":"41987","inReplyTo":"1460342873-28900-5-git-send-email-xiaolong.ye@intel.com","subject":"Re: [PATCH v4 4/4] format-patch: introduce format.base configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T19:47:23Z","receivedAt":"2016-04-12T19:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xiaolong Ye <xiaolong.ye@intel.com> writes:\n\n> +static int config_base_commit;\n\nThis variable is used as a simple boolean whose name is overly broad\n(if it were named \"config_base_auto\" this complaint would not\napply).  If you envision possible future enhancements for this\nconfiguration variable, \"int config_base_commit\" might make sense\nbut I don't think of anything offhand that would be happy with\n\"int\".\n\n> @@ -786,6 +787,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n>  \t}\n>  \tif (!strcmp(var, \"format.outputdirectory\"))\n>  \t\treturn git_config_string(&config_output_directory, var, value);\n> +\tif (!strcmp(var, \"format.base\")){\n\nStyle. s/)){/)) {/\n\n> +\t\tif (value && !strcasecmp(value, \"auto\")) {\n\nDoes it make sense to allow \"Auto\" here?  Given that the command\nline parsing uses strcmp() to require \"auto\", I do not think so.\n\n> +\t\t\tconfig_base_commit = 1;\n> +\t\t\treturn 0;\n> +\t\t}\n\nWhen a value other than \"auto\" is given, is it sane to ignore them\nwithout even warning?\n\nI am wondering if this wants to be a format.useAutoBase boolean\nvariable.\n\n> @@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info *bases,\n>  \tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>  \tdiff_setup_done(&diffopt);\n>  \n> -\tif (!strcmp(base_commit, \"auto\")) {\n> +\tif (base_commit && strcmp(base_commit, \"auto\")) {\n> +\t\tbase = lookup_commit_reference_by_name(base_commit);\n> +\t\tif (!base)\n> +\t\t\tdie(_(\"Unknown commit %s\"), base_commit);\n> +\t\toidcpy(&bases->base_commit, &base->object.oid);\n> +\t} else if ((base_commit && !strcmp(base_commit, \"auto\")) || config_base_commit) {\n\nIt may be a poor design to teach prepare_bases() about \"auto\" thing.\nDoesn't it belong to the caller?  The caller used to say \"If a base\nis given, then call that function, by the way, the base must be a\nconcrete one\", and with the new \"auto\" feature, the caller loosens\nthe last part of the statement and says \"If a base is given, call\nthat function, but if it is specified as \"auto\", I'd have to compute\nit for the user before doing so\".\n"},{"id":"283377","messageId":"20160413144224.GA32367@yexl-desktop","threadId":"41987","inReplyTo":"xmqq7fg2r6fi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/4] format-patch: add '--base' option to record base tree info","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-13T14:42:24Z","receivedAt":"2016-04-13T14:42:24Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Tue, Apr 12, 2016 at 12:08:33PM -0700, Junio C Hamano wrote:\n>Xiaolong Ye <xiaolong.ye@intel.com> writes:\n>\n>> Maintainers or third party testers may want to know the exact base tree\n>> the patch series applies to. Teach git format-patch a '--base' option\n>> to record the base tree info and append it at the end of the_first_\n>\n>It probably was a good idea to add stress during the discussion to\n>compare various possibilities, but there no longer is a need to\n>italicise \"first\" like this, I think.\n>\n>> message(either the cover letter or the first patch in the series).\n>\n>Please have space before \"(\" (also found elsewhere in this message)\n>to make this readable.\n>\n>>\n>> The base tree info consists of the \"base commit\", which is a well-known\n>> commit that is part of the stable part of the project history everybody\n>> else works off of, and zero or more \"prerequisite patches\", which are\n>> well-known patches in flight that is not yet part of the \"base commit\"\n>> that need to be applied on top of \"base commit\" in topological order\n>> before the patches can be applied.\n>>\n>> The \"base commit\" is shown as \"base-commit: \" followed by the 40-hex of\n>> the commit object name.  A \"prerequisite patch\" is shown as\n>> \"prerequisite-patch-id: \" followed by the 40-hex \"patch id\", which is a\n>> sum of SHA-1 of the file diffs associated with a patch, with whitespace\n>> and line numbers ignored, it's reasonably stable and unique.\n>\n>Let's be more helpful to end users.  They do not need to know the\n>exact formula, especially when there is a command to generate or\n>check the id for themselves:\n>\n>    \"patch id\", which can be obtained by passing the patch through the\n>    \"git patch-id --stable\" command\n>\n>or something?  \n>\n>> For example, we have history where base is Z, with three prerequisites\n>> X-Y-Z, before the patch series A-B-C, i.e.\n>\n>base is Z???\n>\n>\tImagine that on top of the public commit P, you applied\n>\twell-known patches X, Y and Z from somebody else, and then\n>\tbuilt your three-patch series A, B, C.\n>\n>perhaps?\n>\n>>\n>> \tP---X---Y---Z---A---B---C\n>>\n>> We could say \"git format-patch --base=P -3 C\"(or variants thereof, e.g.\n>> with \"--cover-letter\" of using \"Z..C\" instead of \"-3 C\" to specify the\n>> range), then we could get base tree information block showing at the\n>> end of _first_ message as below:\n>\n>Again, if \"first\" is _so_ important to stress, it probably is worth\n>saying that by \"first\" you mean either patch 1/n or patch 0/n when\n>the cover letter exists.\n>\n>Also \"could\" may have made sense while we were having discussion on\n>possible design of the hypothetical feature, but with the patch\n>applied, the feature becomes a reality, so you can and should stop\n>living in the hypothetical world and do s/could/can/ the above.\n>\n>\tWith \"git format-patch --base=P -3 C\" (or variants...), the\n>\tbase tree information block is shown at the end of the first\n>\tmessage the command outputs (either the first patch, or the\n>\tcover letter), like this:\n>\n>perhaps?\n>\n>I assume that the patch to the documentation has the same text I\n>commented on the above, so I won't repeat my comments to them.\n>\n\nThanks for the review,  I'll follow all the comments above and\nmake changes to commit log as well as documentation.\n \n>> \tbase-commit: P\n>> \tprerequisite-patch-id: X\n>> \tprerequisite-patch-id: Y\n>> \tprerequisite-patch-id: Z\n>>\n>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>> Helped-by: Wu Fengguang <fengguang.wu@intel.com>\n>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n>> ---\n>>  Documentation/git-format-patch.txt | 56 +++++++++++++++++++++++\n>>  builtin/log.c                      | 92 ++++++++++++++++++++++++++++++++++++++\n>>  t/t4014-format-patch.sh            | 15 +++++++\n>>  3 files changed, 163 insertions(+)\n>> ...\n>> +static void prepare_bases(struct base_tree_info *bases,\n>> +\t\t\t  const char *base_commit,\n>> +\t\t\t  struct commit **list,\n>> +\t\t\t  int total)\n>> +{\n>> +\tstruct commit *base = NULL, *commit;\n>> +\tstruct rev_info revs;\n>> +\tstruct diff_options diffopt;\n>> +\tunsigned char sha1[20];\n>> +\tint i;\n>> +\n>> +\tdiff_setup(&diffopt);\n>> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>> +\tdiff_setup_done(&diffopt);\n>> +\n>> +\tbase = lookup_commit_reference_by_name(base_commit);\n>> +\tif (!base)\n>> +\t\tdie(_(\"Unknown commit %s\"), base_commit);\n>> +\toidcpy(&bases->base_commit, &base->object.oid);\n>> +\n>> +\tinit_revisions(&revs, NULL);\n>> +\trevs.max_parents = 1;\n>> +\trevs.topo_order = 1;\n>> +\tfor (i = 0; i < total; i++) {\n>> +\t\tif (!in_merge_bases(base, list[i]) || base == list[i])\n>> +\t\t\tdie(_(\"base commit should be the ancestor of revision list\"));\n>\n>This check looks overly expensive, but I do not think of a more\n>efficient way to do this, given that \"All the commits from our\n>series must reach the specified base\" is what you seem to want.\n\nYes, that's what I want to make sure, for normal case, if patch\nsubmitter has history as below:\n\n\tP---Z---A---B---C---D\n\nand she may unintentionally specify wrong base by doing\n\"format-patch --base=B -4\" while P or Z is the actual base,\nthe recevier such as robot would get confused or fooled if we\njust provide B as the base commit in this case.\n\n>\n>My understanding is that if base=P is given and you are doing\n>\"format-patch Z..C\" in this picture:\n>\n>    Q---P---Z---B---*---C\n>     \\             /\n>      .-----------A\n>\n>your list would become A, B and C, and you want to detect that P is\n>not an ancestor of A.  merge_bases_many() computes a wrong thing for\n>this use case, and you'd need to go one-by-one.\n>\n>Unless there is some clever trick to take advantage of the previous\n>traversal you made in order to find out A, B and C are the commits\n>that are part of your series somehow.\n>\n>Anybody with clever ideas?\n>\n"},{"id":"283378","messageId":"20160413155558.GA2680@yexl-desktop","threadId":"41987","inReplyTo":"xmqq37qqr4ms.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 4/4] format-patch: introduce format.base configuration","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-13T15:55:58Z","receivedAt":"2016-04-13T15:55:58Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Tue, Apr 12, 2016 at 12:47:23PM -0700, Junio C Hamano wrote:\n>Xiaolong Ye <xiaolong.ye@intel.com> writes:\n>\n>> +static int config_base_commit;\n>\n>This variable is used as a simple boolean whose name is overly broad\n>(if it were named \"config_base_auto\" this complaint would not\n>apply).  If you envision possible future enhancements for this\n>configuration variable, \"int config_base_commit\" might make sense\n>but I don't think of anything offhand that would be happy with\n>\"int\".\n>\n>> @@ -786,6 +787,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n>>  \t}\n>>  \tif (!strcmp(var, \"format.outputdirectory\"))\n>>  \t\treturn git_config_string(&config_output_directory, var, value);\n>> +\tif (!strcmp(var, \"format.base\")){\n>\n>Style. s/)){/)) {/\n>\n>> +\t\tif (value && !strcasecmp(value, \"auto\")) {\n>\n>Does it make sense to allow \"Auto\" here?  Given that the command\n>line parsing uses strcmp() to require \"auto\", I do not think so.\n>\n>> +\t\t\tconfig_base_commit = 1;\n>> +\t\t\treturn 0;\n>> +\t\t}\n>\n>When a value other than \"auto\" is given, is it sane to ignore them\n>without even warning?\n>\n>I am wondering if this wants to be a format.useAutoBase boolean\n>variable.\n>\n\nThanks for the reminder, seems boolean variable is more suitable for\nthis case, I'll adopt to it in next iteration.\n>> @@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info *bases,\n>>  \tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>>  \tdiff_setup_done(&diffopt);\n>>  \n>> -\tif (!strcmp(base_commit, \"auto\")) {\n>> +\tif (base_commit && strcmp(base_commit, \"auto\")) {\n>> +\t\tbase = lookup_commit_reference_by_name(base_commit);\n>> +\t\tif (!base)\n>> +\t\t\tdie(_(\"Unknown commit %s\"), base_commit);\n>> +\t\toidcpy(&bases->base_commit, &base->object.oid);\n>> +\t} else if ((base_commit && !strcmp(base_commit, \"auto\")) || config_base_commit) {\n>\n>It may be a poor design to teach prepare_bases() about \"auto\" thing.\n>Doesn't it belong to the caller?  The caller used to say \"If a base\n\nGood point, as I understand your comments, we need to extract the \"auto\"\nthing from prepare_bases() and call it early, then we could have a\nconcrete base before calling prepare_bases().\n\nThanks,\nXiaolong.\n>is given, then call that function, by the way, the base must be a\n>concrete one\", and with the new \"auto\" feature, the caller loosens\n>the last part of the statement and says \"If a base is given, call\n>that function, but if it is specified as \"auto\", I'd have to compute\n>it for the user before doing so\".\n"},{"id":"283447","messageId":"20160414142333.GA31621@yexl-desktop","threadId":"41987","inReplyTo":"xmqq7fg2r6fi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/4] format-patch: add '--base' option to record base tree info","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-14T14:23:33Z","receivedAt":"2016-04-14T14:23:33Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Tue, Apr 12, 2016 at 12:08:33PM -0700, Junio C Hamano wrote:\n>> +static void prepare_bases(struct base_tree_info *bases,\n>> +\t\t\t  const char *base_commit,\n>> +\t\t\t  struct commit **list,\n>> +\t\t\t  int total)\n>> +{\n>> +\tstruct commit *base = NULL, *commit;\n>> +\tstruct rev_info revs;\n>> +\tstruct diff_options diffopt;\n>> +\tunsigned char sha1[20];\n>> +\tint i;\n>> +\n>> +\tdiff_setup(&diffopt);\n>> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>> +\tdiff_setup_done(&diffopt);\n>> +\n>> +\tbase = lookup_commit_reference_by_name(base_commit);\n>> +\tif (!base)\n>> +\t\tdie(_(\"Unknown commit %s\"), base_commit);\n>> +\toidcpy(&bases->base_commit, &base->object.oid);\n>> +\n>> +\tinit_revisions(&revs, NULL);\n>> +\trevs.max_parents = 1;\n>> +\trevs.topo_order = 1;\n>> +\tfor (i = 0; i < total; i++) {\n>> +\t\tif (!in_merge_bases(base, list[i]) || base == list[i])\n>> +\t\t\tdie(_(\"base commit should be the ancestor of revision list\"));\n>\n>This check looks overly expensive, but I do not think of a more\n>efficient way to do this, given that \"All the commits from our\n>series must reach the specified base\" is what you seem to want.\n>\n>My understanding is that if base=P is given and you are doing\n>\"format-patch Z..C\" in this picture:\n>\n>    Q---P---Z---B---*---C\n>     \\             /\n>      .-----------A\n>\n\nHow about we compute the merge base of the specified rev list in\ncmdline (it should be Q in above case), then check whether specified\nbase (P in this case) could be reachable from it, if it couldn't, we\njust error out.\n\n>your list would become A, B and C, and you want to detect that P is\n>not an ancestor of A.  merge_bases_many() computes a wrong thing for\n>this use case, and you'd need to go one-by-one.\n>\n>Unless there is some clever trick to take advantage of the previous\n>traversal you made in order to find out A, B and C are the commits\n>that are part of your series somehow.\n>\n>Anybody with clever ideas?\n>\n"},{"id":"283455","messageId":"xmqq37qoi2h6.fsf@gitster.mtv.corp.google.com","threadId":"41987","inReplyTo":"20160414142333.GA31621@yexl-desktop","subject":"Re: [PATCH v4 2/4] format-patch: add '--base' option to record base tree info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T16:23:17Z","receivedAt":"2016-04-14T16:23:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ye Xiaolong <xiaolong.ye@intel.com> writes:\n\n>>> +\tfor (i = 0; i < total; i++) {\n>>> +\t\tif (!in_merge_bases(base, list[i]) || base == list[i])\n>>> +\t\t\tdie(_(\"base commit should be the ancestor of revision list\"));\n>>\n>>This check looks overly expensive, but I do not think of a more\n>>efficient way to do this, given that \"All the commits from our\n>>series must reach the specified base\" is what you seem to want.\n>>\n>>My understanding is that if base=P is given and you are doing\n>>\"format-patch Z..C\" in this picture:\n>>\n>>    Q---P---Z---B---*---C\n>>     \\             /\n>>      .-----------A\n>>\n>\n> How about we compute the merge base of the specified rev list in\n> cmdline (it should be Q in above case), then check whether specified\n> base (P in this case) could be reachable from it, if it couldn't, we\n> just error out.\n\nWhat commits are you considering \"the specified rev list in cmdline\"\nin the example?  Do you mean \"commits in the list[], i.e. those to\nbe shown as patches?\"\n\nThat is, you are proposing to find the topologically-youngest common\nancestors of A, B and C, which is Q?\n\nThere is no canned way to compute that (merge_bases_many() is not\nthat function).\n\nYou however can do repeated pair-wise merge base computations to\nreduce the complexity from your O(n) loop to O(log n), I guess.  Do\na pair-wise merge base between A and B (which is Q), and do a merge\nbase between C (which is the remaining one) and Q to arrive at Q.\n"}]}