[PATCH v3 0/7] Make submodules work if .gitmodules is not checked out

[PATCH v3 0/7] Make submodules work if .gitmodules is not checked out

[Antonio Ospite]

Hi,

this series teaches git to try and read the .gitmodules file from the
current branch (HEAD:.gitmodules) when it's not readily available in the
working tree.

This can be used, along with sparse checkouts, to enable submodule usage
with programs like vcsh[1] which manage multiple repositories with their
working trees sharing the same path.

[1] https://github.com/RichiH/vcsh


This is v3 of the series from:
https://public-inbox.org/git/20180802134634.10300-1-ao2@ao2.it/

The cover letter of the first proposal contains more background:
https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

Changes since v2:

  * Removed the extern keyword from the public declaration of
    print_config_from_gitmodules() and
    config_set_in_gitmodules_file_gently()

  * Used test_when_finished in t/t7411-submodule-config.sh and remove
    the problematic commits as soon as they are not needed anymore.

  * Restructured the code in module_config to avoid an unreachable
    section, the code now dies as a fallback if the arguments are not
    supported, as suggested by Jeff.

  * Dropped patches and tests about "submodule--helper config --stage"
    as they are not strictly needed for now and there is no immediate
    benefit from them.

  * Added a check to git-submodule.sh::cdm_add to make it fail earlier
    if the .gitmodules file is not "safely writeable". This also fixes
    one of the new tests which was previously marked as
    "test_expect_failure".

  * Fixed a broken &&-chain in a subshell in one of the new tests, the
    issue was exposed by a recent change in master.

  * Dropped a note about "git rm" and "git mv", it was intended as
    a personal reminder and not for the general public.

  * Squashed t7416-submodule-sparse-gitmodules.sh in the same commit of
    the code it exercises.

  * Dropped the two unrelated patches from v2:
    
      - dir: move is_empty_file() from builtin/am.c to dir.c and make it
        public
      - submodule: remove the .gitmodules file when it is empty

    as they are orthogonal to this series. I will send them as
    a standalone series.

  * Minor wording fixes here and there.


The series looks a lot cleaner and more to the point, thanks for the
review.

Ciao,
   Antonio

Antonio Ospite (7):
  submodule: add a print_config_from_gitmodules() helper
  submodule: factor out a config_set_in_gitmodules_file_gently function
  t7411: be nicer to future tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  t7506: clean up .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out

 builtin/submodule--helper.c            | 29 +++++++++
 cache.h                                |  1 +
 git-submodule.sh                       | 15 +++--
 new                                    |  0
 submodule-config.c                     | 53 ++++++++++++++-
 submodule-config.h                     |  3 +
 submodule.c                            | 10 +--
 t/t7411-submodule-config.sh            | 33 +++++++++-
 t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++
 t/t7506-status-submodule.sh            |  3 +-
 10 files changed, 221 insertions(+), 16 deletions(-)
 create mode 100644 new
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh

-- 
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?

[PATCH v3 1/7] submodule: add a print_config_from_gitmodules() helper

[Antonio Ospite]

Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.

This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 25 +++++++++++++++++++++++++
 submodule-config.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index fc2c41b947..eef96c4198 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
 		submodule_cache_clear(r->submodule_cache);
 }
 
