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

Re: Weak option parsing in `git submodule`

From
Stefan Beller <sbeller@google.com>
Date
May 7, 2018, 19:05 UTC
Message-ID
<CAGZ79kYSanRAchMe+7uJ4spy+GaS7PyU7epPeOSCs_58RsAR8A@mail.gmail.com>
In-Reply-To
<1525630243.15782.4.camel@gmail.com>

On Sun, May 6, 2018 at 11:10 AM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 25 quoted lines
> I recently faced the consequence of the weak option parsing done by in
> `git submodule`. Initially tried to add a new submodule using the `git
> submodule add` sub-command in the following way,
>
>     $ git submodule add ./foo/bar
>
> This cloned the submodule into a new directory named 'bar' in the
> present directory. This was initially confusing as I expected `git
> submodule` to use the actual directory itself as it resides inside a
> sub-directory the main project and thus avoiding the creation of a new
> clone. Then I came to know that `git submodule` wasn't smart enough yet
> to identify this, by reading the documentation. Then, after realizing
> that I would have to specify the path in the command to avoid the
> redundant clone, I corrected the command without consulting the
> documentation (thoroughly) to,
>
>     $ git submodule add ./foo/bar --path ./foo/bar
>
> expecting that the path needs to be specified after an argument. This
> is what triggered the weird consequence of weak option parsing. The
> output I got for the above command was:
>
>     The following path is ignored by one of your .gitignore files:
>     --path
>     Use -f if you really want to add it.
Yuck, that is bad UX.
Show 19 quoted lines
> That really confused me pretty much (being a person who doesn't know
> much about how `git submodule` works). Instead of complaining about an
> inexistent option '--path', it considers '--path' to be the <path>
> argument which seems to be bad. It doesn't even complain about the
> extra argument. I also dug to find the reason behind this totally weird
> message. It seems to be a consequence of the following lousy check
> ($sm_path is the normalized <path> argument):
>
>     if test -z "$force" &&
>             ! git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" > /dev/null 2>&1
>     then
>             eval_gettextln "The following path is ignored by one of your .gitignore files:
>     \$sm_path
>     Use -f if you really want to add it." >&2
>             exit 1
>     fi
>
>     The lack of checking for the reason behind why `git add` fails seems to
>     be the reason behind that weird message.

(from the man page) git submodule [--quiet] add [<options>] [--] <repository> [<path>]

When options are given after <repository> or <path> we can count the arguments and know something is up. (The number of arguments must be 1 or 2. If it is 3 or above, something fishy is going on), which I would suggest as a first step.

Show 10 quoted lines
>     Ways to fix this:
>
>     1. Fix this particular issue by adding a '--' after the '--no-warn-
>     embedded-repo' option in the above check. But that would also
>     require that we allow other parts of the script to accept weird
>     paths such as '--path'. Not so useful/helpful.
>
>     2. Check for the actual return value of `git add` in the check and
>     act accordingly. Also, check if there are unnecessary arguments for
>     `submodule add`.

The second part of this suggestion seems to me as the way to go. Do you want to write a patch?

Show 7 quoted lines
>     3. Tighten option parsing in the `git-submodule` script to ensure
>     this doesn't happen in the long term and helps users to get more
>     relevant error messages.
>
>     I find 3 to be a long term solution but not sure if it's worth trying
>     as there are efforts towards porting `git submodule` to C. So, I guess
>     we should at least do 2 as a short-term solution.

Yeah, bringing it to C, would be nice as a long term solution instead of piling up more and more shell features. :)

Thanks for such a well written bug report with suggested bug fixes. :) Stefan

Previous: Kaartic SivaraamNext: Kaartic Sivaraam
Message 2 of 3 in “Weak option parsing in `git submodule`”
  1. Kaartic SivaraamMay 6, 2018
  2. Stefan BellerMay 7, 2018
  3. Kaartic SivaraamMay 8, 2018

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.