[PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Stefan Beller]

v2:
* Instead of storing the depth, we keep a boolean `shallow` field in the
  `.gitmodules` file.
* slightly renamed options (--recommend-shallow instead of --recommended-depth)
* one more test
* I dropped [PATCH 1/3] submodule update: make use of the existing fetch_in_submodule function
  as Junio picked it up separately as sb/submodule-misc-cleanups

v1:
Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule.<name>.depth' in .gitmodules, which can be used
to indicate the recommended depth.

Thanks,
Stefan

Stefan Beller (2):
  submodule-config: keep shallow recommendation around
  submodule update: learn `--[no-]recommend-shallow` option

 Documentation/git-submodule.txt | 10 ++++++--
 builtin/submodule--helper.c     |  7 +++++-
 git-submodule.sh                |  9 ++++++-
 submodule-config.c              | 10 ++++++++
 submodule-config.h              |  1 +
 t/t5614-clone-submodules.sh     | 52 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.9.0.rc0.2.g145fc64

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b

[PATCH 1/2] submodule-config: keep shallow recommendation around

[Stefan Beller]

The shallow field will be used in a later patch by `submodule update`.
To differentiate between the actual depth (which may be different),
we name it `recommend_shallow` as the field in the .gitmodules file
is only a recommendation by the project.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 10 ++++++++++
 submodule-config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index debab29..e11b35d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -199,6 +199,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	submodule->update_strategy.command = NULL;
 	submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
 	submodule->ignore = NULL;
+	submodule->recommend_shallow = -1;
 
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +354,15 @@ static int parse_config(const char *var, const char *value, void *data)
 		else if (parse_submodule_update_strategy(value,
 			 &submodule->update_strategy) < 0)
 				die(_("invalid value for %s"), var);
