On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html