{"thread":{"id":"41790","subject":"[PATCH v2 0/4] Add an option to git-format-patch to record base tree info","startedAt":"2016-03-23T08:52:23Z","lastAt":"2016-04-01T16:36:58Z","messageCount":12,"participants":["Xiaolong Ye","Junio C Hamano","Ye Xiaolong"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"281527","messageId":"1458723147-7335-1-git-send-email-xiaolong.ye@intel.com","threadId":"41790","inReplyTo":null,"subject":"[PATCH v2 0/4] Add an option to git-format-patch to record base tree info","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-23T08:52:23Z","receivedAt":"2016-03-23T08:52:23Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"This is a re-roll of \n\n   http://article.gmane.org/gmane.comp.version-control.git/286873\n\nIt is almost a rewrite of v1, Changes include:\n\n - Make commit_patch_id() a publib helper function, so it can be available to other modules.\n\n - The new option \"--base\" will not only record the base commit id(a public well-known commit),\n   but also the prerequisite patches's patch ids, then the 0day test robot could get these info,\n   thus to checkout the well-known base tree-ish, apply the prerequisite patches and then the\n   patches being send can be applied to correct tree to be evaluated. (0day catches every patch\n   series to LKML and maintains a patch-id => commit-id database for in-flight patches.)\n\n - For developers' convenience, we propose a new format.base configuration to track the base commit\n   automatically, if current branch the developer work on has its remote-tracking branch,\n   the base commit would be the tip commit of the remote branch, if current branch is not tracked\n   with any remote branch(eg. checkout by a local branch or a commit), thus its upstream could not\n   be obtained, it will just record the parent commit-id of the patch series, as well as patch-id for\n   reference.\n\n   so here is the text UI:\n\n   1) for the cases that exact base commit could be obtained(eithor from manual input --base=<base-commit-id>\n\t   or from upstream if base=auto is set) \n\n\t ** base-commit-info **\n\t base-commit: ab5d01a29eb7380ceab070f0807c2939849c44bc\n\t prerequisite-patch-id: 61400f965fdc0c2fbe8ad9cb5316c3efe6e05c14\n\t prerequisite-patch-id: 063a398ef398647d8d839e6f9090d38ea9e0551c\n\n   2) for cases that exact base commit is failed to obtained\n\t\n\t ** parent-commit-info **\n\t parent-commit: ab5d01a29eb7380ceab070f0807c2939849c44bc\n\t parent-patch-id: caf2dae24db5b4c49a6c4134dd6d9908e898424b\n\nThanks for fengguang and junio's suggestions and prototype of implementation.\n\nThanks,\nXiaolong.\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 |  20 ++++++\n builtin/log.c                      | 137 +++++++++++++++++++++++++++++++++++++\n patch-ids.c                        |   2 +-\n patch-ids.h                        |   2 +\n 4 files changed, 160 insertions(+), 1 deletion(-)\n\n-- \n2.8.0.rc4.4.ga41a987\n\n** base-commit-info **\nbase-commit: 808ecd4cca75acac5e4868f15d3e647fc73698d3\n"},{"id":"281529","messageId":"1458723147-7335-2-git-send-email-xiaolong.ye@intel.com","threadId":"41790","inReplyTo":"1458723147-7335-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v2 1/4] patch-ids: make commit_patch_id() a public helper function","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-23T08:52:24Z","receivedAt":"2016-03-23T08:52:24Z","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\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.0.rc4.4.ga41a987\n"},{"id":"281528","messageId":"1458723147-7335-3-git-send-email-xiaolong.ye@intel.com","threadId":"41790","inReplyTo":"1458723147-7335-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v2 2/4] format-patch: add '--base' option to record base tree info","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-23T08:52:25Z","receivedAt":"2016-03-23T08:52:25Z","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 to\nrecord the base tree info and append this information at the end of the\n_first_ message (either the cover letter or the first patch in the series).\n\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n Documentation/git-format-patch.txt | 15 ++++++\n builtin/log.c                      | 98 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 6821441..a5f145e 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -265,6 +265,21 @@ 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 whole tree\n+\tthe patch series applies to. For example, the patch submitter\n+\thas a commit history of this shape:\n+\n+\t---P---X---Y---Z---A---B---C\n+\n+\twhere \"P\" is the well-known public commit (e.g. one in Linus's tree),\n+\t\"X\", \"Y\", \"Z\" are prerequisite patches in flight, and \"A\", \"B\", \"C\"\n+\tare the work being sent out, the submitter would say \"format-patch\n+\t--base=P -3 C\" (or variants thereof, e.g. with \"--cover\" or using\n+\t\"Z..C\" instead of \"-3 C\" to specify the range), and the identifiers\n+\tfor P, X, Y, Z are appended at the end of the _first_ message (either\n+\tthe cover letter or the first patch in the series).\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\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 0d738d6..d3804b3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1185,6 +1185,88 @@ 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 *prerequisite_head)\n+{\n+\tstruct commit *base = NULL, *commit;\n+\tstruct rev_info revs;\n+\tstruct diff_options diffopt;\n+\tstruct object_id *patch_id;\n+\tunsigned char sha1[20];\n+\tint pos = 0;\n+\n+\tif (!prerequisite_head)\n+\t\treturn;\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+\tif (base == prerequisite_head)\n+\t\treturn;\n+\n+\tif (!in_merge_bases(base, prerequisite_head))\n+\t\tdie(_(\"base commit should be the ancestor of revs you specified\"));\n+\n+\tinit_revisions(&revs, NULL);\n+\trevs.max_parents = 1;\n+\n+\tbase->object.flags |= UNINTERESTING;\n+\tadd_pending_object(&revs, &base->object, \"base\");\n+\tprerequisite_head->object.flags |= 0;\n+\tadd_pending_object(&revs, &prerequisite_head->object, \"prerequisite-head\");\n+\n+\tdiff_setup(&diffopt);\n+\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n+\tdiff_setup_done(&diffopt);\n+\n+\tif (prepare_revision_walk(&revs))\n+\t\tdie(_(\"revision walk setup failed\"));\n+\t/*\n+\t * Traverse the commits list between base and prerequisite head,\n+\t * get the patch ids and stuff them in bases structure.\n+\t */\n+\twhile ((commit = get_revision(&revs)) != NULL) {\n+\t\tif (commit_patch_id(commit, &diffopt, sha1))\n+\t\t\treturn;\n+\t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n+\t\tpatch_id = bases->patch_id + pos;\n+\t\thashcpy(patch_id->hash, sha1);\n+\t\tpos++;\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+\tprintf(\"** base-commit-info **\\n\");\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 = 0; i < bases->nr_patch_id; 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@@ -1209,6 +1291,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@@ -1271,6 +1356,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@@ -1507,6 +1594,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tsignature = strbuf_detach(&buf, NULL);\n \t}\n \n+\tif (base_commit) {\n+\t\tstruct commit *prerequisite_head = NULL;\n+\t\tif (list[nr - 1]->parents)\n+\t\t\tprerequisite_head = list[nr - 1]->parents->item;\n+\t\tmemset(&bases, 0, sizeof(bases));\n+\t\treset_revision_walk();\n+\t\tprepare_bases(&bases, base_commit, prerequisite_head);\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@@ -1520,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@@ -1585,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);\n-- \n2.8.0.rc4.4.ga41a987\n"},{"id":"281530","messageId":"1458723147-7335-4-git-send-email-xiaolong.ye@intel.com","threadId":"41790","inReplyTo":"1458723147-7335-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-23T08:52:26Z","receivedAt":"2016-03-23T08:52:26Z","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 base_commit\nwill be the tip commit of the upstream branch. If upstream branch cannot be determined,\nit will just record the parent's commit id and patch id.\n\nSigned-off-by: Xiaolong Ye <xiaolong.ye@intel.com>\n---\n builtin/log.c | 70 +++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 51 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d3804b3..e8a3964 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1187,6 +1187,8 @@ static int from_callback(const struct option *opt, const char *arg, int unset)\n \n struct base_tree_info {\n \tstruct object_id base_commit;\n+\tstruct object_id parent_commit;\n+\tstruct object_id parent_patch_id;\n \tint nr_patch_id, alloc_patch_id;\n \tstruct object_id *patch_id;\n };\n@@ -1199,15 +1201,38 @@ static void prepare_bases(struct base_tree_info *bases,\n \tstruct rev_info revs;\n \tstruct diff_options diffopt;\n \tstruct object_id *patch_id;\n+\tstruct branch *curr_branch;\n+\tconst char *upstream;\n \tunsigned char sha1[20];\n \tint pos = 0;\n \n \tif (!prerequisite_head)\n \t\treturn;\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+\tdiff_setup(&diffopt);\n+\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n+\tdiff_setup_done(&diffopt);\n+\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\tbase = lookup_commit_or_die(sha1, \"upstream base\");\n+\t\t\toidcpy(&bases->base_commit, &base->object.oid);\n+\t\t} else {\n+\t\t\tcommit_patch_id(prerequisite_head, &diffopt, sha1);\n+\t\t\toidcpy(&bases->parent_commit, &prerequisite_head->object.oid);\n+\t\t\thashcpy(bases->parent_patch_id.hash, sha1);\n+\t\t\treturn;\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 \tif (base == prerequisite_head)\n \t\treturn;\n@@ -1223,10 +1248,6 @@ static void prepare_bases(struct base_tree_info *bases,\n \tprerequisite_head->object.flags |= 0;\n \tadd_pending_object(&revs, &prerequisite_head->object, \"prerequisite-head\");\n \n-\tdiff_setup(&diffopt);\n-\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n-\tdiff_setup_done(&diffopt);\n-\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \t/*\n@@ -1249,22 +1270,33 @@ static void print_bases(struct base_tree_info *bases)\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+\tif (is_null_oid(&bases->base_commit) && is_null_oid(&bases->parent_commit))\n \t\treturn;\n \n-\tprintf(\"** base-commit-info **\\n\");\n+\tif (!is_null_oid(&bases->base_commit)) {\n+\t\tprintf(\"** base-commit-info **\\n\");\n+\n+\t\t/* Show the base commit */\n+\t\tprintf(\"base-commit: %s\\n\", oid_to_hex(&bases->base_commit));\n \n-\t/* Show the base commit */\n-\tprintf(\"base-commit: %s\\n\", oid_to_hex(&bases->base_commit));\n+\t\t/* Show the prerequisite patches */\n+\t\tfor (i = 0; i < bases->nr_patch_id; i++)\n+\t\t\tprintf(\"prerequisite-patch-id: %s\\n\", oid_to_hex(&bases->patch_id[i]));\n \n-\t/* Show the prerequisite patches */\n-\tfor (i = 0; i < bases->nr_patch_id; i++)\n-\t\tprintf(\"prerequisite-patch-id: %s\\n\", oid_to_hex(&bases->patch_id[i]));\n+\t\tfree(bases->patch_id);\n+\t\tbases->nr_patch_id = 0;\n+\t\tbases->alloc_patch_id = 0;\n+\t\toidclr(&bases->base_commit);\n+\t} else if (!is_null_oid(&bases->parent_commit)) {\n+\t\tprintf(\"** parent-commit-info **\\n\");\n \n-\tfree(bases->patch_id);\n-\tbases->nr_patch_id = 0;\n-\tbases->alloc_patch_id = 0;\n-\toidclr(&bases->base_commit);\n+\t\t/* Show the parent commit */\n+\t\tprintf(\"parent-commit: %s\\n\", oid_to_hex(&bases->parent_commit));\n+\t\t/* Show the parent patch id */\n+\t\tprintf(\"parent-patch-id: %s\\n\", oid_to_hex(&bases->parent_patch_id));\n+\n+\t\toidclr(&bases->parent_commit);\n+\t}\n }\n \n int cmd_format_patch(int argc, const char **argv, const char *prefix)\n-- \n2.8.0.rc4.4.ga41a987\n"},{"id":"281531","messageId":"1458723147-7335-5-git-send-email-xiaolong.ye@intel.com","threadId":"41790","inReplyTo":"1458723147-7335-1-git-send-email-xiaolong.ye@intel.com","subject":"[PATCH v2 4/4] format-patch: introduce format.base configuration","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-23T08:52:27Z","receivedAt":"2016-03-23T08:52:27Z","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 |  5 +++++\n builtin/log.c                      | 21 ++++++++++++++-------\n 2 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex a5f145e..7c7a61a 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -279,6 +279,11 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.\n \t\"Z..C\" instead of \"-3 C\" to specify the range), and the identifiers\n \tfor P, X, Y, Z are appended at the end of the _first_ message (either\n \tthe cover letter or the first patch in the series).\n+\tIf 'format.base=auto' is set in configuration file, it is equivalent\n+\tto set '--base=auto' in cmdline, it will track the base commit\n+\tautomatically, the base commit will be the tip commit of upstream\n+\tbranch, if upstream branch cannot be determined, it will just record\n+\tthe parent's commit id and patch id.\n \n --root::\n \tTreat the revision argument as a <revision range>, even if it\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e8a3964..fc468ce 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -699,6 +699,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@@ -780,6 +781,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@@ -1213,7 +1220,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@@ -1227,11 +1239,6 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\t\thashcpy(bases->parent_patch_id.hash, sha1);\n \t\t\treturn;\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 \tif (base == prerequisite_head)\n@@ -1626,7 +1633,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tsignature = strbuf_detach(&buf, NULL);\n \t}\n \n-\tif (base_commit) {\n+\tif (base_commit || config_base_commit) {\n \t\tstruct commit *prerequisite_head = NULL;\n \t\tif (list[nr - 1]->parents)\n \t\t\tprerequisite_head = list[nr - 1]->parents->item;\n-- \n2.8.0.rc4.4.ga41a987\n"},{"id":"281596","messageId":"xmqqfuvhcbjf.fsf@gitster.mtv.corp.google.com","threadId":"41790","inReplyTo":"1458723147-7335-3-git-send-email-xiaolong.ye@intel.com","subject":"Re: [PATCH v2 2/4] format-patch: add '--base' option to record base tree info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-23T18:08:20Z","receivedAt":"2016-03-23T18:08:20Z","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\nReviewing the patch out of order, caller first and then callee.\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> +\tprintf(\"** base-commit-info **\\n\");\n\nI am not sure if you want to have this line (an empty line might not\nhurt), as the \"base-commit: ...\" that comes next is a clear enough\nindication of what these lines are.\n\n> +\tif (base_commit) {\n> +\t\tstruct commit *prerequisite_head = NULL;\n> +\t\tif (list[nr - 1]->parents)\n> +\t\t\tprerequisite_head = list[nr - 1]->parents->item;\n> +\t\tmemset(&bases, 0, sizeof(bases));\n> +\t\treset_revision_walk();\n> +\t\tprepare_bases(&bases, base_commit, prerequisite_head);\n> +\t}\n> +\n\nlist[] holds the commits in reverse topological order, so the first\nparent of the last element in the list[] would hopefully give you\nthe latest change your series depends on, and that is why you are\nworking on list[nr - 1] here.\n\nI however think that is flawed.\n\nWhat if your series A, B and C are on this topology?\n\n    ---P---X---A---M---C\n        \\         /\n         Y---Z---B\n\n\"git format-patch --base=P -3 C\" would show A, B and C.  It may show\nB, A and C, as A and B are independent (you would be flattening the\nhistory into the shape you have in your documentation part of the\npatch in order to adjust for their interactions before running\nformat-patch if that were not the case).  Depending on which one\nhappens to be chosen as prerequisite_head, you would miss either X\nor Y & Z, wouldn't you?\n\nA simpler and safer (but arguably less useful) approach may be to\nuse the logic and limitation of your patch as-is but add code to\ncheck that the history is linear and refuse to do the \"base\" thing.\nBut just in case you want to handle a more general case like the\nabove, read on.\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 *prerequisite_head)\n> +{\n> +\tstruct commit *base = NULL, *commit;\n> +\tstruct rev_info revs;\n> +\tstruct diff_options diffopt;\n> +\tstruct object_id *patch_id;\n> +\tunsigned char sha1[20];\n> +\tint pos = 0;\n> +\n> +\tif (!prerequisite_head)\n> +\t\treturn;\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> +\tif (base == prerequisite_head)\n> +\t\treturn;\n> +\n> +\tif (!in_merge_bases(base, prerequisite_head))\n> +\t\tdie(_(\"base commit should be the ancestor of revs you specified\"));\n> +\n> +\tinit_revisions(&revs, NULL);\n> +\trevs.max_parents = 1;\n> +\n> +\tbase->object.flags |= UNINTERESTING;\n> +\tadd_pending_object(&revs, &base->object, \"base\");\n> +\tprerequisite_head->object.flags |= 0;\n> +\tadd_pending_object(&revs, &prerequisite_head->object, \"prerequisite-head\");\n> +\n> +\tdiff_setup(&diffopt);\n> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n> +\tdiff_setup_done(&diffopt);\n> +\n> +\tif (prepare_revision_walk(&revs))\n> +\t\tdie(_(\"revision walk setup failed\"));\n> +\t/*\n> +\t * Traverse the commits list between base and prerequisite head,\n> +\t * get the patch ids and stuff them in bases structure.\n> +\t */\n> +\twhile ((commit = get_revision(&revs)) != NULL) {\n> +\t\tif (commit_patch_id(commit, &diffopt, sha1))\n> +\t\t\treturn;\n> +\t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n> +\t\tpatch_id = bases->patch_id + pos;\n> +\t\thashcpy(patch_id->hash, sha1);\n> +\t\tpos++;\n> +\t\tbases->nr_patch_id++;\n\nMicronit.  Aren't pos and nr_patch_id always the same?\n\n> +\t}\n> +}\n\nI think the right thing to do is probably to start another revision\nwalk (which you do) but setting the starting points of the traversal\nto all elements in the list[] (which you don't--you use either A^ or\nB^).  And skip the ones in the list[] before grabbing its patch-id\nin the loop.  That way, you do not have to worry about the topology\nof the history tripping you up at all.\n\nSo I'd suggest to redo this function perhaps like so, if you do want\nto handle the more general case:\n\nstatic void prepare_bases(struct base_tree_info *bases,\n\t\t\t  const char *base_commit,\n\t\t\t  struct commit **list,\n                          int total)\n{\n\t... boilerplate ...\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\tadd_pending_commit(&revs, base, UNINTERESTING);\n\tfor (i = 0; i < total; i++)\n        \tadd_pending_commit(&revs, list[i], 0);\n\n\tif (prepare_revision_walk(&revs))\n\t\tdie(_(\"revision walk setup failed\"));\n\n\twhile ((commit = get_revision(&revs)) != NULL) {\n        \tif (COMMIT_IS_IN_LIST(commit))\n                \tcontinue;\n\t\tif (commit_patch_id(commit, &diffopt, sha1))\n                \tdie(\"cannot get patch id\");\n\t\t... do your ptach_id thing ...\n\t}\n}\n\nAnd COMMIT_IS_IN_LIST() can probably be implemented by using commit->util\nfield, e.g. change the part that sets up the traversal\n\n\tfor (i = 0; i < total; i++) {\n        \tadd_pending_commit(&revs, list[i], 0);\n\t\tlist[i]->util = (void *)1;\n\t}\n\nto mark those in list[] as such, and the test would be\n\n\t\tif (commit->util)\n                \tcontinue; /* on list[] */\n\nor something like that.\n"},{"id":"281597","messageId":"xmqqbn65caqi.fsf@gitster.mtv.corp.google.com","threadId":"41790","inReplyTo":"1458723147-7335-4-git-send-email-xiaolong.ye@intel.com","subject":"Re: [PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-23T18:25:41Z","receivedAt":"2016-03-23T18:25:41Z","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> +\n> +\tdiff_setup(&diffopt);\n> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n> +\tdiff_setup_done(&diffopt);\n\nIt is annoying that you moved \"diff\" stuff here (if it can be\ninitialized once at the beginning and then reused over and over,\nit should have been done here from the beginning at PATCH 2/4).\n\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\tbase = lookup_commit_or_die(sha1, \"upstream base\");\n> +\t\t\toidcpy(&bases->base_commit, &base->object.oid);\n> +\t\t} else {\n> +\t\t\tcommit_patch_id(prerequisite_head, &diffopt, sha1);\n> +\t\t\toidcpy(&bases->parent_commit, &prerequisite_head->object.oid);\n> +\t\t\thashcpy(bases->parent_patch_id.hash, sha1);\n> +\t\t\treturn;\n\nWhat happens if you did this sequence?\n\n\t$ git fetch origin\n        $ git checkout -b fork origin/master\n        $ git fetch origin\n        $ git format-patch --base=auto origin..\n\nYou grab the updated origin/master as base and use it here, no?\nAt that point the topology would look like:\n\n          1---2---3 updated upstream\n         /\n\t0---X---Y---Z---A---B---C\n        ^\n        old upstream\n\nso you are basing your worn on \"0\" (old upstream) but setting base\nto \"3\"\n\nWouldn't that trigger \"base must be an ancestor of Z\" check you had\nin [PATCH 2/4]?\n\nI also do not see the point of showing \"parent id\" which as far as I\ncan see is just a random commit object name and show different\noutput that is not even described what it is.  It would be better to\n\n * find the upstream (i.e. 3 in the picture) and then with our range\n   (i.e. A B and C) compute the merge base (i.e. you would find 0)\n   and use it as base;\n\n * if there is no upstream, error out and tell the user that there\n   is no upstream.  The user is intelligent enough and knows what\n   commit the base should be.\n\nI suspect, but I didn't think things through.\n"},{"id":"281631","messageId":"20160324030811.GA26582@yexl-desktop","threadId":"41790","inReplyTo":"xmqqfuvhcbjf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/4] format-patch: add '--base' option to record base tree info","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-24T03:08:11Z","receivedAt":"2016-03-24T03:08:11Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Wed, Mar 23, 2016 at 11:08:20AM -0700, Junio C Hamano wrote:\n>Xiaolong Ye <xiaolong.ye@intel.com> writes:\n>\n>Reviewing the patch out of order, caller first and then callee.\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>> +\tprintf(\"** base-commit-info **\\n\");\n>\n>I am not sure if you want to have this line (an empty line might not\n>hurt), as the \"base-commit: ...\" that comes next is a clear enough\n>indication of what these lines are.\n\nOk, I will remove this extra line.\n>\n>> +\tif (base_commit) {\n>> +\t\tstruct commit *prerequisite_head = NULL;\n>> +\t\tif (list[nr - 1]->parents)\n>> +\t\t\tprerequisite_head = list[nr - 1]->parents->item;\n>> +\t\tmemset(&bases, 0, sizeof(bases));\n>> +\t\treset_revision_walk();\n>> +\t\tprepare_bases(&bases, base_commit, prerequisite_head);\n>> +\t}\n>> +\n>\n>list[] holds the commits in reverse topological order, so the first\n>parent of the last element in the list[] would hopefully give you\n>the latest change your series depends on, and that is why you are\n>working on list[nr - 1] here.\n\nYes, I just considered linear topology before.\n>\n>I however think that is flawed.\n>\n>What if your series A, B and C are on this topology?\n>\n>    ---P---X---A---M---C\n>        \\         /\n>         Y---Z---B\n>\n>\"git format-patch --base=P -3 C\" would show A, B and C.  It may show\n>B, A and C, as A and B are independent (you would be flattening the\n>history into the shape you have in your documentation part of the\n>patch in order to adjust for their interactions before running\n>format-patch if that were not the case).  Depending on which one\n>happens to be chosen as prerequisite_head, you would miss either X\n>or Y & Z, wouldn't you?\n>\n>A simpler and safer (but arguably less useful) approach may be to\n>use the logic and limitation of your patch as-is but add code to\n>check that the history is linear and refuse to do the \"base\" thing.\n>But just in case you want to handle a more general case like the\n>above, read on.\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 *prerequisite_head)\n>> +{\n>> +\tstruct commit *base = NULL, *commit;\n>> +\tstruct rev_info revs;\n>> +\tstruct diff_options diffopt;\n>> +\tstruct object_id *patch_id;\n>> +\tunsigned char sha1[20];\n>> +\tint pos = 0;\n>> +\n>> +\tif (!prerequisite_head)\n>> +\t\treturn;\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>> +\tif (base == prerequisite_head)\n>> +\t\treturn;\n>> +\n>> +\tif (!in_merge_bases(base, prerequisite_head))\n>> +\t\tdie(_(\"base commit should be the ancestor of revs you specified\"));\n>> +\n>> +\tinit_revisions(&revs, NULL);\n>> +\trevs.max_parents = 1;\n>> +\n>> +\tbase->object.flags |= UNINTERESTING;\n>> +\tadd_pending_object(&revs, &base->object, \"base\");\n>> +\tprerequisite_head->object.flags |= 0;\n>> +\tadd_pending_object(&revs, &prerequisite_head->object, \"prerequisite-head\");\n>> +\n>> +\tdiff_setup(&diffopt);\n>> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>> +\tdiff_setup_done(&diffopt);\n>> +\n>> +\tif (prepare_revision_walk(&revs))\n>> +\t\tdie(_(\"revision walk setup failed\"));\n>> +\t/*\n>> +\t * Traverse the commits list between base and prerequisite head,\n>> +\t * get the patch ids and stuff them in bases structure.\n>> +\t */\n>> +\twhile ((commit = get_revision(&revs)) != NULL) {\n>> +\t\tif (commit_patch_id(commit, &diffopt, sha1))\n>> +\t\t\treturn;\n>> +\t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n>> +\t\tpatch_id = bases->patch_id + pos;\n>> +\t\thashcpy(patch_id->hash, sha1);\n>> +\t\tpos++;\n>> +\t\tbases->nr_patch_id++;\n>\n>Micronit.  Aren't pos and nr_patch_id always the same?\nSorry, will only use nr_patch_id.\n>\n>> +\t}\n>> +}\n>\n>I think the right thing to do is probably to start another revision\n>walk (which you do) but setting the starting points of the traversal\n>to all elements in the list[] (which you don't--you use either A^ or\n>B^).  And skip the ones in the list[] before grabbing its patch-id\n>in the loop.  That way, you do not have to worry about the topology\n>of the history tripping you up at all.\n>\n>So I'd suggest to redo this function perhaps like so, if you do want\n>to handle the more general case:\n\nThanks for the elaborated suggestions. I will redo the prepare_bases\nfunction accordingly to handle more general case.\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>                          int total)\n>{\n>\t... boilerplate ...\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>\tadd_pending_commit(&revs, base, UNINTERESTING);\n>\tfor (i = 0; i < total; i++)\n>        \tadd_pending_commit(&revs, list[i], 0);\n>\n>\tif (prepare_revision_walk(&revs))\n>\t\tdie(_(\"revision walk setup failed\"));\n>\n>\twhile ((commit = get_revision(&revs)) != NULL) {\n>        \tif (COMMIT_IS_IN_LIST(commit))\n>                \tcontinue;\n>\t\tif (commit_patch_id(commit, &diffopt, sha1))\n>                \tdie(\"cannot get patch id\");\n>\t\t... do your ptach_id thing ...\n>\t}\n>}\n>\n>And COMMIT_IS_IN_LIST() can probably be implemented by using commit->util\n>field, e.g. change the part that sets up the traversal\n>\n>\tfor (i = 0; i < total; i++) {\n>        \tadd_pending_commit(&revs, list[i], 0);\n>\t\tlist[i]->util = (void *)1;\n>\t}\n>\n>to mark those in list[] as such, and the test would be\n>\n>\t\tif (commit->util)\n>                \tcontinue; /* on list[] */\n>\n>or something like that.\n"},{"id":"281632","messageId":"20160324041925.GB26582@yexl-desktop","threadId":"41790","inReplyTo":"xmqqbn65caqi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-03-24T04:19:25Z","receivedAt":"2016-03-24T04:19:25Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:\n>Xiaolong Ye <xiaolong.ye@intel.com> writes:\n>\n>> +\n>> +\tdiff_setup(&diffopt);\n>> +\tDIFF_OPT_SET(&diffopt, RECURSIVE);\n>> +\tdiff_setup_done(&diffopt);\n>\n>It is annoying that you moved \"diff\" stuff here (if it can be\n>initialized once at the beginning and then reused over and over,\n>it should have been done here from the beginning at PATCH 2/4).\n\nMy bad, I will put \"diff\" stuff at the beginning at PATCH 2/4.\n>\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\tbase = lookup_commit_or_die(sha1, \"upstream base\");\n>> +\t\t\toidcpy(&bases->base_commit, &base->object.oid);\n>> +\t\t} else {\n>> +\t\t\tcommit_patch_id(prerequisite_head, &diffopt, sha1);\n>> +\t\t\toidcpy(&bases->parent_commit, &prerequisite_head->object.oid);\n>> +\t\t\thashcpy(bases->parent_patch_id.hash, sha1);\n>> +\t\t\treturn;\n>\n>What happens if you did this sequence?\n>\n>\t$ git fetch origin\n>        $ git checkout -b fork origin/master\n>        $ git fetch origin\n>        $ git format-patch --base=auto origin..\n>\n>You grab the updated origin/master as base and use it here, no?\n>At that point the topology would look like:\n>\n>          1---2---3 updated upstream\n>         /\n>\t0---X---Y---Z---A---B---C\n>        ^\n>        old upstream\n>\n>so you are basing your worn on \"0\" (old upstream) but setting base\n>to \"3\"\n>\n>Wouldn't that trigger \"base must be an ancestor of Z\" check you had\n>in [PATCH 2/4]?\n\nYes, this is flawed, I will follow your below suggestion to compute\nthe merge base as the base commit through upstream and specified range.\n\n>\n>I also do not see the point of showing \"parent id\" which as far as I\n>can see is just a random commit object name and show different\n>output that is not even described what it is.  It would be better to\n\nHere is our consideration:\nThere is high chance that branch_get_upstream will retrun NULL(thus we\nare not able to get exact base commit), since developers may checkout\nbranch from a local branch or a commit and haven't set \"--set-upstream-to\"\nto track a remote branch, in this case, we want to provide likely useful\ninfo(here is parent commit id and patch id), based on it, 0day robot still\nhave good chance to find the suitable base.\nOtherwise, I'm afraid this annotation system won't work effectively in long run.\n\nThanks,\nXiaolong.\n>\n> * find the upstream (i.e. 3 in the picture) and then with our range\n>   (i.e. A B and C) compute the merge base (i.e. you would find 0)\n>   and use it as base;\n>\n> * if there is no upstream, error out and tell the user that there\n>   is no upstream.  The user is intelligent enough and knows what\n>   commit the base should be.\n>\n>I suspect, but I didn't think things through.\n>\n>\n>--\n>To unsubscribe from this list: send the line \"unsubscribe git\" in\n>the body of a message to majordomo@vger.kernel.org\n>More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"281687","messageId":"xmqq37rfajy1.fsf@gitster.mtv.corp.google.com","threadId":"41790","inReplyTo":"20160324041925.GB26582@yexl-desktop","subject":"Re: [PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-24T17:01:58Z","receivedAt":"2016-03-24T17:01:58Z","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> On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:\n>>I also do not see the point of showing \"parent id\" which as far as I\n>>can see is just a random commit object name and show different\n>>output that is not even described what it is.  It would be better to\n>\n> Here is our consideration:\n> There is high chance that branch_get_upstream will retrun NULL(thus we\n> are not able to get exact base commit), since developers may checkout\n> branch from a local branch or a commit and haven't set \"--set-upstream-to\"\n> to track a remote branch, in this case, we want to provide likely useful\n> info(here is parent commit id and patch id)\n\nI do not agree with that reasoning at all, primarily because your\n\"likely useful\" is not justfied.\n\nCould you explain what makes you think that it is \"likely\" that the\ncommit that matches \"parent commit id\" is available to the recipient\nof the patch?\n\nWhatever the reason is, if it _is_ likely, then I do not see a point\nin spending cycle to do get-upstream and merge-base, or giving an\noption to the user to explicitly specify the base.  Given that this\nseries does these things, I'd have to conclude your \"likely useful\"\nis \"might possibly turn out to be useful in some cases if you are\nlucky but is no way reliable\" at best.\n\nRather than throwing an unreliable information in the output and\nblindly proceeding, I'd think you would want to error out and tell\nthe user to explicitly give the base without producing the patch\noutput.  That way, you will not get patches with unreliable\ninformation.\n\nSuggesting to use set-upstream-to when you give that error message\nmay also be helpful.\n"},{"id":"282429","messageId":"20160401050749.GA25392@yexl-desktop","threadId":"41790","inReplyTo":"xmqq37rfajy1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2016-04-01T05:07:49Z","receivedAt":"2016-04-01T05:07:49Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On Thu, Mar 24, 2016 at 10:01:58AM -0700, Junio C Hamano wrote:\n>Ye Xiaolong <xiaolong.ye@intel.com> writes:\n>\n>> On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:\n>>>I also do not see the point of showing \"parent id\" which as far as I\n>>>can see is just a random commit object name and show different\n>>>output that is not even described what it is.  It would be better to\n>>\n>> Here is our consideration:\n>> There is high chance that branch_get_upstream will retrun NULL(thus we\n>> are not able to get exact base commit), since developers may checkout\n>> branch from a local branch or a commit and haven't set \"--set-upstream-to\"\n>> to track a remote branch, in this case, we want to provide likely useful\n>> info(here is parent commit id and patch id)\n>\n>I do not agree with that reasoning at all, primarily because your\n>\"likely useful\" is not justfied.\n>\n>Could you explain what makes you think that it is \"likely\" that the\n>commit that matches \"parent commit id\" is available to the recipient\n>of the patch?\n>\n\nHi, Junio\n\nStill want to discuss with you about the proposal that showing the \"parent\ncommit-id as well as patch-id\" when exact base commit is failed to get through\ncmdline or upstream\", from 0day robot's view, there would be 2 kinds of base tree\ninfo appended at the bottom of patch message.\n\nFor the info starts with \"base-commit: <xxx> ...\", robot would know it\nis reliable base commit, it would just checkout it and apply the prerequisite\npatches and patchset for the work.\n\nFor the info starts with \"parent-commit: <xxx>; parent-patch-id: <xxx>\",\nthere are 3 situations 0day robot would need to handle:\n\n1) parent commit is well-known public commit(e.g. one in Linus's tree),\n   in this case, robot will just checkout the parent commit and apply the\n   patchset accordingly.\n\n2) parent commit is well-known in-flight patch, since 0day maintains the\n   database of in-flight patches indexed by their patch-ids and\n   commit-ids(of the git tree mentioned below), it also exports a git tree\n   which holds all in-flight patches, where each patchset map to a new branch:\n\n   https://github.com/0day-ci/linux/branches\n\n   0day will find that patch in database by parent patch id, then do the\n   checkout and apply work.\n\n3) parent commit/patch-id is unknown, maybe because it's a\n   - private patch;\n   - public patch that's slightly modified locally;\n   - public patch that's not covered by the 0day database\n   all should be rare cases.\n\nIn practice, most developers may not feed exact commit id by --base=<xxx>\nregularly when using git format-patch, this \"showing parent commit\" solution\ncould save developers' energy and reach a high coverage in the meantime.\n\nThanks,\nXiaolong.\n\n>Whatever the reason is, if it _is_ likely, then I do not see a point\n>in spending cycle to do get-upstream and merge-base, or giving an\n>option to the user to explicitly specify the base.  Given that this\n>series does these things, I'd have to conclude your \"likely useful\"\n>is \"might possibly turn out to be useful in some cases if you are\n>lucky but is no way reliable\" at best.\n>\n>Rather than throwing an unreliable information in the output and\n>blindly proceeding, I'd think you would want to error out and tell\n>the user to explicitly give the base without producing the patch\n>output.  That way, you will not get patches with unreliable\n>information.\n>\n>Suggesting to use set-upstream-to when you give that error message\n>may also be helpful.\n"},{"id":"282480","messageId":"xmqqd1q9jnf9.fsf@gitster.mtv.corp.google.com","threadId":"41790","inReplyTo":"20160401050749.GA25392@yexl-desktop","subject":"Re: [PATCH v2 3/4] format-patch: introduce --base=auto option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T16:36:58Z","receivedAt":"2016-04-01T16:36:58Z","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> For the info starts with \"base-commit: <xxx> ...\", robot would know it\n> is reliable base commit, it would just checkout it and apply the prerequisite\n> patches and patchset for the work.\n>\n> For the info starts with \"parent-commit: <xxx>; parent-patch-id: <xxx>\",\n> there are 3 situations 0day robot would need to handle:\n\nIt all depends on how you are computing this \"parent-commit\" thing.\nWhen I reviewed the patch that implemented the fallback, my\nimpression was that it wasn't computing anything relevant, but just\nwas picking one of the random commit nearby in the history.\n\nAssuming that it is not the case, and it is computing something\nsensible, then I still do not see a point in marking these guessed\nones differently as \"parent-commit\" (not \"base-commit\").  After all,\na \"base-commit\" that was explicitly specified by the user can be\nsomething that is inappropriate (e.g. the user may have thought the\nbase was a well-known one when his work was built on top of an early\ndraft of the patch that was later tweaked), so recipients of the\npatch series need to be prepared to deal with a bogus base anyway.\n\n> 2) parent commit is well-known in-flight patch, since 0day maintains the\n>    database of in-flight patches indexed by their patch-ids and\n>    commit-ids(of the git tree mentioned below), it also exports a git tree\n>    which holds all in-flight patches, where each patchset map to a new branch:\n>\n>    https://github.com/0day-ci/linux/branches\n>\n>    0day will find that patch in database by parent patch id, then do the\n>    checkout and apply work.\n\nIf a user starts from some solid base, applies a well-known\nin-flight series, and builds his series on it and sends his series\nout, and the base is identified by the patch-id of the last patch of\nthe in-flight series, then from that \"parent patch-id\" the robot can\nfind a commit that is the result of applying the same in-flight\nseries on top of some other solid base, as there is no coordination\nbetween the user and 0day-ci system.  I would imagine that the\ndifference between the bases do matter--otherwise you wouldn't be\nbuilding this elaborate \"base-commit\" system that rejects the notion\nthat it is not sufficient for patches to textually apply cleanly and\ninsists that they have to be applied to exactly one known state.\n\nSo, I am not sure why you think \"patch-id\" is a good way to identify\nthe commit to be based on.\n\n> In practice, most developers may not feed exact commit id by --base=<xxx>\n> regularly when using git format-patch, this \"showing parent commit\" solution\n> could save developers' energy and reach a high coverage in the meantime.\n\nSo, while I understand the desire and wish, I am not convinced \"as a\nfallback, use parent patch-id and guess where to apply\" is a good\napprach to make the wish come true.\n"}]}