[RFC PATCH v2 00/12] Make submodules work if .gitmodules is not checked out

[RFC PATCH v2 00/12] Make submodules work if .gitmodules is not checked out

[Antonio Ospite]

Hi,

this is a second version of the RFC from
https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

Please refer to the cover letter of the previous series for the
motivation and background of the changes.

Patches from 01 to 08 should be rather uncontroversial, as they do not
introduce any change of behavior but just prepare the ground for patches
09 and 10.

The last two patches, 11 and  12 are not strictly related to the topic,
it's just an idea I came up with while reading the code for the series;
I could send them separately if you think they are valid, or drop them.

The behavior of accessing HEAD:.gitmodules have been analyzed with the
following test matrix:

Predicates:
  G = .gitmodules exists
  H = HEAD:.gitmodules exists

Operations:
  read = git submodule--helper config name
  write = git submodule--helper config name value
  stage = git submodule--helper config --stage

!G and !H:
  - read: nop
  - write: set value to .gitmodules (we are creating a brand new
    .gitmodules)
  - stage: exit with error, staging requires .gitmodules (like git
    add/rm)

G and !H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules

!G and H:
  - read: get the value from HEAD:.gitmodules
  - write: exit with error, setting a value requires .gitmodules
  - stage: exit with error, staging requires .gitmodules
  
G and H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules


Note that "git rm" and "git mv" will keep working when !G just as
t3600-rm.sh and t7001-mv.sh mandate, I am not sure if they should fail
if "!G and H".

In my previous attempt I also tried to check for the skip-worktree bit,
but I am not sure if this is needed.

My intuition suggested the following behavior to cover the case of
a manually crafted .gitmodules which could be committed and override the
hidden one:

S = .gitmodules has the skip-worktree bit set

S:
  - read: get value from .gitmodules if G or from HEAD:.gitmodules if H
  - write: exit with error, .gitmodules is not supposed to be changed
  - stage: exit with error, .gitmodules is not supposed to be changed

However it looks like git gives the user quite some freedom to add,
stage, commit, remove files even when they have the skip-worktree bit
set (which was a little surprising to me TBH) so I am leaving this out
for now.


Changes since v1:

  * Added a helper to print config values from the gitmodules
    configuration, the code is now in submoudle-config.c instead of
    buildtin/submodule--helper.c

  * Renamed config_gitmodules_set to
    config_set_in_gitmodules_file_gently and put it in
    submodule-config.c instead of submodule.c

    The naming follows the style of git config functions, and also takes
    into account Stefan Beller's comment about the final purpose of the
    function: we only write gitmodules config to the actual .gitmodules
    file, so we may as well reflect that in the function name,

  * Started using "working tree" instead of "work tree" as the former is
    the official term for the set of checked out files.

  * Start referring to HEAD as the "current branch" instead of "the
    index", the former should be more accurate.

  * Renamed GITMODULES_BLOB to GITMODULES_HEAD because GITMODULES_BLOB
    was added in commit 59e7b080 ("fsck: detect gitmodules files",
    2018-05-02)

  * Do not check for the skip-worktree bit on .gitmodules anymore, just
    check for file existence when appropriate.

  * Renamed t7415-submodule-sparse-gitmodules.sh to
    t7416-submodule-sparse-gitmodules.sh because t7415 was taken by
    t7415-submodule-names.sh

  * Made "git mv" and "git rm" work again when .gitmodules does not
    exist.

  * Removed checks about the  skip-worktree bit.


Thanks,
   Antonio

Antonio Ospite (12):
  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
  submodule--helper: add a '--stage' option to the 'config' sub command
  submodule: use 'submodule--helper config --stage' to stage .gitmodules
  t7506: cleanup .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out
  t7416: add new test about HEAD:.gitmodules and not existing
    .gitmodules
  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

 builtin/am.c                           |  15 ----
 builtin/submodule--helper.c            |  82 ++++++++++++++++++
 cache.h                                |   1 +
 dir.c                                  |  16 ++++
 dir.h                                  |   1 +
 git-submodule.sh                       |  10 +--
 submodule-config.c                     |  53 +++++++++++-
 submodule-config.h                     |   3 +
 submodule.c                            |  20 +++--
 t/t3600-rm.sh                          |  32 +++----
 t/t7400-submodule-basic.sh             |  11 +++
 t/t7411-submodule-config.sh            |  65 +++++++++++++-
 t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
 t/t7506-status-submodule.sh            |   3 +-
 14 files changed, 375 insertions(+), 49 deletions(-)
 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?

[RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

[Antonio Ospite]

This will be used to print values just like "git config -f .gitmodules"
would.

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 2a7259ba8b..6f6f5f9960 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..6fec3caadd 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);
 
+extern 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

Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> This will be used to print values just like "git config -f .gitmodules"
> would.
>
> 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 2a7259ba8b..6f6f5f9960 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..6fec3caadd 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);
>
> +extern int print_config_from_gitmodules(const char *key);

The only real pushback for this patch I'd have is lack of documentation
in public functions, though this is pretty self explanatory; so I'd be fine
for lacking the docs here.

In case a resend is needed, please drop the extern keyword here.

Thanks,
Stefan

Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

[Antonio Ospite]

On Thu, 2 Aug 2018 11:05:02 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
[...]
> > +extern int print_config_from_gitmodules(const char *key);
> 
> The only real pushback for this patch I'd have is lack of documentation
> in public functions, though this is pretty self explanatory; so I'd be fine
> for lacking the docs here.
> 
> In case a resend is needed, please drop the extern keyword here.
> 

I'll drop the extern keyword also for the public function added in
patch 02 then.

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?

[RFC PATCH v2 02/12] 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 6f6f5f9960..702d40dd6b 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 6fec3caadd..074e74a01c 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);
 
 extern int print_config_from_gitmodules(const char *key);
