Re: [PATCH 1/2] fix passing a name for config from submodules
Heiko Voigt 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. -- 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
Re: [PATCH 1/2] fix passing a name for config from submodules
On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote:
> On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt 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, ¶meter);
>
> 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
--
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
Re: [PATCH 1/2] fix passing a name for config from submodules
Stefan Beller 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, ¶meter); > > 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. -- 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
Re: [PATCH 1/2] fix passing a name for config from submodules
On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt 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, ¶meter);
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
--
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/2] 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
---
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, ¶meter);
+ strbuf_release(&rev);
free(config);
switch (lookup_type) {
--
2.4.2.387.gf86f31a
--
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
