Re: [PATCH 01/10] submodule: decouple url and submodule existence

[Brandon Williams]

On 03/01, Stefan Beller wrote:

Sorry I've been slow at rerolling this.  I'll send out a reroll today.

> IIRC most of the series is actually refactoring, i.e.
> providing a central function that answers
> "is this submodule active/initialized?" and then making use of this
> function.
> 
> Maybe it would be easier to reroll these refactorings first without adding
> in the change of behavior.

I agree, when I resend out the series I can drop the first patch so that
the series as a whole just moves to using a standardized function to
check that a submodule is active.  I can then resend out the first patch
in the series on its own so that we can more easily discuss the best
route to take.

> 
> Off the list we talked about different models how to indicate that
> a submodule is active.
> 
> Current situation:
> submodule..URL is used as a boolean flag
> 
> Possible future A:
> 1) If submodule.active exists use that
> 2) otherwise go be individual .URL configs
> 
> submodule.active is a config that contains a pathspec,
> i.e. specifies the group of submodules instead of marking
> each submodule individually.
> 
> How to get to future A:
> * apply this patch series
> 
> Possible future B:
> 1) the boolean submodule..exists is used
> if existent
> 2) If that boolean is missing we'd respect a grouping
>feature such as submodule.active
> 3) fallback to the .URL as a boolean indicator.

Thinking on this, and to maintain simplicity and uniformity, we could
have the per-submodule boolean flag to be "submodule..active"
while the general pathspec based config option would be
"submodule.active".

> 
> How to get to future B:
> 1) possibly take the refactoring part from this series
> 2) implement the .exists flag (that can solve the worktree
>   problem, as then we don't have to abuse the .URL
>   as a boolean indicator any more.)
> 3) implement the grouping feature that takes precedence
>   over .URL, but yields precedence to the .exists flag.
>   (This would solve the grouping problem)
> 
> By having two different series (2) and (3) we'd solve
> just one thing at a time with each series, such that
> this seems easier to understand and review.
> 
> The question is if this future B is also easier to use for
> users and I think it would be, despite being technically more
> complex.
> 
> Most users only have a few submodules. Also the assumption
> is to have either interest in very few specific submodules
> or all of them. For both of these cases the .exists is a good idea
> as it is very explicit, but verbose.
> 
> The grouping feature would help in the case of lots of
> submodules, e.g. to select all of them you would just give
> an easy pathspec "." and that would help going forward
> as by such a submodule spec all new submodules would
> be "auto-on".

And if you wanted all but one you could have submodule.active="." while
submodule.dontwant.active=false instead of having a multi value
submodule.active with an exclusion pathspec.  Both work but one may be
easier for a user to understand.

> 
> Possible future C:
> Similar to B, but with branch specific configuration.
> submodule..active would work as a
> grouping feature. I wonder how it would work with
> the .exists flag.
> 
> Stefan

-- 
Brandon Williams

Re: [PATCH 01/10] submodule: decouple url and submodule existence

[Junio C Hamano]

Brandon Williams  writes:

> On 02/24, Junio C Hamano wrote:
>
>> Also as a grouping measure, submodule.active that lists submodule
>> paths feels hard to use.  When switching between two branches in the
>> superproject that have the same submodule bound at two different
>> paths, who is responsible for updating submodule.active in
>> superproject's config?  If it were a list of submodule names, this
>> objection does not apply, though.
>
> I agree that if you are listing every submodule path by hand then this
> may not be the best approach and would be difficult to use.  The idea is
> that this would allow a user to set a general pathspec to identify a
> group of modules they are interested in.  Perhaps once attributes can be
> used in pathspecs a user could group submodules by setting a particular
> attribute and then submodule.active would have a value like
> ":(attr:foo)" to indicate I'm interested in all submodules with the
> "foo" attribute.

OK.  As .gitattributes is tracked just like .gitmodules in tree, the
project can adjust the path pattern that matches a renamed submodule
and gives it an attribute in .gitattributes in the same commit in
the superproject that moves the directory to which the submodule is
bound, just like it updates .gitmodules to adjust the name<->path
mapping.  So that plan solves my specific worry about using "path"
for grouping and not "name".

Thanks.

Re: [PATCH 01/10] submodule: decouple url and submodule existence

[Stefan Beller]

IIRC most of the series is actually refactoring, i.e.
providing a central function that answers
"is this submodule active/initialized?" and then making use of this
function.

Maybe it would be easier to reroll these refactorings first without adding
in the change of behavior.

Off the list we talked about different models how to indicate that
a submodule is active.

Current situation:
submodule..URL is used as a boolean flag

Possible future A:
1) If submodule.active exists use that
2) otherwise go be individual .URL configs

submodule.active is a config that contains a pathspec,
i.e. specifies the group of submodules instead of marking
each submodule individually.

How to get to future A:
* apply this patch series

Possible future B:
1) the boolean submodule..exists is used
if existent
2) If that boolean is missing we'd respect a grouping
   feature such as submodule.active
3) fallback to the .URL as a boolean indicator.