+extern 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 2a6381864e..2af09068d7 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

[RFC PATCH v2 03/12] 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.

Since those commits are not needed anymore remove both of them.

The modified line is in the last test of the file, so this does not
change the current behavior, it only affects future tests.

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

Note that test_when_finished is not used here, both to keep the current style
and also because it does not work in sub-shells.

 t/t7411-submodule-config.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850ac..248da0bc4f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 			HEAD submodule \
 				>actual &&
 		test_cmp expect_error actual  &&
-		git reset --hard HEAD^
+		# Remove both the commits which add errors to .gitmodules,
+		# the one from this test and the one from a previous test.
+		git reset --hard HEAD~2
 	)
 '
 
-- 
2.18.0

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[SZEDER Gábor]

> 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.
> 
> Since those commits are not needed anymore remove both of them.
> 
> The modified line is in the last test of the file, so this does not
> change the current behavior, it only affects future tests.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
> 
> Note that test_when_finished is not used here, both to keep the current style
> and also because it does not work in sub-shells.

That's true, but I think that this:

  test_when_finished git -C super reset --hard HEAD~2

at the very beginning of the test should work.

>  t/t7411-submodule-config.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..248da0bc4f 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  			HEAD submodule \
>  				>actual &&
>  		test_cmp expect_error actual  &&
> -		git reset --hard HEAD^
> +		# Remove both the commits which add errors to .gitmodules,
> +		# the one from this test and the one from a previous test.
> +		git reset --hard HEAD~2
>  	)
>  '
>  
> -- 
> 2.18.0
> 
> 

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[Stefan Beller]

On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor <szeder.dev@gmail.com> 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.
> >
> > Since those commits are not needed anymore remove both of them.
> >
> > The modified line is in the last test of the file, so this does not
> > change the current behavior, it only affects future tests.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > Note that test_when_finished is not used here, both to keep the current style
> > and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Yeah that is a better way to do it.
Even better would be to have 2 of these for both tests 5 and 8,
such that each of them could be skipped individually and any following
tests still work fine.

> >  t/t7411-submodule-config.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index 0bde5850ac..248da0bc4f 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >                       HEAD submodule \
> >                               >actual &&
> >               test_cmp expect_error actual  &&
> > -             git reset --hard HEAD^
> > +             # Remove both the commits which add errors to .gitmodules,
> > +             # the one from this test and the one from a previous test.
> > +             git reset --hard HEAD~2

I am a bit hesitant to removing the commits though, as it is expected to have
potentially broken history and submodules still working.

The config --unset already fixes the gitmodules file,
so I think we can rather do

    git commit -a -m 'now the .gitmodules file is fixed at HEAD \
        but has a messy history'

But as I have only read up to here, not knowing what the future tests will
bring this is all speculation at this point.

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[Antonio Ospite]

On Thu, 2 Aug 2018 11:15:03 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
[...]
> > >
> > > Note that test_when_finished is not used here, both to keep the current style
> > > and also because it does not work in sub-shells.
> >
> > That's true, but I think that this:
> >
> >   test_when_finished git -C super reset --hard HEAD~2
> >
> > at the very beginning of the test should work.
> 
> Yeah that is a better way to do it.
> Even better would be to have 2 of these for both tests 5 and 8,
> such that each of them could be skipped individually and any following
> tests still work fine.
>

Test 6 also relies on the error introduced in test 5.

So the options would be either to remove one commit at the time in
test 6 and 8 (with a comment in test 6 to note that the commit is from
the previous test), or to remove both the commits in test 8. I am going
to go with the former, using test_when_finished.

> > >  t/t7411-submodule-config.sh | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > > index 0bde5850ac..248da0bc4f 100755
> > > --- a/t/t7411-submodule-config.sh
> > > +++ b/t/t7411-submodule-config.sh
> > > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> > >                       HEAD submodule \
> > >                               >actual &&
> > >               test_cmp expect_error actual  &&
> > > -             git reset --hard HEAD^
> > > +             # Remove both the commits which add errors to .gitmodules,
> > > +             # the one from this test and the one from a previous test.
> > > +             git reset --hard HEAD~2
> 
> I am a bit hesitant to removing the commits though, as it is expected to have
> potentially broken history and submodules still working.
>

The commits which are removed only affected .gitmoudles, no "submodule
init" nor "submoudle update" is ever called after they are added, so I
don't know what problems there could be.

Would a revert be any different?

> The config --unset already fixes the gitmodules file,
> so I think we can rather do
> 
>     git commit -a -m 'now the .gitmodules file is fixed at HEAD \
>         but has a messy history'
> 
> But as I have only read up to here, not knowing what the future tests will
> bring this is all speculation at this point.

IIUC the "config --unset" is used to cause the error, not to fix it, I
am not sure I understand this point.

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: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> 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.
>> 
>> Since those commits are not needed anymore remove both of them.
>> 
>> The modified line is in the last test of the file, so this does not
>> change the current behavior, it only affects future tests.
>> 
>> Signed-off-by: Antonio Ospite <ao2@ao2.it>
>> ---
>> 
>> Note that test_when_finished is not used here, both to keep the current style
>> and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Assuming that the operations to create these two extra commits
always succeed, yes, that would be a more robust and preferrable
option.

I don't know if that assumption hold true offhand, though.  Don't we
have a more stable point to anchor that going-back-to commit to?

[RFC PATCH v2 04/12] 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>
---

Note that the tests follow the predominant style in the file: subshell and cd
right at the start of the sub-shell opening.

 builtin/submodule--helper.c | 17 +++++++++++++++++
 t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..14f0845d30 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2029,6 +2029,22 @@ 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)
