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). > > 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. I'll keep that in mind for future patches. I figured that since it was fixed in the end that would be fine but if things shouldn't be broken at any state in the patch series I'll make sure to not do that in the future. > There are some other options that are ignored (neither disabled nor > passed along to children). Most of them are related to exclusions, which > I _think_ are safe to ignore (they do not do anything interesting unless > you specify "-o", which is explicitly disabled). I'm not sure about > --with-tree, though (or what it would even mean in the context of > recursing). These other features that are disabled now could be enabled in a future patch. You're right though I'd have to think about the --with-tree option a bit more and what it would mean with submodules. -Brandon