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

[Junio C Hamano]

Stefan Beller  writes:

> Exactly, sorry for not writing my chain of thoughts down completely.
>
> To make them reusable, I'd assume we want them to be easy to
> understand, and by using a well known way in git it is easier to
> understand.

I already said I do not care too deeply either way, but I have to
point out that "git -C $prefix" would put extra cognitive burden on
those who will be picking it up where you left off.  When they start
by first literally translating shell to C, "git -C" would mislead
them to think if they have to chdir(2) to the directory first, which
is not the case at all.  "git -C $prefix submodule--helper" in the
code after [3/4] is there only because submodule--helper code no
longer is told what the caller already knows (i.e. what is the
prefix) and is forced to find it out by itself.  Contrasting to
that, with an explicit --prefix option, the reader would know there
is no need to chdir(2) anywhere at that point, but the paths that
are held in variables it uses from the surrounding code are not
relative to the current working directory when the code is called
and $prefix is there to help adjusting it.

The reason I do not care too deeply was that I thought people who
will be taking over after you are done (well, after all that might
include you; plans would change) are clueful, but there is no need
for us to make their life more difficult than necessary.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Fri, Mar 25, 2016 at 10:25 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> The way all the subcommand written in C works is
>>
>>  - The start-up sequence does the repository discovery, which
>>involves crawling up to the top-level of the working tree, and
>>compute "prefix", where the end-user was when the command was
>>invoked;
>>
>>  - The subcommand implementation is called with this "prefix";
>>
>>  - When the subcommand implementation interprets the command line
>>arguments and option arguments, it prefixes the "prefix" as
>>needed.  If, for example, "git grep -f patterns" is invoked
>>inside "sub/" subdirectory, when the command line and option
>>arguments are processed, the process is already at the top level
>>of the working tree, so it needs to read the patterns from
>>"sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
>>"sub/" subdirectory needs to limit the output to those that match
>>not "Makefile", but "sub/Makefil*".
>>
>> The hope of doing an incremental rewrite of the whole thing by
>> enriching submodule--helper is that the bulk of the code there will
>> be reusable when the entirety of "git submodule" is rewritten in C,
>> so they need to take the "prefix" the same way, whether the caller
>> is calling from "git-submodule.sh" script via submodule--helper, or
>> the eventual C implementation of "git submodule" is making direct
>> calls to them.  As long as the correct "prefix" is passed to the
>> routines that are driven via submodule--helper, it does not matter
>> and I do not care how it is done.
>>
>> The current code of "git submodule" whose higher parts are still in
>> shell would would:
>>
>>  - The start-up sequence in shell does the cd_to_toplevel and finds
>>the prefix;
>>
>>  - "git submodule--helper list --prefix=$prefix $other_args" is
>>called; as this is called from the top-level of the working tree,
>>internally its "prefix" is empty, but $other_args must be
>>interpreted relative to the $prefix passed with --prefix option.
>>
>> If we instead call "git -C "$prefix" submodule--helper list $other_args",
>> then
>>
>>  - This command first does another chdir(2) back to $prefix;
>>
>>  - The start-up sequence of "submodule--helper" does the usual
>>repository discovery again, which involves crawling up to the
>>top-level of the working tree, and compute "prefix".  This
>>happens to match what -C "$prefix" gave the command.
>>
>> Making calls to submodule--helper via "git -C" feels a little bit
>> roundabout because of this "caller tells to chdir, and then it has
>> to again chdir back up" only to rediscover the same information.
>
> Just to make sure that the discussion is complete.
>
> Another way a script can use the "prefix" information is to use the
> "prefix" as such.  Knowing that the cd_to_toplevel() took you to the
> root level of the working tree, instead of "git -C $prefix" or
> "--prefix $prefix", it could do "git $cmd $prefix$filename".
>
> One consideration when choosing among the approaches is how the
> $filename is reported back to the user (e.g. as part of an error
> message).  "git $cmd $prefix$filename" invocation would complain
> about the full "$prefix$filename" path, but what the user gave it
> may only be $filename part.