+{
+	if (argc < 2 || argc > 3)
+		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+
+	/* 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]);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2057,6 +2073,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/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 248da0bc4f..bbca4b421d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -141,4 +141,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: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> 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>
> ---
>
> Note that the tests follow the predominant style in the file: subshell and cd
> right at the start of the sub-shell opening.
>
>  builtin/submodule--helper.c | 17 +++++++++++++++++
>  t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..14f0845d30 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,22 @@ 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)
> +{
> +       if (argc < 2 || argc > 3)
> +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +
> +       /* 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]);
> +
> +       return 0;

Technically we cannot reach this point here?
Maybe it would be more defensive to

    BUG("How did we get here?");

or at least return something !=0 ?

>
> +test_expect_success 'reading submodules config with "submodule--helper config"' '

I shortly wondered if it would make sense to put these tests at the
beginning of either
this or a new file, as the functionality is rather fundamental. (I
never thought about
it, but actually it is better to go from common basic to more exotic
tests as you scroll
down the file), but this place is ok, if you choose to do so.

Thanks,
Stefan

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Jeff King]

On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:

> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +       if (argc < 2 || argc > 3)
> > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > +
> > +       /* 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]);
> > +
> > +       return 0;
> 
> Technically we cannot reach this point here?
> Maybe it would be more defensive to
> 
>     BUG("How did we get here?");
> 
> or at least return something !=0 ?

When I find myself reaching for a BUG(), it is often a good time to see
if we can restructure the code so that the logic more naturally falls
out.  Here the issue is that the first if conditional repeats the "else"
for the other two. So I think we could just write:

  if (argc == 2)
	return ...
  if (argc == 3)
	return ...

  die("need 1 or 2 arguments");

-Peff

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

On Thu, 2 Aug 2018 15:20:33 -0400
Jeff King <peff@peff.net> wrote:

> On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:
> 
> > > +static int module_config(int argc, const char **argv, const char *prefix)
> > > +{
> > > +       if (argc < 2 || argc > 3)
> > > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > > +
> > > +       /* 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]);
> > > +
> > > +       return 0;
> > 
> > Technically we cannot reach this point here?
> > Maybe it would be more defensive to
> > 
> >     BUG("How did we get here?");
> > 
> > or at least return something !=0 ?
> 
> When I find myself reaching for a BUG(), it is often a good time to see
> if we can restructure the code so that the logic more naturally falls
> out.  Here the issue is that the first if conditional repeats the "else"
> for the other two. So I think we could just write:
> 
>   if (argc == 2)
> 	return ...
>   if (argc == 3)
> 	return ...
> 
>   die("need 1 or 2 arguments");
> 

Hi Jeff,

I like that, I'll see how this plays out after patch 06 which adds
another option, and decide whether to use this style; validating
arguments beforehand may still look a little cleaner.

Thanks for the comment.

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?

[RFC PATCH v2 05/12] 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>
---

Note that there are other instances of "git config -f .gitmodules" in test
files, but I am not touching those for now.


 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: [RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> Use the 'submodule--helper config' command in git-submodules.sh to avoid
> referring explicitly to .gitmodules by the hardcoded file path.

ccol! This is the corner stone to the work of the previous patches. Nicely done!

> This makes it possible to access the submodules configuration in a more
> controlled way.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Note that there are other instances of "git config -f .gitmodules" in test
> files, but I am not touching those for now.

I just checked and this converts all of git-submodule.sh which would
have been my expectation as that is the real product.

The tests are fine to use "git config -f .gitmodules" as there we want to
setup a specific environment to be tested? So I would think even future
patches in this series will not touch test files for such a conversion
as in here.

Stefan

[RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

Add a --stage option to the 'submodule--helper config' command so that
the .gitmodules file can be staged without referring to it explicitly by
its file path.

In practice the config will still be written to .gitmodules, there are
no plans to change the file path, but having this level of indirection
makes it possible to perform additional checks before staging the file.

Note that "git submodule--helper config --stage" will fail when
.gitmodules does not exist, this is to be consistent with how "git add"
and "git rm" work: if the file to be staged does not exist they will
complain.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 54 +++++++++++++++++++++++++++++++++++--
 t/t7411-submodule-config.sh | 35 ++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 14f0845d30..c388f4ee6f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "quote.h"
 #include "pathspec.h"
+#include "lockfile.h"
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
@@ -2031,8 +2032,57 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
-	if (argc < 2 || argc > 3)
-		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+	int stage_gitmodules = 0;
+
+	struct option module_config_options[] = {
+		OPT_BOOL(0, "stage", &stage_gitmodules,
+			    N_("stage the .gitmodules file content")),
+		OPT_END()
+	};
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper config name [value]"),
+		N_("git submodule--helper config --stage"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_config_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if ((stage_gitmodules && argc > 1) ||
+	    (!stage_gitmodules && (argc < 2 || argc > 3)))
+		usage_with_options(git_submodule_helper_usage, module_config_options);
+
+	/*
+	 * Equivalent to "git add --force .gitmodules".
+	 *
+	 * Silently override already staged changes, to support consecutive
+	 * invocations of "git submodule add".
+	 */
+	if (stage_gitmodules) {
+		static struct lock_file lock_file;
+
+		hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
+
+		if (read_cache() < 0)
+			die(_("index file corrupt"));
+
+		/*
+		 * Do not use is_staging_gitmodules_ok() here to make it
+		 * possible to stage multiple changes before committing them,
+		 * in particular this covers the case of consecutive calls of
+		 * "git submodule add".
+		 */
+		if (!file_exists(GITMODULES_FILE))
+			die(_("%s does not exists"), GITMODULES_FILE);
+
+		stage_updated_gitmodules(&the_index);
+
+		if (write_locked_index(&the_index, &lock_file,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+			die(_("Unable to write new index file"));
+
+		return 0;
+	}
 
 	/* Equivalent to ACTION_GET in builtin/config.c */
 	if (argc == 2)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index bbca4b421d..8ff6ed444a 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -167,4 +167,39 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
 	)
 '
 
+test_expect_success 'staging submodules config with "submodule--helper config"' '
+	(cd super &&
+		: >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git submodule--helper config --stage &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting staged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "even_newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "even_newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 're-staging overridden submodule config with "submodule--helper config"' '
+	(cd super &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		echo "bogus config" >.gitmodules &&
+		git submodule--helper config --stage &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git reset HEAD .gitmodules &&
+		git checkout .gitmodules
+	)
+'
+
 test_done
-- 
2.18.0

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

> Add a --stage option to the 'submodule--helper config' command so that
> the .gitmodules file can be staged without referring to it explicitly by
> its file path.

Sorry, but I have no clue what the above is trying to say.

The original 's--h config <name> [<value>]' is quite understandable.  It
is like "git config <name> [<value>]", i.e. either get the current value
for the name, or  set a new value to the name.

What does this 's--h config --stage' that does not take any option
do?  "git add .gitmodules"?  Something else?  In what meaning is the
word "stage" used?  Is it used as a verb "to stage"?

In a series that lets us use the data in the .gitmodules file without
materializing the file in the working tree, I would have expected
that you would want an option to specify which .gitmodules among
(1) the one in the working tree (i.e. the only option we currently
have), (2) the one in the index, and (3) the one in the HEAD, and
when I saw the title, I would have expected that

	git submodule--helper config --stage name

may be a way to read from the .gitmodules in the index to find the
value for name (however, we we follow the option naming convention
in gitcli.txt, it should be called --cached, I would think).

> In practice the config will still be written to .gitmodules, there are
> no plans to change the file path, but having this level of indirection
> makes it possible to perform additional checks before staging the file.

Again, a claim without explanation or justification.

If you are planning to something like

 - prepare trial contents in .gitmodules.new file
 - do whatever "additional checks" on .gitmodules.new
 - add the contents to it to the index as a new .gitmodules blob

Then you do not need such an option.  "submodule--helper" is purely
a helper for scripts, and not for human consumption, so scripts can
just hash-object the blob contents out and update-index --cacheinfo
to register the blob at any location of choice.

But as I said, this step is way under-explained, so my guess above
may not match what you really wanted to do.

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

On Thu, 02 Aug 2018 11:57:14 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
> > Add a --stage option to the 'submodule--helper config' command so that
> > the .gitmodules file can be staged without referring to it explicitly by
> > its file path.
> 
> Sorry, but I have no clue what the above is trying to say.
>

You got me :), this was one of the changes I was having the more doubts
about, basically about the user interface, and it must have showed in
the commit message.

> The original 's--h config <name> [<value>]' is quite understandable.  It
> is like "git config <name> [<value>]", i.e. either get the current value
> for the name, or  set a new value to the name.
> 
> What does this 's--h config --stage' that does not take any option
> do?  "git add .gitmodules"?  Something else?  In what meaning is the
> word "stage" used?  Is it used as a verb "to stage"?
>

"s--h config --stage" is meant to replace "git add -f .gitmodules" as
shown in patch 07, maybe the two patches might have been merged to give
more context, but I kept the changes separate to follow how things were
done with patches 04 and 05.

The rationale behind the change is to close the circle started with 04
and 05 and stop referring to .gitmodules explicitly by file path in git
commands. The change is not strictly needed for the series, it's for
consistency sake.

The name "stage" is meant as a verb and was inspired by the "git stage"
command and by the stage_updated_gitmodules() function, I used an
option flag with no arguments because I saw this has been done for
verbs before, e.g. "git config --list". I did not know about
"gitcli.txt", sorry, I'll check it out.

Maybe a better name could be "submodule--helper config --add"?

The commit message clearly needs improvements.

> In a series that lets us use the data in the .gitmodules file without
> materializing the file in the working tree, I would have expected
> that you would want an option to specify which .gitmodules among
> (1) the one in the working tree (i.e. the only option we currently
> have), (2) the one in the index, and (3) the one in the HEAD, and
> when I saw the title, I would have expected that
> 
> 	git submodule--helper config --stage name
> 
> may be a way to read from the .gitmodules in the index to find the
> value for name (however, we we follow the option naming convention
> in gitcli.txt, it should be called --cached, I would think).
>

For my use case the automatic behavior of falling back to
HEAD:.gitmodules is enough, so I focused on that to have the
functionality merged.

The option you suggest may be useful but I'd leave that as a possible
future addition in case someone else needed it.

> > In practice the config will still be written to .gitmodules, there are
> > no plans to change the file path, but having this level of indirection
> > makes it possible to perform additional checks before staging the file.
> 
> Again, a claim without explanation or justification.
> 
> If you are planning to something like
> 
>  - prepare trial contents in .gitmodules.new file
>  - do whatever "additional checks" on .gitmodules.new
>  - add the contents to it to the index as a new .gitmodules blob
> 
> Then you do not need such an option.  "submodule--helper" is purely
> a helper for scripts, and not for human consumption, so scripts can
> just hash-object the blob contents out and update-index --cacheinfo
> to register the blob at any location of choice.
> 
> But as I said, this step is way under-explained, so my guess above
> may not match what you really wanted to do.

As stated, the motivation for now is just syntactical: remove hardcoded
references to .gitmodules in scripts.

No new "additional checks" are added as of now, the commit message is
misleading indeed.

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: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

> The rationale behind the change is to close the circle started with 04
> and 05 and stop referring to .gitmodules explicitly by file path in git
> commands. The change is not strictly needed for the series, it's for
> consistency sake.

Sorry, but I am still not quite sure what consistency you are
referring to.

I also do not see a reason why we want to stop referring to
.gitmodules explicitly by name.  We do not hide the fact that
in-tree .gitignore and .gitattributes files are used to hold the
metainformation about the project tree, saying that it is an
implementation detail.  Is there a good reason why .gitmodules
should be different from these other two?

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

On Fri, 03 Aug 2018 09:24:14 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
> > The rationale behind the change is to close the circle started with 04
> > and 05 and stop referring to .gitmodules explicitly by file path in git
> > commands. The change is not strictly needed for the series, it's for
> > consistency sake.
> 
> Sorry, but I am still not quite sure what consistency you are
> referring to.
>

Hi Junio,

to recap: in previous patches we removed all instances of "git config
-f .gitmodules" (read/write operations), because in that case
encapsulating access to .gitmodules would directly enable us to add
new functionality (i.e.: read from HEAD:.gitmodules).

So, to me, it looked more organic to remove also the last explicit
reference to .gitmodules in git-submodule.sh ("git add -f .gitmodules",
the "stage" operation), even if in this case no new behavior about
staging is going to be added for the time being. that is: the change is
not strictly necessary.

The consistency I was referring to is merely in the mechanism used to
deal with .gitmodules: by always using "submodule--helper config".

As a side argument: in some previous discussion Stefan mentioned the
possibility that, in the future, he may be interested to explore
different locations for submodules configuration, like a special ref,
to prevent .gitmodules from being in the working tree at all, not even
for writing submodule configuration.

Having an opaque "stage submodule configuration" operation would
prepare for that too, but as I said this is not my immediate goal.

> I also do not see a reason why we want to stop referring to
> .gitmodules explicitly by name.  We do not hide the fact that
> in-tree .gitignore and .gitattributes files are used to hold the
> metainformation about the project tree, saying that it is an
> implementation detail.  Is there a good reason why .gitmodules
> should be different from these other two?

Not sure about that, but one difference I can see
between .gitignore/.gitattributes and .gitmodules is that I got the
impression that editing the latter by hand is strongly discouraged, if
that is indeed the case a layer of indirection can make sense IMHO to
make the actual file path less relevant.

Having said that I can drop patches 06/07 if this can help moving things
forward; or if I had some success in convincing you I can improve the
user interface (any advice?) and the commit message.

Thank you,
   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: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

>> I also do not see a reason why we want to stop referring to
>> .gitmodules explicitly by name.  We do not hide the fact that
>> in-tree .gitignore and .gitattributes files are used to hold the
>> metainformation about the project tree, saying that it is an
>> implementation detail.  Is there a good reason why .gitmodules
>> should be different from these other two?
>
> Not sure about that, but one difference I can see
> between .gitignore/.gitattributes and .gitmodules is that I got the
> impression that editing the latter by hand is strongly discouraged, if
> that is indeed the case a layer of indirection can make sense IMHO to
> make the actual file path less relevant.

I do not think we discourage hand editing of .gitmodules more than
others, say .gitignore; and I do not see a sane reason to do so.

"If you commit broken .gitmodules and let another person clone it,
submodules will not be checked out correctly" is *not* a sane
reason, as exactly the same thing can be said for incorrect checkout
of files with broken .gitattributes.

Quite honestly, I just want to get over with this minor detail that
won't help any scripts (after all submodule--helper is not meant to
be used by humans) and focus on other parts of the patch series.

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

On Mon, 06 Aug 2018 10:38:20 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
> >> I also do not see a reason why we want to stop referring to
> >> .gitmodules explicitly by name.  We do not hide the fact that
> >> in-tree .gitignore and .gitattributes files are used to hold the
> >> metainformation about the project tree, saying that it is an
> >> implementation detail.  Is there a good reason why .gitmodules
> >> should be different from these other two?
> >
> > Not sure about that, but one difference I can see
> > between .gitignore/.gitattributes and .gitmodules is that I got the
> > impression that editing the latter by hand is strongly discouraged, if
> > that is indeed the case a layer of indirection can make sense IMHO to
> > make the actual file path less relevant.
> 
> I do not think we discourage hand editing of .gitmodules more than
> others, say .gitignore; and I do not see a sane reason to do so.
> 
> "If you commit broken .gitmodules and let another person clone it,
> submodules will not be checked out correctly" is *not* a sane
> reason, as exactly the same thing can be said for incorrect checkout
> of files with broken .gitattributes.
>

OK, I see, maybe I got the wrong impression because
while .gitignore/.gitattributes are expected to be hand edited, in my
limited usage of submodules I never had to edit .gitmodules manually
since git did it for me; I guess that does not necessarily mean it's
discouraged and it may even be necessary for more advanced usages.

> Quite honestly, I just want to get over with this minor detail that
> won't help any scripts (after all submodule--helper is not meant to
> be used by humans) and focus on other parts of the patch series.

