On Fri, Jan 20, 2017 at 11:17 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:
>
>> Now let's ask the same question for "git -C sub status ." (which is a
>> command that is only reading and not writing to the repository)
>>
>> 1) If the submodule is populated, the user clearly intended to know
>> more about the submodules status
>> 2) It is unclear if the user wanted to learn about the submodules state
>> (So ideally: "The submodule 'sub' is not initialized. To init ...")
>> or the status check should be applied to the superproject instead.
>>
>> Avoid the confusion in 2) as well and just error out for now. Later on
>> we may want to add another flag to git.c to allow commands to be run
>> inside unpopulated submodules and each command reacts appropriately.
>
> I like the general idea of catching commands in unpopulated submodules,
> but I'm somewhat uncomfortable with putting an unconditional check into
> git.c, for two reasons:
>
> 1. Reading the index can be expensive. You would not want "git
> rev-parse" to incur this cost.
Well, I would want rev-parse to not be run in the wrong repo.
(intended to rev-parse something in the submodule, but got results for
the superproject).
Talking about rev-parse, I was about to propose an extension in reply to
"[PATCH] git-prompt.sh: add submodule indicator" that rev-parse could
learn a flag similar to --show-toplevel, named:
--show-superproject-if-any or
--indicate-if-in-submodule-possibly
which would help out there.
>
> 2. How does this interact with commands which do interact with the
> index? Don't they expect to find the_index unpopulated?
That is another sloppiness in this RFC patch, as I haven't nailed down
the corner cases yet.
>
> (I notice that it's effectively tied to RUN_SETUP, which is good.
> But that also means that many commands, like "diff", won't get the
> benefit. Not to mention non-builtins).
>
> I'd rather see it in the commands themselves. Especially given the
> "ideal" in your status example, which requires command-specific
> knowledge.
So you rather want to go bottom up, i.e. add it to each command individually
for which it makes sense, instead of rather first having a catch-it-all like
this and then we can have a flag similar to RUN_SETUP, e.g.
ALLOW_IN_UNPOP_SUBMODULE, which allows commands to
take over the responsibility to act responsibly in this case?
status may be the first command for going that route; I wonder if we'd
want to add this feature unconditionally or only in the porcelain case.
(In plumbing you're supposed to know what you're doing... so there is
no need as well as our promise to not change it)
Thanks,
Stefan
>
> -Peff