Re: [PATCH v3 00/20] refs backend rebase on pu

From: Jeff King <peff@peff.net>
Date: Feb 4, 2016, 02:58 UTC
Message-ID: <20160204025821.GA21105@sigill.intra.peff.net>
In-Reply-To: <56B2AF64.5020502@ramsayjones.plus.com>

On Thu, Feb 04, 2016 at 01:54:44AM +0000, Ramsay Jones wrote:

Show 11 quoted lines

> > 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);


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.

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

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

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

  - the config callback unconditionally dereferences "value", which will
    segfault if the submodule's extensions.refstorage is a pure boolean
    like:

        [extensions]
	refstorage

    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.

-Peff