+static int config_print_callback(const char *key_, const char *value_, void *cb_data)
+{
+	char *key = cb_data;
+
+	if (!strcmp(key, key_))
+		printf("%s\n", value_);
+
+	return 0;
+}
+
+int print_config_from_gitmodules(const char *key)
+{
+	int ret;
+	char *store_key;
+
+	ret = git_config_parse_key(key, &store_key, NULL);
+	if (ret < 0)
+		return CONFIG_INVALID_KEY;
+
+	config_from_gitmodules(config_print_callback, the_repository, store_key);
+
+	free(store_key);
+	return 0;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index dc7278eea4..ed40e9a478 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
  */
 int check_submodule_name(const char *name);
 
+int print_config_from_gitmodules(const char *key);
+
 /*
  * Note: these helper functions exist solely to maintain backward
  * compatibility with 'fetch' and 'update_clone' storing configuration in
-- 
2.18.0

[PATCH v3 2/7] submodule: factor out a config_set_in_gitmodules_file_gently function

[Antonio Ospite]

Introduce a new config_set_in_gitmodules_file_gently() function to write
config values to the .gitmodules file.

This is in preparation for a future change which will use the function
to write to the .gitmodules file in a more controlled way instead of
using "git config -f .gitmodules".

The purpose of the change is mainly to centralize the code that writes
to the .gitmodules file to avoid some duplication.

The naming follows git_config_set_in_file_gently() but the git_ prefix
is removed to communicate that this is not a generic git-config API.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 12 ++++++++++++
 submodule-config.h |  1 +
 submodule.c        | 10 +++-------
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index eef96c4198..b7ef055c63 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -707,6 +707,18 @@ int print_config_from_gitmodules(const char *key)
 	return 0;
 }
 
+int config_set_in_gitmodules_file_gently(const char *key, const char *value)
+{
+	int ret;
+
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	if (ret < 0)
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not update .gitmodules entry %s"), key);
+
+	return ret;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index ed40e9a478..9957bcbbfa 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,6 +57,7 @@ void submodule_free(struct repository *r);
 int check_submodule_name(const char *name);
 
 int print_config_from_gitmodules(const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
 
 /*
  * Note: these helper functions exist solely to maintain backward
diff --git a/submodule.c b/submodule.c
index 6e14547e9e..fd95cb76b3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -89,6 +89,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 {
 	struct strbuf entry = STRBUF_INIT;
 	const struct submodule *submodule;
+	int ret;
 
 	if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
 		return -1;
@@ -104,14 +105,9 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	strbuf_addstr(&entry, "submodule.");
 	strbuf_addstr(&entry, submodule->name);
 	strbuf_addstr(&entry, ".path");
-	if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) < 0) {
-		/* Maybe the user already did that, don't error out here */
-		warning(_("Could not update .gitmodules entry %s"), entry.buf);
-		strbuf_release(&entry);
-		return -1;
-	}
+	ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
 	strbuf_release(&entry);
