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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Jul 26, 2016 at 10:22:07AM -0700, Stefan Beller wrote:
> On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt <hvoigt@hvoigt.net> 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, &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?)

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