[PATCHv2 0/3] submodule-config: clarify/cleanup docs and header

9 messages, 2 authors, 2016-11-22 · open the first message on its own page

[PATCHv2 0/3] submodule-config: clarify/cleanup docs and header

From: Stefan Beller <hidden>
Date: 2016-11-21 23:27:16

replacing sb/submodule-config-cleanup

v2:
addressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.

Thanks,
Stefan

interdiff to v1:

diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt

@@ -49,7 +49,7 @@ Functions
 
 `const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
 
-       Lookup values for one submodule by its commit_sha1 and path.
+       Lookup values for one submodule by its commit_or_tree and path.
 
 `const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::

v1:
A small series that would have helped me understand the submodule config
once again.

Thanks,
Stefan

Stefan Beller (3):
  submodule config: inline config_from_{name, path}
  submodule-config: rename commit_sha1 to commit_or_tree
  submodule-config: clarify parsing of null_sha1 element

 Documentation/technical/api-submodule-config.txt | 13 ++++--
 submodule-config.c                               | 59 ++++++++++--------------
 submodule-config.h                               |  4 +-
 t/t7411-submodule-config.sh                      | 11 +++++
 4 files changed, 45 insertions(+), 42 deletions(-)

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv2 1/3] submodule config: inline config_from_{name, path}

From: Stefan Beller <hidden>
Date: 2016-11-21 23:27:21

There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.

Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 submodule-config.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c

@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	return submodule;
 }
 
-static const struct submodule *config_from_path(struct submodule_cache *cache,
-		const unsigned char *commit_sha1, const char *path)
-{
-	return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
-		const unsigned char *commit_sha1, const char *name)
-{
-	return config_from(cache, commit_sha1, name, lookup_name);
-}
-
 static void ensure_cache_init(void)
 {
 	if (is_cache_init)

@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
 		const char *name)
 {
 	ensure_cache_init();
-	return config_from_name(&the_submodule_cache, commit_sha1, name);
+	return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
 }
 
 const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path)
 {
 	ensure_cache_init();
-	return config_from_path(&the_submodule_cache, commit_sha1, path);
+	return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
 }
 
 void submodule_free(void)

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Stefan Beller <hidden>
Date: 2016-11-21 23:27:24

It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.

Signed-off-by: Stefan Beller <redacted>
---
 Documentation/technical/api-submodule-config.txt |  8 ++--
 submodule-config.c                               | 47 ++++++++++++------------
 submodule-config.h                               |  4 +-
 t/t7411-submodule-config.sh                      | 11 ++++++
 4 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt

@@ -47,15 +47,15 @@ Functions
 	Can be passed to the config parsing infrastructure to parse
 	local (worktree) submodule configurations.
 
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char *commit_or_tree, const char *path)`::
 
-	Lookup values for one submodule by its commit_sha1 and path.
+	Lookup values for one submodule by its commit_or_tree and path.
 
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
 
 	The same as above but lookup by name.
 
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
 submodule will be returned (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).

diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..4c5f5d074b 100644
--- a/submodule-config.c
+++ b/submodule-config.c

@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, const char *arg)
 	return parse_push_recurse(opt, arg, 1);
 }
 
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
 				 const char *name, const char *option)
 {
 	const char *commit_string = "WORKTREE";
-	if (commit_sha1)
-		commit_string = sha1_to_hex(commit_sha1);
+	if (commit_or_tree)
+		commit_string = sha1_to_hex(commit_or_tree);
 	warning("%s:.gitmodules, multiple configurations found for "
 			"'submodule.%s.%s'. Skipping second one!",
 			commit_string, name, option);

@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,
 
 struct parse_config_parameter {
 	struct submodule_cache *cache;
-	const unsigned char *commit_sha1;
+	const unsigned char *commit_or_tree;
 	const unsigned char *gitmodules_sha1;
 	int overwrite;
 };

@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, void *data)
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->path)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					"path");
 		else {
 			if (submodule->path)

@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, void *data)
 		int die_on_error = is_null_sha1(me->gitmodules_sha1);
 		if (!me->overwrite &&
 		    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					"fetchrecursesubmodules");
 		else
 			submodule->fetch_recurse = parse_fetch_recurse(

@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, void *data)
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->ignore)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					"ignore");
 		else if (strcmp(value, "untracked") &&
 			 strcmp(value, "dirty") &&

@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, void *data)
 		if (!value) {
 			ret = config_error_nonbool(var);
 		} else if (!me->overwrite && submodule->url) {
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					"url");
 		} else {
 			free((void *) submodule->url);

@@ -351,21 +351,21 @@ static int parse_config(const char *var, const char *value, void *data)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite &&
 			 submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					     "update");
 		else if (parse_submodule_update_strategy(value,
 			 &submodule->update_strategy) < 0)
 				die(_("invalid value for %s"), var);
 	} else if (!strcmp(item.buf, "shallow")) {
 		if (!me->overwrite && submodule->recommend_shallow != -1)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					     "shallow");
 		else
 			submodule->recommend_shallow =
 				git_config_bool(var, value);
 	} else if (!strcmp(item.buf, "branch")) {
 		if (!me->overwrite && submodule->branch)
-			warn_multiple_config(me->commit_sha1, submodule->name,
+			warn_multiple_config(me->commit_or_tree, submodule->name,
 					     "branch");
 		else {
 			free((void *)submodule->branch);

@@ -379,18 +379,18 @@ static int parse_config(const char *var, const char *value, void *data)
 	return ret;
 }
 
-static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+static int gitmodule_sha1_from_commit(const unsigned char *commit_or_tree,
 				      unsigned char *gitmodules_sha1,
 				      struct strbuf *rev)
 {
 	int ret = 0;
 
-	if (is_null_sha1(commit_sha1)) {
+	if (is_null_sha1(commit_or_tree)) {
 		hashclr(gitmodules_sha1);
 		return 1;
 	}
 
-	strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+	strbuf_addf(rev, "%s:.gitmodules", sha1_to_hex(commit_or_tree));
 	if (get_sha1(rev->buf, gitmodules_sha1) >= 0)
 		ret = 1;

@@ -402,7 +402,7 @@ static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
  * revisions.
  */
 static const struct submodule *config_from(struct submodule_cache *cache,
-		const unsigned char *commit_sha1, const char *key,
+		const unsigned char *commit_or_tree, const char *key,
 		enum lookup_type lookup_type)
 {
 	struct strbuf rev = STRBUF_INIT;

@@ -418,7 +418,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	 * return the first submodule. Can be used to check whether
 	 * there are any submodules parsed.
 	 */
-	if (!commit_sha1 || !key) {
+	if (!commit_or_tree || !key) {
 		struct hashmap_iter iter;
 		struct submodule_entry *entry;

@@ -428,7 +428,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 		return entry->config;
 	}
 
-	if (!gitmodule_sha1_from_commit(commit_sha1, sha1, &rev))
+	if (!gitmodule_sha1_from_commit(commit_or_tree, sha1, &rev))
 		goto out;
 
 	switch (lookup_type) {

@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 
 	/* fill the submodule config into the cache */
 	parameter.cache = cache;
-	parameter.commit_sha1 = commit_sha1;
+	// todo: get the actual tree here:
+	parameter.commit_or_tree = commit_or_tree;
 	parameter.gitmodules_sha1 = sha1;
 	parameter.overwrite = 0;
 	git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,

@@ -484,7 +485,7 @@ int parse_submodule_config_option(const char *var, const char *value)
 {
 	struct parse_config_parameter parameter;
 	parameter.cache = &the_submodule_cache;
-	parameter.commit_sha1 = NULL;
+	parameter.commit_or_tree = NULL;
 	parameter.gitmodules_sha1 = null_sha1;
 	parameter.overwrite = 1;

@@ -492,18 +493,18 @@ int parse_submodule_config_option(const char *var, const char *value)
 	return parse_config(var, value, &parameter);
 }
 
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
 		const char *name)
 {
 	ensure_cache_init();
-	return config_from(&the_submodule_cache, commit_sha1, name, lookup_name);
+	return config_from(&the_submodule_cache, commit_or_tree, name, lookup_name);
 }
 
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
 		const char *path)
 {
 	ensure_cache_init();
-	return config_from(&the_submodule_cache, commit_sha1, path, lookup_path);
+	return config_from(&the_submodule_cache, commit_or_tree, path, lookup_path);
 }
 
 void submodule_free(void)

diff --git a/submodule-config.h b/submodule-config.h
index d05c542d2c..99df8e593c 100644
--- a/submodule-config.h
+++ b/submodule-config.h

@@ -25,9 +25,9 @@ struct submodule {
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
 int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
 		const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
+const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
 		const char *path);
 void submodule_free(void);

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..301ed5e48f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh

@@ -93,6 +93,17 @@ test_expect_success 'error message contains blob reference' '
 	)
 '
 
+test_expect_success 'using tree sha1 works' '
+	(
+		cd super &&
+		tree=$(git rev-parse HEAD^{tree}) &&
+		commit=$(git rev-parse HEAD^{commit}) &&
+		test-submodule-config $commit b >expect &&
+		test-submodule-config $tree b >actual &&
+		test_cmp expect actual
+	)
+'
+
 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.11.0.rc2.18.g0126045.dirty

[PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element

From: Stefan Beller <hidden>
Date: 2016-11-21 23:27:27

Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/technical/api-submodule-config.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 768458580f..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt

@@ -55,8 +55,11 @@ Functions
 
 	The same as above but lookup by name.
 
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
+zeroed out.  So in the normal case, when HEAD:.gitmodules is parsed first and
+then overlayed with the repository configuration, the null_sha1 entry contains
+the local configuration of a submodule (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
 For an example usage see test-submodule-config.c.

-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element

From: Brandon Williams <hidden>
Date: 2016-11-22 00:09:52

On 11/21, Stefan Beller wrote:

quoted hunk

Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/technical/api-submodule-config.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt
index 768458580f..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt

@@ -55,8 +55,11 @@ Functions
 
 	The same as above but lookup by name.
 
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1

s/will be overwrite/will overwrite

+zeroed out.  So in the normal case, when HEAD:.gitmodules is parsed first and
+then overlayed with the repository configuration, the null_sha1 entry contains
+the local configuration of a submodule (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
 For an example usage see test-submodule-config.c.
-- 
2.11.0.rc2.18.g0126045.dirty

-- 
Brandon Williams

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Brandon Williams <hidden>
Date: 2016-11-22 00:11:55

On 11/21, Stefan Beller wrote:

quoted hunk

 
 	switch (lookup_type) {

@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 
 	/* fill the submodule config into the cache */
 	parameter.cache = cache;
-	parameter.commit_sha1 = commit_sha1;
+	// todo: get the actual tree here:

s/todo/TODO

Makes it more clear that this is a TODO


-- 
Brandon Williams

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Stefan Beller <hidden>
Date: 2016-11-22 00:15:52

On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams [off-list ref] wrote:

On 11/21, Stefan Beller wrote:

quoted

      switch (lookup_type) {

@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,

      /* fill the submodule config into the cache */
      parameter.cache = cache;
-     parameter.commit_sha1 = commit_sha1;
+     // todo: get the actual tree here:

s/todo/TODO

Makes it more clear that this is a TODO

The // is more annoying here. I'll review these changes closely
before sending out v3.

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Brandon Williams <hidden>
Date: 2016-11-22 00:17:05

On 11/21, Stefan Beller wrote:

On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams [off-list ref] wrote:

quoted

On 11/21, Stefan Beller wrote:

quoted

      switch (lookup_type) {

@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,

      /* fill the submodule config into the cache */
      parameter.cache = cache;
-     parameter.commit_sha1 = commit_sha1;
+     // todo: get the actual tree here:

s/todo/TODO

Makes it more clear that this is a TODO

The // is more annoying here. I'll review these changes closely
before sending out v3.

Well I prefer // to /* */ but that's not the style we use :)

-- 
Brandon Williams

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Stefan Beller <hidden>
Date: 2016-11-22 01:25:28

On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams [off-list ref] wrote:

On 11/21, Stefan Beller wrote:

quoted

On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams [off-list ref] wrote:

quoted

On 11/21, Stefan Beller wrote:

quoted

      switch (lookup_type) {

@@ -448,7 +448,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,

      /* fill the submodule config into the cache */
      parameter.cache = cache;
-     parameter.commit_sha1 = commit_sha1;
+     // todo: get the actual tree here:

s/todo/TODO

Makes it more clear that this is a TODO

The // is more annoying here. I'll review these changes closely
before sending out v3.

Well I prefer // to /* */ but that's not the style we use :)

background: Initially I assume we would need to do work here as that part
is exposed to the user in error messages, such that we maybe want to
go the reverse direction and not state a tree but instead the commit containing
it. Since then I decided that it is not worth to optimize for some
hypothetical future
and hence I did not go with the internal todo note I put there. Then I
forgot about
it and it just showed up in the patch here.

Having looked through the patch again, the rest looks clean to me.

Stefan

--
Brandon Williams

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help