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

[David Turner <dturner@twopensource.com>]

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!