patchdoc: remove mentions of .gitmodules !command syntax

From: Petar Vutov <pvutov@imap.cc>

To mitigate CVE-2019-19604, the capability to configure `git submodule update` to execute custom commands was removed in v2.20.2.

The git-submodule documentation still mentions the now-unsupported syntax, which is misleading.

Remove the leftover documentation.

Signed-off-by: Petar Vutov <pvutov@imap.cc>
---
 Documentation/git-submodule.txt | 6 ------
 1 file changed, 6 deletions(-)

pvutov@imap.cc writes:

The change during v2.20.2 timeperiod you have in mind may be e904deb8 (submodule: reject submodule.update = !command in .gitmodules, 2019-12-05). The key phrase is "in .gitmodules" as it did not forbid writing update command in the configuration.

The pre-context lines of your patch (see below) say that the 'custom command' option and 'none' option are only available via the `submodule.<name>.update` configuration variable. IOW, this part of the documentation does not talk about the .gitmodules file---it talks about what you can say in the configuration file (which is under your local control).

I think the existing text that came from fc01a5d2 (submodule update documentation: don't repeat ourselves, 2016-12-27) may be misleading, and may has room for improvement, but I do not think it is wrong per-se. If we remove it, there is nowhere else that teaches users !cmd can be set in their configuration files, or is there?

Thanks.

On 7/12/23 18:40, Junio C Hamano wrote:

Thanks for the review. I was not aware of the .gitconfig use case. I hit that paragraph while trying to enforce sparse-checkout via .gitmodules. Yet the gitmodules doc is clear enough:

"See description of update command in git-submodule[1] for their meaning. For security reasons, the !command form is not accepted here."

Clearly I followed the link in the first sentence without reading the second :)

Perhaps the term "configuration variable" in

"The following update procedures are only available via the submodule.<name>.update configuration variable:"

is more specific and technical than immediately obvious - I would have expected the contents of .gitmodules to be a form of (repository) configuration. But that is just bikeshedding.

Petar Vutov <pvutov@imap.cc> writes:

Not necessarily. It is always good to learn how "normal people" (read: those other than who develop and write documentation for Git) would react to and understand what we write in the manual pages, as that is the only way we can find what is confusing, ambiguous and can be improved.

Perhaps

    "... available via the ... configuration variable, and cannot be
    used in the .gitmodules file"

would have helped you?

Thanks.

On 7/12/23 19:54, Junio C Hamano wrote:

I think that addition is good and makes the intent clearer.

But it's slightly wrong - only 'custom command' cannot be used in .gitmodules. 'none' is legal (and mentioned in the gitmodules doc as such).

What about something like this?

 From 7b2fcd9a56b4954863cc74e1cf89a4b9d9d3ad52 Mon Sep 17 00:00:00 2001
From: Petar Vutov <pvutov@imap.cc>
Date: Wed, 12 Jul 2023 20:40:10 +0200
Subject: [PATCH] docs: highlight that .gitmodules does not support !command

The `custom command` and `none` entries are described as sharing the same limitations, but one is allowed in .gitmodules and the other is not. Instead, describe their limitations separately and with slightly more detail.

Signed-off-by: Petar Vutov <pvutov@imap.cc>
---
  Documentation/git-submodule.txt | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

Petar Vutov <pvutov@imap.cc> writes:

