Re: [PATCH 4/4] builtin/clone: support submodule groups

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> writes:

> When invocing clone with the groups option,
> the repository will be configured to the specified groups.
> This has implications for the submodule commands, such as
> update.
>
> Having groups configured will imply init for all uninitialized
> submodules belonging to at least one of the configured groups,
> such that new submodules in the group are initialized
> automatically.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
>  Documentation/git-clone.txt | 13 +++++++++++
>  builtin/clone.c             | 46 +++++++++++++++++++++++++++++++++++---
>  t/t7400-submodule-basic.sh  | 54 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 6db7b6d..685fb7c 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -214,6 +214,19 @@ objects from the source repository into a pack in the cloned repository.
>  	repository does not have a worktree/checkout (i.e. if any of
>  	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
>  
> +--group::

This is misnamed but in an opposite way as the variables and fields
in previous patches.  "Clone" is not primarily about submodules, so
a plain "group" in the context of "clone" does not hint that this
option is about submodules.  It also does not tell the user anything
about what the parameter is for (i.e. there are submodules that are
and are not in the group that is specified with this option.  What
special things happens to the ones in the group?)

I am guessing that it is about the auto-vivifying behaviour like 3/4
did, and if that is the case, perhaps "--init-submodule=<group>" or
something like that?

Oh by the way, I think you meant this option to take an argument by
reading the description (you say "part of the given groups").  Follow
the example of existing option you see below in the context and make
sure that the reader knows they have to give an argument to it.

    A tangent. I think it would be handy to have a way to name a
    single submodule and have the code treat as if there were a
    group that contains that single submodule already defined and
    the user specified that group.  Then this option (and any other
    option that takes a submodule group name) can instead take a
    submodule name and the user can expect a natural thing to happen.

> +	After the clone is created, all submodules which are part of the
> +	given groups are cloned. To specify multiple groups, you can either
> +	give the group argument multiple times or comma separate the groups.
> +	This option will be recorded in the `submodule.groups` config,
> +	which will affect the behavior of other submodule related commands,
> +	such as `git submodule update`.
> +	This option implies recursive submodule checkout. If you don't
> +	want to recurse into nested submodules, you need to specify
> +	`--no-recursive`. The group selection will be passed on recursively,
> +	i.e. if a submodule is cloned because of group membership, its
> +	submodules will be cloned according to group membership, too.
> +

>  --separate-git-dir=<git dir>::
>  	Instead of placing the cloned repository where it is supposed
>  	to be, place the cloned repository at the specified directory,
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b004fb4..72aea3a 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -51,6 +51,22 @@ static struct string_list option_config;
>  static struct string_list option_reference;
>  static int option_dissociate;
>  static int max_jobs = -1;
> +static struct string_list submodule_groups;
> +
> +static int groups_cb(const struct option *opt, const char *arg, int unset)
> +{
> +	struct string_list_item *item;
> +	struct string_list sl = STRING_LIST_INIT_DUP;
> +
> +	if (unset)
> +		return -1;
> +
> +	string_list_split(&sl, arg, ',', -1);

Why not just do "--group A --group B" without "--group A,B"?  I do
not think you accept "git submodule add --group A,B" anyway [*1*],
so doing "nice" things like this is just being inconsistent and does
not help the users.  And I do personally do not think we should make
things consistent by accepting "--group A,B" everywhere.

[Footnote]

*1* Technically, your 'add --group' accepts A;B, I think, but then
this ',' is still inconsistent with it ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]