On Mon, 20 Aug 2018 23:37:55 +0200
Antonio Ospite <ao2@xxxxxx> wrote:
> On Tue, 14 Aug 2018 13:36:17 -0700
> Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> > Antonio Ospite <ao2@xxxxxx> 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?
