git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: Git Mailing list <git@vger.kernel.org>,
	Jens Lehmann <Jens.Lehmann@web.de>
Subject: Re: Weak option parsing in `git submodule`
Date: Tue, 8 May 2018 13:26:15 +0530
Message-ID: <7b573a6b-0486-f9bd-4499-deb9b1394f78@gmail.com> (raw)
In-Reply-To: <CAGZ79kYSanRAchMe+7uJ4spy+GaS7PyU7epPeOSCs_58RsAR8A@mail.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 --]

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-06 18:10 Kaartic Sivaraam
2018-05-07 19:05 ` Stefan Beller
2018-05-08  7:56   ` Kaartic Sivaraam [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7b573a6b-0486-f9bd-4499-deb9b1394f78@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox