Weak option parsing in `git submodule`

Weak option parsing in `git submodule`

[Kaartic Sivaraam]

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.

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.

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

    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.

    Thoughts??

    -- 
    Sivaraam

    QUOTE:

    “The most valuable person on any team is the person who makes everyone
    else on the team more valuable, not the person who knows the most.”

          - Joel Spolsky


    Sivaraam?

    You possibly might have noticed that my signature recently changed from
    'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
    new signature to be better for several reasons one of which is that the
    former signature has a lot of ambiguities as it is a common name (NOTE:
    it's not a common spelling, just a common name). So, I switched
    signatures before it's too late.

    That said, I won't mind you calling me 'Kaartic' if you like it [of
    course ;-)]. You can always call me using either of the names.


    KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

    As I'm not a native English speaker myself, there might be mistaeks in
    my usage of English. I apologise for any mistakes that I make.

    It would be "helpful" if you take the time to point out the mistakes.

    It would be "super helpful" if you could provide suggestions about how
    to correct those mistakes.

    Thanks in advance!

Re: Weak option parsing in `git submodule`

[Stefan Beller]

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

Re: Weak option parsing in `git submodule`

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 3996 bytes --]

On Tuesday 08 May 2018 12:35 AM, Stefan Beller wrote:
>>     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?
> 

Incidentally, I was writing a patch to check for the return value of
`git add` to fix the particular issue I noted in my initial message.
Then I was in a dilemma as to whether this was the right way to do it.
So, I thought it would be better to ask before continuing with the patch
and hence started this thread. I wasn't counting the arguments to `git
submodule add` at that time.

Now that I'm ensured that my initial approach is not the worst way to do
things and as I'm about to write a patch for this, I'll sum up what I'm
about to achieve in the short-term fix patch, for the sake of clarity.

	1. Check the return value of `git add ...` and throw an error
	   appropriately.

	2. Check the no. of arguments to `submodule add` and throw an
	   error if there are more arguments than there should be.

	   I require a little clarification with this. How should this
	   be done. Does checking whether the number of arguments after
	   <repository> is not more than one do the job? Or am I missing
	   something?


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

Hope the day it is brought into C comes soon.


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

You're welcome :-)


-- 
Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]