Agreed, let's drop 06 and 07 then.

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: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Stefan Beller]

> The consistency I was referring to is merely in the mechanism used to
> deal with .gitmodules: by always using "submodule--helper config".
>
> As a side argument: in some previous discussion Stefan mentioned the
> possibility that, in the future, he may be interested to explore
> different locations for submodules configuration, like a special ref,
> to prevent .gitmodules from being in the working tree at all, not even
> for writing submodule configuration.
>
> Having an opaque "stage submodule configuration" operation would
> prepare for that too, but as I said this is not my immediate goal.
>

Thanks for demonstrating that this will be easy to achieve in the future!

> Having said that I can drop patches 06/07 if this can help moving things
> forward; or if I had some success in convincing you I can improve the
> user interface (any advice?) and the commit message.
>

I think dropping the patches would be a good idea,
given that this is not your immediate goal.

Thanks,
Stefan

[RFC PATCH v2 07/12] submodule: use 'submodule--helper config --stage' to stage .gitmodules

[Antonio Ospite]

Use 'git submodule--helper config --stage' in git-submodule.sh to remove
the last place where the .gitmodules file is mentioned explicitly by its
file path.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..f20c37d92d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -289,7 +289,7 @@ or you are unsure what this means choose another name with the '--name' option."
 	then
 		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
