Re: Weak option parsing in `git submodule`

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

[-- 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 --]