Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

From: Stefan Beller <sbeller@google.com>
Date: Mar 25, 2016, 16:49 UTC
Message-ID: <CAGZ79kZ684W46df9zPQATr3zWKt+e1BhGY6DZ84psfXWH4tGNw@mail.gmail.com>
In-Reply-To: <xmqqmvpn5awo.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 24, 2016 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>
> So this change may not be wrong per-se, but if the lossage of prefix
> is the final goal (as opposed to an approach to gain other benefits,
> e.g. "now we do not have to use prefix, we can simplify these other
> things"), I do not know if it is worth it.
>

It is the final goal of this series. As motivation, see Jacobs comment above:

    I had wondered why we used --prefix before.

Also when the submodule helper got in, reviewers (Jonathan) were confused and asked for clarification of the prefix term. So either document the prefix term (and come up with a reason why we don't use the standard mechanism, which as you outlined in the other mail, may be performance as we skip one chdir, but that sounds like weak argument to me) or remove the confusing part of having a prefix, which is not the standard prefix inside C.

The other reason you gave below is also convincing: By having it in the prefix, the C code is more likely correct and future proof.

On rewriting the whole submodule command in C (probably reiterating): It is not my endgoal to rewrite every submodule related part in C. It would be nice if it would happen eventually, but for now I only rewrite parts that I need in C. (i.e. paralllelisation is hard in shell, if not impossible using posix shell with no additional dependencies [xargs, gnu parallel], so I moved that part to C. Certain parts need a performance boost? Ok I'll do it in C.

That said, we may have the shell/C architecture for a longer time than planned, which makes it important to comment/document the confusing parts. Instead I'd rather not have confusing parts, so I see benefit in having the goal of this series to remove the --prefix.