How to get to future B:
1) possibly take the refactoring part from this series
2) implement the .exists flag (that can solve the worktree
  problem, as then we don't have to abuse the .URL
  as a boolean indicator any more.)
3) implement the grouping feature that takes precedence
  over .URL, but yields precedence to the .exists flag.
  (This would solve the grouping problem)

By having two different series (2) and (3) we'd solve
just one thing at a time with each series, such that
this seems easier to understand and review.

The question is if this future B is also easier to use for
users and I think it would be, despite being technically more
complex.

Most users only have a few submodules. Also the assumption
is to have either interest in very few specific submodules
or all of them. For both of these cases the .exists is a good idea
as it is very explicit, but verbose.

The grouping feature would help in the case of lots of
submodules, e.g. to select all of them you would just give
an easy pathspec "." and that would help going forward
as by such a submodule spec all new submodules would
be "auto-on".

Possible future C:
Similar to B, but with branch specific configuration.
submodule..active would work as a
grouping feature. I wonder how it would work with
the .exists flag.

Stefan

Re: [PATCH 01/10] submodule: decouple url and submodule existence

[Brandon Williams]

On 02/24, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Currently the submodule..url config option is used to determine
> > if a given submodule exists and is interesting to the user.  This
> > however doesn't work very well because the URL is a config option for
> > the scope of a repository, whereas the existence of a submodule is an
> > option scoped to the working tree.
> >
> > In a future with worktree support for submodules, there will be multiple
> > working trees, each of which may only need a subset of the submodules
> > checked out.  The URL (which is where the submodule repository can be
> > obtained) should not differ between different working trees.
> >
> > It may also be convenient for users to more easily specify groups of
> > submodules they are interested in as apposed to running "git submodule
> > init " on each submodule they want checked out in their working
> > tree.
> >
> > To this end, the config option submodule.active is introduced which
> > holds a pathspec that specifies which submodules should exist in the
> > working tree.
> 
> Hmph.  submodule.active in .git/config would be shared the same way
> submodule..url in .git/config is shared across the worktrees
> that stems from the same primary repository, no?
> 
> Perhaps there are some other uses of this submodule.active idea, but
> I do not see how it is relevant to solving "multiple worktrees"
> issue.  Per-worktree config would solve it with the current
> submodule..url without submodule.active list, I would think [*1*].

Correct, I should update the language to indicate this allows the URL to
be shared between worktrees, but a per-worktree config must exist before
submodule.active can actually be used to select different groups of
submodules per-worktree.  The idea is that if submodule.active is set
then you no longer look at the URLs to see what is interesting but
rather at the paths.

> Also as a grouping measure, submodule.active that lists submodule
> paths feels hard to use.  When switching between two branches in the
> superproject that have the same submodule bound at two different
> paths, who is responsible for updating submodule.active in
> superproject's config?  If it were a list of submodule names, this
> objection does not apply, though.

I agree that if you are listing every submodule path by hand then this
may not be the best approach and would be difficult to use.  The idea is
that this would allow a user to set a general pathspec to identify a
group of modules they are interested in.  Perhaps once attributes can be
used in pathspecs a user could group submodules by setting a particular
attribute and then submodule.active would have a value like
":(attr:foo)" to indicate I'm interested in all submodules with the
"foo" attribute.

> 
> 
> 
> [Footnote]
> 
> *1* At the conceptual level, I agree that .url that also means "we
> are interested in this one" feels like somewhat an unclean
> design, but that is not what you are "fixing", is it?
> 

-- 
Brandon Williams

Re: [PATCH 01/10] submodule: decouple url and submodule existence

[Junio C Hamano]

Brandon Williams  writes:

> Currently the submodule..url config option is used to determine
> if a given submodule exists and is interesting to the user.  This
> however doesn't work very well because the URL is a config option for
> the scope of a repository, whereas the existence of a submodule is an
> option scoped to the working tree.
>
> In a future with worktree support for submodules, there will be multiple
> working trees, each of which may only need a subset of the submodules
> checked out.  The URL (which is where the submodule repository can be
> obtained) should not differ between different working trees.
>
> It may also be convenient for users to more easily specify groups of
> submodules they are interested in as apposed to running "git submodule
> init " on each submodule they want checked out in their working
> tree.
>
> To this end, the config option submodule.active is introduced which
> holds a pathspec that specifies which submodules should exist in the
> working tree.

Hmph.  submodule.active in .git/config would be shared the same way
submodule..url in .git/config is shared across the worktrees
that stems from the same primary repository, no?

Perhaps there are some other uses of this submodule.active idea, but
I do not see how it is relevant to solving "multiple worktrees"
issue.  Per-worktree config would solve it with the current
submodule..url without submodule.active list, I would think [*1*].

Also as a grouping measure, submodule.active that lists submodule
paths feels hard to use.  When switching between two branches in the
superproject that have the same submodule bound at two different
paths, who is responsible for updating submodule.active in
superproject's config?  If it were a list of submodule names, this
objection does not apply, though.



[Footnote]

*1* At the conceptual level, I agree that .url that also means "we
are interested in this one" feels like somewhat an unclean
design, but that is not what you are "fixing", is it?