Right. Using either "git -C $prefix" or "git --prefix $prefix" would report
the $filename only, because both cases assume $prefix was cut
using cd_to_toplevel and the user expects $filename only.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano  writes:

> The way all the subcommand written in C works is
>
>  - The start-up sequence does the repository discovery, which
>involves crawling up to the top-level of the working tree, and
>compute "prefix", where the end-user was when the command was
>invoked;
>
>  - The subcommand implementation is called with this "prefix";
>
>  - When the subcommand implementation interprets the command line
>arguments and option arguments, it prefixes the "prefix" as
>needed.  If, for example, "git grep -f patterns" is invoked
>inside "sub/" subdirectory, when the command line and option
>arguments are processed, the process is already at the top level
>of the working tree, so it needs to read the patterns from
>"sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
>"sub/" subdirectory needs to limit the output to those that match
>not "Makefile", but "sub/Makefil*".
>
> The hope of doing an incremental rewrite of the whole thing by
> enriching submodule--helper is that the bulk of the code there will
> be reusable when the entirety of "git submodule" is rewritten in C,
> so they need to take the "prefix" the same way, whether the caller
> is calling from "git-submodule.sh" script via submodule--helper, or
> the eventual C implementation of "git submodule" is making direct
> calls to them.  As long as the correct "prefix" is passed to the
> routines that are driven via submodule--helper, it does not matter
> and I do not care how it is done.
>
> The current code of "git submodule" whose higher parts are still in
> shell would would:
>
>  - The start-up sequence in shell does the cd_to_toplevel and finds
>the prefix;
>
>  - "git submodule--helper list --prefix=$prefix $other_args" is
>called; as this is called from the top-level of the working tree,
>internally its "prefix" is empty, but $other_args must be
>interpreted relative to the $prefix passed with --prefix option.
>
> If we instead call "git -C "$prefix" submodule--helper list $other_args",
> then
>
>  - This command first does another chdir(2) back to $prefix;
>
>  - The start-up sequence of "submodule--helper" does the usual
>repository discovery again, which involves crawling up to the
>top-level of the working tree, and compute "prefix".  This
>happens to match what -C "$prefix" gave the command.
>
> Making calls to submodule--helper via "git -C" feels a little bit
> roundabout because of this "caller tells to chdir, and then it has
> to again chdir back up" only to rediscover the same information.

Just to make sure that the discussion is complete.

Another way a script can use the "prefix" information is to use the
"prefix" as such.  Knowing that the cd_to_toplevel() took you to the
root level of the working tree, instead of "git -C $prefix" or
"--prefix $prefix", it could do "git $cmd $prefix$filename".

