git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: sbeller@google.com, git@vger.kernel.org
Subject: Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C
Date: Sun, 23 Apr 2017 19:24:28 -0700	[thread overview]
Message-ID: <xmqqfugy35lv.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170422195804.18477-1-pc44800@gmail.com> (Prathamesh Chavan's message of "Sun, 23 Apr 2017 01:28:04 +0530")

> +static void foreach_submodule(int argc, const char **argv, const char *path,
> +			      const unsigned char *sha1, const char *prefix,
> +			      int quiet, int recursive)
> +{

I think that a reader would expect that a function whose name is
foreach_something() to iterate over some collection and do something
on each of them, but this is not.  It is "do something on one thing"
part in a loop that exists elsewhere.

Do not call such a helper "foreach_<something>".

Would it make more sense to use the "struct object_id" (instead of
uchar[40]) interface for new functions like this?

> +	const char *toplevel = xgetcwd();
> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf sub_sha1 = STRBUF_INIT;
> +	struct strbuf file = STRBUF_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	if (prefix && get_super_prefix()) {
> +		die("BUG: cannot have prefix and superprefix");
> +	} else if (prefix) {
> +		displaypath = xstrdup(relative_path(path, prefix,  &sb));

An extra SP after the last comma?

> +	} else if (get_super_prefix()) {
> +		strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
> +		displaypath = strbuf_detach(&sb, NULL);
> +	} else {
> +		displaypath = xstrdup(path);
> +	}
> +
> +	sub = submodule_from_path(null_sha1, path);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +	strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);

Why add-unique-abbrev if we do not want to abbreviate at all (with
hardcoded constant 40)?  IOW, shouldn't this be

	strbuf_addstr(&sub_sha1, sha1_to_hex(sha1));

> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);

Why are we sending a value that will always be the same in two
variables?  "git submodule --help" seems to list only name, path,
sha1 and toplevel variables, and not sm_path.  Is it a documentation
bug, or are we clobbering a variable that the end user may be using
for other purposes in her foreach script?

> +	argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	cp.use_shell = 1;
> +	cp.dir = path;
> +
> +	for (i = 0; i < argc; i++)
> +		argv_array_push(&cp.args, argv[i]);
> +
> +	strbuf_addstr(&file, path);
> +	strbuf_addstr(&file, "/.git");
> +
> +	if (!quiet && !access_or_warn(file.buf, R_OK, 0))
> +		printf(_("Entering '%s'\n"), displaypath);
> +
> +	if (!access_or_warn(file.buf, R_OK, 0))
> +		run_command(&cp);
> +
> +	if(recursive) {

Missing SP after "if".

More importantly, if the earlier access-or-warn failed and we didn't
do the run-command for the path, does it even make sense to recurse
into it?

The original scripted version seems to refrain from recursing into
it if the command fails in the submodule in the first place.  This
code discards the exit status from run_command(), which looks wrong.

> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.use_shell = 1;
> +		cpr.dir = path;
> +
> +		argv_array_pushf(&cpr.args, "git");
> +		argv_array_pushf(&cpr.args, "--super-prefix");
> +		argv_array_push(&cpr.args, displaypath);
> +		argv_array_pushf(&cpr.args, "submodule--helper");
> +
> +		if (quiet)
> +			argv_array_pushf(&cpr.args, "--quiet");
> +
> +		argv_array_pushf(&cpr.args, "foreach");
> +		argv_array_pushf(&cpr.args, "--recursive");
> +
> +		for (i = 0; i < argc; i++)
> +			argv_array_push(&cpr.args, argv[i]);
> +
> +		run_command(&cpr);

Similarly, the exit status of this invocation is discarded, which
probably is wrong.  The original seems to pay attention to the
failure from the command invoked via "foreach --recursive" and stops
interation when it sees a failure.

> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +	int i;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper [--quiet] foreach [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);

> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +			die("BUG: module_list_compute should not choke on empty pathspec");

This line would fit within 80-col if it weren't overly indented, I
would think.

One suggestion.

Start by updating "git submodule foreach [--recursive]" tests in t/
directory before writing any more C code.  Arrange to have two
submodules, have your custom command that is run with foreach fail
in the first submodule, and write a test that documents the
behaviour of the scripted version (e.g. your custom command should
not be even run inside the second submodule).  Add a similar test
with two submodules, first of which has a nested submodule, and have
your custom command fail in one of these three places, and document
the behaviour of the scripted version (e.g. your custom command will
run for the first submodule, and if it fails there, does not run in
the nested submodule or in the second submodule; if your custom
command succeeds in the first submodule, it goes to its nested
submodule, and if your custom command fails there, the second
submodule will not visited, etc.).

Also if it is not done in the existing tests, perhaps writing tests
or two with submodules whose names are "submodule 1", "submodule 2",
etc. and making sure that there is no funny splitting of the arguments
would be a healthy thing to do.

After getting that working, then start writing the above C code.
That would catch cases where your "rewrite in C" accidentally
introduces regressions.

Thanks.

  reply	other threads:[~2017-04-24  2:24 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-19 17:05 [GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C 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 [this message]
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
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 publicly 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 using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfugy35lv.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=sbeller@google.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).