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@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html