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  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 
> ---
> 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!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
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(, "%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();
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, )) {
+   strbuf_release();
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();
return submodule;
+   }
 
config = read_sha1_file(sha1, , _size);
-   if (!config)
+   if (!config) {
+   strbuf_release();
return NULL;
+   }
 
if (type != OBJ_BLOB) {
+   strbuf_release();
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, );
+   strbuf_release();
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