{"thread":{"id":"49288","subject":"[PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id","startedAt":"2018-09-05T23:20:00Z","lastAt":"2018-09-06T21:19:01Z","messageCount":6,"participants":["Stefan Beller","Jonathan Nieder","Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"357494","messageId":"20180905231952.28145-1-sbeller@google.com","threadId":"49288","inReplyTo":null,"subject":"[PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-05T23:19:51Z","receivedAt":"2018-09-05T23:20:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All callers use oid_to_hex to convert the desired oid to a string before\ncalling submodule_move_head. Defer the conversion to the\nsubmodule_move_head as it will turn out to be useful in a bit.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis is also part of the other series sent out yesterday, \nhttps://public-inbox.org/git/20180904230149.180332-5-sbeller@google.com/\n\n entry.c        |  6 +++---\n submodule.c    | 12 ++++++------\n submodule.h    |  2 +-\n unpack-trees.c | 13 +++++--------\n 4 files changed, 15 insertions(+), 18 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 2a2ab6c8394..0b676f997b2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -358,7 +358,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tsub = submodule_from_ce(ce);\n \t\tif (sub)\n \t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\tNULL, oid_to_hex(&ce->oid),\n+\t\t\t\tNULL, &ce->oid,\n \t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\tbreak;\n \n@@ -439,10 +439,10 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\t\t\tunlink_or_warn(ce->name);\n \n \t\t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\t\tNULL, oid_to_hex(&ce->oid), 0);\n+\t\t\t\t\tNULL, &ce->oid, 0);\n \t\t\t} else\n \t\t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid),\n+\t\t\t\t\t\"HEAD\", &ce->oid,\n \t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\t}\n \ndiff --git a/submodule.c b/submodule.c\nindex 50cbf5f13ed..da2ed8696f5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)\n  * pass NULL for old or new respectively.\n  */\n int submodule_move_head(const char *path,\n-\t\t\t const char *old_head,\n-\t\t\t const char *new_head,\n-\t\t\t unsigned flags)\n+\t\t\tconst char *old_head,\n+\t\t\tconst struct object_id *new_oid,\n+\t\t\tunsigned flags)\n {\n \tint ret = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1679,7 +1679,7 @@ int submodule_move_head(const char *path,\n \tif (!(flags & SUBMODULE_MOVE_HEAD_FORCE))\n \t\targv_array_push(&cp.args, old_head ? old_head : empty_tree_oid_hex());\n \n-\targv_array_push(&cp.args, new_head ? new_head : empty_tree_oid_hex());\n+\targv_array_push(&cp.args, new_oid ? oid_to_hex(new_oid) : empty_tree_oid_hex());\n \n \tif (run_command(&cp)) {\n \t\tret = error(_(\"Submodule '%s' could not be updated.\"), path);\n@@ -1687,7 +1687,7 @@ int submodule_move_head(const char *path,\n \t}\n \n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n-\t\tif (new_head) {\n+\t\tif (new_oid) {\n \t\t\tchild_process_init(&cp);\n \t\t\t/* also set the HEAD accordingly */\n \t\t\tcp.git_cmd = 1;\n@@ -1696,7 +1696,7 @@ int submodule_move_head(const char *path,\n \n \t\t\tprepare_submodule_repo_env(&cp.env_array);\n \t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\",\n-\t\t\t\t\t \"--no-deref\", new_head, NULL);\n+\t\t\t\t\t \"--no-deref\", oid_to_hex(new_oid), NULL);\n \n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/submodule.h b/submodule.h\nindex 7d476cefa7e..ef34d5a7ca8 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -124,7 +124,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n #define SUBMODULE_MOVE_HEAD_FORCE   (1<<1)\n int submodule_move_head(const char *path,\n \t\t\tconst char *old,\n-\t\t\tconst char *new_head,\n+\t\t\tconst struct object_id *new_oid,\n \t\t\tunsigned flags);\n \n void submodule_unset_core_worktree(const struct submodule *sub);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f25089b878a..75d1b294ade 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -256,7 +256,7 @@ static void display_error_msgs(struct unpack_trees_options *o)\n \n static int check_submodule_move_head(const struct cache_entry *ce,\n \t\t\t\t     const char *old_id,\n-\t\t\t\t     const char *new_id,\n+\t\t\t\t     const struct object_id *new_id,\n \t\t\t\t     struct unpack_trees_options *o)\n {\n \tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n@@ -1517,7 +1517,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint r = check_submodule_move_head(ce,\n-\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid), o);\n+\t\t\t\t\"HEAD\", &ce->oid, o);\n \t\t\tif (r)\n \t\t\t\treturn o->gently ? -1 :\n \t\t\t\t\tadd_rejected_path(o, error_type, ce->name);\n@@ -1591,8 +1591,7 @@ static int verify_clean_submodule(const char *old_sha1,\n \tif (!submodule_from_ce(ce))\n \t\treturn 0;\n \n-\treturn check_submodule_move_head(ce, old_sha1,\n-\t\t\t\t\t oid_to_hex(&ce->oid), o);\n+\treturn check_submodule_move_head(ce, old_sha1, &ce->oid, o);\n }\n \n static int verify_clean_subdirectory(const struct cache_entry *ce,\n@@ -1836,8 +1835,7 @@ static int merged_entry(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint ret = check_submodule_move_head(ce, NULL,\n-\t\t\t\t\t\t\t    oid_to_hex(&ce->oid),\n-\t\t\t\t\t\t\t    o);\n+\t\t\t\t\t\t\t    &ce->oid, o);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n@@ -1865,8 +1863,7 @@ static int merged_entry(const struct cache_entry *ce,\n \n \t\tif (submodule_from_ce(ce)) {\n \t\t\tint ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),\n-\t\t\t\t\t\t\t    oid_to_hex(&ce->oid),\n-\t\t\t\t\t\t\t    o);\n+\t\t\t\t\t\t\t    &ce->oid, o);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357495","messageId":"20180905231952.28145-2-sbeller@google.com","threadId":"49288","inReplyTo":"20180905231952.28145-1-sbeller@google.com","subject":"[PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-05T23:19:52Z","receivedAt":"2018-09-05T23:20:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By checking if a submodule commit exists before attempting the update\nwe can improve the error message from the\n    error(_(\"Submodule '%s' could not be updated.\"), path);\nto the new and more specific\n    error(_(\"Submodule '%s' doesn't have commit '%s'\"),\n          path, oid_to_hex(new_oid));\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex da2ed8696f5..56104af1c7c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1605,6 +1605,7 @@ int submodule_move_head(const char *path,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tconst struct submodule *sub;\n \tint *error_code_ptr, error_code;\n+\tstruct repository subrepo;\n \n \tif (!is_submodule_active(the_repository, path))\n \t\treturn 0;\n@@ -1627,6 +1628,13 @@ int submodule_move_head(const char *path,\n \tif (!sub)\n \t\tBUG(\"could not get submodule information for '%s'\", path);\n \n+\tif (repo_submodule_init(&subrepo, the_repository, path) < 0)\n+\t\twarning(_(\"Could not get submodule repository for submodule 's'\"), path);\n+\telse if (new_oid && !lookup_commit(subrepo, new_oid)) {\n+\t\treturn error(_(\"Submodule '%s' doesn't have commit '%s'\"),\n+\t\t\t     path, oid_to_hex(new_oid));\n+\t}\n+\n \tif (old_head && !(flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n \t\t/* Check if the submodule has a dirty index. */\n \t\tif (submodule_has_dirty_index(sub))\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357496","messageId":"20180905233203.GE120842@aiede.svl.corp.google.com","threadId":"49288","inReplyTo":"20180905231952.28145-2-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-05T23:32:03Z","receivedAt":"2018-09-05T23:32:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Subject: submodule.c: warn about missing submodule commit in recursive actions\n\nNit: the diff already tells me what file the change is in.  What I'd\nbe more interested in is the subsystem or what commands this affects.\nDoes this affect all --recurse-submodules commands, or just some?\n\nHere, I think it's about common submodule code, so I guess\n'submodule:' would be a fine prefix.\n\n> By checking if a submodule commit exists before attempting the update\n> we can improve the error message from the\n>     error(_(\"Submodule '%s' could not be updated.\"), path);\n> to the new and more specific\n>     error(_(\"Submodule '%s' doesn't have commit '%s'\"),\n>           path, oid_to_hex(new_oid));\n\nMaybe it's just me, but I find this formatting where I cannot\ndistinguish between a line that was wrapped early and the start of a\ncallout hard to read.  Some extra line breaks would help:\n\n  By checking if a submodule commit exists before attempting the update\n  we can improve the error message from the\n\n      error(_(\"Submodule '%s' could not be updated.\"), path);\n\n  to the new and more specific\n\n      error(_(\"Submodule '%s' doesn't have commit '%s'\"),\n            path, oid_to_hex(new_oid));\n\nBeyond that, I still don't know what this change does.  Can you give\nan example?  For example, what command would I run before and what bad\nresult would I get, and what result does this patch produce instead?\n\nThanks,\nJonathan\n"},{"id":"357497","messageId":"20180905234057.GF120842@aiede.svl.corp.google.com","threadId":"49288","inReplyTo":"20180905231952.28145-1-sbeller@google.com","subject":"Re: [PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-05T23:40:57Z","receivedAt":"2018-09-05T23:41:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Subject: submodule.c: convert submodule_move_head new argument to object id\n\nSame nit as in https://public-inbox.org/git/20180905233203.GE120842@aiede.svl.corp.google.com/\napplies about wondering which subsystem this is in.\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)\n>   * pass NULL for old or new respectively.\n>   */\n>  int submodule_move_head(const char *path,\n> -\t\t\t const char *old_head,\n> -\t\t\t const char *new_head,\n> -\t\t\t unsigned flags)\n> +\t\t\tconst char *old_head,\n> +\t\t\tconst struct object_id *new_oid,\n> +\t\t\tunsigned flags)\n\nThis seems oddly asymmetrical.  Should the old value be passed as an\nobject_id as well?\n\nCurious,\nJonathan\n"},{"id":"357516","messageId":"CAN0heSq1fNb6Wc1P0B3JQL3PA_yi329vbkG5pjWsCpYf8SDn5Q@mail.gmail.com","threadId":"49288","inReplyTo":"20180905231952.28145-2-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-06T06:22:20Z","receivedAt":"2018-09-06T06:22:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"> +       if (repo_submodule_init(&subrepo, the_repository, path) < 0)\n> +               warning(_(\"Could not get submodule repository for submodule 's'\"), path);\n\nMissing \"%\" in format specifier, so `path` will never be used. Also,\ns/C/c/ at the start of the warning.\n\nThanks for marking with _().\n\nMartin\n"},{"id":"357584","messageId":"xmqqa7ouz5mm.fsf@gitster-ct.c.googlers.com","threadId":"49288","inReplyTo":"20180905231952.28145-1-sbeller@google.com","subject":"Re: [PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T21:18:57Z","receivedAt":"2018-09-06T21:19:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> All callers use oid_to_hex to convert the desired oid to a string before\n> calling submodule_move_head. Defer the conversion to the\n> submodule_move_head as it will turn out to be useful in a bit.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> This is also part of the other series sent out yesterday, \n> https://public-inbox.org/git/20180904230149.180332-5-sbeller@google.com/\n\nAnd what is your intention wrt the other one?\n\nDo you want two separate copies of essentially the same patch queued\nas part of two separate topics?  Did you change your mind since\nyesterday and decided to retract these patches, and instead\nconcentrate on these two patches that address an issue with tigher\nfocus before moving on to other things?\n\nExactly the same comment applies to this copy.  It is somewhat\ndisturbing to see that new and old, which correspond to each other\nand ought to be symmetrical, take the same \"commit object\" in\ndifferent shapes.\n"}]}