Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
On Tue, 14 Aug 2018 13:36:17 -0700 Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
>
> > /* Equivalent to ACTION_SET in builtin/config.c */
> > - if (argc == 3)
> > + if (argc == 3) {
> > + struct object_id oid;
> > +
> > + /*
> > + * If the .gitmodules file is not in the working tree but it
> > + * is in the current branch, stop, as writing new values (and
> > + * staging them) would blindly overwrite ALL the old content.
>
> Hmph, "the file is missing" certainly is a condition we would want
> to notice, but wouldn't we in general want to prevent us from
> overwriting any local modification, where "missing" is merely a very
> special case of local modification? I am wondering if we would want
> to stop if .gitmodules file exists both in the working tree and in
> the index, and the contents of them differ, or something like that.
>TTBOMK checking the index status (with something like is_staging_gitmodules_ok()) when the .gitmodules file *exists* in the working tree would break calling "git submodule add" multiple times before committing the changes.
Show 20 quoted lines
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index ff258e2e8c..b1cb187227 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -159,6 +159,13 @@ cmd_add()
> > shift
> > done
> >
> > + # 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
>
> No SP between redirection '>' and its target '/dev/null'.
>
> 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.
>
Yeah I anticipated this concern in the patch annotation, but I was hoping that it would be OK to have this as a followup change.
I guess I can do it for v4 instead.
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.[...]
Show 24 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?
By reading man gitrevisions(7) and after a quick test with "git cat-file blob :.gitmodules" it looks like this would be more in line with your suggestion, still covering my use case.
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?
Show 57 quoted lines
> > diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
> > new file mode 100755
> > index 0000000000..5341e9b012
> > --- /dev/null
> > +++ b/t/t7416-submodule-sparse-gitmodules.sh
> > @@ -0,0 +1,90 @@
> > +#!/bin/sh
> > +#
> > +# Copyright (C) 2018 Antonio Ospite <ao2@ao2.it>
> > +#
> > +
> > +test_description='Test reading/writing .gitmodules when not in the working tree
> > +
> > +This test verifies that, when .gitmodules is in the current branch but is not
> > +in the working tree reading from it still works but writing to it does not.
> > +
> > +The test setup uses a sparse checkout, however the same scenario can be set up
> > +also by committing .gitmodules and then just removing it from the filesystem.
> > +'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'sparse checkout setup which hides .gitmodules' '
> > + echo file > file &&
>
> No SP between redirection '>' and its target 'file'.
>
> > + git add file &&
> > + test_tick &&
> > + git commit -m upstream &&
> > + git clone . super &&
> > + git clone super submodule &&
> > + git clone super new_submodule &&
> > + (cd super &&
> > + git submodule add ../submodule &&
> > + test_tick &&
> > + git commit -m submodule &&
> > + cat >.git/info/sparse-checkout <<\EOF &&
> > +/*
> > +!/.gitmodules
> > +EOF
>
> You can use <<-\EOF and indent the body of the here-doc, which makes
> the result easier to read, i.e.
>
> cat >target <<-\EOF &&
> line 1
> line 2
> EOF
>
> > + git config core.sparsecheckout true &&
> > + git read-tree -m -u HEAD &&
>
> That's old fashioned---I am curious if this has to be one-way merge
> or can just be a usual "git checkout" (I am merely curious; not
> suggesting to change anything).
>
It was just how I learned to set up a sparse checkout, if there is a better way I'd be happy to use it.
Show 14 quoted lines
> > + test_path_is_missing .gitmodules
> > + )
> > +'
> > +
> > +test_expect_success 'reading gitmodules config file when it is not checked out' '
> > + (cd super &&
> > + echo "../submodule" >expected &&
> > + git submodule--helper config submodule.submodule.url >actual &&
> > + test_cmp expected actual
>
> A minor style thing, but I thought that it was more common in our
> tests to call the expected output 'expect' (which has the same
> number of letters as 'actual') than 'expected'.
>
Show 9 quoted lines
> More importantly, do we want a subshell, or is something like this
> sufficient?
>
> echo "../submodule" >expected &&
> git -C super submodule--helper config ... >actual &&
> test_cmp expect actual
>
> The same comment applies to many tests I see below (omitted).
>
I'll keep the subshell when there are multiple git commands ran in the same sub directory, and remove it in tests which have only one git command per test (which is most of them), how does that sound?
Thanks,
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?
Antonio Ospite