-	return 0;
+	return ret;
 }
 
 /*
-- 
2.18.0

[PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

[Antonio Ospite]

Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
invalid lines in .gitmodules but then only the second commit is removed.

This may affect future subsequent tests if they assume that the
.gitmodules file has no errors.

Remove both the commits as soon as they are not needed anymore.

The error introduced in test 5 is also required by test 6, so the two
commits from above are removed respectively in tests 6 and 8.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7411-submodule-config.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850ac..c6b6cf6fae 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '
 '
 
 test_expect_success 'error message contains blob reference' '
+	# Remove the error introduced in the previous test.
+	# It is not needed in the following tests.
+	test_when_finished "git -C super reset --hard HEAD^" &&
 	(cd super &&
 		sha1=$(git rev-parse HEAD) &&
 		test-tool submodule-config \
@@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
 '
 
 test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
+	test_when_finished "git -C super reset --hard HEAD^" &&
 	(cd super &&
 		git config -f .gitmodules \
 			submodule.submodule.fetchrecursesubmodules blabla &&
@@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 			HEAD b \
 			HEAD submodule \
 				>actual &&
-		test_cmp expect_error actual  &&
-		git reset --hard HEAD^
+		test_cmp expect_error actual
 	)
 '
 
-- 
2.18.0

Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

[Brandon Williams]

On 08/14, Antonio Ospite wrote:
> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
> 
> This may affect future subsequent tests if they assume that the
> .gitmodules file has no errors.
> 
> Remove both the commits as soon as they are not needed anymore.
> 
> The error introduced in test 5 is also required by test 6, so the two
> commits from above are removed respectively in tests 6 and 8.

Thanks for cleaning this up.  We seem to have a habit for leaving
testing state around for longer than is necessary which makes it a bit
more difficult to read and understand when looking at it later.  What
would really be nice is if each test was self-contained...course that
would take a herculean effort to realize in our testsuite so I'm not
suggesting you do that :)

> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  t/t7411-submodule-config.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..c6b6cf6fae 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '
>  '
>  
>  test_expect_success 'error message contains blob reference' '
> +	# Remove the error introduced in the previous test.
> +	# It is not needed in the following tests.
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		sha1=$(git rev-parse HEAD) &&
>  		test-tool submodule-config \
> @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
>  '
>  
>  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		git config -f .gitmodules \
>  			submodule.submodule.fetchrecursesubmodules blabla &&
> @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  			HEAD b \
>  			HEAD submodule \
>  				>actual &&
> -		test_cmp expect_error actual  &&
> -		git reset --hard HEAD^
> +		test_cmp expect_error actual
>  	)
>  '
>  
> -- 
> 2.18.0
> 

-- 
Brandon Williams

Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

>  test_expect_success 'error message contains blob reference' '
> +	# Remove the error introduced in the previous test.
> +	# It is not needed in the following tests.
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		sha1=$(git rev-parse HEAD) &&
>  		test-tool submodule-config \

Antonio Ospite <ao2@ao2.it> writes:

> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
>
> This may affect future subsequent tests if they assume that the
> .gitmodules file has no errors.
>
> Remove both the commits as soon as they are not needed anymore.
>
> The error introduced in test 5 is also required by test 6, so the two
> commits from above are removed respectively in tests 6 and 8.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  t/t7411-submodule-config.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..c6b6cf6fae 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '
>  '
>  
>  test_expect_success 'error message contains blob reference' '
> +	# Remove the error introduced in the previous test.
> +	# It is not needed in the following tests.
> +	test_when_finished "git -C super reset --hard HEAD^" &&

Hmm, that is ugly.  Depending on where in the subshell the previous
test failed, you'd still be taking us to an unexpected place.
Imagine if "git commit -m 'add error'" failed, for example, in the
test before this one.

I am wondering if the proper fix is to merge the previous one and
this one into a single test.  The combined test would

    - remember where the HEAD in super is and arrange to come back
      to it when test is done
    - break .gitmodules and commit it
    - run test-tool and check its output
    - also check its error output

in a single test_expect_success.

> @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
>  '
>  
>  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		git config -f .gitmodules \
>  			submodule.submodule.fetchrecursesubmodules blabla &&
> @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  			HEAD b \
>  			HEAD submodule \
>  				>actual &&
> -		test_cmp expect_error actual  &&
> -		git reset --hard HEAD^
> +		test_cmp expect_error actual
>  	)
>  '

If we want to be more robust, you'd probably need to find a better
anchoring point than HEAD, which can be pointing different commit
depending on where in the subshell the process is hit with ^C,
i.e.

	ORIG=$(git -C super rev-parse HEAD) &&
	test_when_finished "git -C super reset --hard $ORIG" &&
	(
		cd super &&
		...

The patch is still an improvement compared to the current code,
where a broken test-tool that does not produce expected output in
the file 'actual' is guaranteed to leave us at a commit that we do
not expect to be at, but not entirely satisfactory.

Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

[Antonio Ospite]

On Tue, 14 Aug 2018 13:16:38 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
[...]
> >  test_expect_success 'error message contains blob reference' '
> > +	# Remove the error introduced in the previous test.
> > +	# It is not needed in the following tests.
> > +	test_when_finished "git -C super reset --hard HEAD^" &&
> 
> Hmm, that is ugly.  Depending on where in the subshell the previous
> test failed, you'd still be taking us to an unexpected place.
> Imagine if "git commit -m 'add error'" failed, for example, in the
> test before this one.
> 
> I am wondering if the proper fix is to merge the previous one and
> this one into a single test.  The combined test would
> 
>     - remember where the HEAD in super is and arrange to come back
>       to it when test is done
>     - break .gitmodules and commit it
>     - run test-tool and check its output
>     - also check its error output
> 
> in a single test_expect_success.
>

I will try that.

> > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
> >  '
> >  
> >  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> > +	test_when_finished "git -C super reset --hard HEAD^" &&
> >  	(cd super &&
> >  		git config -f .gitmodules \
> >  			submodule.submodule.fetchrecursesubmodules blabla &&
> > @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >  			HEAD b \
> >  			HEAD submodule \
> >  				>actual &&
> > -		test_cmp expect_error actual  &&
> > -		git reset --hard HEAD^
> > +		test_cmp expect_error actual
> >  	)
> >  '
> 
> If we want to be more robust, you'd probably need to find a better
> anchoring point than HEAD, which can be pointing different commit
> depending on where in the subshell the process is hit with ^C,
> i.e.
> 
> 	ORIG=$(git -C super rev-parse HEAD) &&
> 	test_when_finished "git -C super reset --hard $ORIG" &&
> 	(
> 		cd super &&
> 		...
>

I see, ORIG is set and evaluated immediately but the value will be
used only at a later time.

I remember that you raised concerns also in the previous review round
but I didn't quite get what you meant, now I think I do.

> The patch is still an improvement compared to the current code,
> where a broken test-tool that does not produce expected output in
> the file 'actual' is guaranteed to leave us at a commit that we do
> not expect to be at, but not entirely satisfactory.

I can do a v4 with these fixes since there are also some comments about
other patches.

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?

[PATCH v3 4/7] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

Add a new 'config' subcommand to 'submodule--helper', this extra level
of indirection makes it possible to add some flexibility to how the
submodules configuration is handled.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 14 ++++++++++++++
 new                         |  0
 t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
 create mode 100644 new

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..7481d03b63 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2029,6 +2029,19 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
 	return 0;
 }
 
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+	/* Equivalent to ACTION_GET in builtin/config.c */
+	if (argc == 2)
+		return print_config_from_gitmodules(argv[1]);
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3)
+		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+	die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2057,6 +2070,7 @@ static struct cmd_struct commands[] = {
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
+	{"config", module_config, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/new b/new
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index c6b6cf6fae..4afb6f152e 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -142,4 +142,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "../submodule" >expected &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'writing submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "new_url" >expected &&
+		git submodule--helper config submodule.submodule.url "new_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done
-- 
2.18.0

Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand

[Brandon Williams]

On 08/14, Antonio Ospite wrote:
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  builtin/submodule--helper.c | 14 ++++++++++++++

>  new                         |  0

Looks like you may have accidentally left in an empty file "new" it should
probably be removed from this commit before it gets merged.

Aside from that this patch looks good.  I've recently run into issues
where we don't have a good enough abstraction layer around how we
interact with submodules so I'm glad we're moving towards better
abstractions :)

>  t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
>  create mode 100644 new
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..7481d03b63 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,19 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
>  	return 0;
>  }
>  
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +	/* Equivalent to ACTION_GET in builtin/config.c */
> +	if (argc == 2)
> +		return print_config_from_gitmodules(argv[1]);
> +
> +	/* Equivalent to ACTION_SET in builtin/config.c */
> +	if (argc == 3)
> +		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +
> +	die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2057,6 +2070,7 @@ static struct cmd_struct commands[] = {
>  	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
>  	{"is-active", is_active, 0},
>  	{"check-name", check_name, 0},
> +	{"config", module_config, 0},
>  };
>  
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/new b/new
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index c6b6cf6fae..4afb6f152e 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -142,4 +142,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  	)
>  '
>  
> +test_expect_success 'reading submodules config with "submodule--helper config"' '
> +	(cd super &&
> +		echo "../submodule" >expected &&
> +		git submodule--helper config submodule.submodule.url >actual &&
> +		test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'writing submodules config with "submodule--helper config"' '
> +	(cd super &&
> +		echo "new_url" >expected &&
> +		git submodule--helper config submodule.submodule.url "new_url" &&
> +		git submodule--helper config submodule.submodule.url >actual &&
> +		test_cmp expected actual
> +	)
> +'
> +
> +test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
> +	(cd super &&
> +		echo "newer_url" >expected &&
> +		git submodule--helper config submodule.submodule.url "newer_url" &&
> +		git submodule--helper config submodule.submodule.url >actual &&
> +		test_cmp expected actual
> +	)
> +'
> +
>  test_done
> -- 
> 2.18.0
> 

-- 
Brandon Williams

Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

On Tue, 14 Aug 2018 10:10:58 -0700
Brandon Williams <bmwill@google.com> wrote:

> On 08/14, Antonio Ospite wrote:
> > Add a new 'config' subcommand to 'submodule--helper', this extra level
> > of indirection makes it possible to add some flexibility to how the
> > submodules configuration is handled.
> > 
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >  builtin/submodule--helper.c | 14 ++++++++++++++
> 
> >  new                         |  0
> 
> Looks like you may have accidentally left in an empty file "new" it should
> probably be removed from this commit before it gets merged.
> 

Yeah, I had added it to test "git cat-file -e new" for a later patch and
then I must have messed up some rebase. Thanks for pointing it out.

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?

[PATCH v3 5/7] submodule: use the 'submodule--helper config' command

[Antonio Ospite]

Use the 'submodule--helper config' command in git-submodules.sh to avoid
referring explicitly to .gitmodules by the hardcoded file path.

This makes it possible to access the submodules configuration in a more
controlled way.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 8b5ad59bde..ff258e2e8c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -72,7 +72,7 @@ get_submodule_config () {
 	value=$(git config submodule."$name"."$option")
 	if test -z "$value"
 	then
-		value=$(git config -f .gitmodules submodule."$name"."$option")
+		value=$(git submodule--helper config submodule."$name"."$option")
 	fi
 	printf '%s' "${value:-$default}"
 }
@@ -283,11 +283,11 @@ or you are unsure what this means choose another name with the '--name' option."
 	git add --no-warn-embedded-repo $force "$sm_path" ||
 	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
 
-	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
-	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
+	git submodule--helper config submodule."$sm_name".url "$repo" &&
 	if test -n "$branch"
 	then
-		git config -f .gitmodules submodule."$sm_name".branch "$branch"
+		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
 	git add --force .gitmodules ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-- 
2.18.0

Re: [PATCH v3 5/7] submodule: use the 'submodule--helper config' command

[Brandon Williams]

On 08/14, Antonio Ospite wrote:
> Use the 'submodule--helper config' command in git-submodules.sh to avoid
> referring explicitly to .gitmodules by the hardcoded file path.
> 
> This makes it possible to access the submodules configuration in a more
> controlled way.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>

Looks great.  I also like you're approach of introducing the new API and
testing it in one commit, and then using it in the next.  Makes the
patch set very easy to follow.

> ---
>  git-submodule.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8b5ad59bde..ff258e2e8c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -72,7 +72,7 @@ get_submodule_config () {
>  	value=$(git config submodule."$name"."$option")
>  	if test -z "$value"
>  	then
> -		value=$(git config -f .gitmodules submodule."$name"."$option")
> +		value=$(git submodule--helper config submodule."$name"."$option")
>  	fi
>  	printf '%s' "${value:-$default}"
>  }
> @@ -283,11 +283,11 @@ or you are unsure what this means choose another name with the '--name' option."
>  	git add --no-warn-embedded-repo $force "$sm_path" ||
>  	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
>  
> -	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
> -	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
> +	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
> +	git submodule--helper config submodule."$sm_name".url "$repo" &&
>  	if test -n "$branch"
>  	then
> -		git config -f .gitmodules submodule."$sm_name".branch "$branch"
> +		git submodule--helper config submodule."$sm_name".branch "$branch"
>  	fi &&
>  	git add --force .gitmodules ||
>  	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
> -- 
> 2.18.0
> 

-- 
Brandon Williams

[PATCH v3 6/7] t7506: clean up .gitmodules properly before setting up new scenario

[Antonio Ospite]

In t/t7506-status-submodule.sh at some point a new scenario is set up to
test different things, in particular new submodules are added which are
meant to completely replace the previous ones.

However before calling the "git submodule add" commands for the new
layout, the .gitmodules file is removed only from the working tree still
leaving the previous content in current branch.

This can break if, in the future, "git submodule add" starts
differentiating between the following two cases:

  - .gitmodules is not in the working tree but it is in the current
    branch (it may not be safe to add new submodules in this case);

  - .gitmodules is neither in the working tree nor anywhere in the
    current branch (it is safe to add new submodules).

Since the test intends to get rid of .gitmodules anyways, let's
completely remove it from the current branch, to actually start afresh
in the new scenario.

This is more future-proof and does not break current tests.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 943708fb04..08629a6e70 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 	(
 		cd super &&
 		git clean -dfx &&
-		rm .gitmodules &&
+		git rm .gitmodules &&
+		git commit -m "remove .gitmodules" &&
 		git submodule add -f ./sub1 &&
 		git submodule add -f ./sub2 &&
 		git submodule add -f ./sub1 sub3 &&
-- 
2.18.0

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

[Antonio Ospite]

When the .gitmodules file is not available in the working tree, try
using HEAD:.gitmodules from the current branch. This covers the case
when the file is part of the repository but for some reason it is not
checked out, for example because of a sparse checkout.

This makes it possible to use at least the 'git submodule' commands
which *read* the gitmodules configuration file without fully populating
the working tree.

Writing to .gitmodules will still require that the file is checked out,
so check for that before calling config_set_in_gitmodules_file_gently.

Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).

Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

Maybe the check in config_set_in_gitmodules_file_gently and
git-submodule.sh::cmd_add() can share some code:

  - add an is_gitmodules_safely_writeable() helper
  - expose a "submodule--helper config --is-safely-writeable" subcommand

But for now I preferred to keep the changes with v2 to a minimum to avoid
blocking the series.

If adding a new helper is preferred I can do a v4 or send a follow-up patch.

Thank you,
   Antonio


 builtin/submodule--helper.c            | 17 ++++-
 cache.h                                |  1 +
 git-submodule.sh                       |  7 ++
 submodule-config.c                     | 16 ++++-
 t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++
 5 files changed, 128 insertions(+), 3 deletions(-)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7481d03b63..c0370a756b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2036,8 +2036,23 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		return print_config_from_gitmodules(argv[1]);
 
 	/* 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.
+		 *
+		 * This check still makes it possible to create a brand new
+		 * .gitmodules when it is safe to do so: when neither
+		 * GITMODULES_FILE nor GITMODULES_HEAD exist.
+		 */
+		if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD, &oid) >= 0)
+			die(_("please make sure that the .gitmodules file in the current branch is checked out"));
+
 		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+	}
 
 	die("submodule--helper config takes 1 or 2 arguments: name [value]");
 }
