{"thread":{"id":"49583","subject":"[PATCH 00/19] Bring more repository handles into our code base","startedAt":"2018-10-16T23:35:59Z","lastAt":"2018-11-01T19:13:25Z","messageCount":46,"participants":["Stefan Beller","Derrick Stolee","Jonathan Tan","Junio C Hamano","SZEDER Gábor","Carlo Arenas","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":19},"messages":[{"id":"360699","messageId":"20181016233550.251311-1-sbeller@google.com","threadId":"49583","inReplyTo":null,"subject":"[PATCH 00/19] Bring more repository handles into our code base","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:31Z","receivedAt":"2018-10-16T23:35:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This rerolls sb/more-repo-in-api.\nIt applies on nd/the-index merged with ds/reachable and is available via\n git fetch https://github.com/stefanbeller/git object-store-final-3\n\nI addressed all of Jonathans comments, see range-diff below;\nthe last patch (applying the semantic patches) has been omitted as that\nwould produce a lot of merge conflicts. Without that patch, this merges\ncleanly to next.\n\nAs for when to apply the semantic patches, I wondered if we would prefer a dirty merge\n(created via \"make coccicheck && git apply contrib/coccinelle/the_repository.cocci.patch\")\nof the semantic patches, or if we'd actually trickle in the changes over some time?\n\nThanks,\nStefan\n\nAn earlier RFC was sent out at\nhttps://public-inbox.org/git/20181011211754.31369-1-sbeller@google.com/ and said:\n This applies on nd/the-index (b3c7eef9b05) and is the logical continuation of\n the object store series, which I sent over the last year.\n \n The previous series did take a very slow and pedantic approach,\n using a #define trick, see cfc62fc98c for details, but it turns out,\n that it doesn't work:\n When changing the signature of widely used functions, it burdens the\n maintainer in resolving the semantic conflicts.\n \n In the orginal approach this was called a feature, as then we can ensure\n that not bugs creep into the code base during the merge window (while such\n a refactoring series wanders from pu to master). It turns out this\n was not well received and was just burdensome.\n \n The #define trick doesn't buy us much to begin with when dealing with\n non-merge-conflicts. For example, see deref_tag at tag.c:68, which got\n the repository argument in 286d258d4f (tag.c: allow deref_tag to handle\n arbitrary repositories, 2018-06-28) but lost its property of working on any\n repository while 8c4cc32689 (tag: don't warn if target is missing but\n promised, 2018-07-12) was in flight simultaneously.\n \n Another example of failure of this approach is seen in patch 5, which\n shows that the pedantry was missed.\n \n This series takes another approach as it doesn't change the signature of\n functions, but introduces new functions that can deal with arbitrary \n repositories, keeping the old function signature around using a shallow wrapper.\n \n Additionally each patch adds a semantic patch, that would port from the old to\n the new function. These semantic patches are all applied in the very last patch,\n but we could omit applying the last patch if it causes too many merge conflicts\n and trickl in the semantic patches over time when there are no merge conflicts.\n \n \n The original goal of all these refactoring series was to remove add_submodule_odb \n in submodule.c, which was partially reached with this series. I'll investigate the\n remaining calls in another series, but it shows we're close to be done with these\n large refactorings as far as I am concerned.\n \n Thanks,\n Stefan\n \n\nStefan Beller (19):\n sha1_file: allow read_object to read objects in arbitrary repositories\n packfile: allow has_packed_and_bad to handle arbitrary repositories\n object-store: allow read_object_file_extended to read from arbitrary\n repositories\n object-store: prepare read_object_file to deal with arbitrary\n repositories\n object-store: prepare has_{sha1, object}_file[_with_flags] to handle\n arbitrary repositories\n object: parse_object to honor its repository argument\n commit: allow parse_commit* to handle arbitrary repositories\n commit-reach.c: allow paint_down_to_common to handle arbitrary\n repositories\n commit-reach.c: allow merge_bases_many to handle arbitrary\n repositories\n commit-reach.c: allow remove_redundant to handle arbitrary\n repositories\n commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary\n repositories\n commit-reach: prepare get_merge_bases to handle arbitrary repositories\n commit-reach: prepare in_merge_bases[_many] to handle arbitrary\n repositories\n commit: prepare get_commit_buffer to handle arbitrary repositories\n commit: prepare repo_unuse_commit_buffer to handle arbitrary\n repositories\n commit: prepare logmsg_reencode to handle arbitrary repositories\n pretty: prepare format_commit_message to handle arbitrary repositories\n submodule: use submodule repos for object lookup\n submodule: don't add submodule as odb for push\n\n commit-reach.c | 73 +++++++-----\n commit-reach.h | 38 +++++--\n commit.c | 32 ++++--\n commit.h | 39 ++++++-\n contrib/coccinelle/the_repository.cocci | 144 ++++++++++++++++++++++++\n object-store.h | 35 ++++--\n object.c | 6 +-\n packfile.c | 5 +-\n packfile.h | 2 +-\n pretty.c | 28 ++---\n pretty.h | 7 +-\n sha1-file.c | 34 +++---\n streaming.c | 2 +-\n submodule.c | 78 ++++++++++---\n 14 files changed, 402 insertions(+), 121 deletions(-)\n create mode 100644 contrib/coccinelle/the_repository.cocci\n\n git range-diff origin/sb/more-repo-in-api...\n\n -: ---------- > 1: 5227c38566 commit-reach: move walk methods from commit.c\n -: ---------- > 2: 6404355657 commit.h: remove method declarations\n -: ---------- > 3: 1d614d41e5 commit-reach: move ref_newer from remote.c\n -: ---------- > 4: 920f93ca1c commit-reach: move commit_contains from ref-filter\n -: ---------- > 5: f044bb49ad upload-pack: make reachable() more generic\n -: ---------- > 6: 921bf7734f upload-pack: refactor ok_to_give_up()\n -: ---------- > 7: 118be5785e upload-pack: generalize commit date cutoff\n -: ---------- > 8: ba3ca1edce commit-reach: move can_all_from_reach_with_flags\n -: ---------- > 9: ab176ac4ae test-reach: create new test tool for ref_newer\n -: ---------- > 10: 5cd52de326 test-reach: test in_merge_bases\n -: ---------- > 11: 6255232ec1 test-reach: test is_descendant_of\n -: ---------- > 12: 324dec0191 test-reach: test get_merge_bases_many\n -: ---------- > 13: 0c89f715d0 test-reach: test reduce_heads\n -: ---------- > 14: 1792bc1250 test-reach: test can_all_from_reach_with_flags\n -: ---------- > 15: 1fee124257 test-reach: test commit_contains\n -: ---------- > 16: 1e3497a24c commit-reach: replace ref_newer logic\n -: ---------- > 17: 4fbcca4eff commit-reach: make can_all_from_reach... linear\n -: ---------- > 18: 6cc017431c commit-reach: use can_all_from_reach\n -: ---------- > 19: 6621c83874 commit-reach: correct accidental #include of C file\n -: ---------- > 20: b67f6b26e3 commit-reach: properly peel tags\n -: ---------- > 21: 4067a64672 commit-reach: fix memory and flag leaks\n 1: 2c13c6101f = 22: 14c12a5350 sha1_file: allow read_object to read objects in arbitrary repositories\n 2: 82848c2571 = 23: f978950604 packfile: allow has_packed_and_bad to handle arbitrary repositories\n 3: 717023d801 = 24: 852f6696d1 object-store: allow read_object_file_extended to read from arbitrary repositories\n 4: fab6bdfad0 ! 25: b185b88ad8 object-store: prepare read_object_file to deal with arbitrary repositories\n @@ -9,7 +9,7 @@\n \n Introduce repo_read_object_file which takes the repository argument, and\n hide the original read_object_file as a macro behind\n - NO_THE_REPOSITORY_COMPATIBILITY_MACROS, which we planned for in\n + NO_THE_REPOSITORY_COMPATIBILITY_MACROS, similar to\n e675765235 (diff.c: remove implicit dependency on the_index, 2018-09-21)\n \n Add a coccinelle patch to convert existing callers, but do not apply\n -: ---------- > 26: 009d6f350b object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositories\n 5: 9e34d76c53 ! 27: df2feb2baf object: parse_object to honor its repository argument\n @@ -12,6 +12,17 @@\n diff --git a/object.c b/object.c\n --- a/object.c\n +++ b/object.c\n +@@\n + \tif (obj && obj->parsed)\n + \t\treturn obj;\n + \n +-\tif ((obj && obj->type == OBJ_BLOB && has_object_file(oid)) ||\n +-\t (!obj && has_object_file(oid) &&\n ++\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n ++\t (!obj && repo_has_object_file(r, oid) &&\n + \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n + \t\tif (check_object_signature(repl, NULL, 0, NULL) < 0) {\n + \t\t\terror(_(\"sha1 mismatch %s\"), oid_to_hex(oid));\n @@\n \t\treturn lookup_object(r, oid->hash);\n \t}\n 6: 9cb7efc7b2 ! 28: 18b7073c25 commit: allow parse_commit* to handle arbitrary repositories\n @@ -97,9 +97,10 @@\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n - + repo_read_object_file(the_repository,\n - E, F, G)\n - \n + - has_object_file_with_flags(\n + + repo_has_object_file_with_flags(the_repository,\n + E)\n ++\n +@@\n +expression E;\n +expression F;\n @@ -123,4 +124,3 @@\n +- parse_commit(\n ++ repo_parse_commit(the_repository,\n + E)\n -+\n 7: fcab4a4000 ! 29: 48f8999e95 commit.c: allow paint_down_to_common to handle arbitrary repositories\n @@ -1,15 +1,14 @@\n Author: Stefan Beller \n \n - commit.c: allow paint_down_to_common to handle arbitrary repositories\n + commit-reach.c: allow paint_down_to_common to handle arbitrary repositories\n \n As the function is file local and not widely used, migrate it all at once.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n }\n \n 8: 324422f0d3 ! 30: b84d32c972 commit.c: allow merge_bases_many to handle arbitrary repositories\n @@ -1,13 +1,12 @@\n Author: Stefan Beller \n \n - commit.c: allow merge_bases_many to handle arbitrary repositories\n + commit-reach.c: allow merge_bases_many to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n \treturn result;\n }\n 9: 9c43e74d6c ! 31: cd0e595b8d commit.c: allow remove_redundant to handle arbitrary repositories\n @@ -1,13 +1,12 @@\n Author: Stefan Beller \n \n - commit.c: allow remove_redundant to handle arbitrary repositories\n + commit-reach.c: allow remove_redundant to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n \treturn ret;\n }\n10: e5c2e2fa8e ! 32: 40ebd46acd commit: allow get_merge_bases_many_0 to handle arbitrary repositories\n @@ -1,13 +1,12 @@\n Author: Stefan Beller \n \n - commit: allow get_merge_bases_many_0 to handle arbitrary repositories\n + commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n \treturn filled;\n }\n11: 27d4669ccb ! 33: 151cc8cd9b commit: prepare get_merge_bases to handle arbitrary repositories\n @@ -1,6 +1,6 @@\n Author: Stefan Beller \n \n - commit: prepare get_merge_bases to handle arbitrary repositories\n + commit-reach: prepare get_merge_bases to handle arbitrary repositories\n \n Similarly to previous patches, the get_merge_base functions are used\n often in the code base, which makes migrating them hard.\n @@ -9,11 +9,10 @@\n functions behind a wrapper macro.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n \treturn result;\n }\n @@ -53,16 +52,21 @@\n \n /*\n \n -diff --git a/commit.h b/commit.h\n ---- a/commit.h\n -+++ b/commit.h\n +diff --git a/commit-reach.h b/commit-reach.h\n +--- a/commit-reach.h\n ++++ b/commit-reach.h\n @@\n - int register_commit_graft(struct repository *r, struct commit_graft *, int);\n - struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n + struct contains_cache;\n + struct ref_filter;\n \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 +-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 +-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 +-struct commit_list *get_merge_bases(struct commit *one, struct commit *two);\n +-struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n -\n +struct commit_list *repo_get_merge_bases(struct repository *r,\n +\t\t\t\t\t struct commit *rev1,\n @@ -71,7 +75,7 @@\n +\t\t\t\t\t struct commit *one, int n,\n +\t\t\t\t\t struct commit **twos);\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 +-struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, struct commit **twos);\n +struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n +\t\t\t\t\t\t struct commit *one, int n,\n +\t\t\t\t\t\t struct commit **twos);\n @@ -80,18 +84,20 @@\n +#define get_merge_bases_many(one, n, two) repo_get_merge_bases_many(the_repository, one, n, two)\n +#define get_merge_bases_many_dirty(one, n, twos) repo_get_merge_bases_many_dirty(the_repository, one, n, twos)\n +#endif\n -+extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n ++\n ++struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n \n - /* largest positive number a signed 32-bit integer can contain */\n - #define INFINITE_DEPTH 0x7fffffff\n + int is_descendant_of(struct commit *commit, struct commit_list *with_commit);\n + int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);\n \n diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n + - parse_commit(\n + repo_parse_commit(the_repository,\n E)\n - \n ++\n +@@\n +expression E;\n +expression F;\n13: 977359ec86 ! 34: b508acf4fd commit: prepare in_merge_bases[_many] to handle arbitrary repositories\n @@ -1,13 +1,12 @@\n Author: Stefan Beller \n \n - commit: prepare in_merge_bases[_many] to handle arbitrary repositories\n + commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n -diff --git a/commit.c b/commit.c\n ---- a/commit.c\n -+++ b/commit.c\n +diff --git a/commit-reach.c b/commit-reach.c\n +--- a/commit-reach.c\n ++++ b/commit-reach.c\n @@\n /*\n * Is \"commit\" an ancestor of one of the \"references\"?\n @@ -53,33 +52,36 @@\n \n struct commit_list *reduce_heads(struct commit_list *heads)\n \n -diff --git a/commit.h b/commit.h\n ---- a/commit.h\n -+++ b/commit.h\n +diff --git a/commit-reach.h b/commit-reach.h\n +--- a/commit-reach.h\n ++++ b/commit-reach.h\n @@\n - extern struct trace_key trace_shallow;\n + struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n \n - int is_descendant_of(struct commit *, struct commit_list *);\n --int in_merge_bases(struct commit *, struct commit *);\n --int in_merge_bases_many(struct commit *, int, struct commit **);\n -+int repo_in_merge_bases(struct repository *r, struct commit *, struct commit *);\n -+int repo_in_merge_bases_many(struct repository *r, struct commit *, int, struct commit **);\n + int is_descendant_of(struct commit *commit, struct commit_list *with_commit);\n +-int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);\n +-int in_merge_bases(struct commit *commit, struct commit *reference);\n ++int repo_in_merge_bases(struct repository *r,\n ++\t\t\tstruct commit *commit,\n ++\t\t\tstruct commit *reference);\n ++int repo_in_merge_bases_many(struct repository *r,\n ++\t\t\t struct commit *commit,\n ++\t\t\t int nr_reference, struct commit **reference);\n +#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n +#define in_merge_bases(c1, c2) repo_in_merge_bases(the_repository, c1, c2)\n +#define in_merge_bases_many(c1, n, cs) repo_in_merge_bases_many(the_repository, c1, n, cs)\n +#endif\n \n - extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n - extern int run_add_interactive(const char *revision, const char *patch_mode,\n + /*\n + * Takes a list of commits and returns a new list where those\n \n diff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n - - get_commit_buffer(\n - + repo_get_commit_buffer(the_repository,\n - E, F);\n -+\n + + repo_get_merge_bases_many_dirty(the_repository,\n + E, F, G);\n + \n +@@\n +expression E;\n +expression F;\n12: 044bf2917c ! 35: c825f2703c commit: prepare get_commit_buffer to handle arbitrary repositories\n @@ -3,7 +3,6 @@\n commit: prepare get_commit_buffer to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/commit.c b/commit.c\n --- a/commit.c\n @@ -50,9 +49,10 @@\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n - + repo_get_merge_bases_many_dirty(the_repository,\n + - in_merge_bases_many(\n + + repo_in_merge_bases_many(the_repository,\n E, F, G);\n - \n ++\n +@@\n +expression E;\n +expression F;\n14: 65771970b9 ! 36: bea6ff2e6d commit: prepare repo_unuse_commit_buffer to handle arbitrary repositories\n @@ -3,7 +3,6 @@\n commit: prepare repo_unuse_commit_buffer to handle arbitrary repositories\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/commit.c b/commit.c\n --- a/commit.c\n @@ -46,9 +45,10 @@\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n + - get_commit_buffer(\n + repo_get_commit_buffer(the_repository,\n E, F);\n - \n ++\n +@@\n +expression E;\n +expression F;\n @@ -56,7 +56,3 @@\n +- unuse_commit_buffer(\n ++ repo_unuse_commit_buffer(the_repository,\n + E, F);\n -+\n - @@\n - expression E;\n - expression F;\n15: ddef24685d ! 37: 11ea084cd2 commit: prepare logmsg_reencode to handle arbitrary repositories\n @@ -28,9 +28,9 @@\n --- a/contrib/coccinelle/the_repository.cocci\n +++ b/contrib/coccinelle/the_repository.cocci\n @@\n - - in_merge_bases_many(\n - + repo_in_merge_bases_many(the_repository,\n - E, F, G);\n + - unuse_commit_buffer(\n + + repo_unuse_commit_buffer(the_repository,\n + E, F);\n +\n +@@\n +expression E;\n16: 8612153e23 = 38: 9f50edd155 pretty: prepare format_commit_message to handle arbitrary repositories\n17: eba296fc18 ! 39: 1a4a392233 submodule: use submodule repos for object lookup\n @@ -7,7 +7,6 @@\n are not added to the main object store.\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n @@ -35,12 +34,15 @@\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n +-/* Helper function to display the submodule header line prior to the full\n +- * summary output. If it can locate the submodule objects directory it will\n +- * attempt to lookup both the left and right commits and put them into the\n +- * left and right pointers.\n +/*\n + * Initialize 'out' based on the provided submodule path.\n + *\n + * Unlike repo_submodule_init, this tolerates submodules not present\n -+ * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is\n -+ * preferrable. This function exists only to preserve historical behavior.\n ++ * in .gitmodules. This function exists only to preserve historical behavior,\n + *\n + * Returns 0 on success, -1 when the submodule is not present.\n + */\n @@ -54,28 +56,38 @@\n +\t}\n +\n +\tout->submodule_prefix = xstrdup(path);\n ++\tout->submodule_prefix = xstrfmt(\"%s%s/\",\n ++\t\t\t\t\tthe_repository->submodule_prefix ?\n ++\t\t\t\t\tthe_repository->submodule_prefix :\n ++\t\t\t\t\t\"\", path);\n +\n +\tstrbuf_release(&sb);\n +\treturn 0;\n +}\n +\n - /* Helper function to display the submodule header line prior to the full\n - * summary output. If it can locate the submodule objects directory it will\n - * attempt to lookup both the left and right commits and put them into the\n - * left and right pointers.\n ++/*\n ++ * Helper function to display the submodule header line prior to the full\n ++ * summary output.\n ++ *\n ++ * If it can locate the submodule git directory it will create a repository\n ++ * handle for the submodule and lookup both the left and right commits and\n ++ * put them into the left and right pointers.\n */\n -static void show_submodule_header(struct diff_options *o, const char *path,\n -+static void show_submodule_header(struct diff_options *o, struct repository *sub,\n ++static void show_submodule_header(struct diff_options *o,\n +\t\tconst char *path,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule,\n ++\t\tstruct repository *sub,\n \t\tstruct commit **left, struct commit **right,\n + \t\tstruct commit_list **merge_bases)\n + {\n @@\n \telse if (is_null_oid(two))\n \t\tmessage = \"(submodule deleted)\";\n \n -\tif (add_submodule_odb(path)) {\n -+\tif (open_submodule(sub, path) < 0) {\n ++\tif (!sub) {\n \t\tif (!message)\n \t\t\tmessage = \"(commits not present)\";\n \t\tgoto output_header;\n @@ -103,30 +115,63 @@\n \tstruct rev_info rev;\n \tstruct commit *left = NULL, *right = NULL;\n \tstruct commit_list *merge_bases = NULL;\n -+\tstruct repository sub;\n ++\tstruct repository subrepo, *sub = &subrepo;\n ++\n ++\tif (open_submodule(&subrepo, path) < 0)\n ++\t\tsub = NULL;\n \n --\tshow_submodule_header(o, path, one, two, dirty_submodule,\n -+\tshow_submodule_header(o, &sub, path, one, two, dirty_submodule,\n - \t\t\t &left, &right, &merge_bases);\n + \tshow_submodule_header(o, path, one, two, dirty_submodule,\n +-\t\t\t &left, &right, &merge_bases);\n ++\t\t\t sub, &left, &right, &merge_bases);\n \n \t/*\n + \t * If we don't have both a left and a right pointer, there is no\n + \t * reason to try and display a summary. The header line should contain\n + \t * all the information the user needs.\n + \t */\n +-\tif (!left || !right)\n ++\tif (!left || !right || !sub)\n + \t\tgoto out;\n + \n + \t/* Treat revision walker failure the same as missing commits */\n @@\n \t\tgoto out;\n \t}\n \n -\tprint_submodule_summary(&rev, o);\n -+\tprint_submodule_summary(&sub, &rev, o);\n ++\tprint_submodule_summary(sub, &rev, o);\n \n out:\n \tif (merge_bases)\n + \t\tfree_commit_list(merge_bases);\n + \tclear_commit_marks(left, ~0);\n + \tclear_commit_marks(right, ~0);\n ++\tif (sub)\n ++\t\trepo_clear(sub);\n + }\n + \n + void show_submodule_inline_diff(struct diff_options *o, const char *path,\n @@\n \tstruct commit_list *merge_bases = NULL;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n -+\tstruct repository sub;\n ++\tstruct repository subrepo, *sub = &subrepo;\n ++\n ++\tif (open_submodule(&subrepo, path) < 0)\n ++\t\tsub = NULL;\n \n --\tshow_submodule_header(o, path, one, two, dirty_submodule,\n -+\tshow_submodule_header(o, &sub, path, one, two, dirty_submodule,\n - \t\t\t &left, &right, &merge_bases);\n + \tshow_submodule_header(o, path, one, two, dirty_submodule,\n +-\t\t\t &left, &right, &merge_bases);\n ++\t\t\t sub, &left, &right, &merge_bases);\n \n \t/* We need a valid left and right commit to display a difference */\n + \tif (!(left || is_null_oid(one)) ||\n +@@\n + \t\tclear_commit_marks(left, ~0);\n + \tif (right)\n + \t\tclear_commit_marks(right, ~0);\n ++\tif (sub)\n ++\t\trepo_clear(sub);\n + }\n + \n + int should_update_submodules(void)\n18: 4b2033017a ! 40: 1ce2d0a867 submodule: don't add submodule as odb for push\n @@ -2,20 +2,13 @@\n \n submodule: don't add submodule as odb for push\n \n - The submodule was added as an alternative in eb21c732d6 (push: teach\n - --recurse-submodules the on-demand option, 2012-03-29), but was\n - not explained, why.\n -\n - In similar code, submodule_has_commits, the submodule is added as an\n - alternative to perform a quick check if we need to dive into the submodule.\n -\n - However in push_submodule\n - (a) for_each_remote_ref_submodule will also provide the quick check and\n - (b) after that we don't need to have submodule objects around, as all\n - further code is to spawn a separate process.\n + In push_submodule(), because we do not actually need access to objects\n + in the submodule, do not invoke add_submodule_odb().\n + (for_each_remote_ref_submodule() does not require access to those\n + objects, and the actual push is done by spawning another process,\n + which handles object access by itself.)\n \n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/submodule.c b/submodule.c\n --- a/submodule.c\n19: c74af1496b < -: ---------- Apply semantic patches from previous patches\n"},{"id":"360700","messageId":"20181016233550.251311-2-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 01/19] sha1_file: allow read_object to read objects in arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:32Z","receivedAt":"2018-10-16T23:36:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Allow read_object (a file local functon in sha1_file) to\nhandle arbitrary repositories by passing the repository down\nto oid_object_info_extended.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n sha1-file.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 308d5e20e2..647068a836 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1361,7 +1361,9 @@ int oid_object_info(struct repository *r,\n \treturn type;\n }\n \n-static void *read_object(const unsigned char *sha1, enum object_type *type,\n+static void *read_object(struct repository *r,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t enum object_type *type,\n \t\t\t unsigned long *size)\n {\n \tstruct object_id oid;\n@@ -1373,7 +1375,7 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,\n \n \thashcpy(oid.hash, sha1);\n \n-\tif (oid_object_info_extended(the_repository, &oid, &oi, 0) < 0)\n+\tif (oid_object_info_extended(r, &oid, &oi, 0) < 0)\n \t\treturn NULL;\n \treturn content;\n }\n@@ -1414,7 +1416,7 @@ void *read_object_file_extended(const struct object_id *oid,\n \t\tlookup_replace_object(the_repository, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(repl->hash, type, size);\n+\tdata = read_object(the_repository, repl->hash, type, size);\n \tif (data)\n \t\treturn data;\n \n@@ -1755,7 +1757,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(oid->hash, &type, &len);\n+\tbuf = read_object(the_repository, oid->hash, &type, &len);\n \tif (!buf)\n \t\treturn error(_(\"cannot read sha1_file for %s\"), oid_to_hex(oid));\n \thdrlen = xsnprintf(hdr, sizeof(hdr), \"%s %lu\", type_name(type), len) + 1;\n-- \n2.19.0\n\n"},{"id":"360701","messageId":"20181016233550.251311-3-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 02/19] packfile: allow has_packed_and_bad to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:33Z","receivedAt":"2018-10-16T23:36:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"has_packed_and_bad is not widely used, so just migrate it all at once.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n packfile.c | 5 +++--\n packfile.h | 2 +-\n sha1-file.c | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex ebcb5742ec..40085fe160 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1024,12 +1024,13 @@ void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1)\n \tp->num_bad_objects++;\n }\n \n-const struct packed_git *has_packed_and_bad(const unsigned char *sha1)\n+const struct packed_git *has_packed_and_bad(struct repository *r,\n+\t\t\t\t\t const unsigned char *sha1)\n {\n \tstruct packed_git *p;\n \tunsigned i;\n \n-\tfor (p = the_repository->objects->packed_git; p; p = p->next)\n+\tfor (p = r->objects->packed_git; p; p = p->next)\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n \t\t\tif (!hashcmp(sha1,\n \t\t\t\t p->bad_object_sha1 + the_hash_algo->rawsz * i))\ndiff --git a/packfile.h b/packfile.h\nindex 630f35cf31..1d2d170bf8 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -136,7 +136,7 @@ extern int packed_object_info(struct repository *r,\n \t\t\t off_t offset, struct object_info *);\n \n extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n-extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);\n+extern const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);\n \n /*\n * Iff a pack file in the given repository contains the object named by sha1,\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 647068a836..13b3c5cb79 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1432,7 +1432,7 @@ void *read_object_file_extended(const struct object_id *oid,\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), path);\n \n-\tif ((p = has_packed_and_bad(repl->hash)) != NULL)\n+\tif ((p = has_packed_and_bad(the_repository, repl->hash)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), p->pack_name);\n \n-- \n2.19.0\n\n"},{"id":"360702","messageId":"20181016233550.251311-4-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 03/19] object-store: allow read_object_file_extended to read from arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:34Z","receivedAt":"2018-10-16T23:36:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"read_object_file_extended is not widely used, so migrate it all at once.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n object-store.h | 5 +++--\n sha1-file.c | 11 ++++++-----\n streaming.c | 2 +-\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 67e66227d9..6bb0ccbf05 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -146,12 +146,13 @@ void sha1_file_name(struct repository *r, struct strbuf *buf, const unsigned cha\n \n void *map_sha1_file(struct repository *r, const unsigned char *sha1, unsigned long *size);\n \n-extern void *read_object_file_extended(const struct object_id *oid,\n+extern void *read_object_file_extended(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n \t\t\t\t enum object_type *type,\n \t\t\t\t unsigned long *size, int lookup_replace);\n static inline void *read_object_file(const struct object_id *oid, enum object_type *type, unsigned long *size)\n {\n-\treturn read_object_file_extended(oid, type, size, 1);\n+\treturn read_object_file_extended(the_repository, oid, type, size, 1);\n }\n \n /* Read and unpack an object file into memory, write memory to an object file */\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 13b3c5cb79..ce47524679 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1403,7 +1403,8 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n * deal with them should arrange to call read_object() and give error\n * messages themselves.\n */\n-void *read_object_file_extended(const struct object_id *oid,\n+void *read_object_file_extended(struct repository *r,\n+\t\t\t\tconst struct object_id *oid,\n \t\t\t\tenum object_type *type,\n \t\t\t\tunsigned long *size,\n \t\t\t\tint lookup_replace)\n@@ -1413,10 +1414,10 @@ void *read_object_file_extended(const struct object_id *oid,\n \tconst char *path;\n \tstruct stat st;\n \tconst struct object_id *repl = lookup_replace ?\n-\t\tlookup_replace_object(the_repository, oid) : oid;\n+\t\tlookup_replace_object(r, oid) : oid;\n \n \terrno = 0;\n-\tdata = read_object(the_repository, repl->hash, type, size);\n+\tdata = read_object(r, repl->hash, type, size);\n \tif (data)\n \t\treturn data;\n \n@@ -1428,11 +1429,11 @@ void *read_object_file_extended(const struct object_id *oid,\n \t\tdie(_(\"replacement %s not found for %s\"),\n \t\t oid_to_hex(repl), oid_to_hex(oid));\n \n-\tif (!stat_sha1_file(the_repository, repl->hash, &st, &path))\n+\tif (!stat_sha1_file(r, repl->hash, &st, &path))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), path);\n \n-\tif ((p = has_packed_and_bad(the_repository, repl->hash)) != NULL)\n+\tif ((p = has_packed_and_bad(r, repl->hash)) != NULL)\n \t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(repl), p->pack_name);\n \ndiff --git a/streaming.c b/streaming.c\nindex d1e6b2dce6..c843a1230f 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -490,7 +490,7 @@ static struct stream_vtbl incore_vtbl = {\n \n static open_method_decl(incore)\n {\n-\tst->u.incore.buf = read_object_file_extended(oid, type, &st->size, 0);\n+\tst->u.incore.buf = read_object_file_extended(the_repository, oid, type, &st->size, 0);\n \tst->u.incore.read_ptr = 0;\n \tst->vtbl = &incore_vtbl;\n \n-- \n2.19.0\n\n"},{"id":"360703","messageId":"20181016233550.251311-5-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 04/19] object-store: prepare read_object_file to deal with arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:35Z","receivedAt":"2018-10-16T23:36:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As read_object_file is a widely used function (which is also regularly used\nin new code in flight between master..pu), changing its signature is painful\nis hard, as other series in flight rely on the original signature. It would\nburden the maintainer if we'd just change the signature.\n\nIntroduce repo_read_object_file which takes the repository argument, and\nhide the original read_object_file as a macro behind\nNO_THE_REPOSITORY_COMPATIBILITY_MACROS, similar to\ne675765235 (diff.c: remove implicit dependency on the_index, 2018-09-21)\n\nAdd a coccinelle patch to convert existing callers, but do not apply\nthe resulting patch from 'make coccicheck' to keep the diff of this\npatch small.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n contrib/coccinelle/the_repository.cocci | 13 +++++++++++++\n object-store.h | 10 ++++++++--\n 2 files changed, 21 insertions(+), 2 deletions(-)\n create mode 100644 contrib/coccinelle/the_repository.cocci\n\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nnew file mode 100644\nindex 0000000000..3c7fa70502\n--- /dev/null\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -0,0 +1,13 @@\n+// This file is used for the ongoing refactoring of\n+// bringing the index or repository struct in all of\n+// our code base.\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- read_object_file(\n++ repo_read_object_file(the_repository,\n+ E, F, G)\n+\ndiff --git a/object-store.h b/object-store.h\nindex 6bb0ccbf05..41ceebca48 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -150,10 +150,16 @@ extern void *read_object_file_extended(struct repository *r,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t enum object_type *type,\n \t\t\t\t unsigned long *size, int lookup_replace);\n-static inline void *read_object_file(const struct object_id *oid, enum object_type *type, unsigned long *size)\n+static inline void *repo_read_object_file(struct repository *r,\n+\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t enum object_type *type,\n+\t\t\t\t\t unsigned long *size)\n {\n-\treturn read_object_file_extended(the_repository, oid, type, size, 1);\n+\treturn read_object_file_extended(r, oid, type, size, 1);\n }\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)\n+#endif\n \n /* Read and unpack an object file into memory, write memory to an object file */\n int oid_object_info(struct repository *r, const struct object_id *, unsigned long *);\n-- \n2.19.0\n\n"},{"id":"360704","messageId":"20181016233550.251311-6-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 05/19] object-store: prepare has_{sha1, object}_file[_with_flags] to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:36Z","receivedAt":"2018-10-16T23:36:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n contrib/coccinelle/the_repository.cocci | 29 +++++++++++++++++++++++++\n object-store.h | 22 ++++++++++++++-----\n sha1-file.c | 15 ++++++++-----\n 3 files changed, 55 insertions(+), 11 deletions(-)\n\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 3c7fa70502..46f3a1b23a 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -11,3 +11,32 @@ expression G;\n + repo_read_object_file(the_repository,\n E, F, G)\n \n+@@\n+expression E;\n+@@\n+- has_sha1_file(\n++ repo_has_sha1_file(the_repository,\n+ E)\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- has_sha1_file_with_flags(\n++ repo_has_sha1_file_with_flags(the_repository,\n+ E)\n+\n+@@\n+expression E;\n+@@\n+- has_object_file(\n++ repo_has_object_file(the_repository,\n+ E)\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- has_object_file_with_flags(\n++ repo_has_object_file_with_flags(the_repository,\n+ E)\ndiff --git a/object-store.h b/object-store.h\nindex 41ceebca48..e1c68d0774 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -197,15 +197,27 @@ int read_loose_object(const char *path,\n * object_info. OBJECT_INFO_SKIP_CACHED is automatically set; pass\n * nonzero flags to also set other flags.\n */\n-extern int has_sha1_file_with_flags(const unsigned char *sha1, int flags);\n-static inline int has_sha1_file(const unsigned char *sha1)\n+int repo_has_sha1_file_with_flags(struct repository *r,\n+\t\t\t\t const unsigned char *sha1, int flags);\n+static inline int repo_has_sha1_file(struct repository *r,\n+\t\t\t\t const unsigned char *sha1)\n {\n-\treturn has_sha1_file_with_flags(sha1, 0);\n+\treturn repo_has_sha1_file_with_flags(r, sha1, 0);\n }\n \n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define has_sha1_file_with_flags(sha1, flags) repo_has_sha1_file_with_flags(the_repository, sha1, flags)\n+#define has_sha1_file(sha1) repo_has_sha1_file(the_repository, sha1)\n+#endif\n+\n /* Same as the above, except for struct object_id. */\n-extern int has_object_file(const struct object_id *oid);\n-extern int has_object_file_with_flags(const struct object_id *oid, int flags);\n+int repo_has_object_file(struct repository *r, const struct object_id *oid);\n+int repo_has_object_file_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid, int flags);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define has_object_file(oid) repo_has_object_file(the_repository, oid)\n+#define has_object_file_with_flags(oid, flags) repo_has_object_file_with_flags(the_repository, oid, flags)\n+#endif\n \n /*\n * Return true iff an alternate object database has a loose object\ndiff --git a/sha1-file.c b/sha1-file.c\nindex ce47524679..6153cedcf3 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1768,24 +1768,27 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \treturn ret;\n }\n \n-int has_sha1_file_with_flags(const unsigned char *sha1, int flags)\n+int repo_has_sha1_file_with_flags(struct repository *r,\n+\t\t\t\t const unsigned char *sha1, int flags)\n {\n \tstruct object_id oid;\n \tif (!startup_info->have_repository)\n \t\treturn 0;\n \thashcpy(oid.hash, sha1);\n-\treturn oid_object_info_extended(the_repository, &oid, NULL,\n+\treturn oid_object_info_extended(r, &oid, NULL,\n \t\t\t\t\tflags | OBJECT_INFO_SKIP_CACHED) >= 0;\n }\n \n-int has_object_file(const struct object_id *oid)\n+int repo_has_object_file(struct repository *r,\n+\t\t\t const struct object_id *oid)\n {\n-\treturn has_sha1_file(oid->hash);\n+\treturn repo_has_sha1_file(r, oid->hash);\n }\n \n-int has_object_file_with_flags(const struct object_id *oid, int flags)\n+int repo_has_object_file_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid, int flags)\n {\n-\treturn has_sha1_file_with_flags(oid->hash, flags);\n+\treturn repo_has_sha1_file_with_flags(r, oid->hash, flags);\n }\n \n static void check_tree(const void *buf, size_t size)\n-- \n2.19.0\n\n"},{"id":"360705","messageId":"20181016233550.251311-7-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 06/19] object: parse_object to honor its repository argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:37Z","receivedAt":"2018-10-16T23:36:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In 8e4b0b6047 (object.c: allow parse_object to handle\narbitrary repositories, 2018-06-28), we forgot to pass the\nrepository down to the read_object_file.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n object.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 51c4594515..a32afa14df 100644\n--- a/object.c\n+++ b/object.c\n@@ -259,8 +259,8 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \tif (obj && obj->parsed)\n \t\treturn obj;\n \n-\tif ((obj && obj->type == OBJ_BLOB && has_object_file(oid)) ||\n-\t (!obj && has_object_file(oid) &&\n+\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n+\t (!obj && repo_has_object_file(r, oid) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n \t\tif (check_object_signature(repl, NULL, 0, NULL) < 0) {\n \t\t\terror(_(\"sha1 mismatch %s\"), oid_to_hex(oid));\n@@ -270,7 +270,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \t\treturn lookup_object(r, oid->hash);\n \t}\n \n-\tbuffer = read_object_file(oid, &type, &size);\n+\tbuffer = repo_read_object_file(r, oid, &type, &size);\n \tif (buffer) {\n \t\tif (check_object_signature(repl, buffer, size, type_name(type)) < 0) {\n \t\t\tfree(buffer);\n-- \n2.19.0\n\n"},{"id":"360706","messageId":"20181016233550.251311-8-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 07/19] commit: allow parse_commit* to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:38Z","receivedAt":"2018-10-16T23:36:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just like the previous commit, parse_commit and friends are used a lot\nand are found in new patches, so we cannot change their signature easily.\n\nRe-introduce these function prefixed with 'repo_' that take a repository\nargument and keep the original as a shallow macro.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n commit.c | 18 +++++++++++-------\n commit.h | 17 +++++++++++++----\n contrib/coccinelle/the_repository.cocci | 24 ++++++++++++++++++++++++\n 3 files changed, 48 insertions(+), 11 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 5781f9bc67..fe942754e2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -443,7 +443,10 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \treturn 0;\n }\n \n-int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_commit_graph)\n+int repo_parse_commit_internal(struct repository *r,\n+\t\t\t struct commit *item,\n+\t\t\t int quiet_on_missing,\n+\t\t\t int use_commit_graph)\n {\n \tenum object_type type;\n \tvoid *buffer;\n@@ -454,9 +457,9 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n-\tif (use_commit_graph && parse_commit_in_graph(the_repository, item))\n+\tif (use_commit_graph && parse_commit_in_graph(r, item))\n \t\treturn 0;\n-\tbuffer = read_object_file(&item->object.oid, &type, &size);\n+\tbuffer = repo_read_object_file(r, &item->object.oid, &type, &size);\n \tif (!buffer)\n \t\treturn quiet_on_missing ? -1 :\n \t\t\terror(\"Could not read %s\",\n@@ -467,18 +470,19 @@ int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_com\n \t\t\t oid_to_hex(&item->object.oid));\n \t}\n \n-\tret = parse_commit_buffer(the_repository, item, buffer, size, 0);\n+\tret = parse_commit_buffer(r, item, buffer, size, 0);\n \tif (save_commit_buffer && !ret) {\n-\t\tset_commit_buffer(the_repository, item, buffer, size);\n+\t\tset_commit_buffer(r, item, buffer, size);\n \t\treturn 0;\n \t}\n \tfree(buffer);\n \treturn ret;\n }\n \n-int parse_commit_gently(struct commit *item, int quiet_on_missing)\n+int repo_parse_commit_gently(struct repository *r,\n+\t\t\t struct commit *item, int quiet_on_missing)\n {\n-\treturn parse_commit_internal(item, quiet_on_missing, 1);\n+\treturn repo_parse_commit_internal(r, item, quiet_on_missing, 1);\n }\n \n void parse_commit_or_die(struct commit *item)\ndiff --git a/commit.h b/commit.h\nindex e2c99d9b04..58a0c5409b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -79,12 +79,21 @@ struct commit *lookup_commit_reference_by_name(const char *name);\n struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref_name);\n \n int parse_commit_buffer(struct repository *r, struct commit *item, const void *buffer, unsigned long size, int check_graph);\n-int parse_commit_internal(struct commit *item, int quiet_on_missing, int use_commit_graph);\n-int parse_commit_gently(struct commit *item, int quiet_on_missing);\n-static inline int parse_commit(struct commit *item)\n+int repo_parse_commit_internal(struct repository *r, struct commit *item,\n+\t\t\t int quiet_on_missing, int use_commit_graph);\n+int repo_parse_commit_gently(struct repository *r,\n+\t\t\t struct commit *item,\n+\t\t\t int quiet_on_missing);\n+static inline int repo_parse_commit(struct repository *r, struct commit *item)\n {\n-\treturn parse_commit_gently(item, 0);\n+\treturn repo_parse_commit_gently(r, item, 0);\n }\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use)\n+#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)\n+#define parse_commit(item) repo_parse_commit(the_repository, item)\n+#endif\n+\n void parse_commit_or_die(struct commit *item);\n \n struct buffer_slab;\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 46f3a1b23a..b185fe0a1d 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -40,3 +40,27 @@ expression F;\n - has_object_file_with_flags(\n + repo_has_object_file_with_flags(the_repository,\n E)\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- parse_commit_internal(\n++ repo_parse_commit_internal(the_repository,\n+ E, F, G)\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- parse_commit_gently(\n++ repo_parse_commit_gently(the_repository,\n+ E, F)\n+\n+@@\n+expression E;\n+@@\n+- parse_commit(\n++ repo_parse_commit(the_repository,\n+ E)\n-- \n2.19.0\n\n"},{"id":"360707","messageId":"20181016233550.251311-9-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 08/19] commit-reach.c: allow paint_down_to_common to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:39Z","receivedAt":"2018-10-16T23:36:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the function is file local and not widely used, migrate it all at once.\n\nSigned-off-by: Stefan Beller \n---\n commit-reach.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 5a845440a9..2f7ae20bd4 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -30,7 +30,8 @@ static int queue_has_nonstale(struct prio_queue *queue)\n }\n \n /* all input commits in one and twos[] must have been parsed! */\n-static struct commit_list *paint_down_to_common(struct commit *one, int n,\n+static struct commit_list *paint_down_to_common(struct repository *r,\n+\t\t\t\t\t\tstruct commit *one, int n,\n \t\t\t\t\t\tstruct commit **twos,\n \t\t\t\t\t\tint min_generation)\n {\n@@ -80,7 +81,7 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n,\n \t\t\tparents = parents->next;\n \t\t\tif ((p->object.flags & flags) == flags)\n \t\t\t\tcontinue;\n-\t\t\tif (parse_commit(p))\n+\t\t\tif (repo_parse_commit(r, p))\n \t\t\t\treturn NULL;\n \t\t\tp->object.flags |= flags;\n \t\t\tprio_queue_put(&queue, p);\n@@ -113,7 +114,7 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co\n \t\t\treturn NULL;\n \t}\n \n-\tlist = paint_down_to_common(one, n, twos, 0);\n+\tlist = paint_down_to_common(the_repository, one, n, twos, 0);\n \n \twhile (list) {\n \t\tstruct commit *commit = pop_commit(&list);\n@@ -184,8 +185,8 @@ static int remove_redundant(struct commit **array, int cnt)\n \t\t\tif (array[j]->generation < min_generation)\n \t\t\t\tmin_generation = array[j]->generation;\n \t\t}\n-\t\tcommon = paint_down_to_common(array[i], filled, work,\n-\t\t\t\t\t min_generation);\n+\t\tcommon = paint_down_to_common(the_repository, array[i], filled,\n+\t\t\t\t\t work, min_generation);\n \t\tif (array[i]->object.flags & PARENT2)\n \t\t\tredundant[i] = 1;\n \t\tfor (j = 0; j < filled; j++)\n@@ -319,7 +320,9 @@ int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit *\n \tif (commit->generation > min_generation)\n \t\treturn ret;\n \n-\tbases = paint_down_to_common(commit, nr_reference, reference, commit->generation);\n+\tbases = paint_down_to_common(the_repository, commit,\n+\t\t\t\t nr_reference, reference,\n+\t\t\t\t commit->generation);\n \tif (commit->object.flags & PARENT2)\n \t\tret = 1;\n \tclear_commit_marks(commit, all_flags);\n-- \n2.19.0\n\n"},{"id":"360708","messageId":"20181016233550.251311-10-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 09/19] commit-reach.c: allow merge_bases_many to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:40Z","receivedAt":"2018-10-16T23:36:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit-reach.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 2f7ae20bd4..aacd8cdc1e 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -92,7 +92,9 @@ static struct commit_list *paint_down_to_common(struct repository *r,\n \treturn result;\n }\n \n-static struct commit_list *merge_bases_many(struct commit *one, int n, struct commit **twos)\n+static struct commit_list *merge_bases_many(struct repository *r,\n+\t\t\t\t\t struct commit *one, int n,\n+\t\t\t\t\t struct commit **twos)\n {\n \tstruct commit_list *list = NULL;\n \tstruct commit_list *result = NULL;\n@@ -107,14 +109,14 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co\n \t\t\treturn commit_list_insert(one, &result);\n \t}\n \n-\tif (parse_commit(one))\n+\tif (repo_parse_commit(r, one))\n \t\treturn NULL;\n \tfor (i = 0; i < n; i++) {\n-\t\tif (parse_commit(twos[i]))\n+\t\tif (repo_parse_commit(r, twos[i]))\n \t\t\treturn NULL;\n \t}\n \n-\tlist = paint_down_to_common(the_repository, one, n, twos, 0);\n+\tlist = paint_down_to_common(r, one, n, twos, 0);\n \n \twhile (list) {\n \t\tstruct commit *commit = pop_commit(&list);\n@@ -221,7 +223,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,\n \tstruct commit_list *result;\n \tint cnt, i;\n \n-\tresult = merge_bases_many(one, n, twos);\n+\tresult = merge_bases_many(the_repository, one, n, twos);\n \tfor (i = 0; i < n; i++) {\n \t\tif (one == twos[i])\n \t\t\treturn result;\n-- \n2.19.0\n\n"},{"id":"360709","messageId":"20181016233550.251311-11-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 10/19] commit-reach.c: allow remove_redundant to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:41Z","receivedAt":"2018-10-16T23:36:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit-reach.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex aacd8cdc1e..bce4169f1f 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -153,7 +153,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)\n \treturn ret;\n }\n \n-static int remove_redundant(struct commit **array, int cnt)\n+static int remove_redundant(struct repository *r, struct commit **array, int cnt)\n {\n \t/*\n \t * Some commit in the array may be an ancestor of\n@@ -171,7 +171,7 @@ static int remove_redundant(struct commit **array, int cnt)\n \tALLOC_ARRAY(filled_index, cnt - 1);\n \n \tfor (i = 0; i < cnt; i++)\n-\t\tparse_commit(array[i]);\n+\t\trepo_parse_commit(r, array[i]);\n \tfor (i = 0; i < cnt; i++) {\n \t\tstruct commit_list *common;\n \t\tuint32_t min_generation = array[i]->generation;\n@@ -187,7 +187,7 @@ static int remove_redundant(struct commit **array, int cnt)\n \t\t\tif (array[j]->generation < min_generation)\n \t\t\t\tmin_generation = array[j]->generation;\n \t\t}\n-\t\tcommon = paint_down_to_common(the_repository, array[i], filled,\n+\t\tcommon = paint_down_to_common(r, array[i], filled,\n \t\t\t\t\t work, min_generation);\n \t\tif (array[i]->object.flags & PARENT2)\n \t\t\tredundant[i] = 1;\n@@ -246,7 +246,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,\n \tclear_commit_marks(one, all_flags);\n \tclear_commit_marks_many(n, twos, all_flags);\n \n-\tcnt = remove_redundant(rslt, cnt);\n+\tcnt = remove_redundant(the_repository, rslt, cnt);\n \tresult = NULL;\n \tfor (i = 0; i < cnt; i++)\n \t\tcommit_list_insert_by_date(rslt[i], &result);\n@@ -367,7 +367,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \t\t\tp->item->object.flags &= ~STALE;\n \t\t}\n \t}\n-\tnum_head = remove_redundant(array, num_head);\n+\tnum_head = remove_redundant(the_repository, array, num_head);\n \tfor (i = 0; i < num_head; i++)\n \t\ttail = &commit_list_insert(array[i], tail)->next;\n \tfree(array);\n-- \n2.19.0\n\n"},{"id":"360710","messageId":"20181016233550.251311-12-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 11/19] commit-reach.c: allow get_merge_bases_many_0 to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:42Z","receivedAt":"2018-10-16T23:36:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit-reach.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex bce4169f1f..df93274966 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -213,7 +213,8 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt\n \treturn filled;\n }\n \n-static struct commit_list *get_merge_bases_many_0(struct commit *one,\n+static struct commit_list *get_merge_bases_many_0(struct repository *r,\n+\t\t\t\t\t\t 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@@ -223,7 +224,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,\n \tstruct commit_list *result;\n \tint cnt, i;\n \n-\tresult = merge_bases_many(the_repository, one, n, twos);\n+\tresult = merge_bases_many(r, one, n, twos);\n \tfor (i = 0; i < n; i++) {\n \t\tif (one == twos[i])\n \t\t\treturn result;\n@@ -246,7 +247,7 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,\n \tclear_commit_marks(one, all_flags);\n \tclear_commit_marks_many(n, twos, all_flags);\n \n-\tcnt = remove_redundant(the_repository, rslt, cnt);\n+\tcnt = remove_redundant(r, rslt, cnt);\n \tresult = NULL;\n \tfor (i = 0; i < cnt; i++)\n \t\tcommit_list_insert_by_date(rslt[i], &result);\n@@ -258,19 +259,19 @@ 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+\treturn get_merge_bases_many_0(the_repository, 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+\treturn get_merge_bases_many_0(the_repository, 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_0(one, 1, &two, 1);\n+\treturn get_merge_bases_many_0(the_repository, one, 1, &two, 1);\n }\n \n /*\n-- \n2.19.0\n\n"},{"id":"360711","messageId":"20181016233550.251311-13-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 12/19] commit-reach: prepare get_merge_bases to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:43Z","receivedAt":"2018-10-16T23:36:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similarly to previous patches, the get_merge_base functions are used\noften in the code base, which makes migrating them hard.\n\nImplement the new functions, prefixed with 'repo_' and hide the old\nfunctions behind a wrapper macro.\n\nSigned-off-by: Stefan Beller \n---\n commit-reach.c | 24 +++++++++++++---------\n commit-reach.h | 26 +++++++++++++++---------\n contrib/coccinelle/the_repository.cocci | 27 +++++++++++++++++++++++++\n 3 files changed, 57 insertions(+), 20 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex df93274966..aa3cde5c8a 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -255,23 +255,27 @@ static struct commit_list *get_merge_bases_many_0(struct repository *r,\n \treturn result;\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+struct commit_list *repo_get_merge_bases_many(struct repository *r,\n+\t\t\t\t\t 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(the_repository, one, n, twos, 1);\n+\treturn get_merge_bases_many_0(r, 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+struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n+\t\t\t\t\t\t struct commit *one,\n+\t\t\t\t\t\t int n,\n+\t\t\t\t\t\t struct commit **twos)\n {\n-\treturn get_merge_bases_many_0(the_repository, one, n, twos, 0);\n+\treturn get_merge_bases_many_0(r, one, n, twos, 0);\n }\n \n-struct commit_list *get_merge_bases(struct commit *one, struct commit *two)\n+struct commit_list *repo_get_merge_bases(struct repository *r,\n+\t\t\t\t\t struct commit *one,\n+\t\t\t\t\t struct commit *two)\n {\n-\treturn get_merge_bases_many_0(the_repository, one, 1, &two, 1);\n+\treturn get_merge_bases_many_0(r, one, 1, &two, 1);\n }\n \n /*\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 7d313e2975..52667d64ac 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -8,17 +8,23 @@ struct commit_list;\n struct contains_cache;\n struct ref_filter;\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-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-struct commit_list *get_merge_bases(struct commit *one, struct commit *two);\n-struct commit_list *get_octopus_merge_bases(struct commit_list *in);\n-\n+struct commit_list *repo_get_merge_bases(struct repository *r,\n+\t\t\t\t\t struct commit *rev1,\n+\t\t\t\t\t struct commit *rev2);\n+struct commit_list *repo_get_merge_bases_many(struct repository *r,\n+\t\t\t\t\t struct commit *one, int n,\n+\t\t\t\t\t struct commit **twos);\n /* To be used only when object flags after this call no longer matter */\n-struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, struct commit **twos);\n+struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\n+\t\t\t\t\t\t struct commit *one, int n,\n+\t\t\t\t\t\t struct commit **twos);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define get_merge_bases(r1, r2) repo_get_merge_bases(the_repository, r1, r2)\n+#define get_merge_bases_many(one, n, two) repo_get_merge_bases_many(the_repository, one, n, two)\n+#define get_merge_bases_many_dirty(one, n, twos) repo_get_merge_bases_many_dirty(the_repository, one, n, twos)\n+#endif\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 in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex b185fe0a1d..5e037fe428 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -64,3 +64,30 @@ expression E;\n - parse_commit(\n + repo_parse_commit(the_repository,\n E)\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- get_merge_bases(\n++ repo_get_merge_bases(the_repository,\n+ E, F);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- get_merge_bases_many(\n++ repo_get_merge_bases_many(the_repository,\n+ E, F, G);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- get_merge_bases_many_dirty(\n++ repo_get_merge_bases_many_dirty(the_repository,\n+ E, F, G);\n+\n-- \n2.19.0\n\n"},{"id":"360712","messageId":"20181016233550.251311-14-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 13/19] commit-reach: prepare in_merge_bases[_many] to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:44Z","receivedAt":"2018-10-16T23:36:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit-reach.c | 15 +++++++++------\n commit-reach.h | 12 ++++++++++--\n contrib/coccinelle/the_repository.cocci | 16 ++++++++++++++++\n 3 files changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex aa3cde5c8a..1877f356e3 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -309,16 +309,17 @@ int is_descendant_of(struct commit *commit, struct commit_list *with_commit)\n /*\n * Is \"commit\" an ancestor of one of the \"references\"?\n */\n-int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference)\n+int repo_in_merge_bases_many(struct repository *r, struct commit *commit,\n+\t\t\t int nr_reference, struct commit **reference)\n {\n \tstruct commit_list *bases;\n \tint ret = 0, i;\n \tuint32_t min_generation = GENERATION_NUMBER_INFINITY;\n \n-\tif (parse_commit(commit))\n+\tif (repo_parse_commit(r, commit))\n \t\treturn ret;\n \tfor (i = 0; i < nr_reference; i++) {\n-\t\tif (parse_commit(reference[i]))\n+\t\tif (repo_parse_commit(r, reference[i]))\n \t\t\treturn ret;\n \t\tif (reference[i]->generation < min_generation)\n \t\t\tmin_generation = reference[i]->generation;\n@@ -327,7 +328,7 @@ int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit *\n \tif (commit->generation > min_generation)\n \t\treturn ret;\n \n-\tbases = paint_down_to_common(the_repository, commit,\n+\tbases = paint_down_to_common(r, commit,\n \t\t\t\t nr_reference, reference,\n \t\t\t\t commit->generation);\n \tif (commit->object.flags & PARENT2)\n@@ -341,9 +342,11 @@ int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit *\n /*\n * Is \"commit\" an ancestor of (i.e. reachable from) the \"reference\"?\n */\n-int in_merge_bases(struct commit *commit, struct commit *reference)\n+int repo_in_merge_bases(struct repository *r,\n+\t\t\tstruct commit *commit,\n+\t\t\tstruct commit *reference)\n {\n-\treturn in_merge_bases_many(commit, 1, &reference);\n+\treturn repo_in_merge_bases_many(r, commit, 1, &reference);\n }\n \n struct commit_list *reduce_heads(struct commit_list *heads)\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 52667d64ac..a0d4a29d25 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -27,8 +27,16 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,\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 in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);\n-int in_merge_bases(struct commit *commit, struct commit *reference);\n+int repo_in_merge_bases(struct repository *r,\n+\t\t\tstruct commit *commit,\n+\t\t\tstruct commit *reference);\n+int repo_in_merge_bases_many(struct repository *r,\n+\t\t\t struct commit *commit,\n+\t\t\t int nr_reference, struct commit **reference);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define in_merge_bases(c1, c2) repo_in_merge_bases(the_repository, c1, c2)\n+#define in_merge_bases_many(c1, n, cs) repo_in_merge_bases_many(the_repository, c1, n, cs)\n+#endif\n \n /*\n * Takes a list of commits and returns a new list where those\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 5e037fe428..8c6a71bf64 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -91,3 +91,19 @@ expression G;\n + repo_get_merge_bases_many_dirty(the_repository,\n E, F, G);\n \n+@@\n+expression E;\n+expression F;\n+@@\n+- in_merge_bases(\n++ repo_in_merge_bases(the_repository,\n+ E, F);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- in_merge_bases_many(\n++ repo_in_merge_bases_many(the_repository,\n+ E, F, G);\n-- \n2.19.0\n\n"},{"id":"360713","messageId":"20181016233550.251311-15-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 14/19] commit: prepare get_commit_buffer to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:45Z","receivedAt":"2018-10-16T23:36:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit.c | 8 +++++---\n commit.h | 7 ++++++-\n contrib/coccinelle/the_repository.cocci | 8 ++++++++\n 3 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex fe942754e2..cc5321af8c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -297,13 +297,15 @@ const void *get_cached_commit_buffer(struct repository *r, const struct commit *\n \treturn v->buffer;\n }\n \n-const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep)\n+const void *repo_get_commit_buffer(struct repository *r,\n+\t\t\t\t const struct commit *commit,\n+\t\t\t\t unsigned long *sizep)\n {\n-\tconst void *ret = get_cached_commit_buffer(the_repository, commit, sizep);\n+\tconst void *ret = get_cached_commit_buffer(r, commit, sizep);\n \tif (!ret) {\n \t\tenum object_type type;\n \t\tunsigned long size;\n-\t\tret = read_object_file(&commit->object.oid, &type, &size);\n+\t\tret = repo_read_object_file(r, &commit->object.oid, &type, &size);\n \t\tif (!ret)\n \t\t\tdie(\"cannot read commit object %s\",\n \t\t\t oid_to_hex(&commit->object.oid));\ndiff --git a/commit.h b/commit.h\nindex 58a0c5409b..b8437f66e2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -117,7 +117,12 @@ const void *get_cached_commit_buffer(struct repository *, const struct commit *,\n * from disk. The resulting memory should not be modified, and must be given\n * to unuse_commit_buffer when the caller is done.\n */\n-const void *get_commit_buffer(const struct commit *, unsigned long *size);\n+const void *repo_get_commit_buffer(struct repository *r,\n+\t\t\t\t const struct commit *,\n+\t\t\t\t unsigned long *size);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define get_commit_buffer(c, s) repo_get_commit_buffer(the_repository, c, s)\n+#endif\n \n /*\n * Tell the commit subsytem that we are done with a particular commit buffer.\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 8c6a71bf64..4018e6eaf7 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -107,3 +107,11 @@ expression G;\n - in_merge_bases_many(\n + repo_in_merge_bases_many(the_repository,\n E, F, G);\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- get_commit_buffer(\n++ repo_get_commit_buffer(the_repository,\n+ E, F);\n-- \n2.19.0\n\n"},{"id":"360714","messageId":"20181016233550.251311-16-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 15/19] commit: prepare repo_unuse_commit_buffer to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:46Z","receivedAt":"2018-10-16T23:36:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n commit.c | 6 ++++--\n commit.h | 7 ++++++-\n contrib/coccinelle/the_repository.cocci | 8 ++++++++\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex cc5321af8c..8e7b1138f7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -318,10 +318,12 @@ const void *repo_get_commit_buffer(struct repository *r,\n \treturn ret;\n }\n \n-void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n+void repo_unuse_commit_buffer(struct repository *r,\n+\t\t\t const struct commit *commit,\n+\t\t\t const void *buffer)\n {\n \tstruct commit_buffer *v = buffer_slab_peek(\n-\t\tthe_repository->parsed_objects->buffer_slab, commit);\n+\t\tr->parsed_objects->buffer_slab, commit);\n \tif (!(v && v->buffer == buffer))\n \t\tfree((void *)buffer);\n }\ndiff --git a/commit.h b/commit.h\nindex b8437f66e2..f3ef497723 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -130,7 +130,12 @@ const void *repo_get_commit_buffer(struct repository *r,\n * from an earlier call to get_commit_buffer. The buffer may or may not be\n * freed by this call; callers should not access the memory afterwards.\n */\n-void unuse_commit_buffer(const struct commit *, const void *buffer);\n+void repo_unuse_commit_buffer(struct repository *r,\n+\t\t\t const struct commit *,\n+\t\t\t const void *buffer);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define unuse_commit_buffer(c, b) repo_unuse_commit_buffer(the_repository, c, b)\n+#endif\n \n /*\n * Free any cached object buffer associated with the commit.\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 4018e6eaf7..516f19ffee 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -115,3 +115,11 @@ expression F;\n - get_commit_buffer(\n + repo_get_commit_buffer(the_repository,\n E, F);\n+\n+@@\n+expression E;\n+expression F;\n+@@\n+- unuse_commit_buffer(\n++ repo_unuse_commit_buffer(the_repository,\n+ E, F);\n-- \n2.19.0\n\n"},{"id":"360715","messageId":"20181016233550.251311-17-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 16/19] commit: prepare logmsg_reencode to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:47Z","receivedAt":"2018-10-16T23:36:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n commit.h | 8 ++++++++\n contrib/coccinelle/the_repository.cocci | 9 +++++++++\n pretty.c | 13 +++++++------\n 3 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex f3ef497723..b69a4b140d 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -180,6 +180,14 @@ extern int has_non_ascii(const char *text);\n extern const char *logmsg_reencode(const struct commit *commit,\n \t\t\t\t char **commit_encoding,\n \t\t\t\t const char *output_encoding);\n+const char *repo_logmsg_reencode(struct repository *r,\n+\t\t\t\t const struct commit *commit,\n+\t\t\t\t char **commit_encoding,\n+\t\t\t\t const char *output_encoding);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define logmsg_reencode(c, enc, out) repo_logmsg_reencode(the_repository, c, enc, out)\n+#endif\n+\n extern const char *skip_blank_lines(const char *msg);\n \n /** Removes the first commit from a list sorted by date, and adds all\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex 516f19ffee..f5b42cfc62 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -123,3 +123,12 @@ expression F;\n - unuse_commit_buffer(\n + repo_unuse_commit_buffer(the_repository,\n E, F);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+@@\n+- logmsg_reencode(\n++ repo_logmsg_reencode(the_repository,\n+ E, F, G);\ndiff --git a/pretty.c b/pretty.c\nindex 98cf5228f9..26e44472bb 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -595,14 +595,15 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \treturn strbuf_detach(&tmp, NULL);\n }\n \n-const char *logmsg_reencode(const struct commit *commit,\n-\t\t\t char **commit_encoding,\n-\t\t\t const char *output_encoding)\n+const char *repo_logmsg_reencode(struct repository *r,\n+\t\t\t\t const struct commit *commit,\n+\t\t\t\t char **commit_encoding,\n+\t\t\t\t const char *output_encoding)\n {\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n-\tconst char *msg = get_commit_buffer(commit, NULL);\n+\tconst char *msg = repo_get_commit_buffer(r, commit, NULL);\n \tchar *out;\n \n \tif (!output_encoding || !*output_encoding) {\n@@ -630,7 +631,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * the cached copy from get_commit_buffer, we need to duplicate it\n \t\t * to avoid munging the cached copy.\n \t\t */\n-\t\tif (msg == get_cached_commit_buffer(the_repository, commit, NULL))\n+\t\tif (msg == get_cached_commit_buffer(r, commit, NULL))\n \t\t\tout = xstrdup(msg);\n \t\telse\n \t\t\tout = (char *)msg;\n@@ -644,7 +645,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t */\n \t\tout = reencode_string(msg, output_encoding, use_encoding);\n \t\tif (out)\n-\t\t\tunuse_commit_buffer(commit, msg);\n+\t\t\trepo_unuse_commit_buffer(r, commit, msg);\n \t}\n \n \t/*\n-- \n2.19.0\n\n"},{"id":"360716","messageId":"20181016233550.251311-18-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 17/19] pretty: prepare format_commit_message to handle arbitrary repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:48Z","receivedAt":"2018-10-16T23:36:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n contrib/coccinelle/the_repository.cocci | 10 ++++++++++\n pretty.c | 15 ++++++++-------\n pretty.h | 7 ++++++-\n 3 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/coccinelle/the_repository.cocci b/contrib/coccinelle/the_repository.cocci\nindex f5b42cfc62..2ee702ecf7 100644\n--- a/contrib/coccinelle/the_repository.cocci\n+++ b/contrib/coccinelle/the_repository.cocci\n@@ -132,3 +132,13 @@ expression G;\n - logmsg_reencode(\n + repo_logmsg_reencode(the_repository,\n E, F, G);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+expression H;\n+@@\n+- format_commit_message(\n++ repo_format_commit_message(the_repository,\n+ E, F, G, H);\ndiff --git a/pretty.c b/pretty.c\nindex 26e44472bb..948f5346cf 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1505,9 +1505,10 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n \tstrbuf_release(&dummy);\n }\n \n-void format_commit_message(const struct commit *commit,\n-\t\t\t const char *format, struct strbuf *sb,\n-\t\t\t const struct pretty_print_context *pretty_ctx)\n+void repo_format_commit_message(struct repository *r,\n+\t\t\t\tconst struct commit *commit,\n+\t\t\t\tconst char *format, struct strbuf *sb,\n+\t\t\t\tconst struct pretty_print_context *pretty_ctx)\n {\n \tstruct format_commit_context context;\n \tconst char *output_enc = pretty_ctx->output_encoding;\n@@ -1521,9 +1522,9 @@ void format_commit_message(const struct commit *commit,\n \t * convert a commit message to UTF-8 first\n \t * as far as 'format_commit_item' assumes it in UTF-8\n \t */\n-\tcontext.message = logmsg_reencode(commit,\n-\t\t\t\t\t &context.commit_encoding,\n-\t\t\t\t\t utf8);\n+\tcontext.message = repo_logmsg_reencode(r, commit,\n+\t\t\t\t\t &context.commit_encoding,\n+\t\t\t\t\t utf8);\n \n \tstrbuf_expand(sb, format, format_commit_item, &context);\n \trewrap_message_tail(sb, &context, 0, 0, 0);\n@@ -1547,7 +1548,7 @@ void format_commit_message(const struct commit *commit,\n \t}\n \n \tfree(context.commit_encoding);\n-\tunuse_commit_buffer(commit, context.message);\n+\trepo_unuse_commit_buffer(r, commit, context.message);\n }\n \n static void pp_header(struct pretty_print_context *pp,\ndiff --git a/pretty.h b/pretty.h\nindex 7359d318a9..e6625269cf 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -103,9 +103,14 @@ void pp_remainder(struct pretty_print_context *pp, const char **msg_p,\n * Put the result to \"sb\".\n * Please use this function for custom formats.\n */\n-void format_commit_message(const struct commit *commit,\n+void repo_format_commit_message(struct repository *r,\n+\t\t\tconst struct commit *commit,\n \t\t\tconst char *format, struct strbuf *sb,\n \t\t\tconst struct pretty_print_context *context);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define format_commit_message(c, f, s, con) \\\n+\trepo_format_commit_message(the_repository, c, f, s, con)\n+#endif\n \n /*\n * Parse given arguments from \"arg\", check it for correctness and\n-- \n2.19.0\n\n"},{"id":"360717","messageId":"20181016233550.251311-19-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 18/19] submodule: use submodule repos for object lookup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:49Z","receivedAt":"2018-10-16T23:36:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This converts the 'show_submodule_header' function to use\nthe repository API properly, such that the submodule objects\nare not added to the main object store.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 75 ++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 60 insertions(+), 15 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4ee69cc014..7305ae2e10 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -444,7 +444,7 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n \treturn prepare_revision_walk(rev);\n }\n \n-static void print_submodule_summary(struct rev_info *rev, struct diff_options *o)\n+static void print_submodule_summary(struct repository *r, struct rev_info *rev, struct diff_options *o)\n {\n \tstatic const char format[] = \" %m %s\";\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -455,7 +455,8 @@ static void print_submodule_summary(struct rev_info *rev, struct diff_options *o\n \t\tctx.date_mode = rev->date_mode;\n \t\tctx.output_encoding = get_log_output_encoding();\n \t\tstrbuf_setlen(&sb, 0);\n-\t\tformat_commit_message(commit, format, &sb, &ctx);\n+\t\trepo_format_commit_message(r, commit, format, &sb,\n+\t\t\t\t &ctx);\n \t\tstrbuf_addch(&sb, '\\n');\n \t\tif (commit->object.flags & SYMMETRIC_LEFT)\n \t\t\tdiff_emit_submodule_del(o, sb.buf);\n@@ -482,14 +483,46 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n-/* Helper function to display the submodule header line prior to the full\n- * summary output. If it can locate the submodule objects directory it will\n- * attempt to lookup both the left and right commits and put them into the\n- * left and right pointers.\n+/*\n+ * Initialize 'out' based on the provided submodule path.\n+ *\n+ * Unlike repo_submodule_init, this tolerates submodules not present\n+ * in .gitmodules. This function exists only to preserve historical behavior,\n+ *\n+ * Returns 0 on success, -1 when the submodule is not present.\n+ */\n+static int open_submodule(struct repository *out, const char *path)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n+\t\tstrbuf_release(&sb);\n+\t\treturn -1;\n+\t}\n+\n+\tout->submodule_prefix = xstrdup(path);\n+\tout->submodule_prefix = xstrfmt(\"%s%s/\",\n+\t\t\t\t\tthe_repository->submodule_prefix ?\n+\t\t\t\t\tthe_repository->submodule_prefix :\n+\t\t\t\t\t\"\", path);\n+\n+\tstrbuf_release(&sb);\n+\treturn 0;\n+}\n+\n+/*\n+ * Helper function to display the submodule header line prior to the full\n+ * summary output.\n+ *\n+ * If it can locate the submodule git directory it will create a repository\n+ * handle for the submodule and lookup both the left and right commits and\n+ * put them into the left and right pointers.\n */\n-static void show_submodule_header(struct diff_options *o, const char *path,\n+static void show_submodule_header(struct diff_options *o,\n+\t\tconst char *path,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule,\n+\t\tstruct repository *sub,\n \t\tstruct commit **left, struct commit **right,\n \t\tstruct commit_list **merge_bases)\n {\n@@ -508,7 +541,7 @@ static void show_submodule_header(struct diff_options *o, const char *path,\n \telse if (is_null_oid(two))\n \t\tmessage = \"(submodule deleted)\";\n \n-\tif (add_submodule_odb(path)) {\n+\tif (!sub) {\n \t\tif (!message)\n \t\t\tmessage = \"(commits not present)\";\n \t\tgoto output_header;\n@@ -518,8 +551,8 @@ static void show_submodule_header(struct diff_options *o, const char *path,\n \t * Attempt to lookup the commit references, and determine if this is\n \t * a fast forward or fast backwards update.\n \t */\n-\t*left = lookup_commit_reference(the_repository, one);\n-\t*right = lookup_commit_reference(the_repository, two);\n+\t*left = lookup_commit_reference(sub, one);\n+\t*right = lookup_commit_reference(sub, two);\n \n \t/*\n \t * Warn about missing commits in the submodule project, but only if\n@@ -529,7 +562,7 @@ static void show_submodule_header(struct diff_options *o, const char *path,\n \t (!is_null_oid(two) && !*right))\n \t\tmessage = \"(commits not present)\";\n \n-\t*merge_bases = get_merge_bases(*left, *right);\n+\t*merge_bases = repo_get_merge_bases(sub, *left, *right);\n \tif (*merge_bases) {\n \t\tif ((*merge_bases)->item == *left)\n \t\t\tfast_forward = 1;\n@@ -563,16 +596,20 @@ void show_submodule_summary(struct diff_options *o, const char *path,\n \tstruct rev_info rev;\n \tstruct commit *left = NULL, *right = NULL;\n \tstruct commit_list *merge_bases = NULL;\n+\tstruct repository subrepo, *sub = &subrepo;\n+\n+\tif (open_submodule(&subrepo, path) < 0)\n+\t\tsub = NULL;\n \n \tshow_submodule_header(o, path, one, two, dirty_submodule,\n-\t\t\t &left, &right, &merge_bases);\n+\t\t\t sub, &left, &right, &merge_bases);\n \n \t/*\n \t * If we don't have both a left and a right pointer, there is no\n \t * reason to try and display a summary. The header line should contain\n \t * all the information the user needs.\n \t */\n-\tif (!left || !right)\n+\tif (!left || !right || !sub)\n \t\tgoto out;\n \n \t/* Treat revision walker failure the same as missing commits */\n@@ -581,13 +618,15 @@ void show_submodule_summary(struct diff_options *o, const char *path,\n \t\tgoto out;\n \t}\n \n-\tprint_submodule_summary(&rev, o);\n+\tprint_submodule_summary(sub, &rev, o);\n \n out:\n \tif (merge_bases)\n \t\tfree_commit_list(merge_bases);\n \tclear_commit_marks(left, ~0);\n \tclear_commit_marks(right, ~0);\n+\tif (sub)\n+\t\trepo_clear(sub);\n }\n \n void show_submodule_inline_diff(struct diff_options *o, const char *path,\n@@ -599,9 +638,13 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \tstruct commit_list *merge_bases = NULL;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct repository subrepo, *sub = &subrepo;\n+\n+\tif (open_submodule(&subrepo, path) < 0)\n+\t\tsub = NULL;\n \n \tshow_submodule_header(o, path, one, two, dirty_submodule,\n-\t\t\t &left, &right, &merge_bases);\n+\t\t\t sub, &left, &right, &merge_bases);\n \n \t/* We need a valid left and right commit to display a difference */\n \tif (!(left || is_null_oid(one)) ||\n@@ -662,6 +705,8 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\tclear_commit_marks(left, ~0);\n \tif (right)\n \t\tclear_commit_marks(right, ~0);\n+\tif (sub)\n+\t\trepo_clear(sub);\n }\n \n int should_update_submodules(void)\n-- \n2.19.0\n\n"},{"id":"360718","messageId":"20181016233550.251311-20-sbeller@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"[PATCH 19/19] submodule: don't add submodule as odb for push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-16T23:35:50Z","receivedAt":"2018-10-16T23:36:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In push_submodule(), because we do not actually need access to objects\nin the submodule, do not invoke add_submodule_odb().\n(for_each_remote_ref_submodule() does not require access to those\nobjects, and the actual push is done by spawning another process,\nwhich handles object access by itself.)\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7305ae2e10..e623e6bf7f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1024,9 +1024,6 @@ static int push_submodule(const char *path,\n \t\t\t const struct string_list *push_options,\n \t\t\t int dry_run)\n {\n-\tif (add_submodule_odb(path))\n-\t\treturn 1;\n-\n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \t\targv_array_push(&cp.args, \"push\");\n-- \n2.19.0\n\n"},{"id":"360742","messageId":"feef12c7-e609-dc6e-104d-7381471afb9c@gmail.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"Re: [PATCH 00/19] Bring more repository handles into our code base","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-17T12:41:24Z","receivedAt":"2018-10-17T12:41:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/16/2018 7:35 PM, Stefan Beller wrote:\n> \n> This series takes another approach as it doesn't change the signature of\n> functions, but introduces new functions that can deal with arbitrary\n> repositories, keeping the old function signature around using a shallow wrapper.\nI think this is a good direction, and the changes look good to me.\n\n> Additionally each patch adds a semantic patch, that would port from the old to\n> the new function. These semantic patches are all applied in the very last patch,\n> but we could omit applying the last patch if it causes too many merge conflicts\n> and trickl in the semantic patches over time when there are no merge conflicts.\n\nThe semantic patches are a good idea. At some point in the future, we \ncan submit a series that applies the patches to any leftover calls and \nremoves the old callers. We can hopefully rely on review (and the \nsemantic patches warning that there is work to do) to prevent new \ncallers from being introduced in future topics. That doesn't count \ntopics that come around while this one isn't merged down.\n\nI had one high-level question: How are we testing that these \"arbitrary \nrepository\" changes are safe? I just remember the issue we had with the \ncommit-graph code relying on arbitrary repositories, but then adding a \nreference to the replace objects broke the code (because replace-objects \nwasn't using arbitrary repos correctly, but the commit-graph was tested \nwith arbitrary repositories using \"test-tool repository\"). It would be \nnice to introduce more method calls in t/helper/test-repository.c that \nhelp us know these are safe conversions. Otherwise, we are essentially \nwaiting until we try to take submodule things in-process and find out \nwhat breaks. As we discovered with the refstore, we can't just ensure \nthat all references to the_repository are removed.\n\nThanks,\n-Stolee\n"},{"id":"360766","messageId":"CAGZ79kbpXQURMsZY15_k3rJ-dyH0i4qAGDv8umM8Hmx10ZdMMA@mail.gmail.com","threadId":"49583","inReplyTo":"feef12c7-e609-dc6e-104d-7381471afb9c@gmail.com","subject":"Re: [PATCH 00/19] Bring more repository handles into our code base","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-17T17:53:53Z","receivedAt":"2018-10-17T17:54:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 17, 2018 at 5:41 AM Derrick Stolee wrote:\n>\n> On 10/16/2018 7:35 PM, Stefan Beller wrote:\n> >\n> > This series takes another approach as it doesn't change the signature of\n> > functions, but introduces new functions that can deal with arbitrary\n> > repositories, keeping the old function signature around using a shallow wrapper.\n> I think this is a good direction, and the changes look good to me.\n>\n> > Additionally each patch adds a semantic patch, that would port from the old to\n> > the new function. These semantic patches are all applied in the very last patch,\n> > but we could omit applying the last patch if it causes too many merge conflicts\n> > and trickl in the semantic patches over time when there are no merge conflicts.\n>\n> The semantic patches are a good idea. At some point in the future, we\n> can submit a series that applies the patches to any leftover calls and\n> removes the old callers. We can hopefully rely on review (and the\n> semantic patches warning that there is work to do) to prevent new\n> callers from being introduced in future topics. That doesn't count\n> topics that come around while this one isn't merged down.\n\nFor those topics still in flight, I added re-defines, e.g.\n\n#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n#define get_merge_bases(r1, r2)\nrepo_get_merge_bases(the_repository, r1, r2)\n#endif\n\nso the base function still keeps working, and we can cleanup\nmultiple times, until eventually, we can get rid of the base function.\n\n> I had one high-level question: How are we testing that these \"arbitrary\n> repository\" changes are safe?\n\nI did the bare minimum in conversions in this series, such that the\nsubmodule code tests successfully. So if we'd revert some parts,\nthe submodule tests would break.\n\n\n\n> I just remember the issue we had with the\n> commit-graph code relying on arbitrary repositories, but then adding a\n> reference to the replace objects broke the code (because replace-objects\n> wasn't using arbitrary repos correctly, but the commit-graph was tested\n> with arbitrary repositories using \"test-tool repository\"). It would be\n> nice to introduce more method calls in t/helper/test-repository.c that\n> help us know these are safe conversions.\n\nOr instead we could accelerate the long term plan of removing a\nhard coded the_repository and have each cmd builtin take an additional\nrepository pointer from the init code, such that we'd bring all of Git to\nwork on arbitrary repositories. Then the standard test suite should be\nokay, as there is no special case for the_repository any more.\n\n> Otherwise, we are essentially\n> waiting until we try to take submodule things in-process and find out\n> what breaks. As we discovered with the refstore, we can't just ensure\n> that all references to the_repository are removed.\n\nYes, that is correct. We had a similar case with partial clone,\nas laid out in the cover letter for the RFC.\n\nI'll explore both the test tool approach as well as\nrepository-fication of the code base.\n\nThanks,\nStefan\n"},{"id":"360860","messageId":"20181018183758.81186-1-sbeller@google.com","threadId":"49583","inReplyTo":"CAGZ79kbpXQURMsZY15_k3rJ-dyH0i4qAGDv8umM8Hmx10ZdMMA@mail.gmail.com","subject":"[RFC PATCH 0/2] Bring the_repository into cmd_foo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T18:37:56Z","receivedAt":"2018-10-18T18:38:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> On Wed, Oct 17, 2018 at 5:41 AM Derrick Stolee wrote:\n>> I had one high-level question: How are we testing that these \"arbitrary\n>> repository\" changes are safe?\n> [...]\n> Or instead we could accelerate the long term plan of removing a\n> hard coded the_repository and have each cmd builtin take an additional\n> repository pointer from the init code, such that we'd bring all of Git to\n> work on arbitrary repositories. Then the standard test suite should be\n> okay, as there is no special case for the_repository any more.\n\nDemo'd in this RFC series for git-merge-base.\n\nThe core idea is found in patch 1,\nand the proof of concept is found in patch 2.\n\nWhat do you think?\n\nThanks,\nStefan\n\nStefan Beller (3):\n repository: have get_the_repository() to remove the_repository\n dependency\n builtin/merge-base.c: do not rely on the_repository any more\n\n builtin/merge-base.c | 67 ++++++++++++++++++++++++++-----------------\n repository.c | 10 +++++++\n repository.h | 13 ++++++++-\n t/t6010-merge-base.sh | 3 +-\n\n-- \n2.19.0\n\n"},{"id":"360861","messageId":"20181018183758.81186-2-sbeller@google.com","threadId":"49583","inReplyTo":"20181018183758.81186-1-sbeller@google.com","subject":"[RFC PATCH 1/2] repository: have get_the_repository() to remove the_repository dependency","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T18:37:57Z","receivedAt":"2018-10-18T18:38:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The struct 'the_repo' contains all data that of the main repository.\nAs we move more and more globals into this struct, the usual way of\naccessing these is using 'the_repository', which can be used as a drop in\nreplacement for accessing the migrated globals.\n\nHowever during the migration of globals into the repository object, it is\nnot clear if some code path rely on the_repository or can work on an\narbitrary repository (as we'd eventually want for submodules) due to the\nexcessive use of the_repository throughout the code base.\n\nTo address this, introduce a function 'get_the_repository(void)' which\nwill return the main repository and set the_repository to NULL when the\nenvironment variable GIT_NO_THE_REPOSITORY is set.\n\nThis function is to be strictly used only at the beginning of builtin\ncommand to assign it to a local repository pointer that we'll use to\npass through the code base.\n\nBy having the possibility to set the_repository to NULL, we'll get\na segfault when we try to access the_repository instead of using the\nhandle that we pass around.\n\nThis approach let's us have the_repository in the setup code, which\nin its current form is not yet able to transition into a world where\nthe repository handle is passed around and only test the passing around\nof the repository handle for later stage code.\n\nEventually in the future the setup code will produce the repository\nhandle and each 'cmd_foo(int argc, char **argv)' builtin would get the\nrepository via an additional parameter.\n\nSigned-off-by: Stefan Beller \n---\n repository.c | 10 ++++++++++\n repository.h | 13 ++++++++++++-\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/repository.c b/repository.c\nindex 5dd1486718..aaaababb88 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -20,6 +20,16 @@ void initialize_the_repository(void)\n \trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n }\n \n+struct repository *get_the_repository(void)\n+{\n+\tstruct repository *r = the_repository;\n+\n+\tif (getenv(\"GIT_NO_THE_REPOSITORY\"))\n+\t\tthe_repository = NULL;\n+\n+\treturn r;\n+}\n+\n static void expand_base_dir(char **out, const char *in,\n \t\t\t const char *base_dir, const char *def_in)\n {\ndiff --git a/repository.h b/repository.h\nindex 9f16c42c1e..26f5d64f68 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -114,13 +114,24 @@ void repo_set_gitdir(struct repository *repo, const char *root,\n \t\t const struct set_gitdir_args *extra_args);\n void repo_set_worktree(struct repository *repo, const char *path);\n void repo_set_hash_algo(struct repository *repo, int algo);\n-void initialize_the_repository(void);\n int repo_init(struct repository *r, const char *gitdir, const char *worktree);\n int repo_submodule_init(struct repository *submodule,\n \t\t\tstruct repository *superproject,\n \t\t\tconst char *path);\n void repo_clear(struct repository *repo);\n \n+/*\n+ * Initializes the repository 'the_repository', which is used in the transition\n+ * phase of moving globals into the repository struct.\n+ */\n+void initialize_the_repository(void);\n+\n+/*\n+ * To be called once; after the call use only returned repository, and do not\n+ * use the_repository any more\n+ */\n+struct repository *get_the_repository(void);\n+\n /*\n * Populates the repository's index from its index_file, an index struct will\n * be allocated if needed.\n-- \n2.19.0\n\n"},{"id":"360862","messageId":"20181018183758.81186-3-sbeller@google.com","threadId":"49583","inReplyTo":"20181018183758.81186-1-sbeller@google.com","subject":"[RFC PATCH 2/2 (BREAKS BUILD)] builtin/merge-base.c: do not rely on the_repository any more","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T18:37:58Z","receivedAt":"2018-10-18T18:38:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"To avoid creeping in the dependency of the_repository,\nuse GIT_NO_THE_REPOSITORY in the test to prove it still works.\n\nSigned-off-by: Stefan Beller \n---\n\nThis doesn't work yet, as we have not converted get_oid, yet.\nIt proves that GIT_NO_THE_REPOSITORY works, though.\n\nStefan\n\n builtin/merge-base.c | 67 ++++++++++++++++++++++++++-----------------\n t/t6010-merge-base.sh | 3 +-\n 2 files changed, 43 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 1c92099070..29341f8839 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -1,3 +1,4 @@\n+#define NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #include \"builtin.h\"\n #include \"cache.h\"\n #include \"config.h\"\n@@ -37,27 +38,28 @@ static const char * const merge_base_usage[] = {\n \tNULL\n };\n \n-static struct commit *get_commit_reference(const char *arg)\n+static struct commit *get_commit_reference(struct repository *r,\n+\t\t\t\t\t const char *arg)\n {\n \tstruct object_id revkey;\n-\tstruct commit *r;\n+\tstruct commit *ref;\n \n \tif (get_oid(arg, &revkey))\n \t\tdie(\"Not a valid object name %s\", arg);\n-\tr = lookup_commit_reference(the_repository, &revkey);\n-\tif (!r)\n+\tref = lookup_commit_reference(r, &revkey);\n+\tif (!ref)\n \t\tdie(\"Not a valid commit name %s\", arg);\n \n-\treturn r;\n+\treturn ref;\n }\n \n-static int handle_independent(int count, const char **args)\n+static int handle_independent(struct repository *r, int count, const char **args)\n {\n \tstruct commit_list *revs = NULL, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n-\t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n+\t\tcommit_list_insert(get_commit_reference(r, args[i]), &revs);\n \n \treduce_heads_replace(&revs);\n \n@@ -71,14 +73,16 @@ static int handle_independent(int count, const char **args)\n \treturn 0;\n }\n \n-static int handle_octopus(int count, const char **args, int show_all)\n+static int handle_octopus(struct repository *r,\n+\t\t\t int count, const char **args,\n+\t\t\t int show_all)\n {\n \tstruct commit_list *revs = NULL;\n \tstruct commit_list *result, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n-\t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n+\t\tcommit_list_insert(get_commit_reference(r, args[i]), &revs);\n \n \tresult = get_octopus_merge_bases(revs);\n \tfree_commit_list(revs);\n@@ -97,15 +101,15 @@ static int handle_octopus(int count, const char **args, int show_all)\n \treturn 0;\n }\n \n-static int handle_is_ancestor(int argc, const char **argv)\n+static int handle_is_ancestor(struct repository *r, int argc, const char **argv)\n {\n \tstruct commit *one, *two;\n \n \tif (argc != 2)\n \t\tdie(\"--is-ancestor takes exactly two commits\");\n-\tone = get_commit_reference(argv[0]);\n-\ttwo = get_commit_reference(argv[1]);\n-\tif (in_merge_bases(one, two))\n+\tone = get_commit_reference(r, argv[0]);\n+\ttwo = get_commit_reference(r, argv[1]);\n+\tif (repo_in_merge_bases(r, one, two))\n \t\treturn 0;\n \telse\n \t\treturn 1;\n@@ -116,19 +120,22 @@ struct rev_collect {\n \tint nr;\n \tint alloc;\n \tunsigned int initial : 1;\n+\tstruct repository *repo;\n };\n \n-static void add_one_commit(struct object_id *oid, struct rev_collect *revs)\n+static void add_one_commit(struct repository *r,\n+\t\t\t struct object_id *oid,\n+\t\t\t struct rev_collect *revs)\n {\n \tstruct commit *commit;\n \n \tif (is_null_oid(oid))\n \t\treturn;\n \n-\tcommit = lookup_commit(the_repository, oid);\n+\tcommit = lookup_commit(r, oid);\n \tif (!commit ||\n \t (commit->object.flags & TMP_MARK) ||\n-\t parse_commit(commit))\n+\t repo_parse_commit(r, commit))\n \t\treturn;\n \n \tALLOC_GROW(revs->commit, revs->nr + 1, revs->alloc);\n@@ -144,13 +151,13 @@ static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid\n \n \tif (revs->initial) {\n \t\trevs->initial = 0;\n-\t\tadd_one_commit(ooid, revs);\n+\t\tadd_one_commit(revs->repo, ooid, revs);\n \t}\n-\tadd_one_commit(noid, revs);\n+\tadd_one_commit(revs->repo, noid, revs);\n \treturn 0;\n }\n \n-static int handle_fork_point(int argc, const char **argv)\n+static int handle_fork_point(struct repository *r, int argc, const char **argv)\n {\n \tstruct object_id oid;\n \tchar *refname;\n@@ -173,13 +180,14 @@ static int handle_fork_point(int argc, const char **argv)\n \tif (get_oid(commitname, &oid))\n \t\tdie(\"Not a valid object name: '%s'\", commitname);\n \n-\tderived = lookup_commit_reference(the_repository, &oid);\n+\tderived = lookup_commit_reference(r, &oid);\n \tmemset(&revs, 0, sizeof(revs));\n \trevs.initial = 1;\n+\trevs.repo = r;\n \tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n \n \tif (!revs.nr && !get_oid(refname, &oid))\n-\t\tadd_one_commit(&oid, &revs);\n+\t\tadd_one_commit(r, &oid, &revs);\n \n \tfor (i = 0; i < revs.nr; i++)\n \t\trevs.commit[i]->object.flags &= ~TMP_MARK;\n@@ -217,6 +225,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \tint rev_nr = 0;\n \tint show_all = 0;\n \tint cmdmode = 0;\n+\tstruct repository *r;\n \n \tstruct option options[] = {\n \t\tOPT_BOOL('a', \"all\", &show_all, N_(\"output all common ancestors\")),\n@@ -234,27 +243,33 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, merge_base_usage, 0);\n \n+\t/*\n+\t * TODO: once the config machinery can cope without proper setup of\n+\t * the_repository, move this call up\n+\t */\n+\tr = get_the_repository();\n+\n \tif (cmdmode == 'a') {\n \t\tif (argc < 2)\n \t\t\tusage_with_options(merge_base_usage, options);\n \t\tif (show_all)\n \t\t\tdie(\"--is-ancestor cannot be used with --all\");\n-\t\treturn handle_is_ancestor(argc, argv);\n+\t\treturn handle_is_ancestor(r, argc, argv);\n \t}\n \n \tif (cmdmode == 'r' && show_all)\n \t\tdie(\"--independent cannot be used with --all\");\n \n \tif (cmdmode == 'o')\n-\t\treturn handle_octopus(argc, argv, show_all);\n+\t\treturn handle_octopus(r, argc, argv, show_all);\n \n \tif (cmdmode == 'r')\n-\t\treturn handle_independent(argc, argv);\n+\t\treturn handle_independent(r, argc, argv);\n \n \tif (cmdmode == 'f') {\n \t\tif (argc < 1 || 2 < argc)\n \t\t\tusage_with_options(merge_base_usage, options);\n-\t\treturn handle_fork_point(argc, argv);\n+\t\treturn handle_fork_point(r, argc, argv);\n \t}\n \n \tif (argc < 2)\n@@ -262,6 +277,6 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)\n \n \tALLOC_ARRAY(rev, argc);\n \twhile (argc-- > 0)\n-\t\trev[rev_nr++] = get_commit_reference(*argv++);\n+\t\trev[rev_nr++] = get_commit_reference(r, *argv++);\n \treturn show_merge_base(rev, rev_nr, show_all);\n }\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 44c726ea39..ae1c85ee8d 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -15,7 +15,8 @@ GIT_COMMITTER_EMAIL=git@comm.iter.xz\n GIT_COMMITTER_NAME='C O Mmiter'\n GIT_AUTHOR_NAME='A U Thor'\n GIT_AUTHOR_EMAIL=git@au.thor.xz\n-export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n+GIT_NO_THE_REPOSITORY=1\n+export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_NO_THE_REPOSITORY\n \n doit () {\n \tOFFSET=$1 &&\n-- \n2.19.0\n\n"},{"id":"360877","messageId":"20181018210119.149727-1-jonathantanmy@google.com","threadId":"49583","inReplyTo":"20181018183758.81186-1-sbeller@google.com","subject":"Re: [RFC PATCH 0/2] Bring the_repository into cmd_foo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T21:01:19Z","receivedAt":"2018-10-18T21:01:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Or instead we could accelerate the long term plan of removing a\n> > hard coded the_repository and have each cmd builtin take an additional\n> > repository pointer from the init code, such that we'd bring all of Git to\n> > work on arbitrary repositories. Then the standard test suite should be\n> > okay, as there is no special case for the_repository any more.\n> \n> Demo'd in this RFC series for git-merge-base.\n> \n> The core idea is found in patch 1,\n> and the proof of concept is found in patch 2.\n\nI don't think working around the_repository is sufficient, as there are\nother ways to access the same repository state (the_index, directly\naccessing files through file I/O). Instead I would prefer a test like in\nt/test-repository.c - each patch set would probably only need one test\nfor the last function converted, since typically the last function uses\nevery other function converted.\n\nAlso, even if we decided that working around the_repository is\nsufficient, I don't think this get_the_repository() is a good approach.\nIf (or when) we decide to convert all builtins to not use\nthe_repository, we would have to clean up all such calls.\n\nBetter would be to pass the_repository from the invoker of the cmd\nbuiltin, and reuse NO_THE_REPOSITORY_COMPATIBILITY_MACROS in the\nbuiltin. (I haven't thought much about how to transition to this, but\none way might be to extend \"struct cmd_struct\" in git.c to also have a\nnew-style function pointer, and #define GIT_CMD(c, f, o) {c, NULL, o, f}\nor something like that.)\n\nThis doesn't directly address the fact that the builtin might call lib\ncode that indirectly references the_repository, but I think that this\nwon't be an issue because by the time we're ready to convert builtins to\nnot use the_repository, most if not all of the lib code would have\nNO_THE_REPOSITORY_COMPATIBILITY_MACROS defined anyway.\n"},{"id":"360883","messageId":"CAGZ79kZ8ER_EkuAySzvybg1Y35MtjHPEU-AS9zhCivw==+RyiA@mail.gmail.com","threadId":"49583","inReplyTo":"20181018210119.149727-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/2] Bring the_repository into cmd_foo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-18T23:23:23Z","receivedAt":"2018-10-18T23:23:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 18, 2018 at 2:01 PM Jonathan Tan wrote:\n>\n> > > Or instead we could accelerate the long term plan of removing a\n> > > hard coded the_repository and have each cmd builtin take an additional\n> > > repository pointer from the init code, such that we'd bring all of Git to\n> > > work on arbitrary repositories. Then the standard test suite should be\n> > > okay, as there is no special case for the_repository any more.\n> >\n> > Demo'd in this RFC series for git-merge-base.\n> >\n> > The core idea is found in patch 1,\n> > and the proof of concept is found in patch 2.\n>\n> I don't think working around the_repository is sufficient, as there are\n> other ways to access the same repository state (the_index, directly\n> accessing files through file I/O).\n\nSure, but that would stick out like a sore thumb?\n\n> Instead I would prefer a test like in\n> t/test-repository.c - each patch set would probably only need one test\n> for the last function converted, since typically the last function uses\n> every other function converted.\n\nI'll look into that.\n\n>\n> Also, even if we decided that working around the_repository is\n> sufficient, I don't think this get_the_repository() is a good approach.\n> If (or when) we decide to convert all builtins to not use\n> the_repository, we would have to clean up all such calls.\n\nJust like we have to cleanup the calls to the_repository or the_index\nin general now (c.f. nd/the-index)\n\n> Better would be to pass the_repository from the invoker of the cmd\n> builtin, and reuse NO_THE_REPOSITORY_COMPATIBILITY_MACROS in the\n> builtin. (I haven't thought much about how to transition to this, but\n> one way might be to extend \"struct cmd_struct\" in git.c to also have a\n> new-style function pointer, and #define GIT_CMD(c, f, o) {c, NULL, o, f}\n> or something like that.)\n\nThis sounds like the next step to me.\n\n> This doesn't directly address the fact that the builtin might call lib\n> code that indirectly references the_repository, but I think that this\n> won't be an issue because by the time we're ready to convert builtins to\n> not use the_repository, most if not all of the lib code would have\n> NO_THE_REPOSITORY_COMPATIBILITY_MACROS defined anyway.\n\nAnd until then we double up on tests, one time the regular end-to-end tests\nand additional tests for repository agnostic units in test-repository.c ?\n\nThe whole point of this approach is to keep the testing at the level\nthat we currently have and make the tests more powerful in doubling\nfor both (a) testing existing behavior, (b) getting fairly good coverage\nof repository-fication of the code base by these 2 simple knobs.\n"},{"id":"360903","messageId":"xmqq4ldi1jti.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"Re: [PATCH 00/19] Bring more repository handles into our code base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T07:23:21Z","receivedAt":"2018-10-19T07:23:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This rerolls sb/more-repo-in-api.\n> It applies on nd/the-index merged with ds/reachable and is available via\n> git fetch https://github.com/stefanbeller/git object-store-final-3\n\nThanks. Luckily we have both of these prerequisites in 'master'\nnow, o hopefully this can be applied to 'master' directly and would\nplay well when merged to 'next' and to 'pu'.\n\nWill queue and play with it a bit before sending comments.\n"},{"id":"360979","messageId":"20181019203750.110741-1-jonathantanmy@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-19-sbeller@google.com","subject":"Re: [PATCH 18/19] submodule: use submodule repos for object lookup","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-19T20:37:50Z","receivedAt":"2018-10-19T20:37:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This converts the 'show_submodule_header' function to use\n> the repository API properly, such that the submodule objects\n> are not added to the main object store.\n\nThere is also a side effect in that the submodule now needs to pass all\nthe checks done by repo_init() instead of merely having the \"objects/\"\ndirectory. Can you add information about this to the commit message?\n\nAlso, which tests exercise this functionality? Mention them in the\ncommit message.\n\n> +/*\n> + * Initialize 'out' based on the provided submodule path.\n> + *\n> + * Unlike repo_submodule_init, this tolerates submodules not present\n> + * in .gitmodules. This function exists only to preserve historical behavior,\n> + *\n> + * Returns 0 on success, -1 when the submodule is not present.\n> + */\n> +static int open_submodule(struct repository *out, const char *path)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tout->submodule_prefix = xstrdup(path);\n> +\tout->submodule_prefix = xstrfmt(\"%s%s/\",\n> +\t\t\t\t\tthe_repository->submodule_prefix ?\n> +\t\t\t\t\tthe_repository->submodule_prefix :\n> +\t\t\t\t\t\"\", path);\n\nYou seem to say here [1] that we don't need submodule_prefix, but you're\ninstead setting it twice? :-)\n\n[1] https://public-inbox.org/git/CAGZ79kZTxoNMUyX+EHg0q3Ss2M-Etkf0yiw3E40U3VCt4QMwrQ@mail.gmail.com/\n\n> +/*\n> + * Helper function to display the submodule header line prior to the full\n> + * summary output.\n> + *\n> + * If it can locate the submodule git directory it will create a repository\n> + * handle for the submodule and lookup both the left and right commits and\n> + * put them into the left and right pointers.\n> */\n> -static void show_submodule_header(struct diff_options *o, const char *path,\n> +static void show_submodule_header(struct diff_options *o,\n> +\t\tconst char *path,\n> \t\tstruct object_id *one, struct object_id *two,\n> \t\tunsigned dirty_submodule,\n> +\t\tstruct repository *sub,\n> \t\tstruct commit **left, struct commit **right,\n> \t\tstruct commit_list **merge_bases)\n> {\n\nLocation of the submodule git directory is done by the caller of this\nfunction, not by this function. Also this function doesn't create any\nrepository handles. The documentation probably needs to be updated. Also\nmention what happens if sub is NULL.\n\n> @@ -563,16 +596,20 @@ void show_submodule_summary(struct diff_options *o, const char *path,\n> \tstruct rev_info rev;\n> \tstruct commit *left = NULL, *right = NULL;\n> \tstruct commit_list *merge_bases = NULL;\n> +\tstruct repository subrepo, *sub = &subrepo;\n> +\n> +\tif (open_submodule(&subrepo, path) < 0)\n> +\t\tsub = NULL;\n\nHandling of the subrepo and *sub seems clumsy - it might be better to\njust let open_submodule() return a struct repository pointer.\n\nPrevious 17 patches look good - most are the same as the previous\nversion, which I already was happy with, and I see that the patch I\nrequested in [2] was added.\n\n[2] https://public-inbox.org/git/20181011221114.186377-1-jonathantanmy@google.com/\n"},{"id":"360980","messageId":"20181019203937.111225-1-jonathantanmy@google.com","threadId":"49583","inReplyTo":"20181016233550.251311-20-sbeller@google.com","subject":"Re: [PATCH 19/19] submodule: don't add submodule as odb for push","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-19T20:39:37Z","receivedAt":"2018-10-19T20:39:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> In push_submodule(), because we do not actually need access to objects\n> in the submodule, do not invoke add_submodule_odb().\n> (for_each_remote_ref_submodule() does not require access to those\n> objects, and the actual push is done by spawning another process,\n> which handles object access by itself.)\n\nThe code looks good - my analysis is the same as that in my review of\nthe previous version [1].\n\nCan you mention, in the commit message, the tests that exercise the\nfunctionality here (and say that they still pass)?\n\n[1] https://public-inbox.org/git/20181011230028.200488-1-jonathantanmy@google.com/\n> \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 3 ---\n> 1 file changed, 3 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 7305ae2e10..e623e6bf7f 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1024,9 +1024,6 @@ static int push_submodule(const char *path,\n> \t\t\t const struct string_list *push_options,\n> \t\t\t int dry_run)\n> {\n> -\tif (add_submodule_odb(path))\n> -\t\treturn 1;\n> -\n> \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> \t\targv_array_push(&cp.args, \"push\");\n> -- \n> 2.19.0\n> \n"},{"id":"361172","messageId":"20181022173935.GG30222@szeder.dev","threadId":"49583","inReplyTo":"20181016233550.251311-1-sbeller@google.com","subject":"New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T17:39:35Z","receivedAt":"2018-10-22T17:47:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:\n> the last patch (applying the semantic patches) has been omitted as that\n> would produce a lot of merge conflicts. Without that patch, this merges\n> cleanly to next.\n> \n> As for when to apply the semantic patches, I wondered if we would prefer a dirty merge\n> (created via \"make coccicheck && git apply contrib/coccinelle/the_repository.cocci.patch\")\n> of the semantic patches, or if we'd actually trickle in the changes over some time?\n\n> This series takes another approach as it doesn't change the signature of\n> functions, but introduces new functions that can deal with arbitrary \n> repositories, keeping the old function signature around using a shallow wrapper.\n> \n> Additionally each patch adds a semantic patch, that would port from the old to\n> the new function. These semantic patches are all applied in the very last patch,\n> but we could omit applying the last patch if it causes too many merge conflicts\n> and trickl in the semantic patches over time when there are no merge conflicts.\n\nI don't really like how this or the previous RFC patch series deal\nwith semantic patches (or how some past patch series dealt with them,\nfor that matter), for various reasons:\n\n - Applying the transformations from several semantic patches in one\n single patch makes it harder to review it, because we won't know\n which change came from which semantic patch.\n \n For comparison, see the patch series adding hasheq()/oideq(),\n merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in\n particular the four \"convert to \" patches.\n\n - 'make coccicheck' won't run clean (and the static analysis build\n job on Travis CI will fail) for all commits following adding the\n new semantic patches but before applying the resulting\n transformations.\n\n - These semantic patches interact badly with 'pu' and 'next',\n because those integration branches can contain topic branches\n adding new code that should be transformed by these semanic\n patches. Consequently, 'make coccicheck' won't run clean and the\n static analysis build job will fail until all those topics reach\n 'master', and the remaining transformations are applied on top.\n \n This was (and still is!) an issue with the hasheq()/oideq() series\n as well: that series was added on 2018-08-28, and the static\n analysis build job is red on 'pu' ever since. See the follow-up\n patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and\n one more follow-up will be necessary after the builtin stash topic\n is merged to 'master'.\n\n This makes it harder to review other patch series.\n\n - Is it really necessary to carry these semantic patches in-tree?\n Let me ellaborate. There are basically two main use cases for\n semantic patches:\n\n - To avoid undesirable code patterns, e.g. we should not use\n sha1_to_hex(oid.hash) or strbuf_addf(&sb, \"fixed string\"), but\n use oid_to_hex(&oid) or strbuf_addstr(&sb, \"fixed string\")\n instead. Note that in these cases we don't remove the\n functions sha1_to_hex() or strbuf_addf(), because there are\n good reasons to use them in other scenarios.\n\n Our semantic patches under 'contrib/coccinelle/' fall into\n this category, and we have 'make coccicheck' and the static\n analysis build job on Travis CI to catch these undesirable\n code patterns preferably early, and to prevent them from\n entering our codebase.\n\n - To perform one-off code transformations, e.g. to modify a\n function's name and/or signature and convert all its\n callsites; see e.g. commits abef9020e3 (sha1_file: convert\n sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e\n (sha1_file: convert read_sha1_file to struct object_id,\n 2018-03-12).\n\n As far as I understand this patch series falls into this\n category: once the conversion is complete the old functions\n will be removed. After that there will be no use for these\n semanic patches.\n\n Having said that, it's certainly easier to double-check the\n resulting transformations when one can apply the semantic\n patches locally, and doing so is easier when the semantic\n patches are in tree than when they must be copy-pasted from a\n commit message.\n\nOK, that was already long. Now, can we do better?\n\nHow about introducing the concept of \"pending\" semantic patches,\nstored in 'contrib/coccinelle/.pending.cocci' files, modifying\n'make coccicheck' to skip them, and adding the new 'make\ncoccicheck-pending' target to make it convenient to apply them, e.g.\nsomething like the simple patch at the end.\n\nSo the process would go something like this:\n\n - A new semantic patch should be added as \"pending\", e.g. to the\n file 'the_repository.pending.cocci', together with the resulting\n transformations in the same commit.\n\n This way neither 'make coccicheck' nor the static analysis build\n job would complain in the topic branch or in the two integration\n branches. And if they do complain, then we would know right away\n that they complain because of a well-established semantic patch.\n Yet, anyone interested could run 'make coccicheck-pending' to see\n where are we heading.\n\n - The author of the \"pending\" semanting patch should then keep an\n eye on already cooking topics: whether any of them contain new\n code that should be transformed, and how they progress to\n 'master', and sending followup patch(es) with the remaining\n transformations when applicable.\n \n Futhermore, the author should also pay attention to any new topics\n that branch off after the \"pending\" semantic patch, and whether\n any of them introduce code to be transformed, warning their\n authors as necessary.\n\n - Finally, after all the dust settled, the dev should follow up with\n a patch to:\n \n - promote the \"penging\" patch to '.cocci', if its purpose\n is to avoid undesirable code patterns in the future, or\n \n - remove the semantic patch, if it was used in a one-off\n transformation.\n\nThoughts?\n\n\n -- >8 --\n\nSubject: [PATCH] coccinelle: add support for \"pending\" semantic patches\n\n(should add a 'contrib/coccinelle/README' to explain what\n'.pending.cocci' means)\n\n---\n Makefile | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex d18ab0fe78..67a4441f39 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2728,9 +2728,11 @@ endif\n \tthen \\\n \t\techo ' ' SPATCH result: $@; \\\n \tfi\n-coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci))\n+coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))\n \n-.PHONY: coccicheck\n+coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))\n+\n+.PHONY: coccicheck coccicheck-pending\n \n ### Installation rules\n \n-- \n2.19.1.681.g6bd79da3f5\n\n\n\n"},{"id":"361176","messageId":"CAGZ79kboyakR=dARH60ZJmyN=kT+-X0SSDjgz3aswpkUd+bZmg@mail.gmail.com","threadId":"49583","inReplyTo":"20181022173935.GG30222@szeder.dev","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-22T18:54:06Z","receivedAt":"2018-10-22T18:54:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 22, 2018 at 10:39 AM SZEDER Gábor wrote:\n>\n> On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:\n> > the last patch (applying the semantic patches) has been omitted as that\n> > would produce a lot of merge conflicts. Without that patch, this merges\n> > cleanly to next.\n> >\n> > As for when to apply the semantic patches, I wondered if we would prefer a dirty merge\n> > (created via \"make coccicheck && git apply contrib/coccinelle/the_repository.cocci.patch\")\n> > of the semantic patches, or if we'd actually trickle in the changes over some time?\n>\n> > This series takes another approach as it doesn't change the signature of\n> > functions, but introduces new functions that can deal with arbitrary\n> > repositories, keeping the old function signature around using a shallow wrapper.\n> >\n> > Additionally each patch adds a semantic patch, that would port from the old to\n> > the new function. These semantic patches are all applied in the very last patch,\n> > but we could omit applying the last patch if it causes too many merge conflicts\n> > and trickl in the semantic patches over time when there are no merge conflicts.\n>\n> I don't really like how this or the previous RFC patch series deal\n> with semantic patches (or how some past patch series dealt with them,\n> for that matter), for various reasons:\n>\n> - Applying the transformations from several semantic patches in one\n> single patch makes it harder to review it, because we won't know\n> which change came from which semantic patch.\n\nGood point, so to improve the series sb/more-repo-in-api, I could\nsend the application of the semantic patch just after each patch\nthat adds another semantic patching rule? I personally dislike\napplying the semantic patch in the same patch as where the\nsemantic rule was introduced, as then the mechanical conversions\n(from the semantic patch) drown out reviewers attention to the\nmanual changes.\n\n> For comparison, see the patch series adding hasheq()/oideq(),\n> merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in\n> particular the four \"convert to \" patches.\n\nThe four patches \"convert to only contain the semantic\npatch as well as its effects, the manual changes are separated out\nto later patches, which is quite nice.\n\n> - 'make coccicheck' won't run clean (and the static analysis build\n> job on Travis CI will fail) for all commits following adding the\n> new semantic patches but before applying the resulting\n> transformations.\n>\n> - These semantic patches interact badly with 'pu' and 'next',\n> because those integration branches can contain topic branches\n> adding new code that should be transformed by these semanic\n> patches.\n\nAnd I thought of this as a feature. There is no merge conflict and it\nstill compiles, which makes Junios work easy.\n\nOf course it put the load elsewhere. :/\n\nFor the sake of a good history, I would think running 'make coccicheck'\nand applying the resulting patches would be best as part of the (dirty)\nmerge of any topic that proposes new semantic patches, but that would\nadd load to Junio as it would be an extra step during the merge.\n\nOne could argue that the step of applying such transformations into\nthe dirty merge is cheaper than resolving merge conflicts that are\nhad when the topic includes the transformation.\n\n> Consequently, 'make coccicheck' won't run clean and the\n> static analysis build job will fail until all those topics reach\n> 'master', and the remaining transformations are applied on top.\n>\n> This was (and still is!) an issue with the hasheq()/oideq() series\n> as well: that series was added on 2018-08-28, and the static\n> analysis build job is red on 'pu' ever since. See the follow-up\n> patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and\n> one more follow-up will be necessary after the builtin stash topic\n> is merged to 'master'.\n\nIn my understanding this follow up is a feature, as it helps to avoid\nmerge conflicts with other topics in flight.\n\n> This makes it harder to review other patch series.\n\nas 'make coccicheck' is an integral part of your review?\n\n> - Is it really necessary to carry these semantic patches in-tree?\n> Let me ellaborate. There are basically two main use cases for\n> semantic patches:\n>\n> - To avoid undesirable code patterns, e.g. we should not use\n> sha1_to_hex(oid.hash) or strbuf_addf(&sb, \"fixed string\"), but\n> use oid_to_hex(&oid) or strbuf_addstr(&sb, \"fixed string\")\n> instead. Note that in these cases we don't remove the\n> functions sha1_to_hex() or strbuf_addf(), because there are\n> good reasons to use them in other scenarios.\n>\n> Our semantic patches under 'contrib/coccinelle/' fall into\n> this category, and we have 'make coccicheck' and the static\n> analysis build job on Travis CI to catch these undesirable\n> code patterns preferably early, and to prevent them from\n> entering our codebase.\n>\n> - To perform one-off code transformations, e.g. to modify a\n> function's name and/or signature and convert all its\n> callsites; see e.g. commits abef9020e3 (sha1_file: convert\n> sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e\n> (sha1_file: convert read_sha1_file to struct object_id,\n> 2018-03-12).\n>\n> As far as I understand this patch series falls into this\n> category: once the conversion is complete the old functions\n> will be removed. After that there will be no use for these\n> semanic patches.\n>\n> Having said that, it's certainly easier to double-check the\n> resulting transformations when one can apply the semantic\n> patches locally, and doing so is easier when the semantic\n> patches are in tree than when they must be copy-pasted from a\n> commit message.\n>\n> OK, that was already long. Now, can we do better?\n>\n> How about introducing the concept of \"pending\" semantic patches,\n> stored in 'contrib/coccinelle/.pending.cocci' files, modifying\n> 'make coccicheck' to skip them, and adding the new 'make\n> coccicheck-pending' target to make it convenient to apply them, e.g.\n> something like the simple patch at the end.\n>\n> So the process would go something like this:\n>\n> - A new semantic patch should be added as \"pending\", e.g. to the\n> file 'the_repository.pending.cocci', together with the resulting\n> transformations in the same commit.\n>\n> This way neither 'make coccicheck' nor the static analysis build\n> job would complain in the topic branch or in the two integration\n> branches. And if they do complain, then we would know right away\n> that they complain because of a well-established semantic patch.\n> Yet, anyone interested could run 'make coccicheck-pending' to see\n> where are we heading.\n>\n> - The author of the \"pending\" semanting patch should then keep an\n> eye on already cooking topics: whether any of them contain new\n> code that should be transformed, and how they progress to\n> 'master', and sending followup patch(es) with the remaining\n> transformations when applicable.\n>\n> Futhermore, the author should also pay attention to any new topics\n> that branch off after the \"pending\" semantic patch, and whether\n> any of them introduce code to be transformed, warning their\n> authors as necessary.\n>\n> - Finally, after all the dust settled, the dev should follow up with\n> a patch to:\n>\n> - promote the \"penging\" patch to '.cocci', if its purpose\n> is to avoid undesirable code patterns in the future, or\n>\n> - remove the semantic patch, if it was used in a one-off\n> transformation.\n>\n> Thoughts?\n\nI like the approach of having separate classes of semantic patches:\n(a) the regular \"we need to keep checking these\" as they address\n undesirable code patterns, which is what we currently have,\n and what 'make coccicheck' would complain about.\n(b) The pending patches as you propose. However I would\n argue that we'd not want to include the transformation into\n the same patch as then the patch will have merge conflicts.\n Ideally we'd have an automated process/bot that would apply\n all pending semantic patches onto master and then checks for\n conflicts in HEAD..pu, and only sends off the non-conflicting\n diffs as a topic.\n Then after a couple integration cycles we'd have all pending\n changes in, with no conflicts on Junios side.\n\nSo I think we should add a patch like you post, but we would\nneed to discuss the exact approach how to deal with pending\npatches. Is it the original dev who should push forward on their\nown pending patches, or does it become a pooled effort?\n\nThanks for starting this discussion!\nStefan\n"},{"id":"361225","messageId":"xmqqzhv5tx4m.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"20181022173935.GG30222@szeder.dev","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T22:49:45Z","receivedAt":"2018-10-22T22:49:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> I don't really like how this or the previous RFC patch series deal\n> with semantic patches (or how some past patch series dealt with them,\n> for that matter), for various reasons:\n> ...\n> How about introducing the concept of \"pending\" semantic patches,\n> stored in 'contrib/coccinelle/.pending.cocci' files, modifying\n> 'make coccicheck' to skip them, and adding the new 'make\n> coccicheck-pending' target to make it convenient to apply them, e.g.\n> something like the simple patch at the end.\n>\n> So the process would go something like this:\n>\n> - A new semantic patch should be added as \"pending\", e.g. to the\n> file 'the_repository.pending.cocci', together with the resulting\n> transformations in the same commit.\n>\n> This way neither 'make coccicheck' nor the static analysis build\n> job would complain in the topic branch or in the two integration\n> branches. And if they do complain, then we would know right away\n> that they complain because of a well-established semantic patch.\n> Yet, anyone interested could run 'make coccicheck-pending' to see\n> where are we heading.\n>\n> - The author of the \"pending\" semanting patch should then keep an\n> eye on already cooking topics: whether any of them contain new\n> code that should be transformed, and how they progress to\n> 'master', and sending followup patch(es) with the remaining\n> transformations when applicable.\n> \n> Futhermore, the author should also pay attention to any new topics\n> that branch off after the \"pending\" semantic patch, and whether\n> any of them introduce code to be transformed, warning their\n> authors as necessary.\n>\n> - Finally, after all the dust settled, the dev should follow up with\n> a patch to:\n> \n> - promote the \"penging\" patch to '.cocci', if its purpose\n> is to avoid undesirable code patterns in the future, or\n> \n> - remove the semantic patch, if it was used in a one-off\n> transformation.\n> ...\n> Thoughts?\n\nI actually think this round does a far nicer job playing well with\nother topics than any earlier series. The pain you are observing I\nthink come primarily from my not making the best use of these\npatches.\n\nSteppng back a bit, I'd imagine in an ideal world where \"make\ncoccicheck\" can be done instantaneously _and_ the spatch machinery\nis just as reliable as C compilers. In such a world, I may rename\nall the *.c files in my tree to *.c.in, make the very first step of\nthe \"make all\" to run coccicheck and transform *.c.in to *.c before\nstarting the compilation. There is no need to have changes to\n*.c.in that spatch would fix. Such an idealized set-up has a few\nnice property.\n\n - Mechanical conflict resolutions between topics in flight and a\n series that modifies or adds new .cocci rules would greatly be\n reduced.\n\n - Still, *.c files that are \"compiled\" from *.c.in file used by\n each build will by definition cocci-clean.\n\n - Bugs like the one that required 6afedba8 (\"object_id.cocci: match\n only expressions of type 'struct object_id'\", 2018-10-15) are\n still possible, but some of them may be caught by C compilers\n that inspect the result of spatch compilation from *.c.in to *.c\n\nNow we do not live in that ideal world and there is no separate\n\"turn *.c.in into *.c\" step in our build procedure. In such a\n\"real\" world, if we had a rule like \"each individual commit must\npass 'make coccicheck' cleanly\", anybody who does such a merge and\nresolve huge conflics would essentially be wasting time on something\nthat the tool could do, and then the result of the merge would\nfurther need to be amended for semantic conflicts (i.e. the other\nbranch may have introduced new instances of old pattern .cocci\npatches in our branch would want to transform)). By not insisting\non cocci-cleanness at each commit level, we can gain (or at least\nsumulate gaining) some benefit that we would reap in the ideal\nworld, and Stefan's latest series already does so for the former.\nIf we insisted that these patches must be accompanied with the\nresult of the cocci transformations, such a series will have zero\nchance of landing in 'pu', unless we freeze the world.\n\nWhat I _could_ do (and what I did do only for one round of pushing\nout 'pu') is to queue a coccinelle transformation at the tip of\nintegration branches. If \"make coccicheck\" ran in subsecond, I\ncould even automate it in the script that is used to rebuild 'pu'\nevery day, so that after merging each and every topic, do the \"make\ncoccicheck\" and apply resulting .cocci.patch files and squash that\ninto the merge commit.\n\nBut with the current \"make coccicheck\" performance, that is not\nrealistic.\n\nI am wondering if it is feasible to do it at the tip of 'pu' (which\nis rebuilt two to three times a day), 'next' (which is updated once\nor twice a week) and 'master'.\n\nI find that your \"pending\" idea may be nicer, as it distributes the\nload. Whoever wants to change the world order by updating the .cocci\nrules is primarily responsible for making it happen without breaking\nthe world during the transition. That's more scalable.\n\nBut at some point in the ideal future, it may turn out that it is\nmaking the process scalable by distributing a task that does not\nhave to be done by any human, not by the integrator or by individual\ndeveloper---but the new compilation step from *.c.in to *.c should\nbe able to do. We obviously are not there yet, so...\n"},{"id":"361235","messageId":"CAGZ79kaWaY+oxJfoMbsCcq6MZDPoq1OgOwK0a9mkKR7sUEOHpg@mail.gmail.com","threadId":"49583","inReplyTo":"xmqqzhv5tx4m.fsf@gitster-ct.c.googlers.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-23T00:26:40Z","receivedAt":"2018-10-23T00:26:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Stepping back a bit, I'd imagine in an ideal world where \"make\n> coccicheck\" can be done instantaneously _and_ the spatch machinery\n> is just as reliable as C compilers.\n> [...]\n> Now we do not live in that ideal world and [...]\n> such a series will have zero\n> chance of landing in 'pu', unless we freeze the world.\n\nI wonder if we could approximate the ideal world with\nrerere+spatch a bit more:\n\n1) I resend the series that includes \"apply cocci patches\"\n as the last patch and you queue it as usual\n\n2) The first time such a series is merged, you'd merge\n HEAD^ (i.e. excluding the \"apply the semantic patch)\n to pu instead. I view this as a yet-to-be invented mode\n '--theirs-is-stale-use-tree-instead=THEIRS~1^{tree}',\n then run spatch to reproduce the last patch into the\n dirty merge (which has pu and the last patch as parent)\n\n This step is done to 'pre-heat' the rerere cache.\n\n3) Any further integration (e.g. rebuilding pu) would\n benefit from the hot rerere cache and very little work\n is actually required as the conflicts are resolved by rerere.\n\nAm I overestimating or misunderstanding rerere here?\n\n> What I _could_ do (and what I did do only for one round of pushing\n> out 'pu') is to queue a coccinelle transformation at the tip of\n> integration branches. If \"make coccicheck\" ran in subsecond, I\n> could even automate it in the script that is used to rebuild 'pu'\n> every day, so that after merging each and every topic, do the \"make\n> coccicheck\" and apply resulting .cocci.patch files and squash that\n> into the merge commit.\n>\n> But with the current \"make coccicheck\" performance, that is not\n> realistic.\n\nWould it be realistic for next and master branch instead of pu?\n\nI'd be wary for the master branch, as we may not want to rely on\nspatch without review. (It can produce funny white space issues,\nbut seems to produce working/correct code)\n\n> I am wondering if it is feasible to do it at the tip of 'pu' (which\n> is rebuilt two to three times a day), 'next' (which is updated once\n> or twice a week) and 'master'.\n\nWe could even optimize that, by checking if contrib/cocci/ has\nchanges for the new tip of next/master respectively.\n\nAnother thing I wonder is if we care about the distinction between\nthe (a) pending changes as described by SZEDER, as we introduce\nthese deliberately, whereas (b) undesirable code patterns\n(e.g. free and null instead of FREE_AND_NULL macro) might be\ncaught and reported in pu/next and then someone learns from it.\nAutomatic rewriting the (b) cases seems not just as desirable as\n(a), where we do it purely to avoid resolving merge conflicts by\nhand.\n\n> I find that your \"pending\" idea may be nicer, as it distributes the\n> load. Whoever wants to change the world order by updating the .cocci\n> rules is primarily responsible for making it happen without breaking\n> the world during the transition. That's more scalable.\n\n... and I think SZEDER considers the current world broken as\n'make coccicheck' returns non-empty, so it sounds to me as if\nthe current transition is thought less-than-optimal.\n"},{"id":"361257","messageId":"xmqqva5ts32x.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"CAGZ79kaWaY+oxJfoMbsCcq6MZDPoq1OgOwK0a9mkKR7sUEOHpg@mail.gmail.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T04:24:06Z","receivedAt":"2018-10-23T04:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Am I overestimating or misunderstanding rerere here?\n\nYes.\n\n> Would it be realistic for next and master branch instead of pu?\n>\n> I'd be wary for the master branch, as we may not want to rely on\n> spatch without review. (It can produce funny white space issues,\n> but seems to produce working/correct code)\n\nYes, that is why I think it is a bit too late to do the \"fixup with\nspatch\" after merging to 'next' and 'master'.\n"},{"id":"361265","messageId":"xmqqefchroj4.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"xmqqzhv5tx4m.fsf@gitster-ct.c.googlers.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T09:38:23Z","receivedAt":"2018-10-23T09:38:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I actually think this round does a far nicer job playing well with\n> other topics than any earlier series. The pain you are observing I\n> think come primarily from my not making the best use of these\n> patches.\n>\n> Steppng back a bit, I'd imagine in an ideal world where \"make\n> coccicheck\" can be done instantaneously _and_ the spatch machinery\n> is just as reliable as C compilers....\n>\n> What I _could_ do (and what I did do only for one round of pushing\n> out 'pu') is to queue a coccinelle transformation at the tip of\n> integration branches. If \"make coccicheck\" ran in subsecond, I\n> could even automate it in the script that is used to rebuild 'pu'\n> every day, ...\n\nAnyway, even though \"make coccicheck\" does not run in subsecond,\nI've updated my machinery to rebuild the integration branches so\nthat I can optionally queue generated coccicheck patches, and what I\npushed out tonight has one at the tip of 'pu' and also another at\nthe tip of 'next'. The latter seems to be passing all archs and\nexecuting Windows run.\n\nThe tip of 'pu' has trouble with -Wunused on Apple around the\ndelta-islands series.\n\n\nnext: https://travis-ci.org/git/git/builds/444969406\npu: https://travis-ci.org/git/git/builds/444969342\n\n\n"},{"id":"361268","messageId":"CAPUEsphThzjDTrJJHwvbeP4w=6M=xHWz+DZStfXS62=5tSCbVQ@mail.gmail.com","threadId":"49583","inReplyTo":"xmqqefchroj4.fsf@gitster-ct.c.googlers.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-10-23T10:15:06Z","receivedAt":"2018-10-23T10:15:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote:\n>\n> The tip of 'pu' has trouble with -Wunused on Apple around the\n> delta-islands series.\n\nFWIW the \"problem\" is actually with -Wunused-function and is AFAIK not\nrelated to the semantic changes or Apple (AKA macOS)\n\nIndeed, I saw this issue before also in Linux (Fedora Rawhide) when\nusing the latest clang and presumed it was just expected because the\ntopic branch was most likely incomplete.\n\nCarlo\n"},{"id":"361271","messageId":"xmqqa7n5rmj3.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"CAPUEsphThzjDTrJJHwvbeP4w=6M=xHWz+DZStfXS62=5tSCbVQ@mail.gmail.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-23T10:21:36Z","receivedAt":"2018-10-23T10:21:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote:\n>>\n>> The tip of 'pu' has trouble with -Wunused on Apple around the\n>> delta-islands series.\n>\n> FWIW the \"problem\" is actually with -Wunused-function and is AFAIK not\n> related to the semantic changes or Apple (AKA macOS)\n\nOh, don't get me wrong. By Apple, I meant \"the versions of compiler\nused on the Apple build at TravisCI site\". I could have sent the\nabove two lines in a separate topic, as the issue does not have\nanything to do with \"new semantic patches\" discussion, but I thought\nit would be obvious to everybody who is reading the thread---it\nseems I thought wrong ;-)\n"},{"id":"361303","messageId":"CAGZ79kZO6tAUOjO4YCostLmogGSwMmxGHj7g1kM1fNJWqJoFUA@mail.gmail.com","threadId":"49583","inReplyTo":"xmqqefchroj4.fsf@gitster-ct.c.googlers.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-23T17:30:40Z","receivedAt":"2018-10-23T17:30:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 23, 2018 at 2:38 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > I actually think this round does a far nicer job playing well with\n> > other topics than any earlier series. The pain you are observing I\n> > think come primarily from my not making the best use of these\n> > patches.\n> >\n> > Steppng back a bit, I'd imagine in an ideal world where \"make\n> > coccicheck\" can be done instantaneously _and_ the spatch machinery\n> > is just as reliable as C compilers....\n> >\n> > What I _could_ do (and what I did do only for one round of pushing\n> > out 'pu') is to queue a coccinelle transformation at the tip of\n> > integration branches. If \"make coccicheck\" ran in subsecond, I\n> > could even automate it in the script that is used to rebuild 'pu'\n> > every day, ...\n>\n> Anyway, even though \"make coccicheck\" does not run in subsecond,\n> I've updated my machinery to rebuild the integration branches so\n> that I can optionally queue generated coccicheck patches, and what I\n> pushed out tonight has one at the tip of 'pu' and also another at\n> the tip of 'next'. The latter seems to be passing all archs and\n> executing Windows run.\n\nThat is pretty exciting!\n\nLooking at the commit in next, you also included the suggestion\nfrom [1] to use a postincrement instead of a preincrement and I got\nexcited to see how we express such a thing in coccinelle,\nbut it turns out that it slipped in unrelated to the coccinelle patches.\n\nHow would we go from here?\nIt is not obvious to me how such changes would be integrated,\nas regenerating them on top of pu will not help getting these changes\nmerged down, and applying the semantic patch on next (once\nsb/more-repo-in-api lands in next) would created the merge conflicts for\nall topics that are merged to next after that series.\n\n[1] https://public-inbox.org/git/xmqqin1wyxvz.fsf@gitster-ct.c.googlers.com/\n"},{"id":"361353","messageId":"xmqq5zxsrvef.fsf@gitster-ct.c.googlers.com","threadId":"49583","inReplyTo":"CAGZ79kZO6tAUOjO4YCostLmogGSwMmxGHj7g1kM1fNJWqJoFUA@mail.gmail.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T01:22:16Z","receivedAt":"2018-10-24T03:19:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Anyway, even though \"make coccicheck\" does not run in subsecond,\n>> I've updated my machinery to rebuild the integration branches so\n>> that I can optionally queue generated coccicheck patches, and what I\n>> pushed out tonight has one at the tip of 'pu' and also another at\n>> the tip of 'next'. The latter seems to be passing all archs and\n>> executing Windows run.\n>\n> That is pretty exciting!\n>\n> Looking at the commit in next, you also included the suggestion\n> from [1] to use a postincrement instead of a preincrement and I got\n> excited to see how we express such a thing in coccinelle,\n> but it turns out that it slipped in unrelated to the coccinelle patches.\n\nSee below, which was sitting in my working tree.\n\n> How would we go from here?\n> It is not obvious to me how such changes would be integrated,\n> as regenerating them on top of pu will not help getting these changes\n> merged down, and applying the semantic patch on next (once\n> sb/more-repo-in-api lands in next) would created the merge conflicts for\n> all topics that are merged to next after that series.\n\nConflicts with later topics is indeed worrysome. That is why I did\nit as an experiment. If it becomes too painful, I'd probably stop\ndoing it while merging to anything other than 'pu', and then we can\nfollow the more distributed approach along the lines of what Szeder\nsuggested, to see how smoothly it goes.\n\n-- >8 --\nSubject: [PATCH] cocci: simplify \"if (++u > 1)\" to \"if (u++)\"\n\nIt is more common to use post-increment than pre-increment when the\nside effect is the primary thing we want in our code and in C in\ngeneral (unlike C++).\n\nInitializing a variable to 0, incrementing it every time we do\nsomething, and checking if we have already done that thing to guard\nthe code to do that thing, is easier to understand when written\n\n\tif (u++)\n\t\t; /* we've done that! */\n\telse\n\t\tdo_it(); /* just once. */\n\nbut if you try to use pre-increment, you end up with a less natural\nlooking\n\n\tif (++u > 1)\n\nSigned-off-by: Junio C Hamano \n---\n contrib/coccinelle/preincr.cocci | 5 +++++\n 1 file changed, 5 insertions(+)\n create mode 100644 contrib/coccinelle/preincr.cocci\n\ndiff --git a/contrib/coccinelle/preincr.cocci b/contrib/coccinelle/preincr.cocci\nnew file mode 100644\nindex 0000000000..7fe1e8d2d9\n--- /dev/null\n+++ b/contrib/coccinelle/preincr.cocci\n@@ -0,0 +1,5 @@\n+@ preincrement @\n+identifier i;\n+@@\n+- ++i > 1\n++ i++\n-- \n2.19.1-542-gc4df23f792\n\n"},{"id":"361446","messageId":"20181025015910.GJ30222@szeder.dev","threadId":"49583","inReplyTo":"CAGZ79kboyakR=dARH60ZJmyN=kT+-X0SSDjgz3aswpkUd+bZmg@mail.gmail.com","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-25T01:59:10Z","receivedAt":"2018-10-25T01:59:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote:\n\n> For the sake of a good history, I would think running 'make coccicheck'\n> and applying the resulting patches would be best as part of the (dirty)\n> merge of any topic that proposes new semantic patches, but that would\n> add load to Junio as it would be an extra step during the merge.\n> \n> One could argue that the step of applying such transformations into\n> the dirty merge is cheaper than resolving merge conflicts that are\n> had when the topic includes the transformation.\n\nPlease consider that merge commits' have uglier diffs than regular\ncommits, and that merge commits cause additional complications when\n'git bisect' points the finger at them, both of which are exacerbated\nby additional changes squeezed into evil merges.\n\n> > Consequently, 'make coccicheck' won't run clean and the\n> > static analysis build job will fail until all those topics reach\n> > 'master', and the remaining transformations are applied on top.\n> >\n> > This was (and still is!) an issue with the hasheq()/oideq() series\n> > as well: that series was added on 2018-08-28, and the static\n> > analysis build job is red on 'pu' ever since. See the follow-up\n> > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and\n> > one more follow-up will be necessary after the builtin stash topic\n> > is merged to 'master'.\n> \n> In my understanding this follow up is a feature, as it helps to avoid\n> merge conflicts with other topics in flight.\n\nI don't see how such a follow up patch helps to avoid merge conflicts.\n\nThere were topics that branched off before the introduction of oideq()\ninto 'master', therefore they couldn't make use of this new function\nuntil they were merged to 'master' as well, so they added their own\n!oidcmp() calls. That follow up patch was necessary to transform\nthese new !oidcmp() calls after those topics reached 'master'. Merge\nconflicts had nothing to do with it.\n\nSo this follow up patch is not a feature, but rather an inherent\nconsequence of the project's branching model, with lots of parallel\nrunning topics branching off at different points and progressing at\ndifferent speeds.\n\n> > This makes it harder to review other patch series.\n> \n> as 'make coccicheck' is an integral part of your review?\n\nErm, right, \"review\" was not the right word here. Anyway, as it is,\n'make coccicheck' is an integral part of our automated tests, not only\non Travis CI but on the upcoming Azure thing as well. I just try to\npay attention to its results and the results of a bunch of my\nadditional builds, and complain or even send a fix when something goes\nreproducibly wrong. This has certainly became more cumbersome with\nthe permanently failing static analysis build job in the last couple\nof weeks.\n\n> > How about introducing the concept of \"pending\" semantic patches,\n> > stored in 'contrib/coccinelle/.pending.cocci' files, modifying\n> > 'make coccicheck' to skip them, and adding the new 'make\n> > coccicheck-pending' target to make it convenient to apply them, e.g.\n> > something like the simple patch at the end.\n> >\n> > So the process would go something like this:\n> >\n> > - A new semantic patch should be added as \"pending\", e.g. to the\n> > file 'the_repository.pending.cocci', together with the resulting\n> > transformations in the same commit.\n> >\n> > This way neither 'make coccicheck' nor the static analysis build\n> > job would complain in the topic branch or in the two integration\n> > branches. And if they do complain, then we would know right away\n> > that they complain because of a well-established semantic patch.\n> > Yet, anyone interested could run 'make coccicheck-pending' to see\n> > where are we heading.\n> >\n> > - The author of the \"pending\" semanting patch should then keep an\n> > eye on already cooking topics: whether any of them contain new\n> > code that should be transformed, and how they progress to\n> > 'master', and sending followup patch(es) with the remaining\n> > transformations when applicable.\n> >\n> > Futhermore, the author should also pay attention to any new topics\n> > that branch off after the \"pending\" semantic patch, and whether\n> > any of them introduce code to be transformed, warning their\n> > authors as necessary.\n> >\n> > - Finally, after all the dust settled, the dev should follow up with\n> > a patch to:\n> >\n> > - promote the \"penging\" patch to '.cocci', if its purpose\n> > is to avoid undesirable code patterns in the future, or\n> >\n> > - remove the semantic patch, if it was used in a one-off\n> > transformation.\n> >\n> > Thoughts?\n> \n> I like the approach of having separate classes of semantic patches:\n> (a) the regular \"we need to keep checking these\" as they address\n> undesirable code patterns, which is what we currently have,\n> and what 'make coccicheck' would complain about.\n> (b) The pending patches as you propose. However I would\n> argue that we'd not want to include the transformation into\n> the same patch as then the patch will have merge conflicts.\n\nSince we have a lot of parallel running topics, merge conflicts are\nbasically unavoidable anyway. If the conflicts from the\ntransformation are really that severe, then perhaps the whole series\nshould be postponed to a calmer, more suitable time.\n\nIn the case of 'the_repository.cocci', merging its transformations\ninto 'pu' resulted in only four conflicts, and I found all four on the\neasy side to resolve. I don't think it's worth waiting with the\ntransformations in this particular case.\n\n> Ideally we'd have an automated process/bot that would apply\n> all pending semantic patches onto master and then checks for\n> conflicts in HEAD..pu, and only sends off the non-conflicting\n> diffs as a topic.\n\nNew semantic patches didn't pop up all that frequently in the past, so\nI'm not sure it's worth investing in such an automation. Of course\nthey can become more frequent in the future, and in that case we might\nwant to reconsider it. Unfortunately, however, Coccinelle's results\ncan't be completely trusted, either because our semantic patches or\nbecause Coccinelle itself are buggy...\n\n> Then after a couple integration cycles we'd have all pending\n> changes in, with no conflicts on Junios side.\n> \n> So I think we should add a patch like you post, but we would\n> need to discuss the exact approach how to deal with pending\n> patches. Is it the original dev who should push forward on their\n> own pending patches, or does it become a pooled effort?\n\nWell, it makes sense to me that whoever proposes a change with an\naccompanying new semantic patch should also deal with the necessary\nfollowups. However, it doesn't really matter who deals with them, as\nlong as somebody deals with them. I don't think it's much different\nfrom e.g. sending a followup bugfix to someone else's patch series.\n\n\n"},{"id":"361474","messageId":"20181025053959.GB11460@sigill.intra.peff.net","threadId":"49583","inReplyTo":"20181022173935.GG30222@szeder.dev","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T05:39:59Z","receivedAt":"2018-10-25T05:40:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 07:39:35PM +0200, SZEDER Gábor wrote:\n\n> I don't really like how this or the previous RFC patch series deal\n> with semantic patches (or how some past patch series dealt with them,\n> for that matter), for various reasons:\n> [..]\n\nI am a little late to this thread, but it really seems to me that the\ncrux of the issue here:\n\n> - 'make coccicheck' won't run clean (and the static analysis build\n> job on Travis CI will fail) for all commits following adding the\n> new semantic patches but before applying the resulting\n> transformations.\n> \n> - These semantic patches interact badly with 'pu' and 'next',\n> because those integration branches can contain topic branches\n> adding new code that should be transformed by these semanic\n> patches. Consequently, 'make coccicheck' won't run clean and the\n> static analysis build job will fail until all those topics reach\n> 'master', and the remaining transformations are applied on top.\n\nIs that we are considering it a failure for \"pu\" to have pending\ncoccinelle patches. Are there any coccicheck transformations that aren't\njust \"we prefer to do it like X instead of Y\"?\n\nChanges that actually break things should be caught by the compiler\n(either by their nature, or because we take care to rename or change\ninterfaces when making a semantic change to a function).\n\nI think that's getting at what you're saying here:\n\n> - Is it really necessary to carry these semantic patches in-tree?\n> Let me ellaborate. There are basically two main use cases for\n> semantic patches:\n> \n> - To avoid undesirable code patterns, e.g. we should not use\n> [...]\n> - To perform one-off code transformations, e.g. to modify a\n\nI am mostly thinking of your first type here. And I think it makes sense\nfor people to avoid submitting code that does not pass \"make coccicheck\"\n_as it was at the state of their branch_. But for cocci changes that are\nin flight at the same time as their branch, I do not see any need to\nstrictly adhere to them. The code is \"undesirable\", not \"wrong\".\n\nFor the second type, I agree that we do not necessarily need to carry\nthem in-tree. Eventually the transformation happens, and nobody would\nuse the \"old\" way because it doesn't work anymore. Problem solved.\n\nI do not mind carrying them for a while as a convenience to people who\nwill need to fix up their topics in flight (or more likely to the\nmaintainer, who will need to fixup the merge). But we should make sure\nnot to forget to remove them when their usefulness has passed.\n\nLikewise, we should not forget to occasionally run \"make coccicheck\" on\nmaster to make sure people have a clean base to build on. If Junio is\nable to do that, then great. But other people can also do so and submit\npatches (to go on their respective topics, or to be a new mass-cleanup\ntopic).\n\nI guess there is some lag there if Junio pushes out a master branch that\nfails coccicheck, because contributors may build on it before somebody\ngets around to fixing it.\n\n> Having said that, it's certainly easier to double-check the\n> resulting transformations when one can apply the semantic\n> patches locally, and doing so is easier when the semantic\n> patches are in tree than when they must be copy-pasted from a\n> commit message.\n\nI've wondered if we could have a script that pulls a coccinelle snippet\nfrom a commit message and runs it. It may be a hassle to find the right\ncommit, though (you'd start the procedure from \"oops, my compile now\nseems broken; what was the change that I need to apply to adapt?\").\n\n> - A new semantic patch should be added as \"pending\", e.g. to the\n> file 'the_repository.pending.cocci', together with the resulting\n> transformations in the same commit.\n> \n> This way neither 'make coccicheck' nor the static analysis build\n> job would complain in the topic branch or in the two integration\n> branches. And if they do complain, then we would know right away\n> that they complain because of a well-established semantic patch.\n> Yet, anyone interested could run 'make coccicheck-pending' to see\n> where are we heading.\n\nOK, makes sense.\n\n> - The author of the \"pending\" semanting patch should then keep an\n> eye on already cooking topics: whether any of them contain new\n> code that should be transformed, and how they progress to\n> 'master', and sending followup patch(es) with the remaining\n> transformations when applicable.\n> \n> Futhermore, the author should also pay attention to any new topics\n> that branch off after the \"pending\" semantic patch, and whether\n> any of them introduce code to be transformed, warning their\n> authors as necessary.\n\nThis part seems tricky, though. There's a race condition between\npromoting the patch from pending to not-pending and other topics\nbranching off. And remember that we do not always see other people's\nbranches, which they may work on in private for a long time (though I\nsuppose \"when Junio applies it\" is one effective cutoff).\n\n> - Finally, after all the dust settled, the dev should follow up with\n> a patch to:\n> \n> - promote the \"penging\" patch to '.cocci', if its purpose\n> is to avoid undesirable code patterns in the future, or\n> \n> - remove the semantic patch, if it was used in a one-off\n> transformation.\n\nI'm not wild about leaving these loose ends that need to be tied up\nmanually. It adds a lot of friction and effort.\n\n-Peff\n"},{"id":"361494","messageId":"20181025091406.GK30222@szeder.dev","threadId":"49583","inReplyTo":"20181016233550.251311-19-sbeller@google.com","subject":"Re: [PATCH 18/19] submodule: use submodule repos for object lookup","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-25T09:14:06Z","receivedAt":"2018-10-25T09:14:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Oct 16, 2018 at 04:35:49PM -0700, Stefan Beller wrote:\n> This converts the 'show_submodule_header' function to use\n> the repository API properly, such that the submodule objects\n> are not added to the main object store.\n\nThis patch breaks the test suite with 'GIT_TEST_COMMIT_GRAPH=1', in\nparticular 't4041-diff-submodule-option.sh' fails with:\n\n expecting success:\n git diff-index -p --submodule=log HEAD >actual &&\n cat >expected <<-EOF &&\n Submodule sm1 $head2..$head3 (rewind):\n < Add foo3 ($added foo3)\n < Add foo2 ($added foo2)\n EOF\n test_cmp expected actual\n \n + git diff-index -p --submodule=log HEAD\n + cat\n + test_cmp expected actual\n + diff -u expected actual\n --- expected 2018-10-25 09:10:00.541610016 +0000\n +++ actual 2018-10-25 09:10:00.537609885 +0000\n @@ -1,3 +1,5 @@\n -Submodule sm1 30b9670..dafb207 (rewind):\n +Submodule sm1 30b9670...dafb207:\n < Add foo3 (hinzugefügt foo3)\n < Add foo2 (hinzugefügt foo2)\n + > Add foo1 (hinzugefügt foo1)\n + < Add foo1 (hinzugefügt foo1)\n error: last command exited with $?=1\n not ok 9 - modified submodule(backward)\n\nand 't4060-diff-submodule-option-diff-format.sh' fails with:\n\n expecting success:\n git diff-index -p --submodule=diff HEAD >actual &&\n cat >expected <<-EOF &&\n Submodule sm1 $head2..$head3 (rewind):\n diff --git a/sm1/foo2 b/sm1/foo2\n deleted file mode 100644\n index 54b060e..0000000\n --- a/sm1/foo2\n +++ /dev/null\n @@ -1 +0,0 @@\n -foo2\n diff --git a/sm1/foo3 b/sm1/foo3\n deleted file mode 100644\n index c1ec6c6..0000000\n --- a/sm1/foo3\n +++ /dev/null\n @@ -1 +0,0 @@\n -foo3\n EOF\n test_cmp expected actual\n \n + git diff-index -p --submodule=diff HEAD\n + cat\n + test_cmp expected actual\n + diff -u expected actual\n --- expected 2018-10-25 09:10:38.854868800 +0000\n +++ actual 2018-10-25 09:10:38.854868800 +0000\n @@ -1,4 +1,4 @@\n -Submodule sm1 30b9670..dafb207 (rewind):\n +Submodule sm1 30b9670...dafb207:\n diff --git a/sm1/foo2 b/sm1/foo2\n deleted file mode 100644\n index 54b060e..0000000\n error: last command exited with $?=1\n not ok 10 - modified submodule(backward)\n\n\n"},{"id":"361553","messageId":"CAGZ79kZySpSZC47U2DmP9ia+a-gNw=4MsdkDGGxO0nio9Eo90Q@mail.gmail.com","threadId":"49583","inReplyTo":"20181025015910.GJ30222@szeder.dev","subject":"Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-25T19:25:27Z","receivedAt":"2018-10-25T19:25:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 24, 2018 at 6:59 PM SZEDER Gábor wrote:\n>\n> On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote:\n>\n> > For the sake of a good history, I would think running 'make coccicheck'\n> > and applying the resulting patches would be best as part of the (dirty)\n> > merge of any topic that proposes new semantic patches, but that would\n> > add load to Junio as it would be an extra step during the merge.\n> >\n> > One could argue that the step of applying such transformations into\n> > the dirty merge is cheaper than resolving merge conflicts that are\n> > had when the topic includes the transformation.\n>\n> Please consider that merge commits' have uglier diffs than regular\n> commits, and that merge commits cause additional complications when\n> 'git bisect' points the finger at them, both of which are exacerbated\n> by additional changes squeezed into evil merges.\n>\n> > > Consequently, 'make coccicheck' won't run clean and the\n> > > static analysis build job will fail until all those topics reach\n> > > 'master', and the remaining transformations are applied on top.\n> > >\n> > > This was (and still is!) an issue with the hasheq()/oideq() series\n> > > as well: that series was added on 2018-08-28, and the static\n> > > analysis build job is red on 'pu' ever since. See the follow-up\n> > > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and\n> > > one more follow-up will be necessary after the builtin stash topic\n> > > is merged to 'master'.\n> >\n> > In my understanding this follow up is a feature, as it helps to avoid\n> > merge conflicts with other topics in flight.\n>\n> I don't see how such a follow up patch helps to avoid merge conflicts.\n\nWell, you merge first (the new topic and the cocci patches), and then\ndo the transformation. But that is putting a lot more work on Junio\nas the way to integrate is not just merge a new topic into the pile of\ntopics (whether it is pu/next/master), but to first perform a merge\nof the topic and the coccinelle patches, apply the transformation\nand then merge to the pile, assuming the pile is already transformed\n(as it happened in \"treewide: apply cocci patch\" in next/pu).\n\n> > as 'make coccicheck' is an integral part of your review?\n>\n> Erm, right, \"review\" was not the right word here. Anyway, as it is,\n> 'make coccicheck' is an integral part of our automated tests, not only\n> on Travis CI but on the upcoming Azure thing as well. I just try to\n> pay attention to its results and the results of a bunch of my\n> additional builds, and complain or even send a fix when something goes\n> reproducibly wrong. This has certainly became more cumbersome with\n> the permanently failing static analysis build job in the last couple\n> of weeks.\n\nI seem to not pay as much attention as I should to these,\nmostly because they are unreliable on the aggregate level (a failure\nthere most likely means another topic than the one I am interested\nbroke; except in this case, where we discuss the fallout there via\nthis topic.)\n\n> > I like the approach of having separate classes of semantic patches:\n> > (a) the regular \"we need to keep checking these\" as they address\n> > undesirable code patterns, which is what we currently have,\n> > and what 'make coccicheck' would complain about.\n> > (b) The pending patches as you propose. However I would\n> > argue that we'd not want to include the transformation into\n> > the same patch as then the patch will have merge conflicts.\n>\n> Since we have a lot of parallel running topics, merge conflicts are\n> basically unavoidable anyway. If the conflicts from the\n> transformation are really that severe, then perhaps the whole series\n> should be postponed to a calmer, more suitable time.\n\nMerge conflicts of this kind could be avoided, by running the\ntransformation on both sides before merging (or not running them\non both sides, but only after merging).\n\nSo maybe for these larger 'the_repository.pending.cocci' patches\nwe could get them into the tree and wait until all (most) of the topics\nare including that semantic patch in their base, such that application\nof the patch is easy whether before or after writing a series\n(as the semantic patch is in its base).\n\nAnother short term plan would be renaming the_repository.cocci\nsuch that it would break the 'make coccicheck'\n\n> In the case of 'the_repository.cocci', merging its transformations\n> into 'pu' resulted in only four conflicts, and I found all four on the\n> easy side to resolve. I don't think it's worth waiting with the\n> transformations in this particular case.\n\nI am not worried about the current conflicts, but about those to come\nin new series.\n\n>\n> > Ideally we'd have an automated process/bot that would apply\n> > all pending semantic patches onto master and then checks for\n> > conflicts in HEAD..pu, and only sends off the non-conflicting\n> > diffs as a topic.\n>\n> New semantic patches didn't pop up all that frequently in the past, so\n> I'm not sure it's worth investing in such an automation. Of course\n> they can become more frequent in the future, and in that case we might\n> want to reconsider it. Unfortunately, however, Coccinelle's results\n> can't be completely trusted, either because our semantic patches or\n> because Coccinelle itself are buggy...\n\nFor the first, we could just be become better at reviewing the Cocci\npatch. ;-) (Well, they are harder to review than usual patches, so\nthis doesn't surprise me. Also not fully understanding the whole\ntool may have impact on reviewability)\n\n>\n> > Then after a couple integration cycles we'd have all pending\n> > changes in, with no conflicts on Junios side.\n> >\n> > So I think we should add a patch like you post, but we would\n> > need to discuss the exact approach how to deal with pending\n> > patches. Is it the original dev who should push forward on their\n> > own pending patches, or does it become a pooled effort?\n>\n> Well, it makes sense to me that whoever proposes a change with an\n> accompanying new semantic patch should also deal with the necessary\n> followups. However, it doesn't really matter who deals with them, as\n> long as somebody deals with them. I don't think it's much different\n> from e.g. sending a followup bugfix to someone else's patch series.\n\nOk, I plan on resending these patches after I get\norigin/sb/submodule-recursive-fetch-gets-the-tip going\nresent.\n\nStefan\n"},{"id":"362104","messageId":"ab0f5b37-b4f1-3efc-b30f-f3b34f2a11cb@gmail.com","threadId":"49583","inReplyTo":"20181016233550.251311-19-sbeller@google.com","subject":"Re: [PATCH 18/19] submodule: use submodule repos for object lookup","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-31T13:38:02Z","receivedAt":"2018-10-31T13:38:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/16/2018 7:35 PM, Stefan Beller wrote:\n> @@ -482,14 +483,46 @@ void prepare_submodule_repo_env(struct argv_array *out)\n> \t\t\t DEFAULT_GIT_DIR_ENVIRONMENT);\n> }\n> \n> -/* Helper function to display the submodule header line prior to the full\n> - * summary output. If it can locate the submodule objects directory it will\n> - * attempt to lookup both the left and right commits and put them into the\n> - * left and right pointers.\n> +/*\n> + * Initialize 'out' based on the provided submodule path.\n> + *\n> + * Unlike repo_submodule_init, this tolerates submodules not present\n> + * in .gitmodules. This function exists only to preserve historical behavior,\n> + *\n> + * Returns 0 on success, -1 when the submodule is not present.\n> + */\n> +static int open_submodule(struct repository *out, const char *path)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tif (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tout->submodule_prefix = xstrdup(path);\n> +\tout->submodule_prefix = xstrfmt(\"%s%s/\",\n> +\t\t\t\t\tthe_repository->submodule_prefix ?\n> +\t\t\t\t\tthe_repository->submodule_prefix :\n> +\t\t\t\t\t\"\", path);\n> +\n> +\tstrbuf_release(&sb);\n> +\treturn 0;\n> +}\n\nBased on the recent test coverage report [1], this xstrfmt() call is never\nrun witha non-null the_repository->submodule_prefix. Is there a way we can\nexercise that branch?\n\nThanks,\n-Stolee\n\n[1] \nhttps://public-inbox.org/git/62f0bcf6-aa73-c192-d804-e6d69cac146f@gmail.com/\n"},{"id":"362207","messageId":"CAGZ79kanW632k=LHzT9RhmDMSpCTK8K2sPswsF7H0RRC1aLqVQ@mail.gmail.com","threadId":"49583","inReplyTo":"ab0f5b37-b4f1-3efc-b30f-f3b34f2a11cb@gmail.com","subject":"Re: [PATCH 18/19] submodule: use submodule repos for object lookup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-01T19:13:11Z","receivedAt":"2018-11-01T19:13:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 31, 2018 at 6:38 AM Derrick Stolee wrote:\n>\n> On 10/16/2018 7:35 PM, Stefan Beller wrote:\n> > @@ -482,14 +483,46 @@ void prepare_submodule_repo_env(struct argv_array *out)\n> > DEFAULT_GIT_DIR_ENVIRONMENT);\n> > }\n> >\n> > -/* Helper function to display the submodule header line prior to the full\n> > - * summary output. If it can locate the submodule objects directory it will\n> > - * attempt to lookup both the left and right commits and put them into the\n> > - * left and right pointers.\n> > +/*\n> > + * Initialize 'out' based on the provided submodule path.\n> > + *\n> > + * Unlike repo_submodule_init, this tolerates submodules not present\n> > + * in .gitmodules. This function exists only to preserve historical behavior,\n> > + *\n> > + * Returns 0 on success, -1 when the submodule is not present.\n> > + */\n> > +static int open_submodule(struct repository *out, const char *path)\n> > +{\n> > + struct strbuf sb = STRBUF_INIT;\n> > +\n> > + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {\n> > + strbuf_release(&sb);\n> > + return -1;\n> > + }\n> > +\n> > + out->submodule_prefix = xstrdup(path);\n> > + out->submodule_prefix = xstrfmt(\"%s%s/\",\n> > + the_repository->submodule_prefix ?\n> > + the_repository->submodule_prefix :\n> > + \"\", path);\n> > +\n> > + strbuf_release(&sb);\n> > + return 0;\n> > +}\n>\n> Based on the recent test coverage report [1], this xstrfmt() call is never\n> run witha non-null the_repository->submodule_prefix. Is there a way we can\n> exercise that branch?\n\nNo it's dead code, actually. the_repository never has submodule_prefix set\nas it is the main repository. So this is overly cautious to enable the\n'any repo'\ncase.\nIn a resend we'll go with xstrdup(path);\n"}]}