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 <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 ;)
Show 12 quoted lines
> > 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...
Show 16 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.I also though about that but was not sure whether it would actually make things simple. Will look into that as the second patch.
Show 14 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, ¶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
René Scharfe
