{"thread":{"id":"45540","subject":"[PATCH 1/2] entry.c: submodule recursing: respect force flag correctly","startedAt":"2017-03-29T18:38:04Z","lastAt":"2017-03-29T22:47:14Z","messageCount":8,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"315695","messageId":"20170329183743.27506-1-sbeller@google.com","threadId":"45540","inReplyTo":null,"subject":"[PATCH 1/2] entry.c: submodule recursing: respect force flag correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T18:37:42Z","receivedAt":"2017-03-29T18:38:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In case of a non-forced worktree update, the submodule movement is tested\nin a dry run first, such that it doesn't matter if the actual update is\ndone via the force flag. However for correctness, we want to give the\nflag is specified by the user.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n entry.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex d2b512da90..645121f828 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -287,7 +287,7 @@ int checkout_entry(struct cache_entry *ce,\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\tSUBMODULE_MOVE_HEAD_FORCE);\n+\t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\t}\n \n \t\tif (!changed)\n-- \n2.12.0.rc1.52.g2de7d24de9.dirty\n\n"},{"id":"315696","messageId":"20170329183743.27506-2-sbeller@google.com","threadId":"45540","inReplyTo":"20170329183743.27506-1-sbeller@google.com","subject":"[PATCH 2/2] unpack-trees.c: align submodule error message to the other error messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T18:37:43Z","receivedAt":"2017-03-29T18:38:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the place holder in the error message is for multiple submodules,\nwe don't want to encapsulate the string place holder in single quotes.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8333da2cc9..9f386cc174 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -167,7 +167,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n \t\t_(\"The following working tree files would be removed by sparse checkout update:\\n%s\");\n \tmsgs[ERROR_WOULD_LOSE_SUBMODULE] =\n-\t\t_(\"Submodule '%s' cannot checkout new HEAD\");\n+\t\t_(\"The following submodules cannot checkout a new HEAD:\\n%s\");\n \n \topts->show_all_errors = 1;\n \t/* rejected paths may not have a static buffer */\n-- \n2.12.0.rc1.52.g2de7d24de9.dirty\n\n"},{"id":"315699","messageId":"20170329184519.GW31294@aiede.mtv.corp.google.com","threadId":"45540","inReplyTo":"20170329183743.27506-1-sbeller@google.com","subject":"Re: [PATCH 1/2] entry.c: submodule recursing: respect force flag correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-29T18:45:19Z","receivedAt":"2017-03-29T18:45:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> In case of a non-forced worktree update, the submodule movement is tested\n> in a dry run first, such that it doesn't matter if the actual update is\n> done via the force flag. However for correctness, we want to give the\n> flag is specified by the user.\n\n\"for correctness\" means \"to avoid races\"?\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  entry.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/entry.c b/entry.c\n> index d2b512da90..645121f828 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -287,7 +287,7 @@ int checkout_entry(struct cache_entry *ce,\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\tSUBMODULE_MOVE_HEAD_FORCE);\n> +\t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n\nLooks like a good change.\n\nThis moves past the 80-column margin.  I wish there were a tool like\ngofmt or clang-format that would take care of formatting for us.\n\nThis isn't the only place SUBMODULE_MOVE_HEAD_FORCE is used in the\nfile.  Do they need the same treatment?\n\nThanks,\nJonathan\n"},{"id":"315700","messageId":"20170329184833.GX31294@aiede.mtv.corp.google.com","threadId":"45540","inReplyTo":"20170329183743.27506-2-sbeller@google.com","subject":"Re: [PATCH 2/2] unpack-trees.c: align submodule error message to the other error messages","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-29T18:48:33Z","receivedAt":"2017-03-29T18:48:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> As the place holder in the error message is for multiple submodules,\n> we don't want to encapsulate the string place holder in single quotes.\n\nMakes sense.\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  unpack-trees.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8333da2cc9..9f386cc174 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -167,7 +167,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>  \tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n>  \t\t_(\"The following working tree files would be removed by sparse checkout update:\\n%s\");\n>  \tmsgs[ERROR_WOULD_LOSE_SUBMODULE] =\n> -\t\t_(\"Submodule '%s' cannot checkout new HEAD\");\n> +\t\t_(\"The following submodules cannot checkout a new HEAD:\\n%s\");\n\nNitpicking about wording: unless the user has adopted a strongly\nobject-oriented point of view, it is Git that cannot checkout a new\nHEAD, not the submodule.\n\nHow about:\n\n\t\t_(\"Cannot update submodule:\\n%s\")\n\nThat's vague, but if I understand correctly the way this error gets\nused is equally vague --- i.e., a clearer message would involve\nfiner-grained error codes.\n\nThanks,\nJonathan\n"},{"id":"315707","messageId":"xmqq37dvahzw.fsf@gitster.mtv.corp.google.com","threadId":"45540","inReplyTo":"20170329184519.GW31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] entry.c: submodule recursing: respect force flag correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-29T21:30:11Z","receivedAt":"2017-03-29T21:32:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Stefan Beller wrote:\n>\n>> In case of a non-forced worktree update, the submodule movement is tested\n>> in a dry run first, such that it doesn't matter if the actual update is\n>> done via the force flag. However for correctness, we want to give the\n>> flag is specified by the user.\n>\n> \"for correctness\" means \"to avoid races\"?\n\nSorry, but neither explanation makes much sense to me.\n\nThe codepath the patch touches says \"if the submodule is not\npopulated, then checkout the submodule by switching from NULL\n(nothing checked out) to the commit bound to the index of the\nsuperproject; otherwise, checkout the submodule by switching from\nHEAD (what is currently checked out) to the commit in the index\".\n\nWhere does that \"tested in a dry run first\" come into play?\nWhatever code calls checkout_entry(), does it call it twice, first\nwith a \"--dry-run\" option and then without one?  How does this\ncodepath respond differently to these two invocations, and how does\nthis change affect the way these two invocations behave?\n\n\n\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  entry.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/entry.c b/entry.c\n>> index d2b512da90..645121f828 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -287,7 +287,7 @@ int checkout_entry(struct cache_entry *ce,\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\tSUBMODULE_MOVE_HEAD_FORCE);\n>> +\t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n>\n> Looks like a good change.\n>\n> This moves past the 80-column margin.  I wish there were a tool like\n> gofmt or clang-format that would take care of formatting for us.\n>\n> This isn't the only place SUBMODULE_MOVE_HEAD_FORCE is used in the\n> file.  Do they need the same treatment?\n>\n> Thanks,\n> Jonathan\n"},{"id":"315709","messageId":"xmqqy3vn93a9.fsf@gitster.mtv.corp.google.com","threadId":"45540","inReplyTo":"20170329184833.GX31294@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] unpack-trees.c: align submodule error message to the other error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-29T21:33:18Z","receivedAt":"2017-03-29T21:38:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Stefan Beller wrote:\n>\n>> As the place holder in the error message is for multiple submodules,\n>> we don't want to encapsulate the string place holder in single quotes.\n>\n> Makes sense.\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  unpack-trees.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index 8333da2cc9..9f386cc174 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -167,7 +167,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>>  \tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n>>  \t\t_(\"The following working tree files would be removed by sparse checkout update:\\n%s\");\n>>  \tmsgs[ERROR_WOULD_LOSE_SUBMODULE] =\n>> -\t\t_(\"Submodule '%s' cannot checkout new HEAD\");\n>> +\t\t_(\"The following submodules cannot checkout a new HEAD:\\n%s\");\n>\n> Nitpicking about wording: unless the user has adopted a strongly\n> object-oriented point of view, it is Git that cannot checkout a new\n> HEAD, not the submodule.\n>\n> How about:\n>\n> \t\t_(\"Cannot update submodule:\\n%s\")\n>\n> That's vague, but if I understand correctly the way this error gets\n> used is equally vague --- i.e., a clearer message would involve\n> finer-grained error codes.\n\nMakes sense to me.\n\nThanks for helping.\n"},{"id":"315719","messageId":"20170329223424.4678-1-sbeller@google.com","threadId":"45540","inReplyTo":"xmqqy3vn93a9.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] unpack-trees.c: align submodule error message to the other error messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-29T22:34:24Z","receivedAt":"2017-03-29T22:34:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the place holder in the error message is for multiple submodules,\nwe don't want to encapsulate the string place holder in single quotes.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n> Nitpicking about wording: unless the user has adopted a strongly\n> object-oriented point of view, it is Git that cannot checkout a new\n> HEAD, not the submodule.\n> \n> How about:\n> \n>                 _(\"Cannot update submodule:\\n%s\")\n\n> That's vague, but if I understand correctly the way this error gets\n> used is equally vague --- i.e., a clearer message would involve\n> finer-grained error codes.\n\nMakes sense. Here is the patch.\nLet's roll this as its own instead of waiting for the discussion on the other\npatch to settle.\n\nThanks,\nStefan\n\n unpack-trees.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8333da2cc9..0d82452f7f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -167,7 +167,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n \t\t_(\"The following working tree files would be removed by sparse checkout update:\\n%s\");\n \tmsgs[ERROR_WOULD_LOSE_SUBMODULE] =\n-\t\t_(\"Submodule '%s' cannot checkout new HEAD\");\n+\t\t_(\"Cannot update submodule:\\n%s\")\n \n \topts->show_all_errors = 1;\n \t/* rejected paths may not have a static buffer */\n-- \n2.12.1.442.ge9452a8fbc\n\n"},{"id":"315720","messageId":"xmqq60ir8zv8.fsf@gitster.mtv.corp.google.com","threadId":"45540","inReplyTo":"20170329223424.4678-1-sbeller@google.com","subject":"Re: [PATCH] unpack-trees.c: align submodule error message to the other error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-29T22:47:07Z","receivedAt":"2017-03-29T22:47:14Z","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> As the place holder in the error message is for multiple submodules,\n> we don't want to encapsulate the string place holder in single quotes.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>> Nitpicking about wording: unless the user has adopted a strongly\n>> object-oriented point of view, it is Git that cannot checkout a new\n>> HEAD, not the submodule.\n>> \n>> How about:\n>> \n>>                 _(\"Cannot update submodule:\\n%s\")\n>\n>> That's vague, but if I understand correctly the way this error gets\n>> used is equally vague --- i.e., a clearer message would involve\n>> finer-grained error codes.\n>\n> Makes sense. Here is the patch.\n> Let's roll this as its own instead of waiting for the discussion on the other\n> patch to settle.\n>\n> Thanks,\n> Stefan\n>\n>  unpack-trees.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8333da2cc9..0d82452f7f 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -167,7 +167,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>  \tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n>  \t\t_(\"The following working tree files would be removed by sparse checkout update:\\n%s\");\n>  \tmsgs[ERROR_WOULD_LOSE_SUBMODULE] =\n> -\t\t_(\"Submodule '%s' cannot checkout new HEAD\");\n> +\t\t_(\"Cannot update submodule:\\n%s\")\n\nMissing ';'.  I'll fix locally, but the final integration result\nwon't be pushed out until later tonight, as I need to redo jch and\npu branches with a fixed version.\n\n\n>  \n>  \topts->show_all_errors = 1;\n>  \t/* rejected paths may not have a static buffer */\n"}]}