[PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[René Scharfe]

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, &parameter);
 	free(config);
 
-- 
2.9.2

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Junio C Hamano]

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, &parameter);
>  	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.

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Stefan Beller]

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.

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[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?

---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, &parameter);
+	strbuf_release(&rev);
 	free(config);
 
 	switch (lookup_type) {
-- 
2.4.2.387.gf86f31a

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[René Scharfe]

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, &parameter);
> +	strbuf_release(&rev);
>   	free(config);
>
>   	switch (lookup_type) {
>

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Heiko Voigt]

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

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Junio C Hamano]

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.

[PATCH 1/2] fix passing a name for config from submodules

[Heiko Voigt]

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, &parameter);
+	strbuf_release(&rev);
 	free(config);
 
 	switch (lookup_type) {
-- 
2.4.2.387.gf86f31a

[PATCH 2/2] submodule-config: combine error checking if clauses

[Heiko Voigt]

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

Re: [PATCH 1/2] fix passing a name for config from submodules

[Stefan Beller]

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, &parameter);

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

Re: [PATCH 2/2] submodule-config: combine error checking if clauses

[Stefan Beller]

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

Re: [PATCH 1/2] fix passing a name for config from submodules

[Junio C Hamano]

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, &parameter);
>
> 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.

Re: [PATCH 1/2] fix passing a name for config from submodules

[Heiko Voigt]

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, &parameter);
> 
> 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

[PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs

[Heiko Voigt]

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, &parameter);
+	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

[PATCH 2/3] submodule-config: combine early return code into one goto

[Heiko Voigt]

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

[PATCH 3/3] submodule-config: fix test binary crashing when no arguments given

[Heiko Voigt]

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

[PATCH] document how to reference previous commits

[Heiko Voigt]

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

Re: [PATCH 1/2] fix passing a name for config from submodules

[Junio C Hamano]

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.

Re: [PATCH] document how to reference previous commits

[Junio C Hamano]

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.

Re: [PATCH] document how to reference previous commits

[Stefan Beller]

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!

Re: [PATCH 1/3] submodule-config: passing name reference for .gitmodule blobs

[Stefan Beller]

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!

Re: [PATCH] document how to reference previous commits

[Heiko Voigt]

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

Re: [PATCH] document how to reference previous commits

[Junio C Hamano]

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.