diff --git a/cache.h b/cache.h
index 8dc7134f00..900f9e09e5 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 #define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_HEAD "HEAD:.gitmodules"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
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
+	then
+		 die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
+	fi
+
 	if test -n "$reference_path"
 	then
 		is_absolute_path "$reference_path" ||
diff --git a/submodule-config.c b/submodule-config.c
index b7ef055c63..088dabb56f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "dir.h"
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
@@ -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;
+
+		config_with_options(fn, data, &config_source, &opts);
+
 		free(file);
 	}
 }
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 &&
+	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
+		git config core.sparsecheckout true &&
+		git read-tree -m -u HEAD &&
+		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
+	)
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config submodule.submodule.url newurl
+	)
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule summary
+	)
+'
+
+test_expect_success 'updating submodule when the gitmodules config is not checked out' '
+	(cd submodule &&
+		echo file2 >file2 &&
+		git add file2 &&
+		git commit -m "add file2 to submodule"
+	) &&
+	(cd super &&
+		git submodule update
+	)
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
+	(cd super &&
+		test_must_fail git submodule add ../new_submodule
+	)
+'
+
+# This test checks that the previous "git submodule add" did not leave the
+# repository in a spurious state when it failed.
+test_expect_success 'init submodule still works even after the previous add failed' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_done
-- 
2.18.0

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