+	} else if (!strcmp(item.buf, "shallow")) {
+		if (!me->overwrite &&
+			 submodule->recommend_shallow != -1)
+			warn_multiple_config(me->commit_sha1, submodule->name,
+					     "shallow");
+		else {
+			submodule->recommend_shallow =
+				git_config_bool(var, value);
+		}
 	}
 
 	strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..b1fdcc0 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -18,6 +18,7 @@ struct submodule {
 	struct submodule_update_strategy update_strategy;
 	/* the sha1 blob id of the responsible .gitmodules file */
 	unsigned char gitmodules_sha1[20];
+	int recommend_shallow;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.9.0.rc0.2.g145fc64

Re: [PATCH 1/2] submodule-config: keep shallow recommendation around

[Remi Galan Alfonso]

Hi Stefan,

Stefan Beller <sbeller@google.com> writes:
> [...]
> @ -353,6 +354,15 @@ static int parse_config(const char *var, const char *value, void *data)
>                  else if (parse_submodule_update_strategy(value,
>                           &submodule->update_strategy) < 0)
>                                  die(_("invalid value for %s"), var);
> +        } else if (!strcmp(item.buf, "shallow")) {
> +                if (!me->overwrite &&
> +                         submodule->recommend_shallow != -1)

Nit: You seems to be able to keep the whole condition on the same line:

		if (!me->overwrite && submodule->recommend_shallow != -1)

If you want to keep it in two line, you might want to align it:
		if (!me->overwrite &&
		    submodule->recommend_shallow != -1)

Thanks,
Rémi

Re: [PATCH 1/2] submodule-config: keep shallow recommendation around

[Stefan Beller]

On Thu, May 26, 2016 at 2:02 AM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Hi Stefan,
>
> Stefan Beller <sbeller@google.com> writes:
>> [...]
>> @ -353,6 +354,15 @@ static int parse_config(const char *var, const char *value, void *data)
>>                  else if (parse_submodule_update_strategy(value,
>>                           &submodule->update_strategy) < 0)
>>                                  die(_("invalid value for %s"), var);
>> +        } else if (!strcmp(item.buf, "shallow")) {
>> +                if (!me->overwrite &&
>> +                         submodule->recommend_shallow != -1)
>
> Nit: You seems to be able to keep the whole condition on the same line:
>
>                 if (!me->overwrite && submodule->recommend_shallow != -1)
>
> If you want to keep it in two line, you might want to align it:
>                 if (!me->overwrite &&
>                     submodule->recommend_shallow != -1)

Thanks will fix!

>
> Thanks,
> Rémi

[PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

[Stefan Beller]

Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a boolean field 'submodule.<name>.shallow' in .gitmodules, which can
be used to recommend whether upstream considers the history important.

This field is honored in the initial clone by default, it can be
ignored by giving the `--no-recommend-shallow` option.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt | 10 ++++++--
 builtin/submodule--helper.c     |  7 +++++-
 git-submodule.sh                |  9 ++++++-
 t/t5614-clone-submodules.sh     | 52 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..c928c0d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,9 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
-	      [-f|--force] [--rebase|--merge] [--reference <repository>]
-	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
+	      [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
+	      [--reference <repository>] [--depth <depth>] [--recursive]
+	      [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
@@ -384,6 +385,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
 	clone with a history truncated to the specified number of revisions.
 	See linkgit:git-clone[1]
 
+--[no-]recommended-depth::
+	This option is only valid for the update command.
+	The initial clone of a submodule will use the recommended
+	`submodule.<name>.depth` as provided by the .gitmodules file.
+
 -j <n>::
 --jobs <n>::
 	This option is only valid for the update command.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8da263f..ca33408 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -581,6 +581,7 @@ struct submodule_update_clone {
 
 	/* configuration parameters which are passed on to the children */
 	int quiet;
+	int recommend_shallow;
 	const char *reference;
 	const char *depth;
 	const char *recursive_prefix;
@@ -593,7 +594,7 @@ struct submodule_update_clone {
 	unsigned quickstop : 1;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-	SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+	SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
 	STRING_LIST_INIT_DUP, 0}
 
 
@@ -698,6 +699,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		argv_array_push(&child->args, "--quiet");
 	if (suc->prefix)
 		argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL);
+	if (suc->recommend_shallow && sub->recommend_shallow == 1)
+		argv_array_push(&child->args, "--depth=1");
 	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 	argv_array_pushl(&child->args, "--name", sub->name, NULL);
 	argv_array_pushl(&child->args, "--url", url, NULL);
@@ -780,6 +783,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			      "specified number of revisions")),
 		OPT_INTEGER('j', "jobs", &max_jobs,
 			    N_("parallel jobs")),
+		OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
+			    N_("whether the initial clone should follow the shallow recommendation")),
 		OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
 		OPT_END()
 	};
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..42e0e9f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
    or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
@@ -559,6 +559,12 @@ cmd_update()
 		--checkout)
 			update="checkout"
 			;;
+		--recommend-shallow)
+			recommend_shallow="--recommend-shallow"
+			;;
+		--no-recommend-shallow)
+			recommend_shallow="--no-recommend-shallow"
+			;;
 		--depth)
 			case "$2" in '') usage ;; esac
 			depth="--depth=$2"
@@ -601,6 +607,7 @@ cmd_update()
 		${update:+--update "$update"} \
 		${reference:+--reference "$reference"} \
 		${depth:+--depth "$depth"} \
+		${recommend_shallow:+"$recommend_shallow"} \
 		${jobs:+$jobs} \
 		"$@" || echo "#unmatched"
 	} | {
diff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh
index 62044c5..32d83e2 100755
--- a/t/t5614-clone-submodules.sh
+++ b/t/t5614-clone-submodules.sh
@@ -82,4 +82,56 @@ test_expect_success 'non shallow clone with shallow submodule' '
 	)
 '
 