-	git add --force .gitmodules ||
+	git submodule--helper config --stage ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
 	# NEEDSWORK: In a multi-working-tree world, this needs to be
-- 
2.18.0

[RFC PATCH v2 08/12] t7506: cleanup .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 means 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 b4b74dbe29..af91ba92ff 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

Re: [RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> 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 means 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.

Makes sense.

Thanks,
Stefan

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

[RFC PATCH v2 09/12] 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.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 17 ++++++++++++++++-
 cache.h                     |  1 +
 submodule-config.c          | 16 ++++++++++++++--
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c388f4ee6f..616d5de0a9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2089,8 +2089,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 still makes it possible to create a brand new
+		 * .gitmodules 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]);
+	}
 
 	return 0;
 }
diff --git a/cache.h b/cache.h
index 8b447652a7..8f75cafbb6 100644
--- a/cache.h
+++ b/cache.h
@@ -424,6 +424,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/submodule-config.c b/submodule-config.c
index 702d40dd6b..cf08264220 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);
 	}
 }
-- 
2.18.0

Re: [RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> 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.

The reading part shows how nice our internal config system is, once
everything is put in place.

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

[RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

[Antonio Ospite]

git submodule commands can now access .gitmodules from the current
branch even when it's not in the working tree, add some tests for that
scenario.

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

For the test files I used the most used style in other tests, Stefan suggested
to avoid subshells and use "git -C" but subshells make the test look cleaner
IMHO.

 t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh

diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
new file mode 100755
index 0000000000..3c7a53316b
--- /dev/null
+++ b/t/t7416-submodule-sparse-gitmodules.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
+#
+
+test_description=' Test reading/writing .gitmodules is 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, but the same scenario can be set up
+also by committing .gitmodules and then just removing it from the filesystem.
+
+NOTE: "git mv" and "git rm" are still supposed to work even without
+a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
+'
+
+. ./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 ! -e .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 'not staging gitmodules config when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config --stage
+	)
+'
+
+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
+	)
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned tree in there and adds a config entry to .git/config. This is because
+# no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule
+#
+# then the repository would be in a clean state and the test below would pass.
+#
+# Maybe cmd_add should do the cleanup from above itself when failing to add
+# a submodule.
+test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_done
-- 
2.18.0

Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> git submodule commands can now access .gitmodules from the current
> branch even when it's not in the working tree, add some tests for that
> scenario.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> For the test files I used the most used style in other tests, Stefan suggested
> to avoid subshells and use "git -C" but subshells make the test look cleaner
> IMHO.

Oh well. Let's not dive into a style argument, so let me focus on the tests.

IMHO it would be nice if (at least partially) these tests are in the same patch
that added the reading from HEAD:.gitmodules  (although I like short
patches, too).

>
>  t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
>
> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 0000000000..3c7a53316b
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,112 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
> +#
> +
> +test_description=' Test reading/writing .gitmodules is 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, but the same scenario can be set up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +
> +NOTE: "git mv" and "git rm" are still supposed to work even without
> +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.

"supposed to work" != "tested that it works" ?
I am not sure what the NOTE wants to tell me? (Should I review those
tests to double check them now? or do we just want to tell future readers
of this test there are other tangent tests to this?)


> +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
> +       )
> +'
> +
> +# "git add" in the test above fails as expected, however it still leaves the
> +# cloned tree in there and adds a config entry to .git/config. This is because
> +# no cleanup is done by cmd_add in git-submodule.sh when "git
> +# submodule--helper config" fails to add a new config setting.
> +#
> +# If we added the following commands to the test above:
> +#
> +#   rm -rf .git/modules/new_submodule &&
> +#   git reset HEAD new_submodule &&
> +#   rm -rf new_submodule

Alternatively we could check for the existence of .gitmodules
before starting all these things?

I think it is okay to not clean up if we check all "regular" or rather expected
things such as a non-writable .gitmodules file before actually doing it.
(This is similar to 'checkout' that walks the whole tree and checks if the
checkout is possible given the dirtyness of the tree, to either abort early
or pull through completely. In catastrophic problems such as a full disk
we'd still die in the middle of work)

> +#
> +# then the repository would be in a clean state and the test below would pass.
> +#
> +# Maybe cmd_add should do the cleanup from above itself when failing to add
> +# a submodule.
> +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '

So this comment and test is about explaining why we can fail mid way through,
which we could not before unless we had the catastrophic event.

I think we should check for a "writable" .gitmodules file at the beginning,
which is if (G || (!G && !H)) [using the notation from the cover letter]?

> +       (cd super &&
> +               git submodule init
> +       )
> +'
> +
> +test_done
> --
> 2.18.0
>

Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

[Antonio Ospite]

On Thu, 2 Aug 2018 13:43:05 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > git submodule commands can now access .gitmodules from the current
> > branch even when it's not in the working tree, add some tests for that
> > scenario.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---

[...]
> > +NOTE: "git mv" and "git rm" are still supposed to work even without
> > +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
> 
> "supposed to work" != "tested that it works" ?

"git mv submod new_submod" and "git rm submod" are actually expected to
work without the .gitmodules file, and there are tests about that in
t3600-rm.sh and t7001-mv.sh:

t3600-rm.sh:
  'rm does not complain when no .gitmodules file is found'

t7001-mv.sh:
  'git mv moves a submodule with a .git directory and no .gitmodules'  
  'mv does not complain when no .gitmodules file is found'

> I am not sure what the NOTE wants to tell me? (Should I review those
> tests to double check them now? or do we just want to tell future readers
> of this test there are other tangent tests to this?)
>

Admittedly the NOTE is not useful without any context: during the
development of "submodule--helper config --stage" I initially assumed
that "git mv" and "git rm" should fail if .gitmodules was not available,
because these commands modify .gitmodules and I added code for that in
stage_updated_gitmodules().

But then later I found out that my assumption was wrong and that git has
tests to verify that these operations on submodules succeed even when
.gitmodules does not exist, which was a little of a surprise to me.

So I removed all my code that was conflicting with git assumptions, and
added the NOTE. However I guess that was primarily a note to myself, and
it should have not slipped in the public patches.

I think I will remove the note, it can be confusing and does not really
add anything, and even less considering that "submodule--helper config
--stage" is going to be dropped.

[...]
> > +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> > +       (cd super &&
> > +               test_must_fail git submodule add ../new_submodule
> > +       )
> > +'
> > +
> > +# "git add" in the test above fails as expected, however it still leaves the
> > +# cloned tree in there and adds a config entry to .git/config. This is because
> > +# no cleanup is done by cmd_add in git-submodule.sh when "git
> > +# submodule--helper config" fails to add a new config setting.
> > +#
> > +# If we added the following commands to the test above:
> > +#
> > +#   rm -rf .git/modules/new_submodule &&
> > +#   git reset HEAD new_submodule &&
> > +#   rm -rf new_submodule
> 
> Alternatively we could check for the existence of .gitmodules
> before starting all these things?
>

You mean in cmd_add(), before doing anything?

The following would anticipates the same check which makes "git submodule
add" fail:

diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..b261175143 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
                shift
        done

+       if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules
+       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" ||

This refers to .gitmodules explicitly but we said that we do not care
about that for now, if opaque access was ever needed in the future,
something like "submodule--helper config --is-writeable" could be added.

> I think it is okay to not clean up if we check all "regular" or rather expected
> things such as a non-writable .gitmodules file before actually doing it.
> (This is similar to 'checkout' that walks the whole tree and checks if the
> checkout is possible given the dirtyness of the tree, to either abort early
> or pull through completely. In catastrophic problems such as a full disk
> we'd still die in the middle of work)
> 
> > +#
> > +# then the repository would be in a clean state and the test below would pass.
> > +#
> > +# Maybe cmd_add should do the cleanup from above itself when failing to add
> > +# a submodule.
> > +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
> 
> So this comment and test is about explaining why we can fail mid way through,
> which we could not before unless we had the catastrophic event.
> 
> I think we should check for a "writable" .gitmodules file at the beginning,
> which is if (G || (!G && !H)) [using the notation from the cover letter]?
> 
> > +       (cd super &&
> > +               git submodule init

With the change from above this last test passes.

BTW the check I am using here and in the code of submodule--helper,
corresponds indeed to the boolean expression you mentioned, but
simplified and negated.

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?

[RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

[Antonio Ospite]

The is_empty_file() function can be generally useful, move it to dir.c
and make it public.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/am.c | 15 ---------------
 dir.c        | 16 ++++++++++++++++
 dir.h        |  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6273ea5195..0c04312a50 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -33,21 +33,6 @@
 #include "string-list.h"
 #include "packfile.h"
 
-/**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
 
 /**
  * Returns the length of the first line of msg.
diff --git a/dir.c b/dir.c
index 21e6f2520a..c1f7b90256 100644
--- a/dir.c
+++ b/dir.c
@@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
 	return ret;
 }
 
+/**
+ * Returns 1 if the file is empty or does not exist, 0 otherwise.
+ */
+int is_empty_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
+
 static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 {
 	DIR *dir;
diff --git a/dir.h b/dir.h
index f5fdedbab2..e45aa3f459 100644
--- a/dir.h
+++ b/dir.h
@@ -281,6 +281,7 @@ static inline int is_dot_or_dotdot(const char *name)
 }
 
 extern int is_empty_dir(const char *dir);
+extern int is_empty_file(const char *filename);
 
 extern void setup_standard_excludes(struct dir_struct *dir);
 
-- 
2.18.0

Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> The is_empty_file() function can be generally useful, move it to dir.c
> and make it public.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>

Makes sense,

Thanks,
Stefan

> +++ b/dir.c
> @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
>         return ret;
>  }
>
> +/**
> + * Returns 1 if the file is empty or does not exist, 0 otherwise.
> + */

Please move the comment to the header instead.
/* possibly as a oneliner ? */

Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

[Antonio Ospite]

On Thu, 2 Aug 2018 13:50:55 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > The is_empty_file() function can be generally useful, move it to dir.c
> > and make it public.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> 
> Makes sense,
> 
> Thanks,
> Stefan
> 
> > +++ b/dir.c
> > @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
> >         return ret;
> >  }
> >
> > +/**
> > + * Returns 1 if the file is empty or does not exist, 0 otherwise.
> > + */
> 
> Please move the comment to the header instead.
> /* possibly as a oneliner ? */

Will do.

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?

[RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

[Antonio Ospite]

In particular this makes it possible to really clean things up when
removing the last submodule with "git rm".

The rationale is that if git creates .gitmodules when adding the first
submodule it should also remove it when removing the last submodule.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule.c                | 10 ++++++++--
 t/t3600-rm.sh              | 32 ++++++++++++++++----------------
 t/t7400-submodule-basic.sh | 11 +++++++++++
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2af09068d7..8cfa82231d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -145,8 +145,14 @@ int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(struct index_state *istate)
 {
-	if (add_file_to_index(istate, GITMODULES_FILE, 0))
-		die(_("staging updated .gitmodules failed"));
+	if (is_empty_file(GITMODULES_FILE)) {
+		if (remove_path(GITMODULES_FILE) < 0)
+			die(_("removing .empty gitmodules failed"));
+		remove_file_from_index(&the_index, GITMODULES_FILE);
+	} else {
+		if (add_file_to_index(istate, GITMODULES_FILE, 0))
+			die(_("staging updated .gitmodules failed"));
+	}
 }
 
 /* TODO: remove this function, use repo_submodule_init instead. */
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..bac2054f51 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -296,7 +296,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git rm submod &&
 	test ! -e submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -307,7 +307,7 @@ test_expect_success 'rm removes removed submodule from index and .gitmodules' '
 	rm -rf submod &&
 	git rm submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -318,7 +318,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -329,7 +329,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git rm submod/ &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm fails when given a file with a trailing /' '
@@ -352,7 +352,7 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -433,7 +433,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated submodule with untracked files fails unless forced' '
@@ -448,7 +448,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'setup submodule conflict' '
@@ -485,7 +485,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a conflicted populated submodule with different HEAD fails unless forced' '
@@ -502,7 +502,7 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -521,7 +521,7 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -540,7 +540,7 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a conflicted populated submodule with a .git directory fails even when forced' '
@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
@@ -618,7 +618,7 @@ test_expect_success 'rm recursively removes work tree of unmodified submodules'
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with different nested HEAD fails unless forced' '
@@ -633,7 +633,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with nested modifications fails unless forced' '
@@ -648,7 +648,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with nested untracked files fails unless forced' '
@@ -663,7 +663,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success "rm absorbs submodule's nested .git directory" '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 48fd14fae6..2bb42a4c8f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -99,6 +99,17 @@ inspect() {
 	)
 }
 
+test_expect_success 'removal of last submodule also removes empty .gitmodules' '
+	(
+		cd addtest &&
+		test ! -d .git/modules &&
+		git submodule add -q "$submodurl" first_submod &&
+		test -e .gitmodules &&
+		git rm -f first_submod &&
+		test ! -e .gitmodules
+	)
+'
+
 test_expect_success 'submodule add' '
 	echo "refs/heads/master" >expect &&
 	>empty &&
-- 
2.18.0

Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

[Stefan Beller]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> In particular this makes it possible to really clean things up when
> removing the last submodule with "git rm".

This sentence is a continuation of the subject line, and I had to reread
it to follow along.

>
> The rationale is that if git creates .gitmodules when adding the first
> submodule it should also remove it when removing the last submodule.

I agree with this sentiment. It seems slightly odd to me to have this tied
in the same patch series that changes .gitmodules reading behavior
as I could think of this feature as orthogonal to what this series achieved
up to patch 10.

> -       test_cmp expect actual &&
> +       test_cmp expect.both_deleted actual &&

This seems to be the re-occuring pattern in t3600, and given that
we have

  cat >expect <<EOF
  M  .gitmodules
  D  submod
  EOF
  cat >expect.both_deleted<<EOF
  D  .gitmodules
  D  submod
  EOF

with no other writing of expect in the range, this seems to be correct.
Maybe worth testing that we do not delete a .gitmodules file if we have
more than one submodule? (But I would expect this to be covered implicitly
somewhere in the test suite. If so that would be worth mentioning in the
commit message instead of writing a test -- just looking quickly we
do have " git rm --cached submodule2" in t7406 which might be sufficient?)

>  test_expect_success "rm absorbs submodule's nested .git directory" '
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 48fd14fae6..2bb42a4c8f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -99,6 +99,17 @@ inspect() {
>         )
>  }
>
> +test_expect_success 'removal of last submodule also removes empty .gitmodules' '
> +       (
> +               cd addtest &&
> +               test ! -d .git/modules &&

test_path_is_missing ?

> +               git submodule add -q "$submodurl" first_submod &&
> +               test -e .gitmodules &&

test_path_is_file

> +               git rm -f first_submod &&

Do we need to force it here?

> +               test ! -e .gitmodules

test_path_is_missing

Thanks for this series!
Overall it was a pleasant read, though I had some comments.

Thanks,
Stefan

Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

[Antonio Ospite]

On Thu, 2 Aug 2018 14:15:54 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > In particular this makes it possible to really clean things up when
> > removing the last submodule with "git rm".
> 
> This sentence is a continuation of the subject line, and I had to reread
> it to follow along.
>

OK, I'll fix that.

> >
> > The rationale is that if git creates .gitmodules when adding the first
> > submodule it should also remove it when removing the last submodule.
> 
> I agree with this sentiment. It seems slightly odd to me to have this tied
> in the same patch series that changes .gitmodules reading behavior
> as I could think of this feature as orthogonal to what this series achieved
> up to patch 10.
>

I will send this as a separate series, I briefly mentioned this
possibility in the cover letter.

> > -       test_cmp expect actual &&
> > +       test_cmp expect.both_deleted actual &&
> 
> This seems to be the re-occuring pattern in t3600, and given that
> we have
> 
>   cat >expect <<EOF
>   M  .gitmodules
>   D  submod
>   EOF
>   cat >expect.both_deleted<<EOF
>   D  .gitmodules
>   D  submod
>   EOF
> 
> with no other writing of expect in the range, this seems to be correct.
> Maybe worth testing that we do not delete a .gitmodules file if we have
> more than one submodule? (But I would expect this to be covered implicitly
> somewhere in the test suite. If so that would be worth mentioning in the
> commit message instead of writing a test -- just looking quickly we
> do have " git rm --cached submodule2" in t7406 which might be sufficient?)
>

I think I will remove the new test in t7400 as the changes to t3600
should cover that case already.

I will check about the case of more than one submodule and I'll add a
comment to the commit message.

[...]
> Thanks for this series!
> Overall it was a pleasant read, though I had some comments.
> 

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?