[Brandon Williams]

On 08/14, Antonio Ospite wrote:
> When the .gitmodules file is not available in the working tree, try
> using HEAD:.gitmodules from the current branch. This covers the case
> when the file is part of the repository but for some reason it is not
> checked out, for example because of a sparse checkout.
> 
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the working tree.
> 
> Writing to .gitmodules will still require that the file is checked out,
> so check for that before calling config_set_in_gitmodules_file_gently.
> 
> Add a similar check also in git-submodule.sh::cmd_add() to anticipate
> the eventual failure of the "git submodule add" command when .gitmodules
> is not safely writeable; this prevents the command from leaving the
> repository in a spurious state (e.g. the submodule repository was cloned
> but .gitmodules was not updated because
> config_set_in_gitmodules_file_gently failed).
> 
> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
> from .gitmodules succeeds and that writing to it fails when the file is
> not checked out.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
> 
> Maybe the check in config_set_in_gitmodules_file_gently and
> git-submodule.sh::cmd_add() can share some code:
> 
>   - add an is_gitmodules_safely_writeable() helper
>   - expose a "submodule--helper config --is-safely-writeable" subcommand
> 
> But for now I preferred to keep the changes with v2 to a minimum to avoid
> blocking the series.
> 
> If adding a new helper is preferred I can do a v4 or send a follow-up patch.

