Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
- From
Antonio Ospite <ao2@ao2.it>
- Date
- Aug 22, 2018, 11:51 UTC
- Message-ID
- <20180822135152.1d40cd05d0b0cadb5eefb31f@ao2.it>
- In-Reply-To
- <20180820233755.dc7b6a6927faccc37b25075f@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:
[...]
Show 5 quoted lines
> > > > > > + # 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 > >
[...]
Show 6 quoted lines
> > 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. > >
[...]
Show 8 quoted lines
> 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.
Show 32 quoted lines
> [...]
> > > @@ -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?
> [...]
Show 5 quoted lines
> > 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?