Re: Weak option parsing in `git submodule`

[Stefan Beller <sbeller@google.com>]

On Sun, May 6, 2018 at 11:10 AM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> 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.

> 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.

>     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?

>     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