I see how it would be nice to have the addition of a helper like this.
I think maybe at some point we'd want it but its definitely not needed
now and can easily be added at a later point (maybe we can avoid needing
it until we can convert all of the git-submodule.sh code to C!).

Great work, thanks for working on this.

> 
> Thank you,
>    Antonio
> 
> 
>  builtin/submodule--helper.c            | 17 ++++-
>  cache.h                                |  1 +
>  git-submodule.sh                       |  7 ++
>  submodule-config.c                     | 16 ++++-
>  t/t7416-submodule-sparse-gitmodules.sh | 90 ++++++++++++++++++++++++++
>  5 files changed, 128 insertions(+), 3 deletions(-)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7481d03b63..c0370a756b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2036,8 +2036,23 @@ static int module_config(int argc, const char **argv, const char *prefix)
>  		return print_config_from_gitmodules(argv[1]);
>  
>  	/* 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.
> +		 *
> +		 * This check still makes it possible to create a brand new
> +		 * .gitmodules when it is safe to do so: when neither
> +		 * GITMODULES_FILE nor GITMODULES_HEAD exist.
> +		 */
> +		if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD, &oid) >= 0)
> +			die(_("please make sure that the .gitmodules file in the current branch is checked out"));
> +
>  		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +	}
>  
>  	die("submodule--helper config takes 1 or 2 arguments: name [value]");
>  }
> diff --git a/cache.h b/cache.h
> index 8dc7134f00..900f9e09e5 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -486,6 +486,7 @@ static inline enum object_type object_type(unsigned int mode)
>  #define INFOATTRIBUTES_FILE "info/attributes"
>  #define ATTRIBUTE_MACRO_PREFIX "[attr]"
>  #define GITMODULES_FILE ".gitmodules"
> +#define GITMODULES_HEAD "HEAD:.gitmodules"
>  #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
>  #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
>  #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
> 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
> +	then
> +		 die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
> +	fi
> +
>  	if test -n "$reference_path"
>  	then
>  		is_absolute_path "$reference_path" ||
> diff --git a/submodule-config.c b/submodule-config.c
> index b7ef055c63..088dabb56f 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "dir.h"
>  #include "repository.h"
>  #include "config.h"
>  #include "submodule-config.h"
> @@ -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;
> +
> +		config_with_options(fn, data, &config_source, &opts);
> +
>  		free(file);
>  	}
>  }
> 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 &&
> +	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
> +		git config core.sparsecheckout true &&
> +		git read-tree -m -u HEAD &&
> +		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
> +	)
> +'
> +
> +test_expect_success 'not writing gitmodules config file when it is not checked out' '
> +	(cd super &&
> +		test_must_fail git submodule--helper config submodule.submodule.url newurl
> +	)
> +'
> +
> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> +	(cd super &&
> +		git submodule init
> +	)
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> +	(cd super &&
> +		git submodule summary
> +	)
> +'
> +
> +test_expect_success 'updating submodule when the gitmodules config is not checked out' '
> +	(cd submodule &&
> +		echo file2 >file2 &&
> +		git add file2 &&
> +		git commit -m "add file2 to submodule"
> +	) &&
> +	(cd super &&
> +		git submodule update
> +	)
> +'
> +
> +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> +	(cd super &&
> +		test_must_fail git submodule add ../new_submodule
> +	)
> +'
> +
> +# This test checks that the previous "git submodule add" did not leave the
> +# repository in a spurious state when it failed.
> +test_expect_success 'init submodule still works even after the previous add failed' '
> +	(cd super &&
> +		git submodule init
> +	)
> +'
> +
> +test_done
> -- 
> 2.18.0
> 

