Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out

[Antonio Ospite <ao2@ao2.it>]

On Mon, 20 Aug 2018 23:37:55 +0200
Antonio Ospite <ao2@ao2.it> wrote:

> On Tue, 14 Aug 2018 13:36:17 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Antonio Ospite <ao2@ao2.it> writes:
[...]
> > >  
> > > +	# For more details about this check, see
> > > +	# builtin/submodule--helper.c::module_config()
> > > +	if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules > /dev/null 2>&1
> > 
[...]
> > More importantly, I think it is better to add a submodule--helper
> > subcommand that exposes the check in question, as the code is
> > already written ;-) That approach will guarantee that the logic and
> > the message stay the same between here and in the C code.  Then you
> > do not even need these two line comment.
> >
[...]
> Does the interface suggested in the patch annotation sound acceptable?
> 
> To recap:
> 
>   - add an is_gitmodules_safely_writeable() helper;
>   - expose a "submodule--helper config --is-safely-writeable"
>     subcommand for git-submodule.sh to use.
>

Maybe "submodule--helper config --check-writeable" could be a better
name to avoid confusion between the boolean return value of the C
function (0: false, 1: true) and the exit status returned to the shell
(0: safe to write, !0: unsafe).

I'll use the following to map the returned value, as I saw that in
other places in the code base:

	if (argc == 1 && command == CHECK_WRITEABLE)
		return is_gitmodules_safely_writeable() ? 0 : -1;

I am assuming a command flag to the "config" subcommand is OK instead
of a brand new subcommand.

> [...]
> > > @@ -603,8 +604,19 @@ static void submodule_cache_check_init(struct repository *repo)
> > >  static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
> > >  {
> > >  	if (repo->worktree) {
> > > -		char *file = repo_worktree_path(repo, GITMODULES_FILE);
> > > -		git_config_from_file(fn, file, data);
> > > +		struct git_config_source config_source = { 0 };
> > > +		const struct config_options opts = { 0 };
> > > +		struct object_id oid;
> > > +		char *file;
> > > +
> > > +		file = repo_worktree_path(repo, GITMODULES_FILE);
> > > +		if (file_exists(file))
> > > +			config_source.file = file;
> > > +		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
> > > +			config_source.blob = GITMODULES_HEAD;
> > 
> > What is the reason why we fall back directly to HEAD when working
> > tree file does not exist?  I thought that our usual fallback was to
> > the version in the index for other things like .gitignore/attribute
> > and this codepath look like an oddball.  Are you trying to handle
> > the case where we are in a bare repository without any file checked
> > out (and there is not even the index)?
> >
> 
> My use case is about *reading* .gitmodules when it's ignored in a sparse
> checkout, in this scenario there are usually no staged changes
> to .gitmodules, so I basically just didn't care about the index.
> 
> Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough?
> 
[...]
> 
> If so, what name should I use instead of GITMODULES_HEAD?
> GITMODULES_BLOB is already taken for something different, maybe
> GITMODULES_REF or GITMODULES_OBJECT?
>

If using ":.gitmodules" is good enough I could rename the current use
of GITMODULES_BLOB in fsck.c to GITMODULES_NONBLOB and use
GITMODULES_BLOB for ":.gitmodules" after all.

This is to avoid preprocessor clashes with the symbolic constant
GITMODULES_BLOB currently used in in fsck.c.

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?