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

On 09/25, Junio C Hamano wrote:

Brandon Williams [off-list ref] writes:

quoted

On 09/25, Jeff King wrote:

quoted

On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:

quoted

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?

quoted

quoted

quoted

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