-- 
Brandon Williams

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

[Junio C Hamano]

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.

> 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.

> +	then
> +		 die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
> +	fi
> +

> diff --git a/submodule-config.c b/submodule-config.c
> index b7ef055c63..088dabb56f 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "dir.h"
>  #include "repository.h"
>  #include "config.h"
>  #include "submodule-config.h"
> @@ -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)?

> 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).

> +		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'.

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).

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

[Antonio Ospite]

On Tue, 14 Aug 2018 13:36:17 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> 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.

> > 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.

[...]
> > @@ -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?

> > 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.

> > +		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'.
> 

Will fix.

> 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?

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

[Antonio Ospite]

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?

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

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

> 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).

Perhaps.  The main point was to replace the comment that tells the
developers to keep two things stay in sync with an actually shared
implementation; as long as that is done, I am not too much worried
about the details.

>> > > +		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
>> > > +			config_source.blob = GITMODULES_HEAD;
>> > 
>> Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough?

Yeah, either "instead of", or "in addition" (i.e. "try the index
version in addition, before falling further back to the HEAD
version"), would be more consistent with the remainder of the system
(or, at least where the remainder of the system wants to go).

>> 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?

I do not know why you want to refrain from spelling them out as
"HEAD:.gitmodules" and ":.gitmodules"; at least to me the extra
layer of names do not look like they are making the code easier
to understand that much.

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

[Antonio Ospite]

On Wed, 22 Aug 2018 08:29:25 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
[...]
> >> > > +		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
> >> > > +			config_source.blob = GITMODULES_HEAD;
> >> > 
> >> Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough?
> 
> Yeah, either "instead of", or "in addition" (i.e. "try the index
> version in addition, before falling further back to the HEAD
> version"), would be more consistent with the remainder of the system
> (or, at least where the remainder of the system wants to go).
>

OK, I now tested with both "rm .gitmodules" and "git rm .gitmodules"
and I see why one would want to try _both_ ":.gitmodules" and
"HEAD:.gitmodules".

I'll go with "in addition" then, adding tests for both the scenarios.

> >> 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?
> 
> I do not know why you want to refrain from spelling them out as
> "HEAD:.gitmodules" and ":.gitmodules"; at least to me the extra
> layer of names do not look like they are making the code easier
> to understand that much.
> 

This is in the spirit of commit 4c0eeafe47 (cache.h: add
GITMODULES_FILE macro, 2017-08-02), IIRC this was done mainly to get
help from the preprocessor to spot typos: I caught myself writing
".gitmdoules" several times; GITMDOULES_FILE would not compile.

If this makes sense I'll use GITMODULES_INDEX and GITMODULES_HEAD.

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?