submodule-config: clarify/cleanup docs and header

PATCH Superseded Stefan Beller 3 participants 12 messages Nov 21, 2016 – Nov 22, 2016 Documentation

lore

Share & tools

lore

Stefan Beller

author

2016-11-21 23:27 UTC #0 lore raw

v2 cover submodule-config: clarify/cleanup docs and header

replacing sb/submodule-config-cleanup

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

Thanks,
Stefan

interdiff to v1:

Documentation/technical/api-submodule-config.txt +1 -1

index 1df7a827ff..a91c1f085e 100644
@@ -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(-)

Signature · 1 line

2.11.0.rc2.18.g0126045.dirty

Stefan Beller

author

2016-11-21 23:27 UTC #1 lore raw

PATCH v2 1/3 submodule config: inline config_from_{name, path}

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

Signed-off-by Junio C Hamano <[email protected]>

---
 submodule-config.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

submodule-config.c +2 ~~-14~~

index 098085be69..15ffab6af4 100644
@@ -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)

Signature · 1 line

2.11.0.rc2.18.g0126045.dirty

Junio C Hamano

2016-11-22 03:27 UTC #2 lore raw

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

Quoted 7 lines · Stefan Beller

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 <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
---

Makes sense.

Stefan Beller

author

2016-11-21 23:27 UTC #3 lore raw

PATCH v2 2/3 submodule-config: rename commit_sha1 to commit_or_tree

In reply to Stefan Beller

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

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

Documentation/technical/api-submodule-config.txt +4 -4

index 941fa178dd..768458580f 100644
@@ -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).

submodule-config.c +24 ~~-23~~

163 lines not on this page · raw message

submodule-config.h +2 -2

index d05c542d2c..99df8e593c 100644
@@ -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);

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

index 47562ce465..301ed5e48f 100755
@@ -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: '[email protected]:a.git' for path 'b'
 Submodule url: '[email protected]:submodule.git' for path 'submodule'

Signature · 1 line

2.11.0.rc2.18.g0126045.dirty

Brandon Williams

2016-11-22 00:11 UTC #4 lore raw

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

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

Quoted 8 lines · Stefan Beller

submodule-config.c +1 -1

 	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

Signature · 1 line

Brandon Williams

Stefan Beller

author

2016-11-22 00:15 UTC #5 lore raw

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

Quoted 5 lines · Brandon Williams

Quoted 4 lines · Stefan Beller

      switch (lookup_type) {

(quoted diff) +0 -0

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

Quoted 9 lines

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

Brandon Williams

2016-11-22 00:17 UTC #6 lore raw

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

Quoted 13 lines · Stefan Beller

Quoted 12 lines · Brandon Williams
submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 451
Quoted 8 lines · Stefan Beller
      switch (lookup_type) {
submodule-config.c +1 -1
@@ -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

Quoted 5 lines

Quoted 2 lines
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 :)

Signature · 1 line

Brandon Williams

Stefan Beller

author

2016-11-22 01:25 UTC #7 lore raw

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

Quoted 14 lines · Brandon Williams

Quoted 13 lines · Stefan Beller
Quoted 12 lines · Brandon Williams
submodule-config.c static const struct submodule *config_from(struct submodule_cache *cache, · line 451
Quoted 8 lines · Stefan Beller
      switch (lookup_type) {
submodule-config.c +1 -1
@@ -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

Quoted 7 lines

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

Quoted 3 lines

--
Brandon Williams

Junio C Hamano

2016-11-22 03:37 UTC #8 lore raw

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

In reply to Stefan Beller

Quoted 6 lines · Stefan Beller

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

Yeah, I noticed that while reading the previous round, too, but...

Quoted 4 lines

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

Doesn't (the) "above", aka submodule_from_path() want the same treatment?

Also the explanation of "above" has room for improvement. Namely it says:

Lookup values for one submodule by its commit_sha1 and path.

I do not think the commit-sha1 (or commit-or-tree-object-name) is "ITS" object name for the submodule. The name belongs to the containing superproject commit (or tree), no?

Given a tree-ish in the superproject and a path, return the submodule that is bound at the path in the named tree.

is what I would write for that one. Thinking about it a bit further, "treeish_name" would be a much better name for the variable than "commit_or_tree", as "treeish" is an established short and sweet word that means "commit_or_tree", and having a "name" somewhere in the variable name makes it clear that we are not passing the pointer to an in-core object (e.g. "struct commit *").

t/t7411-submodule-config.sh test_expect_success 'error message contains blob reference' ' · line 105

Quoted 10 lines

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

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

Perhaps also test a tag that points at the commit?

Stefan Beller

author

2016-11-22 20:03 UTC #9 lore raw

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

Quoted 2 lines · Junio C Hamano

Doesn't (the) "above", aka submodule_from_path() want the same treatment?

fixed in a resend.

Quoted 6 lines

I do not think the commit-sha1 (or commit-or-tree-object-name) is "ITS" object name for the submodule. The name belongs to the containing superproject commit (or tree), no?
Given a tree-ish in the superproject and a path, return the submodule that is bound at the path in the named tree.

I'll take this.

Quoted 3 lines

is what I would write for that one. Thinking about it a bit further, "treeish_name" would be a much better name for the variable

ok, so treeish_name it is.

Quoted 3 lines

Quoted 1 line
+'
Perhaps also test a tag that points at the commit?

will fix in a resend.

Stefan Beller

author

2016-11-21 23:27 UTC #10 lore raw

PATCH v2 3/3 submodule-config: clarify parsing of null_sha1 element

In reply to Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 Documentation/technical/api-submodule-config.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Documentation/technical/api-submodule-config.txt +5 -2

index 768458580f..a91c1f085e 100644
@@ -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.

Signature · 1 line

2.11.0.rc2.18.g0126045.dirty

Brandon Williams

2016-11-22 00:09 UTC #11 lore raw

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

Quoted 11 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>
---
 Documentation/technical/api-submodule-config.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Documentation/technical/api-submodule-config.txt +0 -0
index 768458580f..a91c1f085e 100644

Documentation/technical/api-submodule-config.txt Functions · line 59

Quoted 7 lines

Documentation/technical/api-submodule-config.txt +2 -2

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

Quoted 9 lines

Documentation/technical/api-submodule-config.txt +3 -0

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

Signature · 1 line

2.11.0.rc2.18.g0126045.dirty

Signature · 1 line

Brandon Williams