{"thread":{"id":"37849","subject":"[RFC/PATCH] bisect: clean flags after checking merge bases","startedAt":"2014-10-30T19:01:11Z","lastAt":"2014-10-30T19:19:43Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251232","messageId":"xmqq4mulwfew.fsf@gitster.dls.corp.google.com","threadId":"37849","inReplyTo":null,"subject":"[RFC/PATCH] bisect: clean flags after checking merge bases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-30T19:01:11Z","receivedAt":"2014-10-30T19:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Unless there is a good reason to belieave that a particular\ninvocation of a get_merge_bases*() is the last one that cares about\nthe object flags the computation of merge bases leaves on the\nobjects, the \"cleanup\" parameter should always be true, and I do not\nthink there is one in this codepath.\n\nFound by code inspection.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * I am planning to eventually change the function signature for\n   this function to drop \"cleanup\" and change rare selected\n   codepaths that know what they are doing to call a separate\n   function that allows the flags not to be cleaned up.  Two calls\n   in builtin/merge-base.c are both the last ones that can leave the\n   flags on the objects, but I do not think this one, which happens\n   before the main traversal even starts, should pass 0 here.\n\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex df09cbc..ccca3b5 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -777,7 +777,7 @@ static void check_merge_bases(int no_checkout)\n \tint rev_nr;\n \tstruct commit **rev = get_bad_and_good_commits(&rev_nr);\n \n-\tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);\n+\tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 1);\n \n \tfor (; result; result = result->next) {\n \t\tconst unsigned char *mb = result->item->object.sha1;\n"},{"id":"251233","messageId":"xmqqvbn1uzzk.fsf@gitster.dls.corp.google.com","threadId":"37849","inReplyTo":"xmqq4mulwfew.fsf@gitster.dls.corp.google.com","subject":"[PATCH] get_merge_bases(): always clean-up object flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-30T19:19:43Z","receivedAt":"2014-10-30T19:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The callers of get_merge_bases() can choose to leave object flags\nused during the merge-base traversal by passing cleanup=0 as a\nparameter, but in practice a very few callers can afford to do so\n(namely, \"git merge-base\"), as they need to compute merge base in\npreparation for other processing of their own and they need to see\nthe object without contaminate flags.\n\nChange the function signature of get_merge_bases_many() and\nget_merge_bases() to drop the cleanup parameter, so that the\nmajority of the callers do not have to say \", 1\" at the end.\n\nGive a new get_merge_bases_many_dirty() API to support only a few\ncallers that know they do not need to spend cycles cleaning up the\nobject flags.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This comes on top of the previous \"is it kosher for bisect to use\n   the 'cleanup=0' variant in the merge-base computation upfront? I\n   do not think so\" patch.\n\n bisect.c             |  2 +-\n builtin/merge-base.c |  4 ++--\n builtin/merge.c      |  4 ++--\n builtin/rev-parse.c  |  2 +-\n commit.c             | 29 +++++++++++++++++++++--------\n commit.h             |  7 +++++--\n merge-recursive.c    |  2 +-\n notes-merge.c        |  2 +-\n revision.c           |  4 ++--\n sha1_name.c          |  2 +-\n submodule.c          |  2 +-\n 11 files changed, 38 insertions(+), 22 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex ccca3b5..8c6d843 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -777,7 +777,7 @@ static void check_merge_bases(int no_checkout)\n \tint rev_nr;\n \tstruct commit **rev = get_bad_and_good_commits(&rev_nr);\n \n-\tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 1);\n+\tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);\n \n \tfor (; result; result = result->next) {\n \t\tconst unsigned char *mb = result->item->object.sha1;\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 0ecde8d..fdebef6 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -10,7 +10,7 @@ static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n {\n \tstruct commit_list *result;\n \n-\tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);\n+\tresult = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1);\n \n \tif (!result)\n \t\treturn 1;\n@@ -176,7 +176,7 @@ static int handle_fork_point(int argc, const char **argv)\n \tfor (i = 0; i < revs.nr; i++)\n \t\trevs.commit[i]->object.flags &= ~TMP_MARK;\n \n-\tbases = get_merge_bases_many(derived, revs.nr, revs.commit, 0);\n+\tbases = get_merge_bases_many_dirty(derived, revs.nr, revs.commit);\n \n \t/*\n \t * There should be one and only one merge base, when we found\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bebbe5b..5249f4e 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1320,7 +1320,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (!remoteheads)\n \t\t; /* already up-to-date */\n \telse if (!remoteheads->next)\n-\t\tcommon = get_merge_bases(head_commit, remoteheads->item, 1);\n+\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n \telse {\n \t\tstruct commit_list *list = remoteheads;\n \t\tcommit_list_insert(head_commit, &list);\n@@ -1417,7 +1417,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t * merge_bases again, otherwise \"git merge HEAD^\n \t\t\t * HEAD^^\" would be missed.\n \t\t\t */\n-\t\t\tcommon_one = get_merge_bases(head_commit, j->item, 1);\n+\t\t\tcommon_one = get_merge_bases(head_commit, j->item);\n \t\t\tif (hashcmp(common_one->item->object.sha1,\n \t\t\t\tj->item->object.sha1)) {\n \t\t\t\tup_to_date = 0;\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 35d3c43..95328b8 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -279,7 +279,7 @@ static int try_difference(const char *arg)\n \t\t\tstruct commit *a, *b;\n \t\t\ta = lookup_commit_reference(sha1);\n \t\t\tb = lookup_commit_reference(end);\n-\t\t\texclude = get_merge_bases(a, b, 1);\n+\t\t\texclude = get_merge_bases(a, b);\n \t\t\twhile (exclude) {\n \t\t\t\tstruct commit_list *n = exclude->next;\n \t\t\t\tshow_rev(REVERSED,\ndiff --git a/commit.c b/commit.c\nindex 19cf8f9..d13250e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -867,7 +867,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)\n \n \t\tfor (j = ret; j; j = j->next) {\n \t\t\tstruct commit_list *bases;\n-\t\t\tbases = get_merge_bases(i->item, j->item, 1);\n+\t\t\tbases = get_merge_bases(i->item, j->item);\n \t\t\tif (!new)\n \t\t\t\tnew = bases;\n \t\t\telse\n@@ -936,10 +936,10 @@ static int remove_redundant(struct commit **array, int cnt)\n \treturn filled;\n }\n \n-struct commit_list *get_merge_bases_many(struct commit *one,\n-\t\t\t\t\t int n,\n-\t\t\t\t\t struct commit **twos,\n-\t\t\t\t\t int cleanup)\n+static struct commit_list *get_merge_bases_many_0(struct commit *one,\n+\t\t\t\t\t\t  int n,\n+\t\t\t\t\t\t  struct commit **twos,\n+\t\t\t\t\t\t  int cleanup)\n {\n \tstruct commit_list *list;\n \tstruct commit **rslt;\n@@ -977,10 +977,23 @@ struct commit_list *get_merge_bases_many(struct commit *one,\n \treturn result;\n }\n \n-struct commit_list *get_merge_bases(struct commit *one, struct commit *two,\n-\t\t\t\t    int cleanup)\n+struct commit_list *get_merge_bases_many(struct commit *one,\n+\t\t\t\t\t int n,\n+\t\t\t\t\t struct commit **twos)\n+{\n+\treturn get_merge_bases_many_0(one, n, twos, 1);\n+}\n+\n+struct commit_list *get_merge_bases_many_dirty(struct commit *one,\n+\t\t\t\t\t       int n,\n+\t\t\t\t\t       struct commit **twos)\n+{\n+\treturn get_merge_bases_many_0(one, n, twos, 0);\n+}\n+\n+struct commit_list *get_merge_bases(struct commit *one, struct commit *two)\n {\n-\treturn get_merge_bases_many(one, 1, &two, cleanup);\n+\treturn get_merge_bases_many_0(one, 1, &two, 1);\n }\n \n /*\ndiff --git a/commit.h b/commit.h\nindex bc68ccb..28bc553 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -236,10 +236,13 @@ struct commit_graft *read_graft_line(char *buf, int len);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const unsigned char *sha1);\n \n-extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);\n-extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);\n+extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);\n+extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos);\n extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n \n+/* To be used only when object flags after this call no longer matter */\n+extern struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, struct commit **twos);\n+\n /* largest positive number a signed 32-bit integer can contain */\n #define INFINITE_DEPTH 0x7fffffff\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex fdb7d0f..f758f7e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1904,7 +1904,7 @@ int merge_recursive(struct merge_options *o,\n \t}\n \n \tif (!ca) {\n-\t\tca = get_merge_bases(h1, h2, 1);\n+\t\tca = get_merge_bases(h1, h2);\n \t\tca = reverse_commit_list(ca);\n \t}\n \ndiff --git a/notes-merge.c b/notes-merge.c\nindex 7eb9d7a..109ff4e 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -594,7 +594,7 @@ int notes_merge(struct notes_merge_options *o,\n \tassert(local && remote);\n \n \t/* Find merge bases */\n-\tbases = get_merge_bases(local, remote, 1);\n+\tbases = get_merge_bases(local, remote);\n \tif (!bases) {\n \t\tbase_sha1 = null_sha1;\n \t\tbase_tree_sha1 = EMPTY_TREE_SHA1_BIN;\ndiff --git a/revision.c b/revision.c\nindex 75dda92..14e0e03 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1441,7 +1441,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \tother = lookup_commit_or_die(sha1, \"MERGE_HEAD\");\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n-\tbases = get_merge_bases(head, other, 1);\n+\tbases = get_merge_bases(head, other);\n \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n \tfree_commit_list(bases);\n@@ -1546,7 +1546,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t     : lookup_commit_reference(b_obj->sha1));\n \t\t\t\tif (!a || !b)\n \t\t\t\t\tgoto missing;\n-\t\t\t\texclude = get_merge_bases(a, b, 1);\n+\t\t\t\texclude = get_merge_bases(a, b);\n \t\t\t\tadd_rev_cmdline_list(revs, exclude,\n \t\t\t\t\t\t     REV_CMD_MERGE_BASE,\n \t\t\t\t\t\t     flags_exclude);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5b004f5..9737557 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -993,7 +993,7 @@ int get_sha1_mb(const char *name, unsigned char *sha1)\n \ttwo = lookup_commit_reference_gently(sha1_tmp, 0);\n \tif (!two)\n \t\treturn -1;\n-\tmbs = get_merge_bases(one, two, 1);\n+\tmbs = get_merge_bases(one, two);\n \tif (!mbs || mbs->next)\n \t\tst = -1;\n \telse {\ndiff --git a/submodule.c b/submodule.c\nindex 0690dc5..d37d400 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -301,7 +301,7 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n \tleft->object.flags |= SYMMETRIC_LEFT;\n \tadd_pending_object(rev, &left->object, path);\n \tadd_pending_object(rev, &right->object, path);\n-\tmerge_bases = get_merge_bases(left, right, 1);\n+\tmerge_bases = get_merge_bases(left, right);\n \tif (merge_bases) {\n \t\tif (merge_bases->item == left)\n \t\t\t*fast_forward = 1;\n"}]}