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

PATCH In review René Scharfe 4 participants 23 messages Jul 19, 2016 – Aug 17, 2016 Documentation

lore

Share & tools

lore

René Scharfe

author

2016-07-19 19:06 UTC #0 lore raw

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

Use a string constant instead of an empty strbuf to shorten the code and make it easier to read.

Signed-off-by Rene Scharfe <[email protected]>

---
... 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(-)

submodule-config.c +1 -2

index db1847f..44eb162 100644
@@ -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);

Signature · 1 line

2.9.2

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.

Stefan Beller

2016-07-19 19:24 UTC #2 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · 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 <[email protected]>
---
... unless someone can come up with a suitable non-empty string to feed to git_config_from_mem() as its name parameter.

Thanks!

Quoted 5 lines

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. :(

Quoted 2 lines

Will apply. Thanks.

Heiko Voigt

2016-07-20 08:31 UTC #3 lore raw

In reply to René Scharfe

Hi,

Quoted 2 lines · René Scharfe

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.

Quoted 4 lines

Signed-off-by Rene Scharfe <[email protected]>
---
... 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 <[email protected]>

---

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(-)

submodule-config.c +15 -8

index 077db40..dccea59 100644
@@ -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) {

Signature · 1 line

2.4.2.387.gf86f31a

René Scharfe

author

2016-07-21 18:57 UTC #4 lore raw

Am 20.07.2016 um 10:25 schrieb Heiko Voigt:

Quoted 18 lines

Hi,
Quoted 2 lines · René Scharfe
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.
Quoted 4 lines
Signed-off-by Rene Scharfe <[email protected]>
---
... 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():

Quoted 6 lines

	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.

Quoted 9 lines

---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.

Quoted 5 lines

Signed-off-by Heiko Voigt <[email protected]>
---
Its not exactly pretty with all the releases before the returns but this is what I could quickly come up with...

Indeed.

Quoted 57 lines

  submodule-config.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)

submodule-config.c +10 -8

index 077db40..dccea59 100644
@@ -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)

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 450

Quoted 10 lines

submodule-config.c +4 -0

+	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.

Quoted 9 lines

submodule-config.c +1 -0

@@ -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) {

Heiko Voigt

2016-07-25 14:37 UTC #5 lore raw

Hi,

Quoted 4 lines · René Scharfe

Quoted 4 lines
submodule-config.c +0 -0
index 077db40..dccea59 100644

[...]

Quoted 17 lines

submodule-config.c +6 -2

@@ -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;
+	}

Quoted 8 lines

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 450
Quoted 5 lines
submodule-config.c +1 -0
 	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

Junio C Hamano

2016-07-25 14:58 UTC #6 lore raw

Quoted 3 lines · Heiko Voigt

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 <[email protected]>
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.

Heiko Voigt

2016-07-26 09:51 UTC #7 lore raw

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

Signed-off-by Heiko Voigt <[email protected]>

---
This is based on 508a285 in next.

 submodule-config.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

submodule-config.c +17 -9

index 1210b26..853989e 100644
@@ -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) {

Signature · 1 line

2.4.2.387.gf86f31a

Stefan Beller

2016-07-26 17:22 UTC #8 lore raw

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

On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <[email protected]> wrote:

Thanks for continuing on the submodule cache!

Quoted 1 line

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.

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 402

Quoted 8 lines

submodule-config.c +2 -1

@@ -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.

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 436

Quoted 7 lines

submodule-config.c +1 -1

@@ -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

Junio C Hamano

2016-07-26 22:02 UTC #9 lore raw

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

Quoted 6 lines · Stefan Beller

Quoted 6 lines

submodule-config.c +1 -1

@@ -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,

Quoted 8 lines

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 436
Quoted 1 line
submodule-config.c +0 -0
                        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.

Heiko Voigt

2016-07-28 12:49 UTC #10 lore raw

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

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 <[email protected]>

---
Here you go. Now including a test.

 submodule-config.c          | 26 +++++++++++++++++---------
 t/t7411-submodule-config.sh | 11 +++++++++++
 2 files changed, 28 insertions(+), 9 deletions(-)

submodule-config.c +17 -9

index 1210b26..853989e 100644
@@ -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) {

t/t7411-submodule-config.sh +11 -0

index fc97c33..400e2b1 100755
@@ -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: '[email protected]:a.git' for path 'b'
 Submodule url: '[email protected]:submodule.git' for path 'submodule'

Signature · 1 line

2.0.2.832.g083c931

Stefan Beller

2016-07-28 16:26 UTC #11 lore raw

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

Quoted 7 lines · 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.

Quoted 6 lines

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 <[email protected]>
---
Here you go. Now including a test.

All 3 patches look good to me, thanks!

t/t7411-submodule-config.sh test_expect_success 'error in one submodule config lets continue' ' · line 92

Quoted 9 lines

t/t7411-submodule-config.sh +8 -0

+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!

Heiko Voigt

2016-07-28 12:49 UTC #12 lore raw

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

In reply to Junio C Hamano

So we have simpler return handling code and all the cleanup code in almost one place.

Signed-off-by Heiko Voigt <[email protected]>

---
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(-)

submodule-config.c +12 ~~-19~~

index 853989e..a887574 100644
@@ -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,

Signature · 1 line

2.0.2.832.g083c931

Heiko Voigt

2016-07-28 12:50 UTC #13 lore raw

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

In reply to Junio C Hamano

Since arg[0] will be NULL without any argument here and starts_with() does not like NULL-pointers.

Signed-off-by Heiko Voigt <[email protected]>

---
A small fix I found while developing the test.

 test-submodule-config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

test-submodule-config.c +1 -1

index dab8c27..a4e4098 100644
@@ -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"))

Signature · 1 line

2.0.2.832.g083c931

Heiko Voigt

2016-07-28 11:17 UTC #14 lore raw

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

In reply to Stefan Beller

Quoted 3 lines · Stefan Beller

On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <[email protected]> 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 ;)

