{"thread":{"id":"48424","subject":"Weak option parsing in `git submodule`","startedAt":"2018-05-06T18:10:55Z","lastAt":"2018-05-08T07:56:35Z","messageCount":3,"participants":["Kaartic Sivaraam","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"346780","messageId":"1525630243.15782.4.camel@gmail.com","threadId":"48424","inReplyTo":null,"subject":"Weak option parsing in `git submodule`","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-05-06T18:10:43Z","receivedAt":"2018-05-06T18:10:55Z","isPatch":false,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"I recently faced the consequence of the weak option parsing done by in\n`git submodule`. Initially tried to add a new submodule using the `git\nsubmodule add` sub-command in the following way,\n\n $ git submodule add ./foo/bar\n\nThis cloned the submodule into a new directory named 'bar' in the\npresent directory. This was initially confusing as I expected `git\nsubmodule` to use the actual directory itself as it resides inside a\nsub-directory the main project and thus avoiding the creation of a new\nclone. Then I came to know that `git submodule` wasn't smart enough yet\nto identify this, by reading the documentation. Then, after realizing\nthat I would have to specify the path in the command to avoid the\nredundant clone, I corrected the command without consulting the\ndocumentation (thoroughly) to,\n\n $ git submodule add ./foo/bar --path ./foo/bar\n\nexpecting that the path needs to be specified after an argument. This\nis what triggered the weird consequence of weak option parsing. The\noutput I got for the above command was:\n\n The following path is ignored by one of your .gitignore files:\n --path\n Use -f if you really want to add it.\n\nThat really confused me pretty much (being a person who doesn't know\nmuch about how `git submodule` works). Instead of complaining about an\ninexistent option '--path', it considers '--path' to be the \nargument which seems to be bad. It doesn't even complain about the\nextra argument. I also dug to find the reason behind this totally weird\nmessage. It seems to be a consequence of the following lousy check\n($sm_path is the normalized argument):\n\n if test -z \"$force\" &&\n ! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n then\n eval_gettextln \"The following path is ignored by one of your .gitignore files:\n \\$sm_path\n Use -f if you really want to add it.\" >&2\n exit 1\n fi\n\n The lack of checking for the reason behind why `git add` fails seems to\n be the reason behind that weird message.\n\n Ways to fix this:\n\n 1. Fix this particular issue by adding a '--' after the '--no-warn-\n embedded-repo' option in the above check. But that would also\n require that we allow other parts of the script to accept weird\n paths such as '--path'. Not so useful/helpful.\n\n 2. Check for the actual return value of `git add` in the check and\n act accordingly. Also, check if there are unnecessary arguments for\n `submodule add`.\n\n 3. Tighten option parsing in the `git-submodule` script to ensure\n this doesn't happen in the long term and helps users to get more\n relevant error messages.\n\n I find 3 to be a long term solution but not sure if it's worth trying\n as there are efforts towards porting `git submodule` to C. So, I guess\n we should at least do 2 as a short-term solution.\n\n Thoughts??\n\n -- \n Sivaraam\n\n QUOTE:\n\n “The most valuable person on any team is the person who makes everyone\n else on the team more valuable, not the person who knows the most.”\n\n       - Joel Spolsky\n\n\n Sivaraam?\n\n You possibly might have noticed that my signature recently changed from\n 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the\n new signature to be better for several reasons one of which is that the\n former signature has a lot of ambiguities as it is a common name (NOTE:\n it's not a common spelling, just a common name). So, I switched\n signatures before it's too late.\n\n That said, I won't mind you calling me 'Kaartic' if you like it [of\n course ;-)]. You can always call me using either of the names.\n\n\n KIND NOTE TO THE NATIVE ENGLISH SPEAKER:\n\n As I'm not a native English speaker myself, there might be mistaeks in\n my usage of English. I apologise for any mistakes that I make.\n\n It would be \"helpful\" if you take the time to point out the mistakes.\n\n It would be \"super helpful\" if you could provide suggestions about how\n to correct those mistakes.\n\n Thanks in advance!\n"},{"id":"346905","messageId":"CAGZ79kYSanRAchMe+7uJ4spy+GaS7PyU7epPeOSCs_58RsAR8A@mail.gmail.com","threadId":"48424","inReplyTo":"1525630243.15782.4.camel@gmail.com","subject":"Re: Weak option parsing in `git submodule`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-07T19:05:41Z","receivedAt":"2018-05-07T19:05:46Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 6, 2018 at 11:10 AM, Kaartic Sivaraam\n wrote:\n> I recently faced the consequence of the weak option parsing done by in\n> `git submodule`. Initially tried to add a new submodule using the `git\n> submodule add` sub-command in the following way,\n>\n> $ git submodule add ./foo/bar\n>\n> This cloned the submodule into a new directory named 'bar' in the\n> present directory. This was initially confusing as I expected `git\n> submodule` to use the actual directory itself as it resides inside a\n> sub-directory the main project and thus avoiding the creation of a new\n> clone. Then I came to know that `git submodule` wasn't smart enough yet\n> to identify this, by reading the documentation. Then, after realizing\n> that I would have to specify the path in the command to avoid the\n> redundant clone, I corrected the command without consulting the\n> documentation (thoroughly) to,\n>\n> $ git submodule add ./foo/bar --path ./foo/bar\n>\n> expecting that the path needs to be specified after an argument. This\n> is what triggered the weird consequence of weak option parsing. The\n> output I got for the above command was:\n>\n> The following path is ignored by one of your .gitignore files:\n> --path\n> Use -f if you really want to add it.\n\nYuck, that is bad UX.\n\n> That really confused me pretty much (being a person who doesn't know\n> much about how `git submodule` works). Instead of complaining about an\n> inexistent option '--path', it considers '--path' to be the \n> argument which seems to be bad. It doesn't even complain about the\n> extra argument. I also dug to find the reason behind this totally weird\n> message. It seems to be a consequence of the following lousy check\n> ($sm_path is the normalized argument):\n>\n> if test -z \"$force\" &&\n> ! git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" > /dev/null 2>&1\n> then\n> eval_gettextln \"The following path is ignored by one of your .gitignore files:\n> \\$sm_path\n> Use -f if you really want to add it.\" >&2\n> exit 1\n> fi\n>\n> The lack of checking for the reason behind why `git add` fails seems to\n> be the reason behind that weird message.\n\n(from the man page)\ngit submodule [--quiet] add [] [--] []\n\nWhen options are given after or we can count\nthe arguments and know something is up. (The number of arguments\nmust be 1 or 2. If it is 3 or above, something fishy is going on), which\nI would suggest as a first step.\n\n> Ways to fix this:\n>\n> 1. Fix this particular issue by adding a '--' after the '--no-warn-\n> embedded-repo' option in the above check. But that would also\n> require that we allow other parts of the script to accept weird\n> paths such as '--path'. Not so useful/helpful.\n>\n> 2. Check for the actual return value of `git add` in the check and\n> act accordingly. Also, check if there are unnecessary arguments for\n> `submodule add`.\n\nThe second part of this suggestion seems to me as the way to go.\nDo you want to write a patch?\n\n> 3. Tighten option parsing in the `git-submodule` script to ensure\n> this doesn't happen in the long term and helps users to get more\n> relevant error messages.\n>\n> I find 3 to be a long term solution but not sure if it's worth trying\n> as there are efforts towards porting `git submodule` to C. So, I guess\n> we should at least do 2 as a short-term solution.\n\nYeah, bringing it to C, would be nice as a long term solution instead\nof piling up more and more shell features. :)\n\nThanks for such a well written bug report with suggested bug fixes. :)\nStefan\n"},{"id":"346971","messageId":"7b573a6b-0486-f9bd-4499-deb9b1394f78@gmail.com","threadId":"48424","inReplyTo":"CAGZ79kYSanRAchMe+7uJ4spy+GaS7PyU7epPeOSCs_58RsAR8A@mail.gmail.com","subject":"Re: Weak option parsing in `git submodule`","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-05-08T07:56:15Z","receivedAt":"2018-05-08T07:56:35Z","isPatch":false,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 08 May 2018 12:35 AM, Stefan Beller wrote:\n>> The lack of checking for the reason behind why `git add` fails seems to\n>> be the reason behind that weird message.\n> \n> (from the man page)\n> git submodule [--quiet] add [] [--] []\n> \n> When options are given after or we can count\n> the arguments and know something is up. (The number of arguments\n> must be 1 or 2. If it is 3 or above, something fishy is going on), which\n> I would suggest as a first step.\n> \n>> Ways to fix this:\n>>\n>> 1. Fix this particular issue by adding a '--' after the '--no-warn-\n>> embedded-repo' option in the above check. But that would also\n>> require that we allow other parts of the script to accept weird\n>> paths such as '--path'. Not so useful/helpful.\n>>\n>> 2. Check for the actual return value of `git add` in the check and\n>> act accordingly. Also, check if there are unnecessary arguments for\n>> `submodule add`.\n> \n> The second part of this suggestion seems to me as the way to go.\n> Do you want to write a patch?\n> \n\nIncidentally, I was writing a patch to check for the return value of\n`git add` to fix the particular issue I noted in my initial message.\nThen I was in a dilemma as to whether this was the right way to do it.\nSo, I thought it would be better to ask before continuing with the patch\nand hence started this thread. I wasn't counting the arguments to `git\nsubmodule add` at that time.\n\nNow that I'm ensured that my initial approach is not the worst way to do\nthings and as I'm about to write a patch for this, I'll sum up what I'm\nabout to achieve in the short-term fix patch, for the sake of clarity.\n\n\t1. Check the return value of `git add ...` and throw an error\n\t appropriately.\n\n\t2. Check the no. of arguments to `submodule add` and throw an\n\t error if there are more arguments than there should be.\n\n\t I require a little clarification with this. How should this\n\t be done. Does checking whether the number of arguments after\n\t is not more than one do the job? Or am I missing\n\t something?\n\n\n>> 3. Tighten option parsing in the `git-submodule` script to ensure\n>> this doesn't happen in the long term and helps users to get more\n>> relevant error messages.\n>>\n>> I find 3 to be a long term solution but not sure if it's worth trying\n>> as there are efforts towards porting `git submodule` to C. So, I guess\n>> we should at least do 2 as a short-term solution.\n> \n> Yeah, bringing it to C, would be nice as a long term solution instead\n> of piling up more and more shell features. :)\n> \n\nHope the day it is brought into C comes soon.\n\n\n> Thanks for such a well written bug report with suggested bug fixes. :)\n\nYou're welcome :-)\n\n\n-- \nSivaraam\n\nQUOTE:\n\n“The most valuable person on any team is the person who makes everyone\nelse on the team more valuable, not the person who knows the most.”\n\n - Joel Spolsky\n\n\nSivaraam?\n\nYou possibly might have noticed that my signature recently changed from\n'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the\nnew signature to be better for several reasons one of which is that the\nformer signature has a lot of ambiguities in the place I live as it is a\ncommon name (NOTE: it's not a common spelling, just a common name). So,\nI switched signatures before it's too late.\n\nThat said, I won't mind you calling me 'Kaartic' if you like it [of\ncourse ;-)]. You can always call me using either of the names.\n\n\nKIND NOTE TO THE NATIVE ENGLISH SPEAKER:\n\nAs I'm not a native English speaker myself, there might be mistaeks in\nmy usage of English. I apologise for any mistakes that I make.\n\nIt would be \"helpful\" if you take the time to point out the mistakes.\n\nIt would be \"super helpful\" if you could provide suggestions about how\nto correct those mistakes.\n\nThanks in advance!\n\n"}]}