Re: Weak option parsing in `git submodule`
- From
Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
- Date
- May 8, 2018, 07:56 UTC
- Message-ID
- <7b573a6b-0486-f9bd-4499-deb9b1394f78@gmail.com>
- In-Reply-To
- <CAGZ79kYSanRAchMe+7uJ4spy+GaS7PyU7epPeOSCs_58RsAR8A@mail.gmail.com>
On Tuesday 08 May 2018 12:35 AM, Stefan Beller wrote:
Show 25 quoted lines
>> 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?
Show 11 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. :) >
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!