Quoted 7 lines

Quoted 1 line
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).

Quoted 5 lines

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...

Quoted 4 lines

submodule-config.c +0 -0

@@ -397,8 +397,10 @@ static const struct submodule *config_from(struct submodule_cache *cache,
                return entry->config;
        }

Quoted 12 lines

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 402
Quoted 4 lines
submodule-config.c +2 -1
-       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.

Quoted 6 lines

submodule-config.c +1 -1

@@ -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,

Quoted 8 lines

submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 436
Quoted 1 line
submodule-config.c +0 -0
                        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

Heiko Voigt

2016-07-28 12:55 UTC #15 lore raw

PATCH 1/1 document how to reference previous commits

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 <[email protected]>

---

Quoted 12 lines · Heiko Voigt

Quoted 10 lines · Stefan Beller
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.

Quoted 3 lines

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(+)

Documentation/SubmittingPatches +5 -0

index e8ad978..500230c 100644
@@ -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.

Signature · 1 line

2.0.2.832.g083c931

Junio C Hamano

2016-07-28 15:38 UTC #16 lore raw

Re: [PATCH] document how to reference previous commits

Documentation/SubmittingPatches its behaviour. Try to make sure your explanation can be understood · line 128

Quoted 9 lines · Heiko Voigt

Documentation/SubmittingPatches +5 -0

@@ -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.

Stefan Beller

2016-07-28 15:57 UTC #17 lore raw

Re: [PATCH] document how to reference previous commits

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Heiko Voigt

Documentation/SubmittingPatches +0 -0

@@ -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.

Quoted 6 lines

Documentation/SubmittingPatches its behaviour. Try to make sure your explanation can be understood · line 128
Quoted 5 lines
Documentation/SubmittingPatches +5 -0
+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!

Heiko Voigt

2016-08-17 11:36 UTC #18 lore raw

Re: [PATCH] document how to reference previous commits

In reply to Heiko Voigt

Hi,

Quoted 4 lines · 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.

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

Junio C Hamano

2016-08-17 17:46 UTC #19 lore raw

Re: [PATCH] document how to reference previous commits

Quoted 9 lines · Heiko Voigt

Quoted 4 lines · 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.
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.

Junio C Hamano

2016-07-28 14:59 UTC #20 lore raw

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

In reply to Heiko Voigt

Quoted 3 lines · Heiko Voigt

Quoted 3 lines
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.

Quoted 4 lines

Quoted 2 lines
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.

Heiko Voigt

2016-07-26 09:49 UTC #21 lore raw

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

In reply to Junio C Hamano

So we have less return handling code.

Signed-off-by Heiko Voigt <[email protected]>

---
 submodule-config.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

submodule-config.c +1 -6

index 853989e..cb9bf8f 100644
@@ -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;

Signature · 1 line

2.4.2.387.gf86f31a

Stefan Beller

2016-07-26 17:24 UTC #22 lore raw

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

Quoted 3 lines · Heiko Voigt

So we have less return handling code.
Signed-off-by Heiko Voigt <[email protected]>

Thanks,
Stefan