On Sun, May 6, 2018 at 11:10 AM, Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> 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
