Re: [PATCH 1/2] ls-files: adding support for submodules

[Stefan Beller <sbeller@google.com>]

On Wed, Sep 21, 2016 at 9:18 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 21, 2016 at 04:13:22PM -0700, Junio C Hamano wrote:
>
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > yes you mentioned this and I meant to change that before sending it out.
>> > Looks like it slipped through have slipped through.
>>
>> I already fixed it up locally when I sent the reply, but thanks for
>> resending (which assures me that your local copy is up-to-date and I
>> do not have to worry about having to repeat me in the future, if
>> this ever needs further rerolling ;-).
>
> While we are on the subject, the commit message also uses some past
> tense:
>
>   Allow ls-files to recognize submodules in order to retrieve a list of
>   files from a repository's submodules.  This is done by forking off a
>   process to recursively call ls-files on all submodules. Also added a
>   submodule-prefix command in order to prepend paths to child processes.
>
> The final sentence should be "Also add...".
>
> Since this final bit of logic was sufficiently non-obvious that it only
> came about in v2, maybe it is worth describing a little more fully:
>
>   Also add a submodule-prefix option, which instructs the child
>   processes to prepend the prefix to each path they output. This makes
>   the output paths match what is on the filesystem (i.e., as if the
>   submodule boundaries were not there at all).
>
> Should this option just be "--prefix", or maybe "--output-prefix"?

I think --prefix can easily be confused with the internal prefix that we hand
down to each command. --output-prefix works for the ls-files case, but as
soon as we do more than just printing out file names, we'd use that prefix for
more than just output, e.g. in grep it becomes part of the pathspec IIUC.
I agree however that we probably don't want to keep it submodule specific.

> Submodules are the obvious use case here, but I could see somebody
> adapting this for other uses (alternatively, if we _do_ want to keep it
> just as an implementation detail for submodules, we should probably
> discourage people in the documentation from using it themselves).
>
> -Peff