Re: [PATCH 1/2] fix passing a name for config from submodules

From: Stefan Beller <sbeller@google.com>
Date: Jul 26, 2016, 17:22 UTC
Message-ID: <CAGZ79kaOf3NRAXh+krM=onwswSjAF3yy_zpa1d+9CFOBNke6-w@mail.gmail.com>
In-Reply-To: <20160726094913.GA3347@book.hvoigt.net>

On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@hvoigt.net> 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.

Show 8 quoted lines

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

Show 7 quoted lines

> @@ -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, &parameter);

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