* Re: [PATCH v5 0/4] submodule config lookup API @ 2015-08-12 19:13 Stefan Beller 2015-08-12 19:13 ` [PATCH 1/2] Fixup hv/documentation Stefan Beller ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Stefan Beller @ 2015-08-12 19:13 UTC (permalink / raw) To: gitster Cc: hvoigt, git, jens.lehmann, jrnieder, peff, wking, sunshine, karsten.blees, Stefan Beller On Wed, Aug 12, 2015 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> On Mon, Jun 15, 2015 at 2:48 PM, Junio C Hamano <gitster@pobox.com> wrote: >>> Thanks. Will replace and wait for comments from others. >> >> I have reviewed the patches carefully and they look good to me. > > OK, I recall there were a few iterations with review comments before > this round. Is it your impression that they have been addressed > adequately? I payed only little attention to the previous rounds and the review comments thereof, because round 5 was up for quite a long time and nobody commented so far. However just as I was convinced of my review and sent out the email, I started working with it. And I found nits which I'd ask you to squash into the round or put on top. > > Do you prefer it to be rebased to a more recent 'master' before you > build your work on top of it (I think the topic currently builds on > top of v2.5.0-rc0~56)? Looking at the reviews for the "RFC parallel fetch for submodules" I'd like to use `argv_array_pushv` which was introduced via 85b343245b (2015-06-14, argv-array: implement argv_array_pushv()) and is already in origin/next, but not origin/master. But as I first work on the submodule--helper (both the small helper functions as well as the whole "update" thereafter), I think this dependency is not a pressing issue yet. > > Thanks. Stefan Beller (2): Fixup hv/documentation cleanup submodule_config a bit. Documentation/technical/api-submodule-config.txt | 3 +-- submodule-config.c | 12 +++++------- 2 files changed, 6 insertions(+), 9 deletions(-) -- 2.5.0.234.gefc8a62 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Fixup hv/documentation 2015-08-12 19:13 [PATCH v5 0/4] submodule config lookup API Stefan Beller @ 2015-08-12 19:13 ` Stefan Beller 2015-08-12 19:13 ` [PATCH 2/2] cleanup submodule_config a bit Stefan Beller 2015-08-12 19:27 ` [PATCH v5 0/4] submodule config lookup API Junio C Hamano 2 siblings, 0 replies; 7+ messages in thread From: Stefan Beller @ 2015-08-12 19:13 UTC (permalink / raw) To: gitster Cc: hvoigt, git, jens.lehmann, jrnieder, peff, wking, sunshine, karsten.blees, Stefan Beller If you want to look up by name, use `submodule_from_name` instead. Signed-off-by: Stefan Beller <sbeller@google.com> --- Documentation/technical/api-submodule-config.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt index 2ea520a..941fa17 100644 --- a/Documentation/technical/api-submodule-config.txt +++ b/Documentation/technical/api-submodule-config.txt @@ -49,8 +49,7 @@ Functions `const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`:: - Lookup values for one submodule by its commit_sha1 and path or - name. + Lookup values for one submodule by its commit_sha1 and path. `const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`:: -- 2.5.0.234.gefc8a62 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] cleanup submodule_config a bit. 2015-08-12 19:13 [PATCH v5 0/4] submodule config lookup API Stefan Beller 2015-08-12 19:13 ` [PATCH 1/2] Fixup hv/documentation Stefan Beller @ 2015-08-12 19:13 ` Stefan Beller 2015-08-12 21:13 ` Eric Sunshine 2015-08-12 19:27 ` [PATCH v5 0/4] submodule config lookup API Junio C Hamano 2 siblings, 1 reply; 7+ messages in thread From: Stefan Beller @ 2015-08-12 19:13 UTC (permalink / raw) To: gitster Cc: hvoigt, git, jens.lehmann, jrnieder, peff, wking, sunshine, karsten.blees, Stefan Beller In the first hunk, `submodule` is NULL all the time, so we can make it clearer by directly returning NULL. In the second hunk, we can directly return the lookup values as it also makes the coder clearer. Signed-off-by: Stefan Beller <sbeller@google.com> --- submodule-config.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 199692b..08e93cc 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, } if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) - return submodule; + return NULL; switch (lookup_type) { case lookup_name: @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache, switch (lookup_type) { case lookup_name: - submodule = cache_lookup_name(cache, sha1, key); - break; + return cache_lookup_name(cache, sha1, key); case lookup_path: - submodule = cache_lookup_path(cache, sha1, key); - break; + return cache_lookup_path(cache, sha1, key); + default: + return NULL; } - - return submodule; } static const struct submodule *config_from_path(struct submodule_cache *cache, -- 2.5.0.234.gefc8a62 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cleanup submodule_config a bit. 2015-08-12 19:13 ` [PATCH 2/2] cleanup submodule_config a bit Stefan Beller @ 2015-08-12 21:13 ` Eric Sunshine 2015-08-12 21:34 ` Stefan Beller 0 siblings, 1 reply; 7+ messages in thread From: Eric Sunshine @ 2015-08-12 21:13 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Heiko Voigt, Git List, Jens Lehmann, Jonathan Nieder, Jeff King, W. Trevor King, Karsten Blees On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@google.com> wrote: > In the first hunk, `submodule` is NULL all the time, so we can make it clearer > by directly returning NULL. > > In the second hunk, we can directly return the lookup values as it also makes > the coder clearer. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > submodule-config.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/submodule-config.c b/submodule-config.c > index 199692b..08e93cc 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, > } > > if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) > - return submodule; > + return NULL; There are a couple other places which return 'submodule' when it is known to be NULL. One could rightly expect that they deserve the same treatment, otherwise, the code becomes more confusing since it's not obvious why 'return NULL' is used some places but not others. > switch (lookup_type) { > case lookup_name: > @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache, > > switch (lookup_type) { > case lookup_name: > - submodule = cache_lookup_name(cache, sha1, key); > - break; > + return cache_lookup_name(cache, sha1, key); > case lookup_path: > - submodule = cache_lookup_path(cache, sha1, key); > - break; > + return cache_lookup_path(cache, sha1, key); > + default: > + return NULL; > } > - > - return submodule; Earlier in the function, there's effectively a clone of this logic to which you could apply the same transformation. Changing it here, while ignoring the clone, makes the code more confusing (or at least inconsistent) rather than less. > } > > static const struct submodule *config_from_path(struct submodule_cache *cache, > -- > 2.5.0.234.gefc8a62 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cleanup submodule_config a bit. 2015-08-12 21:13 ` Eric Sunshine @ 2015-08-12 21:34 ` Stefan Beller 2015-08-12 22:01 ` Eric Sunshine 0 siblings, 1 reply; 7+ messages in thread From: Stefan Beller @ 2015-08-12 21:34 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Heiko Voigt, Git List, Jens Lehmann, Jonathan Nieder, Jeff King, W. Trevor King, Karsten Blees On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: > On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@google.com> wrote: >> In the first hunk, `submodule` is NULL all the time, so we can make it clearer >> by directly returning NULL. >> >> In the second hunk, we can directly return the lookup values as it also makes >> the coder clearer. >> >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> submodule-config.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/submodule-config.c b/submodule-config.c >> index 199692b..08e93cc 100644 >> --- a/submodule-config.c >> +++ b/submodule-config.c >> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, >> } >> >> if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) >> - return submodule; >> + return NULL; > > There are a couple other places which return 'submodule' when it is > known to be NULL. One could rightly expect that they deserve the same > treatment, otherwise, the code becomes more confusing since it's not > obvious why 'return NULL' is used some places but not others. They were slightly less obvious to me, fixed now as well! > >> switch (lookup_type) { >> case lookup_name: >> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache, >> >> switch (lookup_type) { >> case lookup_name: >> - submodule = cache_lookup_name(cache, sha1, key); >> - break; >> + return cache_lookup_name(cache, sha1, key); >> case lookup_path: >> - submodule = cache_lookup_path(cache, sha1, key); >> - break; >> + return cache_lookup_path(cache, sha1, key); >> + default: >> + return NULL; >> } >> - >> - return submodule; > > Earlier in the function, there's effectively a clone of this logic to > which you could apply the same transformation. Changing it here, while > ignoring the clone, makes the code more confusing (or at least > inconsistent) rather than less. Not quite. Note the `if (submodule)` in the earlier version, so in case cache_lookup_{name, path} return NULL, we keep going. The change I propose is at the end of the function and we definitely return no matter if it is NULL or not. > >> } >> >> static const struct submodule *config_from_path(struct submodule_cache *cache, >> -- >> 2.5.0.234.gefc8a62 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cleanup submodule_config a bit. 2015-08-12 21:34 ` Stefan Beller @ 2015-08-12 22:01 ` Eric Sunshine 0 siblings, 0 replies; 7+ messages in thread From: Eric Sunshine @ 2015-08-12 22:01 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, Heiko Voigt, Git List, Jens Lehmann, Jonathan Nieder, Jeff King, W. Trevor King, Karsten Blees On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller <sbeller@google.com> wrote: > On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote: >> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller <sbeller@google.com> wrote: >>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) >>> - return submodule; >>> + return NULL; >> >> There are a couple other places which return 'submodule' when it is >> known to be NULL. One could rightly expect that they deserve the same >> treatment, otherwise, the code becomes more confusing since it's not >> obvious why 'return NULL' is used some places but not others. > > They were slightly less obvious to me, fixed now as well! > >>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct submodule_cache *cache, >>> switch (lookup_type) { >>> case lookup_name: >>> - submodule = cache_lookup_name(cache, sha1, key); >>> - break; >>> + return cache_lookup_name(cache, sha1, key); >>> case lookup_path: >>> - submodule = cache_lookup_path(cache, sha1, key); >>> - break; >>> + return cache_lookup_path(cache, sha1, key); >>> + default: >>> + return NULL; >>> } >>> - >>> - return submodule; >> >> Earlier in the function, there's effectively a clone of this logic to >> which you could apply the same transformation. Changing it here, while >> ignoring the clone, makes the code more confusing (or at least >> inconsistent) rather than less. > > Not quite. Note the `if (submodule)` in the earlier version, so in case > cache_lookup_{name, path} return NULL, we keep going. The change I > propose is at the end of the function and we definitely return no matter > if it is NULL or not. Okay, cache_lookup_{name, path}() can indeed return NULL, so the same transformation won't work. Sorry for the noise. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 0/4] submodule config lookup API 2015-08-12 19:13 [PATCH v5 0/4] submodule config lookup API Stefan Beller 2015-08-12 19:13 ` [PATCH 1/2] Fixup hv/documentation Stefan Beller 2015-08-12 19:13 ` [PATCH 2/2] cleanup submodule_config a bit Stefan Beller @ 2015-08-12 19:27 ` Junio C Hamano 2 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2015-08-12 19:27 UTC (permalink / raw) To: Stefan Beller Cc: hvoigt, git, jens.lehmann, jrnieder, peff, wking, sunshine, karsten.blees Stefan Beller <sbeller@google.com> writes: > However just as I was convinced of my review and sent out the email, I started > working with it. And I found nits which I'd ask you to squash into the round or > put on top. Good ;-). I'd prefer a full reroll, as it has been quite a while since v5 was originally posted, once you get to a reasonable state where you are reasonably confident that you won't find more of such nits. Hopefully more people might have eyes on the list to review and comment this round. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-08-12 22:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-12 19:13 [PATCH v5 0/4] submodule config lookup API Stefan Beller 2015-08-12 19:13 ` [PATCH 1/2] Fixup hv/documentation Stefan Beller 2015-08-12 19:13 ` [PATCH 2/2] cleanup submodule_config a bit Stefan Beller 2015-08-12 21:13 ` Eric Sunshine 2015-08-12 21:34 ` Stefan Beller 2015-08-12 22:01 ` Eric Sunshine 2015-08-12 19:27 ` [PATCH v5 0/4] submodule config lookup API Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).