+test_expect_success 'clone follows shallow recommendation' '
+	test_when_finished "rm -rf super_clone" &&
+	git config -f .gitmodules submodule.sub.shallow true &&
+	git add .gitmodules &&
+	git commit -m "recommed shallow for sub" &&
+	git clone --recurse-submodules --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git log --oneline >lines &&
+		test_line_count = 4 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 1 lines
+	)
+'
+
+test_expect_success 'get unshallow recommended shallow submodule' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git submodule update --init --no-recommend-shallow &&
+		git log --oneline >lines &&
+		test_line_count = 4 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 3 lines
+	)
+'
+
+test_expect_success 'clone follows non shallow recommendation' '
+	test_when_finished "rm -rf super_clone" &&
+	git config -f .gitmodules submodule.sub.shallow false &&
+	git add .gitmodules &&
+	git commit -m "recommed non shallow for sub" &&
+	git clone --recurse-submodules --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git log --oneline >lines &&
+		test_line_count = 5 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 3 lines
+	)
+'
+
 test_done
-- 
2.9.0.rc0.2.g145fc64

Re: [PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

[Remi Galan Alfonso]

You forgot to update from recommend-depth to recommend-shallow

Stefan Beller <sbeller@google.com> writes:
> [...]
>  'git submodule' [--quiet] init [--] [<path>...]
>  'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> -              [-f|--force] [--rebase|--merge] [--reference <repository>]
> -              [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
> +              [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]

Here...

> +--[no-]recommended-depth::
> +        This option is only valid for the update command.
> +        The initial clone of a submodule will use the recommended
> +        `submodule.<name>.depth` as provided by the .gitmodules file.
> +

... and here.

Thanks,
Rémi

Re: [PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

[Stefan Beller]

On Thu, May 26, 2016 at 2:07 AM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> You forgot to update from recommend-depth to recommend-shallow
>
> Stefan Beller <sbeller@google.com> writes:
>> [...]
>>  'git submodule' [--quiet] init [--] [<path>...]
>>  'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
>>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
>> -              [-f|--force] [--rebase|--merge] [--reference <repository>]
>> -              [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
>> +              [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
>
> Here...
>
>> +--[no-]recommended-depth::
>> +        This option is only valid for the update command.
>> +        The initial clone of a submodule will use the recommended
>> +        `submodule.<name>.depth` as provided by the .gitmodules file.
>> +
>
> ... and here.
>
> Thanks,
> Rémi

Thanks for the review,
these will be fixed in a reroll.

Thanks,
Stefan

Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Sometimes the history of a submodule is not considered important by
> the projects upstream. To make it easier for downstream users, allow
> a field 'submodule.<name>.depth' in .gitmodules, which can be used
> to indicate the recommended depth.

I have a design-level question.

If your project consists of 600 submodules, among which 40 of them
you would recommend making a shallow clone, are there traits, other
than "most people would not care about its history", that are shared
across these 40 subprojects?

What I am trying to get at is that after adding .shallow annotation
to these 40 submodules in .gitmodules, the project may need to add a
different annotation for the same 40 submodules to control another
operation.  Which would be cumbersome, and a level of redirection
might be a good way to solve it.

IIRC, earlier you had talked about a vision where you can just say
'submodule init --group=framework' to prepare your top-level project
tree with its submodules in a state suitable to work on 'the
framework part of the project', and the 'app' folks can substitute
'framework' with 'app' in that command.  I thought the earlier
defaultGroup work (and the attribute limited pathspec work that lays
groundwork for it) was part of that effort.

Perhaps when a user says "submodule init --group=framework", that
"framework" can choose some "developer profile" that indirectly
specifies things like which group of submodules to initialize, which
group of submodules can be shallow, etc.?

Just a strawman (without worrying about details and expressiveness
of the syntax), I am wondering if you want something like this in
your .gitmodules:

    [profile "framework"]
	initialize = $submodule_spec
        shallow = $submodule_spec
        ...

    [submodule "kernel"]
    	url = ...
        path = ...

    ...

where $submodule_spec would be a way to choose modules by various
means.  You may name them by their names.  You may name them by
their paths.  With the submodule-pathspec topic graduated, you may
use ":(attr:framework)*" to choose them by attribute limited
pathspec.

Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Stefan Beller]

On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Sometimes the history of a submodule is not considered important by
>> the projects upstream. To make it easier for downstream users, allow
>> a field 'submodule.<name>.depth' in .gitmodules, which can be used
>> to indicate the recommended depth.
>
> I have a design-level question.
>
> If your project consists of 600 submodules, among which 40 of them
> you would recommend making a shallow clone, are there traits, other
> than "most people would not care about its history", that are shared
> across these 40 subprojects?

>From my understanding these 40 subprojects are a large file storage
done different than Git LFS. In the repo world this was choosen to be
a separate repository, such that you had versioning available as the
large files change a few times (like a precompiled kernel for special
hardware, etc). And this is one of the missing pieces to translate the
current repo-tool workflow to submodules.

>
> What I am trying to get at is that after adding .shallow annotation
> to these 40 submodules in .gitmodules, the project may need to add a
> different annotation for the same 40 submodules to control another
> operation.  Which would be cumbersome, and a level of redirection
> might be a good way to solve it.
>
> IIRC, earlier you had talked about a vision where you can just say
> 'submodule init --group=framework' to prepare your top-level project
> tree with its submodules in a state suitable to work on 'the
> framework part of the project', and the 'app' folks can substitute
> 'framework' with 'app' in that command.  I thought the earlier
> defaultGroup work (and the attribute limited pathspec work that lays
> groundwork for it) was part of that effort.
>
> Perhaps when a user says "submodule init --group=framework", that
> "framework" can choose some "developer profile" that indirectly
> specifies things like which group of submodules to initialize, which
> group of submodules can be shallow, etc.?

So you are proposing another layer of indirection, i.e. instead of giving
a pathspec with ":(attr:label-framework)" we would want to give a profile
which then has the pathspec plus additional information on shallowness
an such.

>
> Just a strawman (without worrying about details and expressiveness
> of the syntax), I am wondering if you want something like this in
> your .gitmodules:
>
>     [profile "framework"]
>         initialize = $submodule_spec
>         shallow = $submodule_spec
>         ...

There could be more operations here, like update strategies.

>
>     [submodule "kernel"]
>         url = ...
>         path = ...

instead here we could also put a (no-)init recommendation additionally
to the shallow recommendation.

>
>     ...
>
> where $submodule_spec would be a way to choose modules by various
> means.  You may name them by their names.  You may name them by
> their paths.  With the submodule-pathspec topic graduated, you may
> use ":(attr:framework)*" to choose them by attribute limited
> pathspec.
>

And you reinvented submodule groups. ;)
IIRC we had a discussion if we want to have the submodule groups
stored at each submodule or at a central "profile/group" setting.
The advantage of putting the setting to each submodule is that it
is easier to organize, i.e. it produces better locality for the settings.

It is easier to know for the [submodule "kernel"] what to set for flags or
labels when looking at that submodule as you're likely to have knowledge
about it. However adding a new group/profile is cumbersome, so we'd want a

    git config --add multi-set
submodule.<pathspec-matching-many-submodules>.label "initialize"

Another idea for having profiles would be to add conditional recommendations

     [submodule "kernel"]
         url = ...
         path = ...
         shallow = true if selected via :(attr:framework)

I have a worse feeling about these conditionals than about the profile, though

I think the profiles are selected using different repo manifest files, i.e. that
would be different .gitmodules/.gitattributes files on different branches.
However in each of these branches we would want to have a way to
recommend which submodules to initialize/checkout/shallow and such.

If keeping these different settings in different branches, we may desire better
merge support for .gitattributes, though (adjacent lines do not influence each
other, like in source code. So if one branch had a change like
     submodule0 ...
    - /submodule1 label-foo
    + /submodule1 label-foo label-bar
     submodule2 ...

and andother branch had

     submodule1 label-foo
    - /submodule2 label-baz
    + /submodule2 label-baz label-bar
     submodule3 ...

we would not want to see the merge conflict as we would with todays merge
strategy.)

Thanks,
Stefan

Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Sometimes the history of a submodule is not considered important by
>>> the projects upstream. To make it easier for downstream users, allow
>>> a field 'submodule.<name>.depth' in .gitmodules, which can be used
>>> to indicate the recommended depth.
>>
>> I have a design-level question.
>>
>> If your project consists of 600 submodules, among which 40 of them
>> you would recommend making a shallow clone, are there traits, other
>> than "most people would not care about its history", that are shared
>> across these 40 subprojects?
>
> From my understanding these 40 subprojects are a large file storage
> done different than Git LFS. In the repo world this was choosen to be
> a separate repository, such that you had versioning available as the
> large files change a few times (like a precompiled kernel for special
> hardware, etc). And this is one of the missing pieces to translate the
> current repo-tool workflow to submodules.

I am not questioning the value of being able to say "this and that
submodule need only a shallow copy".  I am trying to see
"individually mark each and every such submodules" should be the
primary interface to do so.

> So you are proposing another layer of indirection, i.e. instead of giving
> a pathspec with ":(attr:label-framework)" we would want to give a profile
> which then has the pathspec plus additional information on shallowness
> an such.

I do not understand what you mean by "instead of giving a pathspec"
at all.

When you have 40 submodules, with your design the user has to
sprinkle 40 lines of

	shallow = true

for each of [submodule "$n"] sections.  If there are other traits
(see my first question) that are similar, they will have some other
setting next to the "shallow = true" 40 times.  When a new submodule
is added to that same class, they will again have to add these two
lines.

I was wondering if a level of indirection that lets you say
"submodules in this group all share 'shallow=true' (by default)"
would improve that cumbersomeness.  When you add another similar
trait, you add that just once, not 40 times.  When you add another
submodule, you say "this submodule is also in that group", without
mentioning "shallow".

We probably _need_ shallow=true at individual module level, if only
to override the default given by such an indirection scheme.  So
don't take the message you are responding to as "no, your design is
not good, scrap it, and here is a better one".  It is more like "It
would be a good first step, but have you considered where it will
eventually lead us to?  Would the more complete future look like
this, and how well would this first step fit in that world?  Would
it be a good escape hatch, or would it have to be deprecated?"

> And you reinvented submodule groups. ;)
> IIRC we had a discussion if we want to have the submodule groups
> stored at each submodule or at a central "profile/group" setting.

As I said, it was not my intention to get into that; I am not
interested in the exact syntax, and I am not interested whether the
pointer goes from group to individual modules (i.e. [group "bar"]
says "foo" is one of its member modules), or individual modules have
pointers to groups (i.e. "module [foo]" declares its membership in
group "bar") at all.  I really do not care.

What matters in my suggestion was, after you established that group
"bar" exists, you can do:

	[profile "framework"]
        	shallow = "some notation that refers to group bar"

so that you do not have to repeat "shallow = true" many times per
submodule.

By the way, I do not see the "profile" as about "submodules" or
"submodule groups".  It is more about "Who am I?  What am I working
on?  Give me an appropriate set of settings for the 600 submodules
you have, with the knowledge that I am a framework person".

grouping submodules would merely be one mechanism to help specify
that.

Re: [PATCHv2 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Stefan Beller]

On Thu, May 26, 2016 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, May 26, 2016 at 11:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> Sometimes the history of a submodule is not considered important by
>>>> the projects upstream. To make it easier for downstream users, allow
>>>> a field 'submodule.<name>.depth' in .gitmodules, which can be used
>>>> to indicate the recommended depth.
>>>
>>> I have a design-level question.
>>>
>>> If your project consists of 600 submodules, among which 40 of them
>>> you would recommend making a shallow clone, are there traits, other
>>> than "most people would not care about its history", that are shared
>>> across these 40 subprojects?
>>
>> From my understanding these 40 subprojects are a large file storage
>> done different than Git LFS. In the repo world this was choosen to be
>> a separate repository, such that you had versioning available as the
>> large files change a few times (like a precompiled kernel for special
>> hardware, etc). And this is one of the missing pieces to translate the
>> current repo-tool workflow to submodules.
>
> I am not questioning the value of being able to say "this and that
> submodule need only a shallow copy".  I am trying to see
> "individually mark each and every such submodules" should be the
> primary interface to do so.
>
>> So you are proposing another layer of indirection, i.e. instead of giving
>> a pathspec with ":(attr:label-framework)" we would want to give a profile
>> which then has the pathspec plus additional information on shallowness
>> an such.
>
> I do not understand what you mean by "instead of giving a pathspec"
> at all.
>
> When you have 40 submodules, with your design the user has to
> sprinkle 40 lines of
>
>         shallow = true
>
> for each of [submodule "$n"] sections.  If there are other traits
> (see my first question) that are similar, they will have some other
> setting next to the "shallow = true" 40 times.  When a new submodule
> is added to that same class, they will again have to add these two
> lines.

I could not find out about another class or attribute matching like that.
Sure there are other attributes (Labels), but they don't match closely
these 40 submodules, but are arbitrary.

>
> I was wondering if a level of indirection that lets you say
> "submodules in this group all share 'shallow=true' (by default)"
> would improve that cumbersomeness.  When you add another similar,
> trait, you add that just once, not 40 times.  When you add another
> submodule, you say "this submodule is also in that group", without
> mentioning "shallow".
>
> We probably _need_ shallow=true at individual module level, if only
> to override the default given by such an indirection scheme.  So
> don't take the message you are responding to as "no, your design is
> not good, scrap it, and here is a better one".  It is more like "It
> would be a good first step, but have you considered where it will
> eventually lead us to?

Even if we decide we need it later, this doesn't seem to be a blocker
or contradicting such an enhancement.

> Would the more complete future look like
> this, and how well would this first step fit in that world?  Would
> it be a good escape hatch, or would it have to be deprecated?"

Looking at the Android open source project, we have 45 repos,
that are marked with clone-depth="1" , 37/45 are in a path starting
with "./prebuilts/", i.e. competing with Git LFS.
The remaining 8/45 seem to be device specific kernels.
37 of them (a different set than the prebuilts though) carry one label.

So with that data, I would expect that you would not need to mark a
submodule often as shallow, but it's quite obvious at creation time.
And specially at creation time it is easier to edit the module specific section
as opposed to adding the module in a profile or other indirect section.

I note, that we can extend to an indirection concept later if needed, though.

>
>> And you reinvented submodule groups. ;)
>> IIRC we had a discussion if we want to have the submodule groups
>> stored at each submodule or at a central "profile/group" setting.
>
> As I said, it was not my intention to get into that; I am not
> interested in the exact syntax, and I am not interested whether the
> pointer goes from group to individual modules (i.e. [group "bar"]
> says "foo" is one of its member modules), or individual modules have
> pointers to groups (i.e. "module [foo]" declares its membership in
> group "bar") at all.  I really do not care.
>
> What matters in my suggestion was, after you established that group
> "bar" exists, you can do:
>
>         [profile "framework"]
>                 shallow = "some notation that refers to group bar"
>
> so that you do not have to repeat "shallow = true" many times per
> submodule.

Ok, I just wonder how hard that  "some notation that refers to group bar"
is to get right. (Both on the design level for us as well as for users to
understand how to express themselves and then doing it right).

And I would not try to outsmart the user here? The repetition of
40 times "shallow" may be cumbersome, but only for creation,
not for maintaining!

Having more lines in the .gitmodules file is not a bad thing per se, though?
If you have these 600 submodules, you would search for the one you're
currently interested in anyway, so it doesn't matter if the average
number of lines
per modules is 3 or 5.

>
> By the way, I do not see the "profile" as about "submodules" or
> "submodule groups".  It is more about "Who am I?  What am I working
> on?  Give me an appropriate set of settings for the 600 submodules
> you have, with the knowledge that I am a framework person".

And I thought branches with different .gitmodule/.gitattribute files would
solve this.

Compare to the kernel development: a network person will use the
network maintainers branch to start developing; They will not get
Linus copy and set some magic to indicate they're a network person?

>
> grouping submodules would merely be one mechanism to help specify
> that.
>

Thanks for raising concerns, it actually made me think of more issues.

Thanks,
Stefan

[PATCHv3 0/2] Submodule shallow recommendation [WAS: Submodules: have a depth field in the .gitmodules file]

[Stefan Beller]

v3:
* fixed documentation to consistently mention recommend-shallow
* realigned code in the config patch
* Thanks Remi for reviewing! diff to v2:

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index c928c0d..bf3bb37 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
-	      [--[no-]recommended-depth] [-f|--force] [--rebase|--merge]
+	      [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
 	      [--reference <repository>] [--depth <depth>] [--recursive]
 	      [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
@@ -385,10 +385,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
 	clone with a history truncated to the specified number of revisions.
 	See linkgit:git-clone[1]
 
---[no-]recommended-depth::
+--[no-]recommend-shallow::
 	This option is only valid for the update command.
 	The initial clone of a submodule will use the recommended
-	`submodule.<name>.depth` as provided by the .gitmodules file.
+	`submodule.<name>.shallow` as provided by the .gitmodules file
+	by default. To ignore the suggestions use `--no-recommend-shallow`.
 
 -j <n>::
 --jobs <n>::
diff --git a/submodule-config.c b/submodule-config.c
index e11b35d..db1847f 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -355,8 +355,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			 &submodule->update_strategy) < 0)
 				die(_("invalid value for %s"), var);
 	} else if (!strcmp(item.buf, "shallow")) {
-		if (!me->overwrite &&
-			 submodule->recommend_shallow != -1)
+		if (!me->overwrite && submodule->recommend_shallow != -1)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					     "shallow");
 		else {


v2:
* Instead of storing the depth, we keep a boolean `shallow` field in the
  `.gitmodules` file.
* slightly renamed options (--recommend-shallow instead of --recommended-depth)
* one more test
* I dropped [PATCH 1/3] submodule update: make use of the existing fetch_in_submodule function
  as Junio picked it up separately as sb/submodule-misc-cleanups

v1:
Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a field 'submodule.<name>.depth' in .gitmodules, which can be used
to indicate the recommended depth.

Thanks,
Stefan


Stefan Beller (2):
  submodule-config: keep shallow recommendation around
  submodule update: learn `--[no-]recommend-shallow` option

 Documentation/git-submodule.txt | 11 +++++++--
 builtin/submodule--helper.c     |  7 +++++-
 git-submodule.sh                |  9 ++++++-
 submodule-config.c              |  9 +++++++
 submodule-config.h              |  1 +
 t/t5614-clone-submodules.sh     | 52 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.9.0.rc0.2.g145fc64

base-commit: 3a0f269e7c82aa3a87323cb7ae04ac5f129f036b

[PATCH 2/2] submodule update: learn `--[no-]recommend-shallow` option

[Stefan Beller]

Sometimes the history of a submodule is not considered important by
the projects upstream. To make it easier for downstream users, allow
a boolean field 'submodule.<name>.shallow' in .gitmodules, which can
be used to recommend whether upstream considers the history important.

This field is honored in the initial clone by default, it can be
ignored by giving the `--no-recommend-shallow` option.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt | 11 +++++++--
 builtin/submodule--helper.c     |  7 +++++-
 git-submodule.sh                |  9 ++++++-
 t/t5614-clone-submodules.sh     | 52 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..bf3bb37 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,9 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [<path>...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
-	      [-f|--force] [--rebase|--merge] [--reference <repository>]
-	      [--depth <depth>] [--recursive] [--jobs <n>] [--] [<path>...]
+	      [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
+	      [--reference <repository>] [--depth <depth>] [--recursive]
+	      [--jobs <n>] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
@@ -384,6 +385,12 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
 	clone with a history truncated to the specified number of revisions.
 	See linkgit:git-clone[1]
 
+--[no-]recommend-shallow::
+	This option is only valid for the update command.
+	The initial clone of a submodule will use the recommended
+	`submodule.<name>.shallow` as provided by the .gitmodules file
+	by default. To ignore the suggestions use `--no-recommend-shallow`.
+
 -j <n>::
 --jobs <n>::
 	This option is only valid for the update command.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8da263f..ca33408 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -581,6 +581,7 @@ struct submodule_update_clone {
 
 	/* configuration parameters which are passed on to the children */
 	int quiet;
+	int recommend_shallow;
 	const char *reference;
 	const char *depth;
 	const char *recursive_prefix;
@@ -593,7 +594,7 @@ struct submodule_update_clone {
 	unsigned quickstop : 1;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-	SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
+	SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
 	STRING_LIST_INIT_DUP, 0}
 
 
@@ -698,6 +699,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		argv_array_push(&child->args, "--quiet");
 	if (suc->prefix)
 		argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL);
+	if (suc->recommend_shallow && sub->recommend_shallow == 1)
+		argv_array_push(&child->args, "--depth=1");
 	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 	argv_array_pushl(&child->args, "--name", sub->name, NULL);
 	argv_array_pushl(&child->args, "--url", url, NULL);
@@ -780,6 +783,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 			      "specified number of revisions")),
 		OPT_INTEGER('j', "jobs", &max_jobs,
 			    N_("parallel jobs")),
+		OPT_BOOL(0, "recommend-shallow", &suc.recommend_shallow,
+			    N_("whether the initial clone should follow the shallow recommendation")),
 		OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
 		OPT_END()
 	};
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..42e0e9f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
    or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
