On 09/25, Junio C Hamano wrote: > Brandon Williams <bmwill@xxxxxxxxxx> writes: > > > On 09/25, Jeff King wrote: > >> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote: > >> > >> > After looking at the feedback I rerolled a few things, in particular the > >> > --submodule_prefix option that existed to give a submodule context about where > >> > it had been invoked from. People didn't seem to like the idea of exposing this > >> > to the users (yet anyways) so I removed it as an option and instead have it > >> > being passed to a child process via an environment variable > >> > GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to > >> > external users at the moment. > >> > >> I think we can still have it as a command-line argument and declare it > >> internal. It's not like environment variables cannot also be set by our > >> callers. :) > >> > >> I don't mind it as an environment variable, though. In some ways it > >> makes things easier. I just think "internal versus external" and the > >> exact implementation are orthogonal. > > > > We may still want it to be an option at some point in the future. This > > way we can revisit making it an option once we know more about the other > > uses it could have (aside from just being for submodules as someone > > suggested). > > I do not think it makes too much of a difference between environment > and command line option. We need an update to the "git" potty to > say "you told me to use the submodule-prefix feature, but this > subcommand is not prepared to accept it (yet)" and cause it to error > out either way, which would mean that a series that introduces the > feature needs to touch "git.c" anyway, so I would have expected us > to add command line option first, simply because "git.c" is where it > happens, optionally with the support for the environment variable, > not the other way around. In a previous email you mentioned that this feature should be completely hidden from users, which is why I removed the command line option for this latest series. If that isn't what you intended that I can definitely add the option to git.c. And you would rather we perform the checking in git.c to see if a subcommand supports the prefix versus silently ignoring it if it hasn't? I'm assuming this checking would also be done in git.c? > > >> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff > >> > King. > >> > >> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd > >> try not to have a broken state in the history. It's less important in > >> this case, because the breakage is not a regression > >> (--recurse-submodules is a new feature, so you could consider it "not > >> working" until the 3rd patch). But I think it's still a good rule to > >> follow, because it makes the commits easier to review, look at later, > >> etc. > >> > >> For that matter, I do not understand why options like "-s" get enabled > >> in patch 3. I do not mind them starting as disabled in patch 2, but it > >> seems like "pass along some known-safe options" should be its own patch > >> somewhere between patches 2 and 3. > > Yes, exactly. > > An obvious lazy way out to avoid breakage-in-the-middle and make > incremental progress would be to squash everything into one patch, > but we should and we should be able to do better. > > I'd imagine this three-patch series would be more pleasant for > future readers if it were structured like: > > [1/3] introduces the submodule-prefix as a global feature; at the > least it needs a way to invoke (either an environment, or an > option to "git" potty, or both) and prevent mistakes by > erroring out when it is attempted to call a subcommand that > does not support the feature (yet). > > [2/3] adds the --recurse-submodule feature in a limited form to > "ls-files". I'd suggest for this step to pass through all > options and arguments that are safe and reasonably useful > to pass through without needing anything more than "ah, this > option was given, so let's stuff it to the argv-array". An > attempt to give things that are not yet passed through until > 3/3 to lead to an error that says it is not allowed (yet). > > [3-N] each of the remaining steps after 3/N adds support for one > more thing to be passed that 2/3 refrained from doing, by > doing more than just "pass it in argv-array", and then remove > the "not yet supported" error that added by 2/3 for that one > thing. The first of these "more things" would be to support > pathspecs as the receiving side would need code changes for > the matching logic. There may be more, or there may be > nothing else that requires 4/N, 5/N, etc. I can do another rework and structure the patch series more inline with what you are suggesting here. -- Brandon Williams