* [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() @ 2016-07-19 19:05 René Scharfe 2016-07-19 19:15 ` Junio C Hamano 2016-07-20 8:25 ` Heiko Voigt 0 siblings, 2 replies; 23+ messages in thread From: René Scharfe @ 2016-07-19 19:05 UTC (permalink / raw) To: Git List; +Cc: Stefan Beller, Lars Schneider, Heiko Voigt Use a string constant instead of an empty strbuf to shorten the code and make it easier to read. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- ... unless someone can come up with a suitable non-empty string to feed to git_config_from_mem() as its name parameter. submodule-config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index db1847f..44eb162 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -397,7 +397,6 @@ static const struct submodule *config_from(struct submodule_cache *cache, const unsigned char *commit_sha1, const char *key, enum lookup_type lookup_type) { - struct strbuf rev = STRBUF_INIT; unsigned long config_size; char *config; unsigned char sha1[20]; @@ -448,7 +447,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, parameter.commit_sha1 = commit_sha1; parameter.gitmodules_sha1 = sha1; parameter.overwrite = 0; - git_config_from_mem(parse_config, "submodule-blob", rev.buf, + git_config_from_mem(parse_config, "submodule-blob", "", config, config_size, ¶meter); free(config); -- 2.9.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-19 19:05 [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() René Scharfe @ 2016-07-19 19:15 ` Junio C Hamano 2016-07-19 19:24 ` Stefan Beller 2016-07-20 8:25 ` Heiko Voigt 1 sibling, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2016-07-19 19:15 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Stefan Beller, Lars Schneider, Heiko Voigt René Scharfe <l.s.r@web.de> writes: > Use a string constant instead of an empty strbuf to shorten the code > and make it easier to read. > > Signed-off-by: Rene Scharfe <l.s.r@web.de> > --- > ... unless someone can come up with a suitable non-empty string to feed > to git_config_from_mem() as its name parameter. > > submodule-config.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/submodule-config.c b/submodule-config.c > index db1847f..44eb162 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -397,7 +397,6 @@ static const struct submodule *config_from(struct submodule_cache *cache, > const unsigned char *commit_sha1, const char *key, > enum lookup_type lookup_type) > { > - struct strbuf rev = STRBUF_INIT; > unsigned long config_size; > char *config; > unsigned char sha1[20]; > @@ -448,7 +447,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, > parameter.commit_sha1 = commit_sha1; > parameter.gitmodules_sha1 = sha1; > parameter.overwrite = 0; > - git_config_from_mem(parse_config, "submodule-blob", rev.buf, > + git_config_from_mem(parse_config, "submodule-blob", "", > config, config_size, ¶meter); > free(config); Oh. I wondered if we used to do something useful with rev and later that useful thing was removed leaving an always-empty strbuf, but it appears that this strbuf was introduced to always hold an empty string and nothing else, which shows the (lack of) quality of reviews in this area X-<. Will apply. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-19 19:15 ` Junio C Hamano @ 2016-07-19 19:24 ` Stefan Beller 0 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2016-07-19 19:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: René Scharfe, Git List, Lars Schneider, Heiko Voigt On Tue, Jul 19, 2016 at 12:15 PM, Junio C Hamano <gitster@pobox.com> wrote: > René Scharfe <l.s.r@web.de> writes: > >> Use a string constant instead of an empty strbuf to shorten the code >> and make it easier to read. >> >> Signed-off-by: Rene Scharfe <l.s.r@web.de> >> --- >> ... unless someone can come up with a suitable non-empty string to feed >> to git_config_from_mem() as its name parameter. Thanks! > I wondered if we used to do something useful with rev and later that > useful thing was removed leaving an always-empty strbuf, but it > appears that this strbuf was introduced to always hold an empty > string and nothing else, which shows the (lack of) quality of > reviews in this area X-<. I am to blame for this. :( > > Will apply. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-19 19:05 [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() René Scharfe 2016-07-19 19:15 ` Junio C Hamano @ 2016-07-20 8:25 ` Heiko Voigt 2016-07-21 18:57 ` René Scharfe 1 sibling, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-07-20 8:25 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Stefan Beller, Lars Schneider, Junio C Hamano Hi, On Tue, Jul 19, 2016 at 09:05:43PM +0200, René Scharfe wrote: > Use a string constant instead of an empty strbuf to shorten the code > and make it easier to read. This must have been some oversight from my original code. I also can not see any purpose. > Signed-off-by: Rene Scharfe <l.s.r@web.de> > --- > ... unless someone can come up with a suitable non-empty string to feed > to git_config_from_mem() as its name parameter. If we would want to be absolutely correct we could use something like "SHA1:.gitmodules". E.g. like we use to lookup the blob in gitmodule_sha1_from_commit(): strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); And now I see where this was leftover from... before extracting this function this code was filling the strbuf. How about this instead? ---8<------ Subject: [PATCH] fix passing a name for config from submodules In commit 959b5455 we implemented the initial version of the submodule config cache. During development of that initial version we extracted the function gitmodule_sha1_from_commit(). During that process we missed that the strbuf rev was still used in config_from() and now is left empty. Lets fix this by also returning this string. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- Its not exactly pretty with all the releases before the returns but this is what I could quickly come up with... submodule-config.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 077db40..dccea59 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -371,9 +371,9 @@ static int parse_config(const char *var, const char *value, void *data) } static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, - unsigned char *gitmodules_sha1) + unsigned char *gitmodules_sha1, + struct strbuf *rev) { - struct strbuf rev = STRBUF_INIT; int ret = 0; if (is_null_sha1(commit_sha1)) { @@ -381,11 +381,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, return 1; } - strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); - if (get_sha1(rev.buf, gitmodules_sha1) >= 0) + strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); + if (get_sha1(rev->buf, gitmodules_sha1) >= 0) ret = 1; - strbuf_release(&rev); return ret; } @@ -420,8 +419,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, return entry->config; } - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { + strbuf_release(&rev); return NULL; + } switch (lookup_type) { case lookup_name: @@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache, submodule = cache_lookup_path(cache, sha1, key); break; } - if (submodule) + if (submodule) { + strbuf_release(&rev); return submodule; + } config = read_sha1_file(sha1, &type, &config_size); - if (!config) + if (!config) { + strbuf_release(&rev); return NULL; + } if (type != OBJ_BLOB) { + strbuf_release(&rev); free(config); return NULL; } @@ -450,6 +456,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, parameter.overwrite = 0; git_config_from_mem(parse_config, "submodule-blob", rev.buf, config, config_size, ¶meter); + strbuf_release(&rev); free(config); switch (lookup_type) { -- 2.4.2.387.gf86f31a ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-20 8:25 ` Heiko Voigt @ 2016-07-21 18:57 ` René Scharfe 2016-07-25 14:37 ` Heiko Voigt 0 siblings, 1 reply; 23+ messages in thread From: René Scharfe @ 2016-07-21 18:57 UTC (permalink / raw) To: Heiko Voigt; +Cc: Git List, Stefan Beller, Lars Schneider, Junio C Hamano Am 20.07.2016 um 10:25 schrieb Heiko Voigt: > Hi, > > On Tue, Jul 19, 2016 at 09:05:43PM +0200, René Scharfe wrote: >> Use a string constant instead of an empty strbuf to shorten the code >> and make it easier to read. > > This must have been some oversight from my original code. I also can not > see any purpose. > >> Signed-off-by: Rene Scharfe <l.s.r@web.de> >> --- >> ... unless someone can come up with a suitable non-empty string to feed >> to git_config_from_mem() as its name parameter. > > If we would want to be absolutely correct we could use something like > "SHA1:.gitmodules". E.g. like we use to lookup the blob in > gitmodule_sha1_from_commit(): > > strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); > > And now I see where this was leftover from... before extracting this > function this code was filling the strbuf. > > How about this instead? I like it. > ---8<------ > Subject: [PATCH] fix passing a name for config from submodules > > In commit 959b5455 we implemented the initial version of the submodule > config cache. During development of that initial version we extracted > the function gitmodule_sha1_from_commit(). During that process we missed > that the strbuf rev was still used in config_from() and now is left > empty. Lets fix this by also returning this string. > > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> > --- > > Its not exactly pretty with all the releases before the returns but > this is what I could quickly come up with... Indeed. > submodule-config.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/submodule-config.c b/submodule-config.c > index 077db40..dccea59 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -371,9 +371,9 @@ static int parse_config(const char *var, const char *value, void *data) > } > > static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, > - unsigned char *gitmodules_sha1) > + unsigned char *gitmodules_sha1, > + struct strbuf *rev) > { > - struct strbuf rev = STRBUF_INIT; > int ret = 0; > > if (is_null_sha1(commit_sha1)) { > @@ -381,11 +381,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, > return 1; > } > > - strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); > - if (get_sha1(rev.buf, gitmodules_sha1) >= 0) > + strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); > + if (get_sha1(rev->buf, gitmodules_sha1) >= 0) > ret = 1; > > - strbuf_release(&rev); > return ret; > } > > @@ -420,8 +419,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, > return entry->config; > } > > - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) > + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { > + strbuf_release(&rev); > return NULL; > + } > > switch (lookup_type) { > case lookup_name: > @@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache, > submodule = cache_lookup_path(cache, sha1, key); > break; > } > - if (submodule) > + if (submodule) { > + strbuf_release(&rev); > return submodule; > + } > > config = read_sha1_file(sha1, &type, &config_size); > - if (!config) > + if (!config) { > + strbuf_release(&rev); > return NULL; > + } > > if (type != OBJ_BLOB) { > + strbuf_release(&rev); > free(config); > return NULL; > } A separate patch could combine the previous two conditionals; free(NULL) is allowed. > @@ -450,6 +456,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, > parameter.overwrite = 0; > git_config_from_mem(parse_config, "submodule-blob", rev.buf, > config, config_size, ¶meter); > + strbuf_release(&rev); > free(config); > > switch (lookup_type) { > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-21 18:57 ` René Scharfe @ 2016-07-25 14:37 ` Heiko Voigt 2016-07-25 14:58 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-07-25 14:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Stefan Beller, Lars Schneider, René Scharfe Hi, On Thu, Jul 21, 2016 at 08:57:03PM +0200, René Scharfe wrote: > >diff --git a/submodule-config.c b/submodule-config.c > >index 077db40..dccea59 100644 > >--- a/submodule-config.c > >+++ b/submodule-config.c [...] > >@@ -431,14 +432,19 @@ static const struct submodule *config_from(struct submodule_cache *cache, > > submodule = cache_lookup_path(cache, sha1, key); > > break; > > } > >- if (submodule) > >+ if (submodule) { > >+ strbuf_release(&rev); > > return submodule; > >+ } > > > > config = read_sha1_file(sha1, &type, &config_size); > >- if (!config) > >+ if (!config) { > >+ strbuf_release(&rev); > > return NULL; > >+ } > > > > if (type != OBJ_BLOB) { > >+ strbuf_release(&rev); > > free(config); > > return NULL; > > } > > A separate patch could combine the previous two conditionals; free(NULL) is > allowed. Thats a good idea. Can send a patch, but I noticed that your change is already in next and my change has not been picked up. So I am unsure if I should base my patches on next or if Junio wants to keep your change? Cheers Heiko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() 2016-07-25 14:37 ` Heiko Voigt @ 2016-07-25 14:58 ` Junio C Hamano 2016-07-26 9:49 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 2016-07-26 9:49 ` [PATCH 2/2] submodule-config: combine error checking if clauses Heiko Voigt 0 siblings, 2 replies; 23+ messages in thread From: Junio C Hamano @ 2016-07-25 14:58 UTC (permalink / raw) To: Heiko Voigt; +Cc: Git List, Stefan Beller, Lars Schneider, René Scharfe Heiko Voigt <hvoigt@hvoigt.net> writes: > Thats a good idea. Can send a patch, but I noticed that your change is > already in next and my change has not been picked up. So I am unsure if > I should base my patches on next or if Junio wants to keep your change? When you noticed something you want to touch up in 'next' but not in 'master', please try to find which topic the target of your touching-up comes from. git log --first-parent <other options> master..next I often use --oneline for <other options>, because I am reasonably familiar with the topics in 'next', but people may start from "git blame" output, finding the commit that left the text you are touching up, then where it was merged to 'next'. That would give you commit 59dbd5834007c1e4dff25770da095babcb69a009 Merge: 679f992 508a285 Author: Junio C Hamano <gitster@pobox.com> Date: Tue Jul 19 13:43:44 2016 -0700 Merge branch 'rs/submodule-config-code-cleanup' into next Code cleanup. * rs/submodule-config-code-cleanup: submodule-config: use explicit empty string instead of strbuf in config_from() The second parent 508a285c (submodule-config: use explicit empty string instead of strbuf in config_from(), 2016-07-19) is a good place to base your patch. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] fix passing a name for config from submodules 2016-07-25 14:58 ` Junio C Hamano @ 2016-07-26 9:49 ` Heiko Voigt 2016-07-26 17:22 ` Stefan Beller 2016-07-26 9:49 ` [PATCH 2/2] submodule-config: combine error checking if clauses Heiko Voigt 1 sibling, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-07-26 9:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Stefan Beller, Lars Schneider, René Scharfe In commit 959b5455 we implemented the initial version of the submodule config cache. During development of that initial version we extracted the function gitmodule_sha1_from_commit(). During that process we missed that the strbuf rev was still used in config_from() and now is left empty. Lets fix this by also returning this string. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- This is based on 508a285 in next. submodule-config.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 1210b26..853989e 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -349,9 +349,9 @@ static int parse_config(const char *var, const char *value, void *data) } static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, - unsigned char *gitmodules_sha1) + unsigned char *gitmodules_sha1, + struct strbuf *rev) { - struct strbuf rev = STRBUF_INIT; int ret = 0; if (is_null_sha1(commit_sha1)) { @@ -359,11 +359,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, return 1; } - strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); - if (get_sha1(rev.buf, gitmodules_sha1) >= 0) + strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); + if (get_sha1(rev->buf, gitmodules_sha1) >= 0) ret = 1; - strbuf_release(&rev); return ret; } @@ -375,6 +374,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, const unsigned char *commit_sha1, const char *key, enum lookup_type lookup_type) { + struct strbuf rev = STRBUF_INIT; unsigned long config_size; char *config; unsigned char sha1[20]; @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, return entry->config; } - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { + strbuf_release(&rev); return NULL; + } switch (lookup_type) { case lookup_name: @@ -408,14 +410,19 @@ static const struct submodule *config_from(struct submodule_cache *cache, submodule = cache_lookup_path(cache, sha1, key); break; } - if (submodule) + if (submodule) { + strbuf_release(&rev); return submodule; + } config = read_sha1_file(sha1, &type, &config_size); - if (!config) + if (!config) { + strbuf_release(&rev); return NULL; + } if (type != OBJ_BLOB) { + strbuf_release(&rev); free(config); return NULL; } @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache, parameter.commit_sha1 = commit_sha1; parameter.gitmodules_sha1 = sha1; parameter.overwrite = 0; - git_config_from_mem(parse_config, "submodule-blob", "", + git_config_from_mem(parse_config, "submodule-blob", rev.buf, config, config_size, ¶meter); + strbuf_release(&rev); free(config); switch (lookup_type) { -- 2.4.2.387.gf86f31a ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] fix passing a name for config from submodules 2016-07-26 9:49 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt @ 2016-07-26 17:22 ` Stefan Beller 2016-07-26 22:02 ` Junio C Hamano 2016-07-28 11:17 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 0 siblings, 2 replies; 23+ messages in thread From: Stefan Beller @ 2016-07-26 17:22 UTC (permalink / raw) To: Heiko Voigt; +Cc: Junio C Hamano, Git List, Lars Schneider, René Scharfe On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: Thanks for continuing on the submodule cache! > In commit 959b5455 we implemented the initial version of the submodule Usually we refer to the commit by a triple of "abbrev. sha1 (date, subject). See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free) for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings similarly to those in documentation). Apparently we put the subject first and then the date. I always did it the other way round, to there is no strict coding guide line, though it helps a lot to have an understanding for a) how long are we in the "broken" state already as well as b) what was the rationale for introducing it. > @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, > return entry->config; > } > > - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) > + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { > + strbuf_release(&rev); > return NULL; This is a reoccuring pattern below. Maybe it might make sense to just do a s/return.../ goto out/ and at that label we cleanup `rev` and `config` and return a result value? There are currently 6 early returns (not counting the 3 from the last switch), 4 of them return NULL, so that would result in just a "goto out", whereas 2 return an actual value, they would need to assign the result value first before jumping out of the logic. I dunno, just food for though. > @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache, > parameter.commit_sha1 = commit_sha1; > parameter.gitmodules_sha1 = sha1; > parameter.overwrite = 0; > - git_config_from_mem(parse_config, "submodule-blob", "", > + git_config_from_mem(parse_config, "submodule-blob", rev.buf, > config, config_size, ¶meter); Ok, this is the actual fix. Do you want to demonstrate its impact by adding one or two tests that failed before and now work? (As I was using the submodule config API most of the time with null_sha1 to indicate we'd be looking at the current .gitmodules file in the worktree, the actual bug may have not manifested in the users of this API. But still, it would be nice to see what was broken?) Thanks, Stefan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] fix passing a name for config from submodules 2016-07-26 17:22 ` Stefan Beller @ 2016-07-26 22:02 ` Junio C Hamano 2016-07-28 12:49 ` [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs Heiko Voigt ` (2 more replies) 2016-07-28 11:17 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 1 sibling, 3 replies; 23+ messages in thread From: Junio C Hamano @ 2016-07-26 22:02 UTC (permalink / raw) To: Stefan Beller; +Cc: Heiko Voigt, Git List, Lars Schneider, René Scharfe Stefan Beller <sbeller@google.com> writes: >> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache, >> parameter.commit_sha1 = commit_sha1; >> parameter.gitmodules_sha1 = sha1; >> parameter.overwrite = 0; >> - git_config_from_mem(parse_config, "submodule-blob", "", >> + git_config_from_mem(parse_config, "submodule-blob", rev.buf, >> config, config_size, ¶meter); > > Ok, this is the actual fix. Do you want to demonstrate its impact by adding > one or two tests that failed before and now work? > (As I was using the submodule config API most of the time with null_sha1 > to indicate we'd be looking at the current .gitmodules file in the worktree, > the actual bug may have not manifested in the users of this API. > But still, it would be nice to see what was broken?) Sounds like a good idea. I'll keep these two queued on 'pu' and see if Heiko (or somebody else) can find time to do that, so that we can replace them with an improved version when it happens. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs 2016-07-26 22:02 ` Junio C Hamano @ 2016-07-28 12:49 ` Heiko Voigt 2016-07-28 16:26 ` Stefan Beller 2016-07-28 12:49 ` [PATCH 2/3] submodule-config: combine early return code into one goto Heiko Voigt 2016-07-28 12:50 ` [PATCH 3/3] submodule-config: fix test binary crashing when no arguments given Heiko Voigt 2 siblings, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-07-28 12:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Commit 959b5455 (submodule: implement a config API for lookup of .gitmodules values, 2015-08-18) implemented the initial version of the submodule config cache. During development of that initial version we extracted the function gitmodule_sha1_from_commit(). During that process we missed that the strbuf rev was still used in config_from() and now is left empty. Lets fix this by also returning this string. This means that now when reading .gitmodules from revisions, the error messages also contain a reference to the blob they are from. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- Here you go. Now including a test. submodule-config.c | 26 +++++++++++++++++--------- t/t7411-submodule-config.sh | 11 +++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 1210b26..853989e 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -349,9 +349,9 @@ static int parse_config(const char *var, const char *value, void *data) } static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, - unsigned char *gitmodules_sha1) + unsigned char *gitmodules_sha1, + struct strbuf *rev) { - struct strbuf rev = STRBUF_INIT; int ret = 0; if (is_null_sha1(commit_sha1)) { @@ -359,11 +359,10 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, return 1; } - strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); - if (get_sha1(rev.buf, gitmodules_sha1) >= 0) + strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1)); + if (get_sha1(rev->buf, gitmodules_sha1) >= 0) ret = 1; - strbuf_release(&rev); return ret; } @@ -375,6 +374,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, const unsigned char *commit_sha1, const char *key, enum lookup_type lookup_type) { + struct strbuf rev = STRBUF_INIT; unsigned long config_size; char *config; unsigned char sha1[20]; @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, return entry->config; } - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { + strbuf_release(&rev); return NULL; + } switch (lookup_type) { case lookup_name: @@ -408,14 +410,19 @@ static const struct submodule *config_from(struct submodule_cache *cache, submodule = cache_lookup_path(cache, sha1, key); break; } - if (submodule) + if (submodule) { + strbuf_release(&rev); return submodule; + } config = read_sha1_file(sha1, &type, &config_size); - if (!config) + if (!config) { + strbuf_release(&rev); return NULL; + } if (type != OBJ_BLOB) { + strbuf_release(&rev); free(config); return NULL; } @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache, parameter.commit_sha1 = commit_sha1; parameter.gitmodules_sha1 = sha1; parameter.overwrite = 0; - git_config_from_mem(parse_config, "submodule-blob", "", + git_config_from_mem(parse_config, "submodule-blob", rev.buf, config, config_size, ¶meter); + strbuf_release(&rev); free(config); switch (lookup_type) { diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index fc97c33..400e2b1 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,6 +82,17 @@ test_expect_success 'error in one submodule config lets continue' ' ) ' +test_expect_success 'error message contains blob reference' ' + (cd super && + sha1=$(git rev-parse HEAD) && + test-submodule-config \ + HEAD b \ + HEAD submodule \ + 2>actual_err && + grep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + ) +' + cat >super/expect_url <<EOF Submodule url: 'git@somewhere.else.net:a.git' for path 'b' Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule' -- 2.0.2.832.g083c931 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs 2016-07-28 12:49 ` [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs Heiko Voigt @ 2016-07-28 16:26 ` Stefan Beller 0 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2016-07-28 16:26 UTC (permalink / raw) To: Heiko Voigt; +Cc: Junio C Hamano, Git List, Lars Schneider, René Scharfe On Thu, Jul 28, 2016 at 5:49 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: > Commit 959b5455 (submodule: implement a config API for lookup of > .gitmodules values, 2015-08-18) implemented the initial version of the > submodule config cache. During development of that initial version we > extracted the function gitmodule_sha1_from_commit(). During that process > we missed that the strbuf rev was still used in config_from() and now is > left empty. Lets fix this by also returning this string. > > This means that now when reading .gitmodules from revisions, the error > messages also contain a reference to the blob they are from. > > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> > --- > Here you go. Now including a test. All 3 patches look good to me, thanks! > > +test_expect_success 'error message contains blob reference' ' > + (cd super && > + sha1=$(git rev-parse HEAD) && > + test-submodule-config \ > + HEAD b \ > + HEAD submodule \ > + 2>actual_err && > + grep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null Makes sense! ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/3] submodule-config: combine early return code into one goto 2016-07-26 22:02 ` Junio C Hamano 2016-07-28 12:49 ` [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs Heiko Voigt @ 2016-07-28 12:49 ` Heiko Voigt 2016-07-28 12:50 ` [PATCH 3/3] submodule-config: fix test binary crashing when no arguments given Heiko Voigt 2 siblings, 0 replies; 23+ messages in thread From: Heiko Voigt @ 2016-07-28 12:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe So we have simpler return handling code and all the cleanup code in almost one place. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- This is an updated cleanup patch. Now with goto so we have simpler code. submodule-config.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 853989e..a887574 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -376,7 +376,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, { struct strbuf rev = STRBUF_INIT; unsigned long config_size; - char *config; + char *config = NULL; unsigned char sha1[20]; enum object_type type; const struct submodule *submodule = NULL; @@ -397,10 +397,8 @@ static const struct submodule *config_from(struct submodule_cache *cache, return entry->config; } - if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { - strbuf_release(&rev); - return NULL; - } + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) + goto out; switch (lookup_type) { case lookup_name: @@ -410,22 +408,12 @@ static const struct submodule *config_from(struct submodule_cache *cache, submodule = cache_lookup_path(cache, sha1, key); break; } - if (submodule) { - strbuf_release(&rev); - return submodule; - } + if (submodule) + goto out; config = read_sha1_file(sha1, &type, &config_size); - if (!config) { - strbuf_release(&rev); - return NULL; - } - - if (type != OBJ_BLOB) { - strbuf_release(&rev); - free(config); - return NULL; - } + if (!config || type != OBJ_BLOB) + goto out; /* fill the submodule config into the cache */ parameter.cache = cache; @@ -445,6 +433,11 @@ static const struct submodule *config_from(struct submodule_cache *cache, default: return NULL; } + +out: + strbuf_release(&rev); + free(config); + return submodule; } static const struct submodule *config_from_path(struct submodule_cache *cache, -- 2.0.2.832.g083c931 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] submodule-config: fix test binary crashing when no arguments given 2016-07-26 22:02 ` Junio C Hamano 2016-07-28 12:49 ` [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs Heiko Voigt 2016-07-28 12:49 ` [PATCH 2/3] submodule-config: combine early return code into one goto Heiko Voigt @ 2016-07-28 12:50 ` Heiko Voigt 2 siblings, 0 replies; 23+ messages in thread From: Heiko Voigt @ 2016-07-28 12:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Since arg[0] will be NULL without any argument here and starts_with() does not like NULL-pointers. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- A small fix I found while developing the test. test-submodule-config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-submodule-config.c b/test-submodule-config.c index dab8c27..a4e4098 100644 --- a/test-submodule-config.c +++ b/test-submodule-config.c @@ -23,7 +23,7 @@ int main(int argc, char **argv) arg++; my_argc--; - while (starts_with(arg[0], "--")) { + while (arg[0] && starts_with(arg[0], "--")) { if (!strcmp(arg[0], "--url")) output_url = 1; if (!strcmp(arg[0], "--name")) -- 2.0.2.832.g083c931 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] fix passing a name for config from submodules 2016-07-26 17:22 ` Stefan Beller 2016-07-26 22:02 ` Junio C Hamano @ 2016-07-28 11:17 ` Heiko Voigt 2016-07-28 12:55 ` [PATCH] document how to reference previous commits Heiko Voigt 2016-07-28 14:59 ` [PATCH 1/2] fix passing a name for config from submodules Junio C Hamano 1 sibling, 2 replies; 23+ messages in thread From: Heiko Voigt @ 2016-07-28 11:17 UTC (permalink / raw) To: Stefan Beller; +Cc: Junio C Hamano, Git List, Lars Schneider, René Scharfe On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote: > On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: > > Thanks for continuing on the submodule cache! No worries. Its my code so I am happy to fix any bugs in it. Although it was obviously perfect from the beginning ;) > > In commit 959b5455 we implemented the initial version of the submodule > > Usually we refer to the commit by a triple of "abbrev. sha1 (date, subject). > See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free) > for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings > similarly to those in documentation). > > Apparently we put the subject first and then the date. I always did it > the other way > round, to there is no strict coding guide line, though it helps a lot to have an > understanding for a) how long are we in the "broken" state already as well as > b) what was the rationale for introducing it. Ah ok did not know about this format. Will change that. I also will follow-up with a patch to document this in SubmittingPatches so we can point others to that... > > @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache, > > return entry->config; > > } > > > > - if (!gitmodule_sha1_from_commit(commit_sha1, sha1)) > > + if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) { > > + strbuf_release(&rev); > > return NULL; > > This is a reoccuring pattern below. Maybe it might make sense to > just do a s/return.../ goto out/ and at that label we cleanup `rev` and `config` > and return a result value? > There are currently 6 early returns (not counting the 3 from the last switch), > 4 of them return NULL, so that would result in just a "goto out", whereas 2 > return an actual value, they would need to assign the result value first before > jumping out of the logic. I dunno, just food for though. I also though about that but was not sure whether it would actually make things simple. Will look into that as the second patch. > > @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct submodule_cache *cache, > > parameter.commit_sha1 = commit_sha1; > > parameter.gitmodules_sha1 = sha1; > > parameter.overwrite = 0; > > - git_config_from_mem(parse_config, "submodule-blob", "", > > + git_config_from_mem(parse_config, "submodule-blob", rev.buf, > > config, config_size, ¶meter); > > Ok, this is the actual fix. Do you want to demonstrate its impact by adding > one or two tests that failed before and now work? > (As I was using the submodule config API most of the time with null_sha1 > to indicate we'd be looking at the current .gitmodules file in the worktree, > the actual bug may have not manifested in the users of this API. > But still, it would be nice to see what was broken?) This popped up because of Rene's cleanup patch and I wanted to provide a patch of what was originally supposed to go in there. The name (originally representing the filename of the parsed config) is put into the structure that represents the source. I had a quick look and it seems to mostly be used in error messages. E.g.: * in error message git_parse_source() to reference the file * error message in git_die_config_linenr() (filename is derived from name) There is some more, but I will add a test which checks whether the error message actually contains a reference to the blob instead of nothing. E.g. looks like this: error: bad config line 7 in submodule-blob 39a7458d2b5b3e3d1938b01ff2645b14c94ac284:.gitmodules instead of this error: bad config line 7 in submodule-blob That might be quite helpful to find out where the error is when you have one in the history. So we are fixing a real bug here (not just a theoretical one). Cheers Heiko ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] document how to reference previous commits 2016-07-28 11:17 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt @ 2016-07-28 12:55 ` Heiko Voigt 2016-07-28 15:38 ` Junio C Hamano 2016-08-17 11:36 ` Heiko Voigt 2016-07-28 14:59 ` [PATCH 1/2] fix passing a name for config from submodules Junio C Hamano 1 sibling, 2 replies; 23+ messages in thread From: Heiko Voigt @ 2016-07-28 12:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe To reference previous commits people used to put just the abbreviated SHA-1 into commit messages. This is what has evolved as a more stable format for referencing commits. So lets document it for everyone to lookup when needed. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- On Thu, Jul 28, 2016 at 01:16:36PM +0200, Heiko Voigt wrote: > On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote: > > Usually we refer to the commit by a triple of "abbrev. sha1 (date, subject). > > See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free) > > for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings > > similarly to those in documentation). > > > > Apparently we put the subject first and then the date. I always did it > > the other way > > round, to there is no strict coding guide line, though it helps a lot to have an > > understanding for a) how long are we in the "broken" state already as well as > > b) what was the rationale for introducing it. > > Ah ok did not know about this format. Will change that. I also will > follow-up with a patch to document this in SubmittingPatches so we can > point others to that... Here we go. Made this a seperate patch, since it is not really connected to the submodule-config-fix series. Cheers Heiko Documentation/SubmittingPatches | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e8ad978..500230c 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood without external resources. Instead of giving a URL to a mailing list archive, summarize the relevant points of the discussion. +If you want to reference a previous commit in the history of a stable +branch use the format "abbreviated sha1 (subject, date)". So for example +like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) +noticed [...]". + (3) Generate your patch using Git tools out of your commits. -- 2.0.2.832.g083c931 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] document how to reference previous commits 2016-07-28 12:55 ` [PATCH] document how to reference previous commits Heiko Voigt @ 2016-07-28 15:38 ` Junio C Hamano 2016-07-28 15:57 ` Stefan Beller 2016-08-17 11:36 ` Heiko Voigt 1 sibling, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2016-07-28 15:38 UTC (permalink / raw) To: Heiko Voigt; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Heiko Voigt <hvoigt@hvoigt.net> writes: > @@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood > without external resources. Instead of giving a URL to a mailing list > archive, summarize the relevant points of the discussion. > > +If you want to reference a previous commit in the history of a stable > +branch use the format "abbreviated sha1 (subject, date)". So for example > +like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) > +noticed [...]". > + I earlier said that our goal should not be to spell out all the conventions, which would lead to unreadably long document, but this does not look too bad. Small additions however tend to accumulate over time, though ;-) I wondered if we need to also say "why", but we probably shouldn't. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] document how to reference previous commits 2016-07-28 15:38 ` Junio C Hamano @ 2016-07-28 15:57 ` Stefan Beller 0 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2016-07-28 15:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Heiko Voigt, Git List, Lars Schneider, René Scharfe On Thu, Jul 28, 2016 at 8:38 AM, Junio C Hamano <gitster@pobox.com> wrote: > Heiko Voigt <hvoigt@hvoigt.net> writes: > >> @@ -121,6 +121,11 @@ its behaviour. Try to make sure your explanation can be understood >> without external resources. Instead of giving a URL to a mailing list >> archive, summarize the relevant points of the discussion. >> >> +If you want to reference a previous commit in the history of a stable >> +branch use the format "abbreviated sha1 (subject, date)". So for example >> +like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) >> +noticed [...]". >> + > Thanks! ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] document how to reference previous commits 2016-07-28 12:55 ` [PATCH] document how to reference previous commits Heiko Voigt 2016-07-28 15:38 ` Junio C Hamano @ 2016-08-17 11:36 ` Heiko Voigt 2016-08-17 17:45 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-08-17 11:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Hi, On Thu, Jul 28, 2016 at 02:55:14PM +0200, Heiko Voigt wrote: > To reference previous commits people used to put just the abbreviated > SHA-1 into commit messages. This is what has evolved as a more > stable format for referencing commits. So lets document it for everyone > to lookup when needed. A quick ping about this patch. Maybe you missed to include it Junio? I can not find any reference to it in the cooking mails and in your repository. Cheers Heiko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] document how to reference previous commits 2016-08-17 11:36 ` Heiko Voigt @ 2016-08-17 17:45 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2016-08-17 17:45 UTC (permalink / raw) To: Heiko Voigt; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Heiko Voigt <hvoigt@hvoigt.net> writes: > On Thu, Jul 28, 2016 at 02:55:14PM +0200, Heiko Voigt wrote: >> To reference previous commits people used to put just the abbreviated >> SHA-1 into commit messages. This is what has evolved as a more >> stable format for referencing commits. So lets document it for everyone >> to lookup when needed. > > A quick ping about this patch. Maybe you missed to include it Junio? I > can not find any reference to it in the cooking mails and in your > repository. That's because I was undecided. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] fix passing a name for config from submodules 2016-07-28 11:17 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 2016-07-28 12:55 ` [PATCH] document how to reference previous commits Heiko Voigt @ 2016-07-28 14:59 ` Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2016-07-28 14:59 UTC (permalink / raw) To: Heiko Voigt; +Cc: Stefan Beller, Git List, Lars Schneider, René Scharfe Heiko Voigt <hvoigt@hvoigt.net> writes: >> Apparently we put the subject first and then the date. I always did it >> the other way >> round, to there is no strict coding guide line,... Please don't say "this is not spelled out in the guidelines, so I can do whatever I like, *EVEN* *THOUGH* I know that is different from what most others do". Without the "follow the best-current-practice and be consistent", which I think is in an early part of the guideline document (and if not, it should be spelled out as the most important rule), the guidelines document will become too long that nobody would read. >> understanding for a) how long are we in the "broken" state already as well as >> b) what was the rationale for introducing it. > > Ah ok did not know about this format. Will change that. In other words, In commit 959b5455 we implemented the initial version of ... would become something like: 959b5455 (submodule: implement a config API for lookup of .gitmodules values, 2015-08-17) implemented the initial version of ... Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2] submodule-config: combine error checking if clauses 2016-07-25 14:58 ` Junio C Hamano 2016-07-26 9:49 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt @ 2016-07-26 9:49 ` Heiko Voigt 2016-07-26 17:24 ` Stefan Beller 1 sibling, 1 reply; 23+ messages in thread From: Heiko Voigt @ 2016-07-26 9:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Stefan Beller, Lars Schneider, René Scharfe So we have less return handling code. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- submodule-config.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index 853989e..cb9bf8f 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -416,12 +416,7 @@ static const struct submodule *config_from(struct submodule_cache *cache, } config = read_sha1_file(sha1, &type, &config_size); - if (!config) { - strbuf_release(&rev); - return NULL; - } - - if (type != OBJ_BLOB) { + if (!config || type != OBJ_BLOB) { strbuf_release(&rev); free(config); return NULL; -- 2.4.2.387.gf86f31a ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/2] submodule-config: combine error checking if clauses 2016-07-26 9:49 ` [PATCH 2/2] submodule-config: combine error checking if clauses Heiko Voigt @ 2016-07-26 17:24 ` Stefan Beller 0 siblings, 0 replies; 23+ messages in thread From: Stefan Beller @ 2016-07-26 17:24 UTC (permalink / raw) To: Heiko Voigt; +Cc: Junio C Hamano, Git List, Lars Schneider, René Scharfe On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: > So we have less return handling code. > > Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> Thanks, Stefan ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-08-17 17:46 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-19 19:05 [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from() René Scharfe 2016-07-19 19:15 ` Junio C Hamano 2016-07-19 19:24 ` Stefan Beller 2016-07-20 8:25 ` Heiko Voigt 2016-07-21 18:57 ` René Scharfe 2016-07-25 14:37 ` Heiko Voigt 2016-07-25 14:58 ` Junio C Hamano 2016-07-26 9:49 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 2016-07-26 17:22 ` Stefan Beller 2016-07-26 22:02 ` Junio C Hamano 2016-07-28 12:49 ` [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs Heiko Voigt 2016-07-28 16:26 ` Stefan Beller 2016-07-28 12:49 ` [PATCH 2/3] submodule-config: combine early return code into one goto Heiko Voigt 2016-07-28 12:50 ` [PATCH 3/3] submodule-config: fix test binary crashing when no arguments given Heiko Voigt 2016-07-28 11:17 ` [PATCH 1/2] fix passing a name for config from submodules Heiko Voigt 2016-07-28 12:55 ` [PATCH] document how to reference previous commits Heiko Voigt 2016-07-28 15:38 ` Junio C Hamano 2016-07-28 15:57 ` Stefan Beller 2016-08-17 11:36 ` Heiko Voigt 2016-08-17 17:45 ` Junio C Hamano 2016-07-28 14:59 ` [PATCH 1/2] fix passing a name for config from submodules Junio C Hamano 2016-07-26 9:49 ` [PATCH 2/2] submodule-config: combine error checking if clauses Heiko Voigt 2016-07-26 17:24 ` Stefan Beller
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).