Hmph, that will be a bugfix for fc01a5d2 (submodule update documentation: don't repeat ourselves, 2016-12-27). I think you are technically correct.

    Side note: but is there a useful use case to set it to 'none' in
    ".gitmodules" in the first place?  If there is not, saying that
    "'none' is only useful in the configuration files" is not quite
    wrong per-se.

Sounds good.  s/available/allowed/, perhaps.

This side we do not need "only" anywhere in the sentence, do we?

Thanks.

On 7/12/23 22:33, Junio C Hamano wrote:

A twisted person might use it to commit a truly empty folder, without even a .gitkeep file. Whether that's a useful use case, I don't know.

I will address both of those and include your bugfix blurb in v3. Still figuring out how to properly reference the previous patches using git-send-email..

Petar Vutov <pvutov@imap.cc> writes:

$ git send-email --in-reply-to='<7090349c-4485-d5c4-1f26-190974864f72@imap.cc>' ...

From: Petar Vutov <pvutov@imap.cc>

Bugfix for fc01a5d2 (submodule update documentation: don't repeat ourselves, 2016-12-27).

The `custom command` and `none` entries are described as sharing the same limitations, but one is allowed in .gitmodules and the other is not. Instead, describe their limitations separately and in slightly more detail.

Signed-off-by: Petar Vutov <pvutov@imap.cc>
---

Changes from v1: Don't delete the documentation for `!command`. Instead, highlight the differences in the limitations of `none` and `!command`.

Changes from v2: Improve phrasing. Add the bugfix blurb in the commit message.

 Documentation/git-submodule.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

pvutov@imap.cc writes:

Sounds sensible.

But the usual ones like "merge" are also allowed in both places. Does this still need to be said?

I wonder if it makes more sense to swap the order of these two entries, showing "none" without any additional text first, and then describe "custom command" with the note, exactly like you did above.

Oops, this was supposed to go under <7090349c-4485-d5c4-1f26-190974864f72@imap.cc>..

Side question in this side thread: I was tempted to change the mention of "configuration variable" with "git-config variable", to highlight the narrow meaning of the term. But I grepped and that term is used everywhere in the documentation. Changing it only in this section would be inconsistent.

If I were to go through all those mentions and figure out which "configuration variable" mentions refer exclusively to `git-config`, wis there interest in changing them to something like "git-config variable" in a potential future patch? Or do we like the term "configuration variable"?

Petar Vutov <pvutov@imap.cc> writes:

Don't. People should be familiar with "configuration variable", but may not be with "git-config variable".

How about doing it this way? I moved 'none' up, as it is effective in `.gitmodules` and as a configuration variable, just like all others, and then completely rewrote the somewhat awkward explanation of the custom command thing.

----- >8 --------- >8 --------- >8 --------- >8 ----
Subject: submodule: clarify that "!custom command" is the only oddball

We singled out 'none' and 'custom command' as submodule update modes that cannot be specified in the .gitmodules file, but 'none' can appear there, and use of a custom command is the only oddball.

Move the description of 'none' up, clarify how the custom command is used, and explicitly say it cannot be used in the `.gitmodules` file.

Strictly speaking, the last one should not be needed, as we already say `configuration variable`, but to new readers, the distinction between the configuration variable and settings that appear in the .gitmodules file may not be apparent; hopefully the new text will help them understand where it can(not) be used.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-submodule.txt | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

On 7/13/23 21:55, Junio C Hamano wrote:

Right, on closer inspection this collision with .gitmodules is rather niche to start with. Few things in .gitconfig are also legal in .gitmodules.

I prefer the original description as it uses shorter sentences. I can't hold that 5-liner in my head :)

But now I really am bikeshedding, and I've taken enough of your time. I will send a v4 with just the .gitmodules disclaimer, no rewrite, in case you end up agreeing.

Petar Vutov <pvutov@imap.cc> writes:

But you are comparing oranges and apples, aren't you? You are not counting "oh by the way this cannot be in .gitmodules" as part of 5.

After reading the original again and again, I hate more and more the way the original wasted too many words to refer to one thing twice (e.g. "arbitrary command" and then "is the custom command", "a single argument" and then the parenthesized explanation of it) without adding any clarity.

Junio C Hamano <gitster@pobox.com> writes:

Another thing I noticed is that this section is ONLY talking about the configuration variable, so everything both of us have been saying misses the point. The preamble before the choices ("checkout", "rebase", "merge", "custom" and "none") are listed read like so:

    update [--init] [--remote] [-N|--no-f...
    +
    --
    Update the registered submodules to match what the superproject
    expects by cloning missing submodules, fetching missing commits
    in submodules and updating the working tree of
    the submodules. The "updating" can be done in several ways depending
    on command line options and the value of `submodule.<name>.update`
    configuration variable. The command line option takes precedence over
    the configuration variable. If neither is given, a 'checkout' is performed.
    The 'update' procedures supported both from the command line as well as
    through the `submodule.<name>.update` configuration are:

Then why do we even think about referring to ".gitmodules" here? It is because of this behaviour that is described elsewhere:

    init [--] [<path>...]::
            Initialize the submodules recorded in the index (which were
            added and committed elsewhere) by setting `submodule.$name.url`
            in .git/config. It uses the same setting from `.gitmodules` as
            a template. If the URL is relative, it will be resolved using
            the default remote. If there is no default remote, the current
            repository will be assumed to be upstream.
    +
    Optional <path> arguments limit which submodules will be initialized.
    If no path is specified and submodule.active has been configured, submodules
    configured to be active will be initialized, otherwise all submodules are
    initialized.
    +
    When present, it will also copy the value of `submodule.$name.update`.

and this description is very flawed. It says "X is copied", but not "X is copied from A to B". It also does not say that even if you have a custom command there, it does not get copied.

It copies submodule.<name>.update from the ".gitmodules" to the configuraiton. And that is the reason why, the configuration may have "submodule.<name>.update" in it after running "git init" to initialize a submodule. And that is why the choices of update methods listed in the part your patch touched talked about things that can both appear in .gitmodules and the configuration. But the linkage is quite indirect.

So, here is another round, this time the primary change is to stop talking about `.gitmodules` in the "update" section, but explain how `.gitmodules` file is used in the "init" section.

 Documentation/git-submodule.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Thanks. I'll digest this and reply tomorrow. Until then, I remembered something:

On 7/13/23 23:37, Junio C Hamano wrote:

Originally I was trying to write a disclaimer about `none` because the current documentation we're removing seems to be trying to say something like this:

"The following `update` procedures are not supported via the command line: none, custom command"

It's referencing this bit at the top:

 >   The 'update' procedures supported both from the command line as well as
 >   through the `submodule.<name>.update` configuration are:

Petar Vutov <pvutov@imap.cc> writes:

Thanks.

Ah, that is a good thing to notice. Yes, the command line override is also something we need to explain.

On 7/13/23 23:37, Junio C Hamano wrote:

Looks good to me.

I applied your patch and then made some additions, which I'll append at the end of this message. Summary:

* Added your rewrite from yesterday. I like that it's more precise than 
the current docs, but I struggle with the complexity of the first 
sentence. I wanted to make it easier to follow by splitting it in two 
somehow. Instead I ended up with an introductory sentence summarizing 
the functionality with small words.

* In gitmodules.txt, moved the security disclaimer so people are more 
likely to see it before they follow the link to git-submodule.txt.

* Explicitly called out `none` and `custom command` as being unusable on 
the command line (and in .gitmodules). I expect that you won't want that 
.gitmodules mention anymore, but I left it in for now, up to you.

By the way, I ran into an SO question where some people were discussing the same issue: https://stackoverflow.com/q/65744067/876832

The diff below is based on top of your patch from yesterday, since I assume that you don't want to look at a mishmash of both patches.

---
  Documentation/git-submodule.txt | 19 ++++++++++++-------
  Documentation/gitmodules.txt    |  6 +++---
  2 files changed, 15 insertions(+), 10 deletions(-)

Petar Vutov <pvutov@imap.cc> writes:

Sorry for a very slow response; I seem to have missed this one.

The end result looks good. Care to wrap it up into a single patch (iow, make a "mishmash of both patches") with a good log message so we can move the topic forward?

Thanks.

From: Petar Vutov <pvutov@imap.cc>

Bugfix for fc01a5d2 (submodule update documentation: don't repeat ourselves, 2016-12-27).

The `custom command` and `none` options are described as sharing the same limitations, but one is allowed in .gitmodules and the other is not.

Rewrite the description for custom commands to be more precise, and make it easier for readers to notice that custom commands cannot be used in the .gitmodules file.

Signed-off-by: Petar Vutov <pvutov@imap.cc>
---
Combines the custom command description rewrite by Junio C Hamano and
my proposed changes into a single patch.

Apologies for the "From:" line at the top - I don't know if it is safe to remove it, and I can't figure out how set my name in git-send-email so that the line is not automatically added.

 Documentation/git-submodule.txt | 31 +++++++++++++++++++------------
 Documentation/gitmodules.txt    |  6 +++---
 2 files changed, 22 insertions(+), 15 deletions(-)

pvutov@imap.cc writes:

Thanks.  Willl queue.  Let's move it forward.

From: Petar Vutov <pvutov@imap.cc>

Bugfix for fc01a5d2 (submodule update documentation: don't repeat ourselves, 2016-12-27).

The `custom command` and `none` entries are described as sharing the same limitations, but one is allowed in .gitmodules and the other is not. Instead, highlight the limitation of `custom command` only.

Signed-off-by: Petar Vutov <pvutov@imap.cc>
---

Changes from v1: Don't delete the documentation for `!command`. Instead, highlight the differences in the limitations of `none` and `!command`.

Changes from v2: Improve phrasing. Add the bugfix blurb in the commit message.

Changes from v3: Keep the description of `none` unchanged and just move it up.

 Documentation/git-submodule.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)