Re: [PATCH 0/3] recursive support for ls-files

> Brandon Williams <bmwill@google.com> 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.

> 
> >> > 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.