git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.

From
Duy Nguyen <pclouds@gmail.com>
Date
Jan 21, 2017, 12:55 UTC
Message-ID
<CACsJy8DJpueeRqLSbGuY6gsMKeYd+Q0dUzFB4p_Sq-795MhG1A@mail.gmail.com>
In-Reply-To
<20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net>
On Sat, Jan 21, 2017 at 2:17 AM, Jeff King <peff@peff.net> wrote:
Show 32 quoted lines
> 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.
>
>   2. How does this interact with commands which do interact with the
>      index? Don't they expect to find the_index unpopulated?
>
>      (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.

I agree. It's already bad enough for pathspec code to peek into the index, adding a hidden dependency between parse_pathspec() and read_cache(). And I still think parse_pathspec() is not the right place to check submodule paths. Worktree should be checked as well, in the case that the submodule is not yet registered in the index. The right place to do that is per-command, with their consent so to speak, because they may need to set things up (index, .git/config and stuff) properly before explicitly doing this check.

-- 
Duy
Previous: Stefan Beller
Message 13 of 13 in “Disallow commands from within unpopulated submodules.”
  1. Disallow commands from within unpopulated submodules.Stefan Beller, Jan 19, 2017
  2. Jeff KingJan 20, 2017
  3. Stefan BellerJan 20, 2017
  4. Jeff KingJan 20, 2017
  5. Stefan BellerJan 20, 2017
  6. Jeff KingJan 20, 2017
  7. Stefan BellerJan 20, 2017
  8. Junio C HamanoJan 20, 2017
  9. Jeff KingJan 20, 2017
  10. Junio C HamanoJan 20, 2017
  11. Jeff KingJan 20, 2017
  12. Stefan BellerJan 20, 2017
  13. Duy NguyenJan 21, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.