One consideration when choosing among the approaches is how the
$filename is reported back to the user (e.g. as part of an error
message).  "git $cmd $prefix$filename" invocation would complain
about the full "$prefix$filename" path, but what the user gave it
may only be $filename part.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Fri, Mar 25, 2016 at 10:01 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> 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.
>
> Well, what you personally would want to do yourself is irrelevant.
> Doing submodule--helper in such a way that it is sufficient in a
> half-way conversion but cannot be reused in future full rewrite is
> something we would want to avoid, whether you would be doing the
> full rewrite in the future.  In fact, if you are not inclined to do
> so yourself, that is one more reason to make sure that the C pieces
> in submodule--helper are reusable (i.e. your "correct and future
> proof" above); otherwise you'd be making it _more_ difficult for
> other people who would want to pick it up where you left.

Exactly, sorry for not writing my chain of thoughts down completely.

To make them reusable, I'd assume we want them to be easy to
understand, and by using a well known way in git it is easier to
understand.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

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

Well, what you personally would want to do yourself is irrelevant.
Doing submodule--helper in such a way that it is sufficient in a
half-way conversion but cannot be reused in future full rewrite is
something we would want to avoid, whether you would be doing the
full rewrite in the future.  In fact, if you are not inclined to do
so yourself, that is one more reason to make sure that the C pieces
in submodule--helper are reusable (i.e. your "correct and future
proof" above); otherwise you'd be making it _more_ difficult for
other people who would want to pick it up where you left.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Thu, Mar 24, 2016 at 11:28 PM, Junio C Hamano  wrote:
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> The usual early machinery of Git is to change the directory to
>> the top level of the working tree and pass the actual path inside
>> the working tree as `prefix` to the command being run.
>>
>> This is the case both for commands written in C (where the
>> prefix is passed into the command in a function parameter) as
>> well as in git-submodule.sh where the setup code runs...
>>
>> Adhere to Gits standard of passing the relative path inside the
>> working tree by passing it via -C.
>
> While -C _also_ works, I do not think it is "standard" in any sense.
> What made you think so?  -C is a way to tell Git to chdir there
> before doing anything else, without even adjusting the command line
> arguments that might be paths, and "chdir there to go to top" may
> (or may not--I haven't thought things thru) have the same effect as
> passing the prefix into functions, that is merely true as a side
> effect, I would think.
>
> 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.

I actually do not care too deeply either way, as I understand that
the long term goal is to have "git submodule" itself rewritten in C,
so that places that currently call submodule--helper would become an
internal call.

The way all the subcommand written in C works is

 - The start-up sequence does the repository discovery, which
   involves crawling up to the top-level of the working tree, and
   compute "prefix", where the end-user was when the command was
   invoked;

 - The subcommand implementation is called with this "prefix";

 - When the subcommand implementation interprets the command line
   arguments and option arguments, it prefixes the "prefix" as
   needed.  If, for example, "git grep -f patterns" is invoked
   inside "sub/" subdirectory, when the command line and option
   arguments are processed, the process is already at the top level
   of the working tree, so it needs to read the patterns from
   "sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
   "sub/" subdirectory needs to limit the output to those that match
   not "Makefile", but "sub/Makefil*".

The hope of doing an incremental rewrite of the whole thing by
enriching submodule--helper is that the bulk of the code there will
be reusable when the entirety of "git submodule" is rewritten in C,
so they need to take the "prefix" the same way, whether the caller
is calling from "git-submodule.sh" script via submodule--helper, or
the eventual C implementation of "git submodule" is making direct
calls to them.  As long as the correct "prefix" is passed to the
routines that are driven via submodule--helper, it does not matter
and I do not care how it is done.

The current code of "git submodule" whose higher parts are still in
shell would would:

 - The start-up sequence in shell does the cd_to_toplevel and finds
   the prefix;

 - "git submodule--helper list --prefix=$prefix $other_args" is
   called; as this is called from the top-level of the working tree,
   internally its "prefix" is empty, but $other_args must be
   interpreted relative to the $prefix passed with --prefix option.

If we instead call "git -C "$prefix" submodule--helper list $other_args",
then

 - This command first does another chdir(2) back to $prefix;

 - The start-up sequence of "submodule--helper" does the usual
   repository discovery again, which involves crawling up to the
   top-level of the working tree, and compute "prefix".  This
   happens to match what -C "$prefix" gave the command.

Making calls to submodule--helper via "git -C" feels a little bit
roundabout because of this "caller tells to chdir, and then it has
to again chdir back up" only to rediscover the same information.

Again, I actually do not care too deeply either way, though, as long
as the correct "prefix" is passed to the routines that are driven
via submodule--helper, which would assure that the C code you write
today will be reusable when "git submodule" as a whole is redone in
C.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

> The usual early machinery of Git is to change the directory to
> the top level of the working tree and pass the actual path inside
> the working tree as `prefix` to the command being run.
>
> This is the case both for commands written in C (where the
> prefix is passed into the command in a function parameter) as
> well as in git-submodule.sh where the setup code runs...
>
> Adhere to Gits standard of passing the relative path inside the
> working tree by passing it via -C.

While -C _also_ works, I do not think it is "standard" in any sense.
What made you think so?  -C is a way to tell Git to chdir there
before doing anything else, without even adjusting the command line
arguments that might be paths, and "chdir there to go to top" may
(or may not--I haven't thought things thru) have the same effect as
passing the prefix into functions, that is merely true as a side
effect, I would think.

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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jacob Keller]

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller  wrote:
> The usual early machinery of Git is to change the directory to
> the top level of the working tree and pass the actual path inside
> the working tree as `prefix` to the command being run.
> This is the case both for commands written in C (where the
> prefix is passed into the command in a function parameter) as
> well as in git-submodule.sh where the setup code runs
>
>   wt_prefix=$(git rev-parse show-prefix)
>   cd_to_top_level
>
> So the prefix passed into the `submodule--helper list` is actually
> the relative path inside the working tree, but we were not using
> the standard way of passing it through.
>
> Adhere to Gits standard of passing the relative path inside the
> working tree by passing it via -C.
>
> We do not need to pass it for `submodule foreach` as that command
> doesn't take further arguments ('$@') to operate on a subset of
> submodules, such that it is irrelevant for listing the submodules.
> The computation of the displaypath ('Entering ') is done
> separately there.
>
> Signed-off-by: Stefan Beller 
> ---

It is nice to see the format for doing this standardized, and reduce
extra code in the submodule--helper. I had wondered why we used
--prefix before.

Reviewed-by: Jacob Keller 

>  builtin/submodule--helper.c |  5 +
>  git-submodule.sh| 12 ++--
>  2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ed764c9..2983783 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -68,14 +68,11 @@ static int module_list(int argc, const char **argv, const 
> char *prefix)
> struct module_list list = MODULE_LIST_INIT;
>
> struct option module_list_options[] = {
> -   OPT_STRING(0, "prefix", ,
> -  N_("path"),
> -  N_("alternative anchor for relative paths")),
> OPT_END()
> };
>
> const char *const git_submodule_helper_usage[] = {
> -   N_("git submodule--helper list [--prefix=] 
> [...]"),
> +   N_("git submodule--helper list [...]"),
> NULL
> };
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 6b18a03..1f7ad6e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -407,7 +407,7 @@ cmd_foreach()
> # command in the subshell (and a recursive call to this function)
> exec 3<&0
>
> -   git submodule--helper list --prefix "$wt_prefix"|
> +   git submodule--helper list |
> while read mode sha1 stage sm_path
> do
> die_if_unmatched "$mode"
> @@ -467,7 +467,7 @@ cmd_init()
> shift
> done
>
> -   git submodule--helper list --prefix "$wt_prefix" "$@" |
> +   git -C "$wt_prefix" submodule--helper list "$@" |
> while read mode sha1 stage sm_path
> do
> die_if_unmatched "$mode"
> @@ -549,7 +549,7 @@ cmd_deinit()
> die "$(eval_gettext "Use '.' if you really want to 
> deinitialize all submodules")"
> fi
>
> -   git submodule--helper list --prefix "$wt_prefix" "$@" |
> +   git -C "$wt_prefix" submodule--helper list "$@" |
> while read mode sha1 stage sm_path
> do
> die_if_unmatched "$mode"
> @@ -683,7 +683,7 @@ cmd_update()
> fi
>
> cloned_modules=
> -   git submodule--helper list --prefix "$wt_prefix" "$@" | {
> +   git -C "$wt_prefix" submodule--helper list "$@" | {
> err=
> while read mode sha1 stage sm_path
> do
> @@ -1121,7 +1121,7 @@ cmd_status()
> shift
> done
>
> -   git submodule--helper list --prefix "$wt_prefix" "$@" |
> +   git -C "$wt_prefix" submodule--helper list "$@" |
> while read mode sha1 stage sm_path
> do
> die_if_unmatched "$mode"
> @@ -1199,7 +1199,7 @@ cmd_sync()
> esac
> done
> cd_to_toplevel
> -   git submodule--helper list --prefix "$wt_prefix" "$@" |
> +   git -C "$wt_prefix" submodule--helper list "$@" |
> while read mode sha1 stage sm_path
> do
> die_if_unmatched "$mode"
> --
> 2.8.0.rc4.10.g52f3f33
>

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html