@@ -559,6 +559,12 @@ cmd_update()
 		--checkout)
 			update="checkout"
 			;;
+		--recommend-shallow)
+			recommend_shallow="--recommend-shallow"
+			;;
+		--no-recommend-shallow)
+			recommend_shallow="--no-recommend-shallow"
+			;;
 		--depth)
 			case "$2" in '') usage ;; esac
 			depth="--depth=$2"
@@ -601,6 +607,7 @@ cmd_update()
 		${update:+--update "$update"} \
 		${reference:+--reference "$reference"} \
 		${depth:+--depth "$depth"} \
+		${recommend_shallow:+"$recommend_shallow"} \
 		${jobs:+$jobs} \
 		"$@" || echo "#unmatched"
 	} | {
diff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh
index 62044c5..32d83e2 100755
--- a/t/t5614-clone-submodules.sh
+++ b/t/t5614-clone-submodules.sh
@@ -82,4 +82,56 @@ test_expect_success 'non shallow clone with shallow submodule' '
 	)
 '
 
+test_expect_success 'clone follows shallow recommendation' '
+	test_when_finished "rm -rf super_clone" &&
+	git config -f .gitmodules submodule.sub.shallow true &&
+	git add .gitmodules &&
+	git commit -m "recommed shallow for sub" &&
+	git clone --recurse-submodules --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git log --oneline >lines &&
+		test_line_count = 4 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 1 lines
+	)
+'
+
+test_expect_success 'get unshallow recommended shallow submodule' '
+	test_when_finished "rm -rf super_clone" &&
+	git clone --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git submodule update --init --no-recommend-shallow &&
+		git log --oneline >lines &&
+		test_line_count = 4 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 3 lines
+	)
+'
+
+test_expect_success 'clone follows non shallow recommendation' '
+	test_when_finished "rm -rf super_clone" &&
+	git config -f .gitmodules submodule.sub.shallow false &&
+	git add .gitmodules &&
+	git commit -m "recommed non shallow for sub" &&
+	git clone --recurse-submodules --no-local "file://$pwd/." super_clone &&
+	(
+		cd super_clone &&
+		git log --oneline >lines &&
+		test_line_count = 5 lines
+	) &&
+	(
+		cd super_clone/sub &&
+		git log --oneline >lines &&
+		test_line_count = 3 lines
+	)
+'
+
 test_done
-- 
2.9.0.rc0.2.g145fc64