On Wed, 2016-02-03 at 21:58 -0500, Jeff King wrote:
> On Thu, Feb 04, 2016 at 01:54:44AM +0000, Ramsay Jones wrote:
>
> > > They were working for me as-of the time I sent them. I guess
> > > something
> > > must have broken since. I'll rebase, test, and send a new
> > > series.
> >
> > I didn't spend too long looking at it, but I think this interacts
> > with
> > Jeff's patch a2d5156c ("resolve_gitlink_ref: ignore non-repository
> > paths",
> > 22-01-2016) which introduces the new test in 't3000-ls-files
> > -others.sh'
> > which fails for me.
> >
> > The change which Jeff made to resolve_gitlink_ref() is effectively
> > side-stepped
> > by the call to check_submodule_backend() in the new
> > resolve_gitlink_ref().
> > (Jeff's change is now in the 'files' backend version of
> > resolve_gitlink_ref()).
>
> Yeah. The check_submodule_backend() function calls
> strbuf_git_path_submodule(), which unconditionally requires that the
> path be an actual submodule (the irony being that we are using it to
> find out whether we have a submodule!). So I don't think there's a
> conflict between our code, so much as that the new code has the same
> bug
> I fixed in a2d5156c (and we didn't notice earlier, because there was
> no
> test).
>
> The solution in a2d5156 is to use is_nonbare_repository_dir() before
> assuming we have a submodule. I think check_submodule_backend() would
> want to do the same thing. This is the minimal fix:
>
> diff --git a/refs.c b/refs.c
> index 3d4c0a6..7f86c49 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -313,9 +313,8 @@ static void check_submodule_backend(const char
> *submodule)
> if (!submodule)
> goto done;
>
> - strbuf_git_path_submodule(&sb, submodule, "%s", "");
> -
> - if (!is_git_directory(sb.buf))
> + strbuf_addstr(&sb, submodule);
> + if (!is_nonbare_repository_dir(&sb))
> goto done;
>
> strbuf_reset(&sb);
+ one other place, yeah, that does fix it.
> That gets the test passing. But I noticed a few other things, some
> related and some unrelated, while looking at this function:
>
> - in files_resolve_gitlink_ref, if we do find a submodule, we cache
> the result with the ref_cache code. But here, we would read the
> .git
> file repeatedly (and in fact, it happens twice per call, as
> submodule_git_path has to read it itself).
>
> I don't know if it would be worth adding any kind of caching at
> this
> layer. It may be that we don't typically resolve more than one
> ref
> (or do more than one for_each_ref iteration) for a submodule, so
> the
> cache is pointless. I didn't implement it specifically in
> a2d5156,
> it just came for free with the existing ref_cache code.
I'm going to punt on this for now; we can fix it if it becomes a
problem.
> - check_submodule_backend knows whether we have a submodule at all
> and
> is worth proceeding, but does not tell its callers. So we'll end
> up
> in the backend files_resolve_gitlink_ref and make the same check.
> It's probably worth moving this logic to the outer layer so each
> backend doesn't have to implement it (and then the check in
> files_resolve_gitlink_ref can actually go away).
Fixed, thanks.
- for the common case of submodule==NULL (i.e., the main repository),
> check_submodule_backend should be a noop, but it allocates and
> frees
> the submodule_storage_backend string. Probably not a huge deal,
> but
> it can be easily bumped down, and the first "goto done" turned
> into
> a "return".
Fixed, thanks.
> - if the submodule does have a backend configured, we leak the
> memory
> for the default string. I think the submodule_backend() config
> callback needs to free() the previous value.
Fixed.
> - the config callback unconditionally dereferences "value", which
> will
> segfault if the submodule's extensions.refstorage is a pure
> boolean
> like:
>
> [extensions]
> refstorage
Fixed.
> That should never happen, of course, but we should be checking
> and
> dying for "!value" rather than segfaulting. Using
> git_config_string() will do this for you.
>
> Hope that helps.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
