{"thread":{"id":"53737","subject":"[PATCH] commit-reach: avoid is_dependant_of() shim","startedAt":"2020-06-23T08:17:28Z","lastAt":"2020-06-23T18:54:01Z","messageCount":4,"participants":["Carlo Marcelo Arenas Belón","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"400347","messageId":"20200623081701.52607-1-carenas@gmail.com","threadId":"53737","inReplyTo":null,"subject":"[PATCH] commit-reach: avoid is_dependant_of() shim","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-06-23T08:17:01Z","receivedAt":"2020-06-23T08:17:28Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d91d6fbf26 (commit-reach: create repo_is_descendant_of(), 2020-06-17)\nadds a repository aware version of is_dependant_of() and a backward\ncompatibility shim that is barelly used.\n\nUpdate all callers to directly use the new repo_is_dependant_of()\nfunction instead.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n builtin/pull.c        |  3 ++-\n commit-reach.c        | 16 ++++++----------\n commit-reach.h        |  4 +++-\n t/helper/test-reach.c |  2 +-\n 4 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 8e6572d305..babc6a4e36 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1025,7 +1025,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tcommit_list_insert(head, &list);\n \t\t\tmerge_head = lookup_commit_reference(the_repository,\n \t\t\t\t\t\t\t     &merge_heads.oid[0]);\n-\t\t\tif (is_descendant_of(merge_head, list)) {\n+\t\t\tif (repo_is_descendant_of(the_repository,\n+\t\t\t\t\t\t\tmerge_head, list)) {\n \t\t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\t\topt_ff = \"--ff-only\";\n \t\t\t\tran_ff = 1;\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 1761217663..82c73171dd 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -283,9 +283,9 @@ struct commit_list *repo_get_merge_bases(struct repository *r,\n /*\n  * Is \"commit\" a descendant of one of the elements on the \"with_commit\" list?\n  */\n-static int repo_is_descendant_of(struct repository *r,\n-\t\t\t\t struct commit *commit,\n-\t\t\t\t struct commit_list *with_commit)\n+int repo_is_descendant_of(struct repository *r,\n+\t\t\t  struct commit *commit,\n+\t\t\t  struct commit_list *with_commit)\n {\n \tif (!with_commit)\n \t\treturn 1;\n@@ -310,11 +310,6 @@ static int repo_is_descendant_of(struct repository *r,\n \t}\n }\n \n-int is_descendant_of(struct commit *commit, struct commit_list *with_commit)\n-{\n-\treturn repo_is_descendant_of(the_repository, commit, with_commit);\n-}\n-\n /*\n  * Is \"commit\" an ancestor of one of the \"references\"?\n  */\n@@ -433,7 +428,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n \t\treturn 0;\n \n \tcommit_list_insert(old_commit, &old_commit_list);\n-\tret = is_descendant_of(new_commit, old_commit_list);\n+\tret = repo_is_descendant_of(the_repository,\n+\t\t\t\t\tnew_commit, old_commit_list);\n \tfree_commit_list(old_commit_list);\n \treturn ret;\n }\n@@ -554,7 +550,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,\n {\n \tif (filter->with_commit_tag_algo)\n \t\treturn contains_tag_algo(commit, list, cache) == CONTAINS_YES;\n-\treturn is_descendant_of(commit, list);\n+\treturn repo_is_descendant_of(the_repository, commit, list);\n }\n \n static int compare_commits_by_gen(const void *_a, const void *_b)\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 99a43e8b64..b49ad71a31 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -27,7 +27,9 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n \n struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n \n-int is_descendant_of(struct commit *commit, struct commit_list *with_commit);\n+int repo_is_descendant_of(struct repository *r,\n+\t\t\t  struct commit *commit,\n+\t\t\t  struct commit_list *with_commit);\n int repo_in_merge_bases(struct repository *r,\n \t\t\tstruct commit *commit,\n \t\t\tstruct commit *reference);\ndiff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\nindex a0272178b7..1d640f4757 100644\n--- a/t/helper/test-reach.c\n+++ b/t/helper/test-reach.c\n@@ -108,7 +108,7 @@ int cmd__reach(int ac, const char **av)\n \telse if (!strcmp(av[1], \"in_merge_bases\"))\n \t\tprintf(\"%s(A,B):%d\\n\", av[1], in_merge_bases(A, B));\n \telse if (!strcmp(av[1], \"is_descendant_of\"))\n-\t\tprintf(\"%s(A,X):%d\\n\", av[1], is_descendant_of(A, X));\n+\t\tprintf(\"%s(A,X):%d\\n\", av[1], repo_is_descendant_of(r, A, X));\n \telse if (!strcmp(av[1], \"get_merge_bases_many\")) {\n \t\tstruct commit_list *list = get_merge_bases_many(A, X_nr, X_array);\n \t\tprintf(\"%s(A,X):\\n\", av[1]);\n\nbase-commit: 4b34aa94c75220f6f4cd334b08a3fb053128a32d\n-- \n2.27.0.288.g4b34aa94c7\n\n"},{"id":"400366","messageId":"4cf84b43-0837-5737-f937-fad9ca13178b@gmail.com","threadId":"53737","inReplyTo":"20200623081701.52607-1-carenas@gmail.com","subject":"Re: [PATCH] commit-reach: avoid is_dependant_of() shim","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-23T12:59:34Z","receivedAt":"2020-06-23T12:59:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/23/2020 4:17 AM, Carlo Marcelo Arenas Belón wrote:\n> d91d6fbf26 (commit-reach: create repo_is_descendant_of(), 2020-06-17)\n> adds a repository aware version of is_dependant_of() and a backward\n> compatibility shim that is barelly used.> \n> Update all callers to directly use the new repo_is_dependant_of()\n> function instead.\n\nIn your commit message:\n\ns/dependant/descendant/\ns/barelly/barely/\n\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  builtin/pull.c        |  3 ++-\n>  commit-reach.c        | 16 ++++++----------\n>  commit-reach.h        |  4 +++-\n>  t/helper/test-reach.c |  2 +-\n>  4 files changed, 12 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 8e6572d305..babc6a4e36 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1025,7 +1025,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>  \t\t\tcommit_list_insert(head, &list);\n>  \t\t\tmerge_head = lookup_commit_reference(the_repository,\n>  \t\t\t\t\t\t\t     &merge_heads.oid[0]);\n> -\t\t\tif (is_descendant_of(merge_head, list)) {\n> +\t\t\tif (repo_is_descendant_of(the_repository,\n> +\t\t\t\t\t\t\tmerge_head, list)) {\n\nnit: left-align \"merge_head\" with \"the_repository\". Should look like this:\n\n+\t\t\tif (repo_is_descendant_of(the_repository,\n+\t\t\t\t\t\t  merge_head, list)) {\n\n>  \t\t\t\t/* we can fast-forward this without invoking rebase */\n>  \t\t\t\topt_ff = \"--ff-only\";\n>  \t\t\t\tran_ff = 1;\n> diff --git a/commit-reach.c b/commit-reach.c\n> index 1761217663..82c73171dd 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -283,9 +283,9 @@ struct commit_list *repo_get_merge_bases(struct repository *r,\n>  /*\n>   * Is \"commit\" a descendant of one of the elements on the \"with_commit\" list?\n>   */\n> -static int repo_is_descendant_of(struct repository *r,\n> -\t\t\t\t struct commit *commit,\n> -\t\t\t\t struct commit_list *with_commit)\n> +int repo_is_descendant_of(struct repository *r,\n> +\t\t\t  struct commit *commit,\n> +\t\t\t  struct commit_list *with_commit)\n>  {\n>  \tif (!with_commit)\n>  \t\treturn 1;\n> @@ -310,11 +310,6 @@ static int repo_is_descendant_of(struct repository *r,\n>  \t}\n>  }\n>  \n> -int is_descendant_of(struct commit *commit, struct commit_list *with_commit)\n> -{\n> -\treturn repo_is_descendant_of(the_repository, commit, with_commit);\n> -}\n> -\n>  /*\n>   * Is \"commit\" an ancestor of one of the \"references\"?\n>   */\n> @@ -433,7 +428,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n>  \t\treturn 0;\n>  \n>  \tcommit_list_insert(old_commit, &old_commit_list);\n> -\tret = is_descendant_of(new_commit, old_commit_list);\n> +\tret = repo_is_descendant_of(the_repository,\n> +\t\t\t\t\tnew_commit, old_commit_list);\n\nnit: align the whitespace so \"new_commit\" is left-aligned with the start\nof \"the_repository\" in the line above. It should look like this:\n\n+\tret = repo_is_descendant_of(the_repository,\n+\t\t\t\t    new_commit, old_commit_list);\n\n>  \tfree_commit_list(old_commit_list);\n>  \treturn ret;\n>  }\n> @@ -554,7 +550,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,\n>  {\n>  \tif (filter->with_commit_tag_algo)\n>  \t\treturn contains_tag_algo(commit, list, cache) == CONTAINS_YES;\n> -\treturn is_descendant_of(commit, list);\n> +\treturn repo_is_descendant_of(the_repository, commit, list);\n>  }\n>  \n>  static int compare_commits_by_gen(const void *_a, const void *_b)\n> diff --git a/commit-reach.h b/commit-reach.h\n> index 99a43e8b64..b49ad71a31 100644\n> --- a/commit-reach.h\n> +++ b/commit-reach.h\n> @@ -27,7 +27,9 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n>  \n>  struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n>  \n> -int is_descendant_of(struct commit *commit, struct commit_list *with_commit);\n> +int repo_is_descendant_of(struct repository *r,\n> +\t\t\t  struct commit *commit,\n> +\t\t\t  struct commit_list *with_commit);\n>  int repo_in_merge_bases(struct repository *r,\n>  \t\t\tstruct commit *commit,\n>  \t\t\tstruct commit *reference);\n> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\n> index a0272178b7..1d640f4757 100644\n> --- a/t/helper/test-reach.c\n> +++ b/t/helper/test-reach.c\n> @@ -108,7 +108,7 @@ int cmd__reach(int ac, const char **av)\n>  \telse if (!strcmp(av[1], \"in_merge_bases\"))\n>  \t\tprintf(\"%s(A,B):%d\\n\", av[1], in_merge_bases(A, B));\n>  \telse if (!strcmp(av[1], \"is_descendant_of\"))\n> -\t\tprintf(\"%s(A,X):%d\\n\", av[1], is_descendant_of(A, X));\n> +\t\tprintf(\"%s(A,X):%d\\n\", av[1], repo_is_descendant_of(r, A, X));\n\nThis is the most subtle of the changes, but it is done correctly.\n\nThis is a worthwhile patch. Some of the callers _look_ more complicated,\nbut overall this simplifies the codebase. Further: it pushes more\nthe_repository references higher up the stack.\n\nThanks,\n-Stolee\n\n"},{"id":"400420","messageId":"20200623184222.54201-1-carenas@gmail.com","threadId":"53737","inReplyTo":"20200623081701.52607-1-carenas@gmail.com","subject":"[PATCH v2] commit-reach: avoid is_descendant_of() shim","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-06-23T18:42:22Z","receivedAt":"2020-06-23T18:42:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d91d6fbf26 (commit-reach: create repo_is_descendant_of(), 2020-06-17)\nadds a repository aware version of is_descendant_of() and a backward\ncompatibility shim that is barely used.\n\nUpdate all callers to directly use the new repo_is_descendant_of()\nfunction instead; making the codebase simpler and pushing more\nthe_repository references higher up the stack.\n\nHelped-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n builtin/pull.c        |  3 ++-\n commit-reach.c        | 16 ++++++----------\n commit-reach.h        |  4 +++-\n t/helper/test-reach.c |  2 +-\n 4 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 8e6572d305..8159c5d7c9 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1025,7 +1025,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tcommit_list_insert(head, &list);\n \t\t\tmerge_head = lookup_commit_reference(the_repository,\n \t\t\t\t\t\t\t     &merge_heads.oid[0]);\n-\t\t\tif (is_descendant_of(merge_head, list)) {\n+\t\t\tif (repo_is_descendant_of(the_repository,\n+\t\t\t\t\t\t  merge_head, list)) {\n \t\t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\t\topt_ff = \"--ff-only\";\n \t\t\t\tran_ff = 1;\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 1761217663..f846d30f22 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -283,9 +283,9 @@ struct commit_list *repo_get_merge_bases(struct repository *r,\n /*\n  * Is \"commit\" a descendant of one of the elements on the \"with_commit\" list?\n  */\n-static int repo_is_descendant_of(struct repository *r,\n-\t\t\t\t struct commit *commit,\n-\t\t\t\t struct commit_list *with_commit)\n+int repo_is_descendant_of(struct repository *r,\n+\t\t\t  struct commit *commit,\n+\t\t\t  struct commit_list *with_commit)\n {\n \tif (!with_commit)\n \t\treturn 1;\n@@ -310,11 +310,6 @@ static int repo_is_descendant_of(struct repository *r,\n \t}\n }\n \n-int is_descendant_of(struct commit *commit, struct commit_list *with_commit)\n-{\n-\treturn repo_is_descendant_of(the_repository, commit, with_commit);\n-}\n-\n /*\n  * Is \"commit\" an ancestor of one of the \"references\"?\n  */\n@@ -433,7 +428,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)\n \t\treturn 0;\n \n \tcommit_list_insert(old_commit, &old_commit_list);\n-\tret = is_descendant_of(new_commit, old_commit_list);\n+\tret = repo_is_descendant_of(the_repository,\n+\t\t\t\t    new_commit, old_commit_list);\n \tfree_commit_list(old_commit_list);\n \treturn ret;\n }\n@@ -554,7 +550,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,\n {\n \tif (filter->with_commit_tag_algo)\n \t\treturn contains_tag_algo(commit, list, cache) == CONTAINS_YES;\n-\treturn is_descendant_of(commit, list);\n+\treturn repo_is_descendant_of(the_repository, commit, list);\n }\n \n static int compare_commits_by_gen(const void *_a, const void *_b)\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 99a43e8b64..b49ad71a31 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -27,7 +27,9 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n \n struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n \n-int is_descendant_of(struct commit *commit, struct commit_list *with_commit);\n+int repo_is_descendant_of(struct repository *r,\n+\t\t\t  struct commit *commit,\n+\t\t\t  struct commit_list *with_commit);\n int repo_in_merge_bases(struct repository *r,\n \t\t\tstruct commit *commit,\n \t\t\tstruct commit *reference);\ndiff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\nindex a0272178b7..1d640f4757 100644\n--- a/t/helper/test-reach.c\n+++ b/t/helper/test-reach.c\n@@ -108,7 +108,7 @@ int cmd__reach(int ac, const char **av)\n \telse if (!strcmp(av[1], \"in_merge_bases\"))\n \t\tprintf(\"%s(A,B):%d\\n\", av[1], in_merge_bases(A, B));\n \telse if (!strcmp(av[1], \"is_descendant_of\"))\n-\t\tprintf(\"%s(A,X):%d\\n\", av[1], is_descendant_of(A, X));\n+\t\tprintf(\"%s(A,X):%d\\n\", av[1], repo_is_descendant_of(r, A, X));\n \telse if (!strcmp(av[1], \"get_merge_bases_many\")) {\n \t\tstruct commit_list *list = get_merge_bases_many(A, X_nr, X_array);\n \t\tprintf(\"%s(A,X):\\n\", av[1]);\n\nbase-commit: 4b34aa94c75220f6f4cd334b08a3fb053128a32d\n-- \n2.27.0.288.g4b34aa94c7\n\n"},{"id":"400421","messageId":"dbd85d9a-6fc9-1187-0268-d28e532ec507@gmail.com","threadId":"53737","inReplyTo":"20200623184222.54201-1-carenas@gmail.com","subject":"Re: [PATCH v2] commit-reach: avoid is_descendant_of() shim","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-23T18:53:57Z","receivedAt":"2020-06-23T18:54:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/23/2020 2:42 PM, Carlo Marcelo Arenas Belón wrote:\n> d91d6fbf26 (commit-reach: create repo_is_descendant_of(), 2020-06-17)\n> adds a repository aware version of is_descendant_of() and a backward\n> compatibility shim that is barely used.\n> \n> Update all callers to directly use the new repo_is_descendant_of()\n> function instead; making the codebase simpler and pushing more\n> the_repository references higher up the stack.\n\nThanks for the update! This version looks good to me.\n\n-Stolee\n\n"}]}