git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Johannes Sixt <j6t@kdbg.org>, Christian Couder <christian.couder@gmail.com>, Ramsay Jones <ramsay@ramsayjones.plus.com>
Subject: Re: [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C
Date: Fri, 2 Jun 2017 19:13:48 -0700
Message-ID: <CAGZ79kbDpyO5uRCPnhCWmaFKhyLEUN7mYO9PhU3xF0f=W35ofw@mail.gmail.com> (raw)
In-Reply-To: <20170602112428.11131-2-pc44800@gmail.com>

On Fri, Jun 2, 2017 at 4:24 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
>
> The first function module_foreach first parses the options present in
> argv, and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
>
> The second function for_each_submodule_list traverses through the
> list, and calls function fn (which in case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
>
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
>
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
>

Is the commit message still accurate?
You describe the changes between the versions below the --- line,
that is not recorded in the permanent commit history.

In the commit message is less important to write "what" is happening,
because that can easily be read from the patch/commit itself, but rather
"why" things happen, such as design choices, maybe:

    This aims to make git-submodule foreach a builtin. This is the very
    first step taken in this direction. Hence, 'foreach' is ported to
    submodule--helper, and submodule--helper is called from git-submodule.sh.

    We'll introduce 3 functions, one that is exposed to the command line
    and handles command line arguments, one to iterate over a set of
    submodules, and finally one to execute an arbitrary shell command
    in the submodule.

    Attention must be paid to the 'path' variable, see 64394e3ae9
    (git-submodule.sh: Don't use $path variable in eval_gettext string,
    2012-04-17) details. The path varialbe is not exposed into the environment
    of the invoked shell, but we just give "path=%s;" as the first argument.

    We do not need to condition on the number of arguments as in 1c4fb136db
    (submodule foreach: skip eval for more than one argument, 2013-09-27)
    as we will run exactly one shell in the submodules directory.

    Sign-off-...

>
> Other than that, additionally the case of no. of arugments in <command>
> being equal to 1 is also considered separetly.
> THe reason of having this change in the shell script was given in the
> commit 1c4fb136db.
> According to my understanding, eval "$1" executes $1 in same shell,
> whereas "$@" gets executed in a separate shell, which doesn't allow
> "$@" to access the env variables $name, $path, etc.
> Hence, to keep the ported function similar, this condition is also
> added.

This paragraph would be a good candidate for the commit message, too.
However as we rewrite it in C, we will spawn exactly one shell no matter
how many arguments we have (well for 0 we have no shell, but for 1 or more
arguments we'll spawn exactly one shell?)

> +       } else if (prefix) {
> +               struct strbuf sb = STRBUF_INIT;
> +               char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> +               strbuf_release(&sb);
> +               return displaypath;

Note to self (or any other that is interested in long term clean code):
    I have seen this pattern a couple of times, a strbuf just to appease
    the argument list of relative_path.
    (init_submodule, prepare_to_clone_next_submodule,
    resolve_relative_path in submodule--helper
    cmd_rev_parse in builtin/rev-parse
    connect_work_tree_and_git_dir in dir.c
    write_name_quoted_relative in quote.c
    get_superproject_working_tree in submodule.c
    cmd_main in test-path-utils;
    actually all uses of this function :( )
    We should really make a relative_path function that can work
    without the need of a strbuf, maybe just wrap the 3 lines into a new
    function, or remove the strbuf from the argument list.
    (The potential memleak is horrible to fix though. But as seen here
    we could just always return an allocated string and
    mandate the caller to free it)

> +struct cb_foreach {
> +       int argc;
> +       const char **argv;
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +                                   void *cb_data)

As we only ever use list_item->name, we could also change
the argument list to take a "const char *[submodule_]path".

> +       prepare_submodule_repo_env(&cp.env_array);
> +       cp.use_shell = 1;
> +       cp.dir = list_item->name;
> +
> +       if (info->argc == 1) {
> +               argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +               argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +               argv_array_pushf(&cp.env_array, "sha1=%s",
> +                                oid_to_hex(&list_item->oid));
> +               argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +               argv_array_pushf(&cp.args, "path=%s; %s", list_item->name,
> +                                info->argv[0]);

In the argc != 1 case we also want to have the env_array filled with
useful variables. (It seems we do not have tests for that?)
To test for that we'd need a test similar as in 1c4fb136d,
just with
    git submodule foreach echo \$sm_path \$dpath
for example.

So I think you can move the pushes to the env array outside this condition.
As we set cp.use_shell unconditionally, we do not need to construct
the first argument specially with path preset, but instead we can just prepend
it in the array:

    argv_array_pushf(&cp.env_array, "name=%s", sub->name);
    ... // more env stuff

    argv_array_pushf(&cp.args, "path=%s;", list_item->name);
    for (i = 0; i < info->argc; i++)
        argv_array_push(&cp.args, info->argv[i]);

should do?


> +
> +       if (info->argv[0] && run_command(&cp))
> +               die(_("run_command returned non-zero status for %s\n."),

This would rather be
    die(_("Stopping at '%s'; script returned non-zero status."), displaypath);
to imitate the shell version faithfully?


> +
> +               if (run_command(&cpr))
> +                       die(_("run_command returned non-zero status while"
> +                             "recursing in the nested submodules of %s\n."),
> +                             displaypath);

same here. As the inner process would have already printed the "Stopping..."
we would not need to repeat it, though.

So maybe

    /* no need to report error, child does: */
    run_command(&cpr);

The rest below looks good. :)

Thanks,
Stefan

  reply index

Thread overview: 48+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-04-19 17:05 [GSoC][RFC/PATCH] " Prathamesh Chavan
2017-04-19 18:08 ` Stefan Beller
2017-04-22 19:58   ` [GSoC][RFC/PATCH v2] " Prathamesh Chavan
2017-04-24  2:24     ` Junio C Hamano
2017-04-24 20:03     ` Stefan Beller
2017-04-24 22:11       ` Ramsay Jones
2017-04-24 22:17         ` Stefan Beller
2017-04-24 22:43           ` Ramsay Jones
2017-05-12 11:44             ` [GSoC][RFC/PATCH v3 1/2] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-12 11:44               ` [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-15 17:22                 ` Stefan Beller
2017-05-15 18:34                 ` Brandon Williams
2017-05-21 12:58                   ` [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-21 12:58                     ` [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-22 20:04                       ` Stefan Beller
2017-05-23 19:09                         ` Brandon Williams
2017-05-23 19:36                       ` Brandon Williams
2017-05-23 20:57                         ` Stefan Beller
2017-05-23 21:05                           ` Brandon Williams
2017-05-26 15:17                       ` [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value Prathamesh Chavan
2017-05-26 15:17                         ` [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-26 16:19                           ` Stefan Beller
2017-05-26 16:33                           ` Brandon Williams
2017-05-26 15:17                         ` [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-26 16:14                           ` Stefan Beller
2017-05-26 16:44                           ` Brandon Williams
2017-05-26 21:54                           ` Johannes Sixt
2017-05-26 22:03                             ` Brandon Williams
2017-05-27  1:20                             ` Ramsay Jones
2017-05-27 14:06                               ` Ramsay Jones
2017-05-27 21:24                                 ` Johannes Sixt
2017-05-26 16:31                         ` [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value Ramsay Jones
2017-05-26 17:07                           ` Stefan Beller
2017-05-27  1:10                             ` Ramsay Jones
2017-05-30 21:53                               ` Stefan Beller
2017-05-30 23:07                                 ` Ramsay Jones
2017-05-30 23:29                                   ` Stefan Beller
2017-05-31  0:13                                     ` Ramsay Jones
2017-05-31  0:48                                       ` Ramsay Jones
2017-06-02 11:24                                         ` [GSoC][PATCH v6 1/2] " Prathamesh Chavan
2017-06-02 11:24                                           ` [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-06-03  2:13                                             ` Stefan Beller [this message]
2017-06-04 10:32                                               ` Prathamesh Chavan
2017-05-23 19:06                     ` [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added Brandon Williams
2017-06-03  0:37                   ` [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir Stefan Beller
2017-06-03 14:07                     ` Ramsay Jones
2017-06-04 15:05                       ` Ramsay Jones
2017-06-05 22:20                     ` Jonathan Nieder

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGZ79kbDpyO5uRCPnhCWmaFKhyLEUN7mYO9PhU3xF0f=W35ofw@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=pc44800@gmail.com \
    --cc=ramsay@ramsayjones.plus.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox