{"thread":{"id":"40073","subject":"[PATCH 1/2] Fixup hv/documentation","startedAt":"2015-08-12T19:13:56Z","lastAt":"2015-08-12T22:01:32Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"267936","messageId":"1439406838-6290-1-git-send-email-sbeller@google.com","threadId":"40073","inReplyTo":null,"subject":"Re: [PATCH v5 0/4] submodule config lookup API","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-12T19:13:56Z","receivedAt":"2015-08-12T19:13:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 12, 2015 at 10:53 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Mon, Jun 15, 2015 at 2:48 PM, Junio C Hamano wrote:\n>>> Thanks. Will replace and wait for comments from others.\n>>\n>> I have reviewed the patches carefully and they look good to me.\n>\n> OK, I recall there were a few iterations with review comments before\n> this round. Is it your impression that they have been addressed\n> adequately?\n\nI payed only little attention to the previous rounds and the review comments\nthereof, because round 5 was up for quite a long time and nobody commented so far.\n\nHowever just as I was convinced of my review and sent out the email, I started\nworking with it. And I found nits which I'd ask you to squash into the round or\nput on top.\n\n>\n> Do you prefer it to be rebased to a more recent 'master' before you\n> build your work on top of it (I think the topic currently builds on\n> top of v2.5.0-rc0~56)?\n\nLooking at the reviews for the \"RFC parallel fetch for submodules\"\nI'd like to use `argv_array_pushv` which was introduced via\n85b343245b (2015-06-14, argv-array: implement argv_array_pushv())\nand is already in origin/next, but not origin/master.\nBut as I first work on the submodule--helper (both the small helper\nfunctions as well as the whole \"update\" thereafter), I think this\ndependency is not a pressing issue yet.\n\n>\n> Thanks.\n\nStefan Beller (2):\n Fixup hv/documentation\n cleanup submodule_config a bit.\n\n Documentation/technical/api-submodule-config.txt | 3 +--\n submodule-config.c | 12 +++++-------\n 2 files changed, 6 insertions(+), 9 deletions(-)\n\n-- \n2.5.0.234.gefc8a62\n"},{"id":"267935","messageId":"1439406838-6290-2-git-send-email-sbeller@google.com","threadId":"40073","inReplyTo":"1439406838-6290-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] Fixup hv/documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-12T19:13:57Z","receivedAt":"2015-08-12T19:13:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If you want to look up by name, use `submodule_from_name` instead.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/technical/api-submodule-config.txt | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt\nindex 2ea520a..941fa17 100644\n--- a/Documentation/technical/api-submodule-config.txt\n+++ b/Documentation/technical/api-submodule-config.txt\n@@ -49,8 +49,7 @@ Functions\n \n `const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::\n \n-\tLookup values for one submodule by its commit_sha1 and path or\n-\tname.\n+\tLookup values for one submodule by its commit_sha1 and path.\n \n `const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::\n \n-- \n2.5.0.234.gefc8a62\n"},{"id":"267937","messageId":"1439406838-6290-3-git-send-email-sbeller@google.com","threadId":"40073","inReplyTo":"1439406838-6290-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] cleanup submodule_config a bit.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-12T19:13:58Z","receivedAt":"2015-08-12T19:13:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the first hunk, `submodule` is NULL all the time, so we can make it clearer\nby directly returning NULL.\n\nIn the second hunk, we can directly return the lookup values as it also makes\nthe coder clearer.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 199692b..08e93cc 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \t}\n \n \tif (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n-\t\treturn submodule;\n+\t\treturn NULL;\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n@@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \n \tswitch (lookup_type) {\n \tcase lookup_name:\n-\t\tsubmodule = cache_lookup_name(cache, sha1, key);\n-\t\tbreak;\n+\t\treturn cache_lookup_name(cache, sha1, key);\n \tcase lookup_path:\n-\t\tsubmodule = cache_lookup_path(cache, sha1, key);\n-\t\tbreak;\n+\t\treturn cache_lookup_path(cache, sha1, key);\n+\tdefault:\n+\t\treturn NULL;\n \t}\n-\n-\treturn submodule;\n }\n \n static const struct submodule *config_from_path(struct submodule_cache *cache,\n-- \n2.5.0.234.gefc8a62\n"},{"id":"267939","messageId":"xmqq614kl33d.fsf@gitster.dls.corp.google.com","threadId":"40073","inReplyTo":"1439406838-6290-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v5 0/4] submodule config lookup API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T19:27:18Z","receivedAt":"2015-08-12T19:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> However just as I was convinced of my review and sent out the email, I started\n> working with it. And I found nits which I'd ask you to squash into the round or\n> put on top.\n\nGood ;-). I'd prefer a full reroll, as it has been quite a while\nsince v5 was originally posted, once you get to a reasonable state\nwhere you are reasonably confident that you won't find more of such\nnits. Hopefully more people might have eyes on the list to review\nand comment this round.\n\nThanks.\n"},{"id":"267946","messageId":"CAPig+cTcWME_U+nYATK7kK0vOSSCjJmGgXmUVmSawBiV5M+02g@mail.gmail.com","threadId":"40073","inReplyTo":"1439406838-6290-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/2] cleanup submodule_config a bit.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-12T21:13:01Z","receivedAt":"2015-08-12T21:13:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:\n> In the first hunk, `submodule` is NULL all the time, so we can make it clearer\n> by directly returning NULL.\n>\n> In the second hunk, we can directly return the lookup values as it also makes\n> the coder clearer.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule-config.c | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 199692b..08e93cc 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n> }\n>\n> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n> - return submodule;\n> + return NULL;\n\nThere are a couple other places which return 'submodule' when it is\nknown to be NULL. One could rightly expect that they deserve the same\ntreatment, otherwise, the code becomes more confusing since it's not\nobvious why 'return NULL' is used some places but not others.\n\n> switch (lookup_type) {\n> case lookup_name:\n> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>\n> switch (lookup_type) {\n> case lookup_name:\n> - submodule = cache_lookup_name(cache, sha1, key);\n> - break;\n> + return cache_lookup_name(cache, sha1, key);\n> case lookup_path:\n> - submodule = cache_lookup_path(cache, sha1, key);\n> - break;\n> + return cache_lookup_path(cache, sha1, key);\n> + default:\n> + return NULL;\n> }\n> -\n> - return submodule;\n\nEarlier in the function, there's effectively a clone of this logic to\nwhich you could apply the same transformation. Changing it here, while\nignoring the clone, makes the code more confusing (or at least\ninconsistent) rather than less.\n\n> }\n>\n> static const struct submodule *config_from_path(struct submodule_cache *cache,\n> --\n> 2.5.0.234.gefc8a62\n"},{"id":"267947","messageId":"CAGZ79kbV45LqxCha516dRNYAjAH71mxce6Xazvfmf1deLMzUxQ@mail.gmail.com","threadId":"40073","inReplyTo":"CAPig+cTcWME_U+nYATK7kK0vOSSCjJmGgXmUVmSawBiV5M+02g@mail.gmail.com","subject":"Re: [PATCH 2/2] cleanup submodule_config a bit.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-12T21:34:03Z","receivedAt":"2015-08-12T21:34:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine wrote:\n> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:\n>> In the first hunk, `submodule` is NULL all the time, so we can make it clearer\n>> by directly returning NULL.\n>>\n>> In the second hunk, we can directly return the lookup values as it also makes\n>> the coder clearer.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule-config.c | 12 +++++-------\n>> 1 file changed, 5 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/submodule-config.c b/submodule-config.c\n>> index 199692b..08e93cc 100644\n>> --- a/submodule-config.c\n>> +++ b/submodule-config.c\n>> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>> }\n>>\n>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n>> - return submodule;\n>> + return NULL;\n>\n> There are a couple other places which return 'submodule' when it is\n> known to be NULL. One could rightly expect that they deserve the same\n> treatment, otherwise, the code becomes more confusing since it's not\n> obvious why 'return NULL' is used some places but not others.\n\nThey were slightly less obvious to me, fixed now as well!\n\n>\n>> switch (lookup_type) {\n>> case lookup_name:\n>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>>\n>> switch (lookup_type) {\n>> case lookup_name:\n>> - submodule = cache_lookup_name(cache, sha1, key);\n>> - break;\n>> + return cache_lookup_name(cache, sha1, key);\n>> case lookup_path:\n>> - submodule = cache_lookup_path(cache, sha1, key);\n>> - break;\n>> + return cache_lookup_path(cache, sha1, key);\n>> + default:\n>> + return NULL;\n>> }\n>> -\n>> - return submodule;\n>\n> Earlier in the function, there's effectively a clone of this logic to\n> which you could apply the same transformation. Changing it here, while\n> ignoring the clone, makes the code more confusing (or at least\n> inconsistent) rather than less.\n\nNot quite. Note the `if (submodule)` in the earlier version, so in case\ncache_lookup_{name, path} return NULL, we keep going. The change I\npropose is at the end of the function and we definitely return no matter\nif it is NULL or not.\n\n>\n>> }\n>>\n>> static const struct submodule *config_from_path(struct submodule_cache *cache,\n>> --\n>> 2.5.0.234.gefc8a62\n"},{"id":"267956","messageId":"CAPig+cT0+zh2v=NfB3RTwadfHoqB+VRTvbnMcdZ_g3RwtVzisg@mail.gmail.com","threadId":"40073","inReplyTo":"CAGZ79kbV45LqxCha516dRNYAjAH71mxce6Xazvfmf1deLMzUxQ@mail.gmail.com","subject":"Re: [PATCH 2/2] cleanup submodule_config a bit.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-12T22:01:32Z","receivedAt":"2015-08-12T22:01:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller wrote:\n> On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine wrote:\n>> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:\n>>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))\n>>> - return submodule;\n>>> + return NULL;\n>>\n>> There are a couple other places which return 'submodule' when it is\n>> known to be NULL. One could rightly expect that they deserve the same\n>> treatment, otherwise, the code becomes more confusing since it's not\n>> obvious why 'return NULL' is used some places but not others.\n>\n> They were slightly less obvious to me, fixed now as well!\n>\n>>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n>>> switch (lookup_type) {\n>>> case lookup_name:\n>>> - submodule = cache_lookup_name(cache, sha1, key);\n>>> - break;\n>>> + return cache_lookup_name(cache, sha1, key);\n>>> case lookup_path:\n>>> - submodule = cache_lookup_path(cache, sha1, key);\n>>> - break;\n>>> + return cache_lookup_path(cache, sha1, key);\n>>> + default:\n>>> + return NULL;\n>>> }\n>>> -\n>>> - return submodule;\n>>\n>> Earlier in the function, there's effectively a clone of this logic to\n>> which you could apply the same transformation. Changing it here, while\n>> ignoring the clone, makes the code more confusing (or at least\n>> inconsistent) rather than less.\n>\n> Not quite. Note the `if (submodule)` in the earlier version, so in case\n> cache_lookup_{name, path} return NULL, we keep going. The change I\n> propose is at the end of the function and we definitely return no matter\n> if it is NULL or not.\n\nOkay, cache_lookup_{name, path}() can indeed return NULL, so the same\ntransformation won't work. Sorry for the noise.\n"}]}