[RFC/PATCH] ls-files: adding support for submodules

[RFC/PATCH] ls-files: adding support for submodules

[Brandon Williams]

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.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
Hey git developers!

I'm new to the community and this is the first patch for an open source project
that I have worked on.

I'm looking forward to working on the project!
Brandon Williams

 Documentation/git-ls-files.txt         |   7 ++-
 builtin/ls-files.c                     |  58 +++++++++++++++++++
 t/t3007-ls-files-recurse-submodules.sh | 103 +++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..c428a51 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,6 +29,7 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -152,6 +154,45 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ *  Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf name = STRBUF_INIT;
+	int submodule_name_len;
+	FILE *fp;
+
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	cp.out = -1;
+	start_command(&cp);
+	fp = fdopen(cp.out, "r");
+
+	/*
+	 * The ls-files child process produces filenames relative to
+	 * the submodule. Prefix each line with the submodule path
+	 * to make it relative to the current repository.
+	 */
+	strbuf_addstr(&name, ce->name);
+	strbuf_addch(&name, '/');
+	submodule_name_len = name.len;
+	while (strbuf_getline(&buf, fp) != EOF) {
+		strbuf_addbuf(&name, &buf);
+		write_name(name.buf);
+		strbuf_setlen(&name, submodule_name_len);
+	}
+
+	finish_command(&cp);
+	strbuf_release(&buf);
+	strbuf_release(&name);
+	fclose(fp);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
 	int len = max_prefix_len;
@@ -163,6 +204,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 			    len, ps_matched,
 			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
 		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+		return;
+	}
 
 	if (tag && *tag && show_valid_bit &&
 	    (ce->ce_flags & CE_VALID)) {
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +566,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol))
+		die("ls-files --recurse-submodules can only be used in "
+		    "--cached mode");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support path "
+		    "arguments");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100644
index 0000000..78deded
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	mkdir submodule &&
+	(
+		cd submodule &&
+		git init &&
+		echo c >c &&
+		git add c &&
+		git commit -m "add c"
+	) &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+cat >expect <<EOF
+.gitmodules
+a
+b/b
+submodule/c
+EOF
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	echo a >not_added &&
+	echo b >b/not_added &&
+	(
+		cd submodule &&
+		echo c >not_added
+	) &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<EOF
+.gitmodules
+a
+b/b
+submodule/.gitmodules
+submodule/c
+submodule/subsub/d
+EOF
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	(
+		cd submodule &&
+		mkdir subsub &&
+		(
+			cd subsub &&
+			git init &&
+			echo d >d &&
+			git add d &&
+			git commit -m "add d"
+		) &&
+		git submodule add ./subsub &&
+		git commit -m "added subsub"
+	) &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+cat >expect_error <<EOF
+fatal: ls-files --recurse-submodules does not support path arguments
+EOF
+
+test_expect_success 'error when using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_cmp expect_error actual
+'
+
+cat >expect_error <<EOF
+fatal: ls-files --recurse-submodules can only be used in --cached mode
+EOF
+
+test_expect_success 'error when using different modes' '
+	for opt in {v,t}; do
+		test_must_fail git ls-files --recurse-submodules -$opt 2>actual &&
+		test_cmp expect_error actual
+	done &&
+	for opt in {deleted,modified,others,ignored,stage,killed,unmerged,eol}; do
+		test_must_fail git ls-files --recurse-submodules --$opt 2>actual &&
+		test_cmp expect_error actual
+	done
+'
+
+test_done
-- 
2.8.0.rc3.226.g39d4020

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

[Jeff King]

On Fri, Sep 09, 2016 at 02:53:24PM -0700, Brandon Williams wrote:

> 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.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Hey git developers!
> 
> I'm new to the community and this is the first patch for an open source project
> that I have worked on.
> 
> I'm looking forward to working on the project!

Welcome. :)

Submodules are not really my area of expertise, so I don't have any
commentary on the goal of the patch, except that it sounds reasonable to
my layman's ears.

The implementation looks fairly clean. A few comments:

> +static void show_gitlink(const struct cache_entry *ce)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf name = STRBUF_INIT;
> +	int submodule_name_len;
> +	FILE *fp;
> +
> +	argv_array_push(&cp.args, "ls-files");
> +	argv_array_push(&cp.args, "--recurse-submodules");
> +	cp.git_cmd = 1;
> +	cp.dir = ce->name;
> +	cp.out = -1;
> +	start_command(&cp);
> +	fp = fdopen(cp.out, "r");

You should error-check the result of start_command(). I guess the
reasonable outcome would be to die(), as it is a sign that we could not
fork, find git, etc.

Ditto for fdopen (you can use xfdopen for that).

> +	/*
> +	 * The ls-files child process produces filenames relative to
> +	 * the submodule. Prefix each line with the submodule path
> +	 * to make it relative to the current repository.
> +	 */
> +	strbuf_addstr(&name, ce->name);
> +	strbuf_addch(&name, '/');
> +	submodule_name_len = name.len;
> +	while (strbuf_getline(&buf, fp) != EOF) {
> +		strbuf_addbuf(&name, &buf);
> +		write_name(name.buf);
> +		strbuf_setlen(&name, submodule_name_len);
> +	}

What happens if the filename in the submodule needs quoting? You'll get
the quoted value in your buffer, and then re-quote it again in
write_name().

The simplest thing would probably be to use "ls-files -z" for the
recursive invocation, and then split on NUL bytes (we have
strbuf_getline_nul for that).

> +	finish_command(&cp);

What should happen if finish_command() tells us that the ls-files
sub-process reported an error? It may not be worth aborting the rest of
the listing, but we might want to propagate that in our own return code.

> +	strbuf_release(&buf);
> +	strbuf_release(&name);
> +	fclose(fp);
> +}

A minor style nit, but I would generally fclose(fp) before running
finish_command() (i.e., resource clean up in the reverse order of
allocation). It doesn't matter in this case because "fp" is output from
the process, and we know we've already read to EOF. For other cases, it
could cause a deadlock (e.g., we end up in wait() for the child process
to finish, but it is blocked in write() waiting for us to read). So I
think it's a good habit to get into.

> @@ -519,6 +566,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>  	if (require_work_tree && !is_inside_work_tree())
>  		setup_work_tree();
>  
> +	if (recurse_submodules &&
> +	    (show_stage || show_deleted || show_others || show_unmerged ||
> +	     show_killed || show_modified || show_resolve_undo ||
> +	     show_valid_bit || show_tag || show_eol))
> +		die("ls-files --recurse-submodules can only be used in "
> +		    "--cached mode");

Woah, that list of variables is getting rather long. This is not a
problem introduced by your patch, so it's not a blocker. But I wonder if
some of them are mutually exclusive and could be collapsed to a single
variable.

I guess the reason for this "only with --cached" is that you do not
propagate the options down to the recursive process. If we were to do
that, then this big list of restrictions would go away. I'd be OK with
starting with more limited functionality like your patch, though. I
think doing the recursive thing correctly would also involve parsing the
output of each to append the filename prefix.

So I suppose another option would be to teach ls-files a "prefix" option
to add to each filename, and just pass in the submodule path. Then you
can let the sub-processes write directly to the common stdout, and I
think it would be safe to blindly pass the parent argv into the child
processes.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I suppose another option would be to teach ls-files a "prefix" option
> to add to each filename, and just pass in the submodule path. Then you
> can let the sub-processes write directly to the common stdout, and I
> think it would be safe to blindly pass the parent argv into the child
> processes.

I think that is a sensible way to do this, instead of reading from -z
and showing things depending on various output modes you were told
to use.

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

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

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

While I see why "ls-files --recurse-submodules" sounds nice ("hey, I
can get list of _all_ the files here"), and I am quite happy with
the quality of implementation (not just the code but its
documentation and test) especially from a first-time contributor, I
am not quite sure what the utility of this new feature would be,
especially given that the command is a plumbing, i.e. meant to be a
useful building block for scripts.

If I get

	$ git ls-files --recurse-submodules
	Makefile
        lib/Makefile
        lib/hello.py
	main.py
	goodbye.py

out of the command, what can I do with it without knowing where the
submodule boundaries are?  It's not like I can just do

	git ls-files --recurse-submodule |
        while read path
        do
        	git update-index --add "$path"
	done

when "lib/" is a submodule.  Instead, I'd need to go to "lib/" and
then add "Makefile" and "hello.py" from there.

> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> new file mode 100644
> index 0000000..78deded
> --- /dev/null
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -0,0 +1,103 @@
> +...
> +test_expect_success 'setup directory structure and submodules' '
> +...
> +'
> +
> +cat >expect <<EOF
> +.gitmodules
> +a
> +b/b
> +submodule/c
> +EOF

We used to do that when we didn't know better.

Please don't do things outside test_expect_* block, especially in a
new script.

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

[Stefan Beller]

On Fri, Sep 9, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> 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.
>
> While I see why "ls-files --recurse-submodules" sounds nice ("hey, I
> can get list of _all_ the files here"), and I am quite happy with
> the quality of implementation (not just the code but its
> documentation and test)

The plan is to hook the ls-files machinery into
git-grep as the way of obtaining files to grep for a pattern.

As git-grep has a thread pool already, getting the list of files
single threaded  is more beneficial as e.g. calling git-grep recursively
for submodules, as then you may have a fork bomb.

>
>> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh

> We used to do that when we didn't know better.
>

Oops, a lot of stuff slipped through the first internal review.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The plan is to hook the ls-files machinery into
> git-grep as the way of obtaining files to grep for a pattern.

That does not make much sense to me for exactly the same reason why
the "grab the list of paths and run 'git add' on them" example in
the message you are responding to does not make sense.  The use of
the thread-pool would still need to honor the submodule boundary so
that one thread may be assigned files in the top-level superproject
while another may be assigned files in lib/ submodule repository,
and the latter would be doing a rough equivalent of "git -C lib
grep" perhaps with a new option "--output-path-prefix=lib/" that
makes any and all paths that are reported from the command prefixed
with the specified string, so the result of its grepping in Makefile
may be reported as findings in lib/Makefile.

For that, it is not sufficient for the enumeration of paths done in
the top-level to just list lib/Makefile and lib/hello.py along with
Makefile and main.py, is it?  You would somehow need to have a way
to tell that 'lib/' and everything in there is inside a separate
repository.  Without knowing that "lib/" is its own repository, you
would not even know which files under "lib/" hierarchy in the
filesystem are actually tracked files, which you would learn only by
reading lib/.git/index, or what textconv filtering needs to be done
on them, which you would learn only by reading lib/.gitattributes
and/or lib/.git/config.

So a "ls-files" that is done internally in the end-user facing "git
grep --recurse-submodules" needs to be run _without_ recursing
itself at least once to learn "lib/" is a submodule.  A flat "here
are everything we have" does not sound like a good building block.

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

[Jeff King]

On Sun, Sep 11, 2016 at 03:10:13PM -0700, Junio C Hamano wrote:

> So a "ls-files" that is done internally in the end-user facing "git
> grep --recurse-submodules" needs to be run _without_ recursing
> itself at least once to learn "lib/" is a submodule.  A flat "here
> are everything we have" does not sound like a good building block.

I do not use submodules myself, but I could imagine that you may have
scripts outside of git that do not care about the submodule divisions at
all, and would be happy with the flat block. E.g., our "make tags"
target uses "git ls-files" so find all of the source files. I could
imagine projects with submodules that would want to do so recursively (I
could also imagine projects that do _not_ want to do so; it would depend
on your workflow and how tightly bound the submodules are). Another
plausible example would be something grep-like that has features
git-grep does not (I don't use "ack", but perhaps "git ls-files
--recurse-submodules -z | xargs --null ack ..." is something people
would want to do).

-Peff

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

[Jeff King]

On Sun, Sep 11, 2016 at 08:51:06PM -0400, Jeff King wrote:

> On Sun, Sep 11, 2016 at 03:10:13PM -0700, Junio C Hamano wrote:
> 
> > So a "ls-files" that is done internally in the end-user facing "git
> > grep --recurse-submodules" needs to be run _without_ recursing
> > itself at least once to learn "lib/" is a submodule.  A flat "here
> > are everything we have" does not sound like a good building block.
> 
> I do not use submodules myself, but I could imagine that you may have
> scripts outside of git that do not care about the submodule divisions at
> all, and would be happy with the flat block. E.g., our "make tags"
> target uses "git ls-files" so find all of the source files. I could
> imagine projects with submodules that would want to do so recursively (I
> could also imagine projects that do _not_ want to do so; it would depend
> on your workflow and how tightly bound the submodules are). Another
> plausible example would be something grep-like that has features
> git-grep does not (I don't use "ack", but perhaps "git ls-files
> --recurse-submodules -z | xargs --null ack ..." is something people
> would want to do).

None of that negates your point, btw, which is that this does not seem
like a great building block for "git grep --recurse-submodules". Just
that it seems plausible to me that people could find recursive
"ls-files" useful on its own.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I do not use submodules myself, but I could imagine that you may have
>> scripts outside of git that do not care about the submodule divisions at
>> all, and would be happy with the flat block. ...
>> git-grep does not (I don't use "ack", but perhaps "git ls-files
>> --recurse-submodules -z | xargs --null ack ..." is something people
>> would want to do).
>
> None of that negates your point, btw, which is that this does not seem
> like a great building block for "git grep --recurse-submodules". Just
> that it seems plausible to me that people could find recursive
> "ls-files" useful on its own.

I do think it is a good argument why ls-files (with or without -z)
that recurses into submodules would help "git grep" that does not
look at the object store and does the search in the working tree
files that are tracked.  In normal Git world view, if you want a
list of files that are tracked, you are the one who is supposed to
go to the repository and look at the index there, but if you can
somehow magically get that list out-of-band, the "in the working
tree files" mode of "git grep" can work just like the "xargs -z ack"
pipeline in your example.

So I tend to agree that the enhancement in question is a useful
thing on its own, at least the part that gives a list of paths in
the index.

I do not know if all other operationg modes are useful, though.  For
eaxmple, the mode that lists untracked paths might be useful to do a
recursive "git clean".  On the other hand, "ls-files -s" with the
patch may produce the "correct" result (i.e. the object name given
for each path would be the same one that are found in the index of
the submodule the path was taken from), but the correctness may not
necessarily translate to usefulness, exactly because you need to
know which submodule's repository has the named object that is not
given by the flattened list.

Having to say that "this command produces correct result, but not
all correct things are necessarily useful" makes me wonder if that
is the direction in which we would want to go.  For example, what
would be our answer to an end-user question: I now know that the
recursive ls-files can give me all the untracked files in the
top-level and submodules.  How can I feed that to "git add" to
update their respective index files with them?

I am not convinced that we would always want to make "git add
lib/Makefile" to automagically run "git -C lib/ add Makefile" when
lib/ is a submodule (for that matter, even if we wanted to, I do not
know if that is something we can efficiently do with pathspecs that
have wildcards).

So...

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

[Brandon Williams]

Thanks for all the comments.  What it sounds like is that using ls-files as
a means to power a recursive git-grep may not be like the best approach (I
assumed that would be the case but thought it a decent place to start).

I agree that not all operating modes would be useful for a recursive
ls-files, which is why I initially don't have support for them.  I guess
the question would be which modes would be worth supporting in a recursive
case?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So a "ls-files" that is done internally in the end-user facing "git
> grep --recurse-submodules" needs to be run _without_ recursing
> itself at least once to learn "lib/" is a submodule.  A flat "here
> are everything we have" does not sound like a good building block.

... unless you are _only_ interested in grepping (or in general
working outside Git repository) in the files in the working tree,
i.e. "git grep" without <tree-ish> nor "--cached".

A lot of the time you are interested in the current state of files,
not even in "the state recorded in the tip of the history" but in
"the state as I have in my working tree, the state as my compiler
sees it".

I am a bit torn.  Clearly this is an important special case, but it
would make the codepath for object database case and working tree
case even further apart between "git grep [--cached | <tree-ish>]"
and the "find in the working tree" codepath, which does not sound
friendly to the codebase.

[PATCH v2] ls-files: adding support for submodules

[Brandon Williams]

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 an
output-path-prefix command in order to prepend paths to child processes.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         | 11 +++-
 builtin/ls-files.c                     | 60 +++++++++++++++++++++
 t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..a623ebf 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,9 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--output-path-prefix=<path>]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +139,13 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode.
+
+--output-path-prefix=<path>::
+	Prepend the provided path to the output of each file
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..c0bce00 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static const char *output_path_prefix = "";
+static int recurse_submodules;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+	 * by the caller.
+	 *
+	 * full_name get reused across output lines to minimize the allocation
+	 * churn.
+	 */
+	static struct strbuf full_name = STRBUF_INIT;
+	if (output_path_prefix != '\0') {
+		strbuf_reset(&full_name);
+		strbuf_addstr(&full_name, output_path_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
@@ -152,6 +170,25 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+	argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
+			 output_path_prefix, ce->name);
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
 	int len = max_prefix_len;
@@ -163,6 +200,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 			    len, ps_matched,
 			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
 		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+		return;
+	}
 
 	if (tag && *tag && show_valid_bit &&
 	    (ce->ce_flags & CE_VALID)) {
@@ -468,6 +509,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_STRING(0, "output-path-prefix", &output_path_prefix,
+			N_("path"), N_("prepend <path> to each file")),
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +564,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol))
+		die("ls-files --recurse-submodules can only be used in "
+		    "--cached mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support path "
+		    "arguments");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..caf3815
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'can only be used in --cached mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2] ls-files: adding support for submodules

[Brandon Williams]

>  static void write_name(const char *name)
>  {
>         /*
> +        * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> +        * by the caller.
> +        *
> +        * full_name get reused across output lines to minimize the allocation
> +        * churn.
> +        */
> +       static struct strbuf full_name = STRBUF_INIT;
> +       if (output_path_prefix != '\0') {

It was pointed out to me that this should be:
    if (*output_path_prefix != '\0') {


> +               strbuf_reset(&full_name);
> +               strbuf_addstr(&full_name, output_path_prefix);
> +               strbuf_addstr(&full_name, name);
> +               name = full_name.buf;
> +       }

Re: [PATCH v2] ls-files: adding support for submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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 an
> output-path-prefix command in order to prepend paths to child processes.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
>  static void write_name(const char *name)
>  {
>  	/*
> +	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> +	 * by the caller.
> +	 *
> +	 * full_name get reused across output lines to minimize the allocation
> +	 * churn.
> +	 */
> +	static struct strbuf full_name = STRBUF_INIT;
> +	if (output_path_prefix != '\0') {
> +		strbuf_reset(&full_name);
> +		strbuf_addstr(&full_name, output_path_prefix);
> +		strbuf_addstr(&full_name, name);
> +		name = full_name.buf;
> +	}

At first glance it was surprising that no test caught this lack of
dereference; the reason is because you initialize output_path_prefix
to an empty string, not NULL, causing full_name.buf always used,
which does not have an impact on the output.

I think initializing it to NULL is a more typical way to say "this
option has not been given", and if you took that route, the
condition would become

	if (output_path_prefix && *output_path_prefix) {
        	...

In any case, the fact that only this much change was required to add
output-path-prefix shows two good things: (1) the original code was
already well structured, funneling any pathname we need to emit
through this single function so that we can do this kind of updates,
and (2) the author of the patch was competent to spot this single
point that needs to be updated.

Nice.

> +	status = run_command(&cp);
> +	if (status)
> +		exit(status);

run_command()'s return value comes from either start_command() or
finish_command().  These signal failure by returning a non-zero
value, and in practice they are negative small integers.  Feeding
negative value to exit() is not quite kosher.  Perhaps exit(128)
to mimick as if we called die() is better.

If your primary interest is to support the "find in the working tree
files that are tracked, recursively in submodules" grep, I think
this "when we hit a submodule, spawn a separate ls-files in there"
is sufficient and a solid base to build on it.

On the other hand, if you are more ambitious and "grep" is merely an
example of things that can be helped by having a list of paths
across module boundaries, we may want to "libify" ls-files in such a
way that a single process can instantiate one or more instances of
"ls-files machinery", that takes which repository to work in and
other arguments that specifies which paths to report, and instead of
always showing the result to the standard output, makes repeated
calls to a callback function to report the discovered path and other
attributes associated with the path that were asked for (the object
name, values of tag_*, etc.), without spawning a separate "ls-files"
process.

The latter would be a lot bigger task and I do not necessarily think
it is needed, but that is one possible future direction to keep in
mind.

Thanks, will queue with a minimum fix.

Re: [PATCH v2] ls-files: adding support for submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks, will queue with a minimum fix.

So here are two squashable patches, one is the "minimum" one, the
other is a bit more invasive one to use "a pointer to an optional
setting is set to NULL" convention.  I am undecided, and I'll stay
to be without further comments from others, on the latter one.

I understand that many internal changes in your work environment
titles their changes like "DOing X", but our convention around here
is to label them "DO X", as if you are giving an order to somebody
else, either telling the codebase "to be like so", or telling the
patch-monkey maintainer "to make it so".  So I'd retitle it

	ls-files: optionally recurse into submodules

or something like that.  It is an added advantage of being a lot
more descriptive than "adding support", which does not say what kind
of support it is adding.

[PATCH 1/2] SQUASH???

[Junio C Hamano]

---
 builtin/ls-files.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0bce00..6e78c71 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -78,7 +78,7 @@ static void write_name(const char *name)
 	 * churn.
 	 */
 	static struct strbuf full_name = STRBUF_INIT;
-	if (output_path_prefix != '\0') {
+	if (*output_path_prefix) {
 		strbuf_reset(&full_name);
 		strbuf_addstr(&full_name, output_path_prefix);
 		strbuf_addstr(&full_name, name);
-- 
2.10.0-458-g97b4043

[PATCH 2/2] SQUASH??? Undecided

[Junio C Hamano]

If we were to follow the convention to leave an optional string
variable to NULL, we'd need to do this on top.  I am not sure if it
is a good change, though.
---
 builtin/ls-files.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6e78c71..687e475 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
-static const char *output_path_prefix = "";
+static const char *output_path_prefix;
 static int recurse_submodules;
 
 static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)
 	 * churn.
 	 */
 	static struct strbuf full_name = STRBUF_INIT;
-	if (*output_path_prefix) {
+	if (output_path_prefix && *output_path_prefix) {
 		strbuf_reset(&full_name);
 		strbuf_addstr(&full_name, output_path_prefix);
 		strbuf_addstr(&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 	argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
-			 output_path_prefix, ce->name);
+			 output_path_prefix ? output_path_prefix : "",
+			 ce->name);
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
-- 
2.10.0-458-g97b4043

Re: [PATCH 2/2] SQUASH??? Undecided

[Stefan Beller]

+ cc Brandon

On Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If we were to follow the convention to leave an optional string
> variable to NULL, we'd need to do this on top.  I am not sure if it
> is a good change, though.

I think it is a good change.

Thanks,
Stefan

> ---
>  builtin/ls-files.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 6e78c71..687e475 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -29,7 +29,7 @@ static int show_valid_bit;
>  static int line_terminator = '\n';
>  static int debug_mode;
>  static int show_eol;
> -static const char *output_path_prefix = "";
> +static const char *output_path_prefix;
>  static int recurse_submodules;
>
>  static const char *prefix;
> @@ -78,7 +78,7 @@ static void write_name(const char *name)
>          * churn.
>          */
>         static struct strbuf full_name = STRBUF_INIT;
> -       if (*output_path_prefix) {
> +       if (output_path_prefix && *output_path_prefix) {
>                 strbuf_reset(&full_name);
>                 strbuf_addstr(&full_name, output_path_prefix);
>                 strbuf_addstr(&full_name, name);
> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
>         argv_array_push(&cp.args, "ls-files");
>         argv_array_push(&cp.args, "--recurse-submodules");
>         argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
> -                        output_path_prefix, ce->name);
> +                        output_path_prefix ? output_path_prefix : "",
> +                        ce->name);
>         cp.git_cmd = 1;
>         cp.dir = ce->name;
>         status = run_command(&cp);
> --
> 2.10.0-458-g97b4043
>

Re: [PATCH 2/2] SQUASH??? Undecided

[Brandon Williams]

Yeah if that is the convention then I have no problem with the change.

-Brandon

On Thu, Sep 15, 2016 at 2:12 PM, Stefan Beller <sbeller@google.com> wrote:
> + cc Brandon
>
> On Thu, Sep 15, 2016 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> If we were to follow the convention to leave an optional string
>> variable to NULL, we'd need to do this on top.  I am not sure if it
>> is a good change, though.
>
> I think it is a good change.
>
> Thanks,
> Stefan
>
>> ---
>>  builtin/ls-files.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index 6e78c71..687e475 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -29,7 +29,7 @@ static int show_valid_bit;
>>  static int line_terminator = '\n';
>>  static int debug_mode;
>>  static int show_eol;
>> -static const char *output_path_prefix = "";
>> +static const char *output_path_prefix;
>>  static int recurse_submodules;
>>
>>  static const char *prefix;
>> @@ -78,7 +78,7 @@ static void write_name(const char *name)
>>          * churn.
>>          */
>>         static struct strbuf full_name = STRBUF_INIT;
>> -       if (*output_path_prefix) {
>> +       if (output_path_prefix && *output_path_prefix) {
>>                 strbuf_reset(&full_name);
>>                 strbuf_addstr(&full_name, output_path_prefix);
>>                 strbuf_addstr(&full_name, name);
>> @@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
>>         argv_array_push(&cp.args, "ls-files");
>>         argv_array_push(&cp.args, "--recurse-submodules");
>>         argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
>> -                        output_path_prefix, ce->name);
>> +                        output_path_prefix ? output_path_prefix : "",
>> +                        ce->name);
>>         cp.git_cmd = 1;
>>         cp.dir = ce->name;
>>         status = run_command(&cp);
>> --
>> 2.10.0-458-g97b4043
>>

Re: [PATCH v2] ls-files: adding support for submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thanks, will queue with a minimum fix.

So here are two squashable patches, one is the "minimum" one, the
other is a bit more invasive one to use "a pointer to an optional
setting is set to NULL" convention.  I am undecided, and I'll stay
to be without further comments from others, on the latter one.

I understand that many internal changes in your work environment are
titled like "DOing X", but our convention around here is to label
them "DO X", as if you are giving an order to somebody else, either
telling the codebase "to be like so", or telling the patch-monkey
maintainer "to make it so".  So I'd retitle it

	ls-files: optionally recurse into submodules

or something like that.  It is an added advantage of being a lot
more descriptive than "adding support", which does not say what kind
of support it is adding.

Re: [PATCH v2] ls-files: adding support for submodules

[Junio C Hamano]

Here is an absolute mininum fix ;-)

 builtin/ls-files.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0bce00..6e78c71 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -78,7 +78,7 @@ static void write_name(const char *name)
 	 * churn.
 	 */
 	static struct strbuf full_name = STRBUF_INIT;
-	if (output_path_prefix != '\0') {
+	if (*output_path_prefix) {
 		strbuf_reset(&full_name);
 		strbuf_addstr(&full_name, output_path_prefix);
 		strbuf_addstr(&full_name, name);
-- 
2.10.0-458-g97b4043

Re: [PATCH v2] ls-files: adding support for submodules

[Junio C Hamano]

If we were to follow the convention to leave an optional string
variable to NULL, we'd need to do this on top.  I am not sure if it
is a good change, though.
---
 builtin/ls-files.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6e78c71..687e475 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
-static const char *output_path_prefix = "";
+static const char *output_path_prefix;
 static int recurse_submodules;
 
 static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)
 	 * churn.
 	 */
 	static struct strbuf full_name = STRBUF_INIT;
-	if (*output_path_prefix) {
+	if (output_path_prefix && *output_path_prefix) {
 		strbuf_reset(&full_name);
 		strbuf_addstr(&full_name, output_path_prefix);
 		strbuf_addstr(&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 	argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
-			 output_path_prefix, ce->name);
+			 output_path_prefix ? output_path_prefix : "",
+			 ce->name);
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
-- 
2.10.0-458-g97b4043

Re: [PATCHv5] Another squash on run-command: add an asynchronous parallel child processor

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I agree the commit 7087c5f3 (SQUASH??? on top of run-command: add an
> asynchronous parallel child processor) makes sense; I arrived at the same
> patch after adding in the feedback.

I dunno.  As I said, while a small part of that commit is necessary
to be squashed into [6/14] (i.e. the "failed to start" bugfix and
redefinition of the return value of start_one()), the remainder of
the commit is primarily to illustrate possible future enhancements
that the basic structure of your code must support, so that we can
get the fundamentals right.  Each of the actual "possible future
enhancements" may or may not be a good idea and there is nothing
that back it up.  In such a vacuum, I'd prefer to leave them simple
and avoid starting performance tuning prematurely.

One thing that is sort-of backed up already by your "cap to 4"
experiment is that some sort of slow-start ramping up is far better
than letting thundering herd stampede, so I am OK if we kept that
SPAWN_CAP part of the commit.

But even then, we do not know if tying that cap to online_cpu() is a
good idea.  Neither of us have a good argument backed by data on it.

It is tempting to imagine, when you have N cores on an otherwise
idle box, the setting of SPAWN_CAP shouldn't make much difference to
how well the first child process makes its initial progress as long
as it does not exceed the number of idle cores N-1 you have at hand.

But that assumes that the task is CPU bound and you have infinite
memory bandwidth.  Once the task needs a lot of disk bandwidth to
make its initial progress, which certainly is the case for fetch,
the first child that is spawned together with (SPAWN_CAP-1) other
processes would be competing for the shared resource, and having
more online_cpus() would not help you.

If we are not doing analysis that takes into such factors (and it is
way too premature for us to be tuning), even "online_cpu() - 1" is
unnecessarily too complex than a hardcoded small number (say, "2",
or even "1").

The same thing can be said for the output_timeout selection.  "Do
not get stuck for too long until we have fully ramped up.  Do not
spin too frequently when there is no more room for a new child" was
something I came up out of thin air as an example of something we
might want to do, and I did write such a code in that commit, but
that was primarily done so that you can clearly see that a better
design would be to allow the caller, i.e. the scheduling loop,
specify output_timeout to buffer_stderr(), and to keep the latter a
"dumb" helper that can be controlled by a more smart caller (as
opposed to hiding such a logic in buffer_stderr() and have a "dumb"
driver call it).  The actual output_timeout computation logic is not
well thought out---it may even turn out to be that we are better off
if we lengthened the timeout before we have fully ramped up, to
encourage the first process to produce some output before we give
chance to other new processes to be spawned in the later round.

So for that change, while I think adding that parameter to
buffer_stderr() is something we would want to keep, I'd prefer to
keep the caller simpler by always passing a hardcoded 100 in the
initial version, before we start tuning.  And I do not think we want
to start tuning before building a solid foundation to tune.

In short, if I were amending that SQUASH??? commit, I'd probably be
making it do less, not more, than what it does, something along the
line of the attached.

 run-command.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/run-command.c b/run-command.c
index b6d8b39..829b6fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1108,20 +1108,19 @@ static void pp_collect_finished(struct parallel_processes *pp)
 }
 
 
-#define SPAWN_CAP (pp.max_processes + 1) /* spawn as many as possible */
-
 int run_processes_parallel(int n, void *data,
 			   get_next_task_fn get_next_task,
 			   start_failure_fn start_failure,
 			   return_value_fn return_value)
 {
 	struct parallel_processes pp;
-	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
 
+	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
 	while (1) {
-		int no_more_task, cnt, output_timeout;
+		int no_more_task, cnt, output_timeout = 100;
+		int spawn_cap = 2;
 
-		for (cnt = SPAWN_CAP, no_more_task = 0;
+		for (cnt = spawn_cap, no_more_task = 0;
 		     cnt && pp.nr_processes < pp.max_processes;
 		     cnt--) {
 			if (!pp_start_one(&pp)) {
@@ -1132,12 +1131,6 @@ int run_processes_parallel(int n, void *data,
 
 		if (no_more_task && !pp.nr_processes)
 			break;
-		if (!cnt)
-			output_timeout = 50;
-		else if (pp.nr_processes < pp.max_processes)
-			output_timeout = 100;
-		else
-			output_timeout = 1000;
 		pp_buffer_stderr(&pp, output_timeout);
 
 		pp_output(&pp);

[PATCH 0/2] Another squash on run-command: add an asynchronous parallel child processor

[Stefan Beller]

Here is another proposal for squashing.
It's basically both squash proposals by Junio, plus another issues I think
should be done right from the beginning.

 [added by sb:]
 * If you do not die() in start_failure_fn or return_value_fn, you
   don't want to write to stderr directly as you would destroy the fine
   ordering of the processes output. So make the err strbuf available in
   both these functions, and make sure the strbuf is appended to the
   buffered output in both cases.

Stefan Beller (2):
  SQUASH???
  SQUASH for "fetch_populated_submodules: use new parallel job
    processing"

-- 
2.5.0.273.g6fa2560.dirty

[PATCH 1/2] SQUASH???

[Stefan Beller]

This is a mixed bag of squashes.

 * pp_start_one() stuffed unborn child to the list of children when
   start_command() failed and start_failure() did not die(); return
   to the caller without corrupting children[] list in this case.

 * make poll(2) timeout used in pp_buffer_stderr() configurable by
   the scheduling loop.

 * allow slow-start of the whole process, so that we do not spawn
   tons of processes before starting to read from any of them, to
   give a better first-byte latency.

 * fix the semantics of the value returned from pp_start_one() and
   adjust the scheduling loop for it.

[added by sb:]
 * If you do not die() in start_failure_fn or return_value_fn, you
   don't want to write to stderr directly as you would destroy the fine
   ordering of the processes output. So make the err strbuf available in
   both these functions, and make sure the strbuf is appended to the
   buffered output in both cases

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 43 ++++++++++++++++++++++++++++++-------------
 run-command.h |  1 +
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 494e1f8..0d22291 100644
--- a/run-command.c
+++ b/run-command.c
@@ -907,6 +907,7 @@ void default_start_failure(void *data,
 
 void default_return_value(void *data,
 			  struct child_process *cp,
+			  struct strbuf *err,
 			  int result)
 {
 	int i;
@@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
 			"output will be degraded");
 }
 
-/* returns 1 if a process was started, 0 otherwise */
+/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
 static int pp_start_one(struct parallel_processes *pp)
 {
 	int i;
@@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
 	if (!pp->get_next_task(pp->data,
 			       &pp->children[i].process,
 			       &pp->children[i].err))
-		return 1;
+		return 0;
 
-	if (start_command(&pp->children[i].process))
+	if (start_command(&pp->children[i].process)) {
 		pp->start_failure(pp->data,
 				  &pp->children[i].process,
 				  &pp->children[i].err);
+		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
+		strbuf_reset(&pp->children[i].err);
+		return -1;
+	}
 
 	set_nonblocking(pp->children[i].process.err);
 
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
 	pp->pfd[i].fd = pp->children[i].process.err;
-	return 0;
+	return 1;
 }
 
-static void pp_buffer_stderr(struct parallel_processes *pp)
+static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
 {
 	int i;
 
-	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
+	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
 		if (errno == EINTR)
 			continue;
 		pp_cleanup(pp);
@@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)
 			error("waitpid is confused (%s)",
 			      pp->children[i].process.argv[0]);
 
-		pp->return_value(pp->data, &pp->children[i].process, code);
+		pp->return_value(pp->data, &pp->children[i].process,
+				 &pp->children[i].err, code);
 
 		argv_array_clear(&pp->children[i].process.args);
 		argv_array_clear(&pp->children[i].process.env_array);
@@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
 			   return_value_fn return_value)
 {
 	struct parallel_processes pp;
-	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
 
+	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
 	while (1) {
-		while (pp.nr_processes < pp.max_processes &&
-		       !pp_start_one(&pp))
-			; /* nothing */
-		if (!pp.nr_processes)
+		int no_more_task, cnt;
+		int output_timeout = 100;
+		int spawn_cap = 4;
+
+		for (cnt = spawn_cap, no_more_task = 0;
+		     cnt && pp.nr_processes < pp.max_processes;
+		     cnt--) {
+			if (!pp_start_one(&pp)) {
+				no_more_task = 1;
+				break;
+			}
+		}
+
+		if (no_more_task && !pp.nr_processes)
 			break;
-		pp_buffer_stderr(&pp);
+		pp_buffer_stderr(&pp, output_timeout);
+
 		pp_output(&pp);
 		pp_collect_finished(&pp);
 	}
diff --git a/run-command.h b/run-command.h
index 3807fd1..f7035cb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,
 
 typedef void (*return_value_fn)(void *data,
 				struct child_process *cp,
+				struct strbuf *err,
 				int result);
 
 /**
-- 
2.5.0.273.g6fa2560.dirty

Re: [PATCH 1/2] SQUASH???

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  * If you do not die() in start_failure_fn or return_value_fn, you
>    don't want to write to stderr directly as you would destroy the fine
>    ordering of the processes output. So make the err strbuf available in
>    both these functions, and make sure the strbuf is appended to the
>    buffered output in both cases

I think that is a sensible change.  Don't we want the same for the
other failure handler, though.  Capture any message from it and
append it to the output of the process that just finished, or
something?

By the way, I understand that these two are solely for early review
and I'll be getting them as either new patches or part of updated
patches in the next reroll (i.e. you are not expecting me to split
these apart and do "rebase -i" for you to the last-posted version).
Asking only to make sure we are on the same wavelength.

Thanks.

>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 43 ++++++++++++++++++++++++++++++-------------
>  run-command.h |  1 +
>  2 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 494e1f8..0d22291 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -907,6 +907,7 @@ void default_start_failure(void *data,
>  
>  void default_return_value(void *data,
>  			  struct child_process *cp,
> +			  struct strbuf *err,
>  			  int result)
>  {
>  	int i;
> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
>  			"output will be degraded");
>  }
>  
> -/* returns 1 if a process was started, 0 otherwise */
> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
>  static int pp_start_one(struct parallel_processes *pp)
>  {
>  	int i;
> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
>  	if (!pp->get_next_task(pp->data,
>  			       &pp->children[i].process,
>  			       &pp->children[i].err))
> -		return 1;
> +		return 0;
>  
> -	if (start_command(&pp->children[i].process))
> +	if (start_command(&pp->children[i].process)) {
>  		pp->start_failure(pp->data,
>  				  &pp->children[i].process,
>  				  &pp->children[i].err);
> +		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
> +		strbuf_reset(&pp->children[i].err);
> +		return -1;
> +	}
>  
>  	set_nonblocking(pp->children[i].process.err);
>  
>  	pp->nr_processes++;
>  	pp->children[i].in_use = 1;
>  	pp->pfd[i].fd = pp->children[i].process.err;
> -	return 0;
> +	return 1;
>  }
>  
> -static void pp_buffer_stderr(struct parallel_processes *pp)
> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>  {
>  	int i;
>  
> -	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
> +	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
>  		if (errno == EINTR)
>  			continue;
>  		pp_cleanup(pp);
> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)
>  			error("waitpid is confused (%s)",
>  			      pp->children[i].process.argv[0]);
>  
> -		pp->return_value(pp->data, &pp->children[i].process, code);
> +		pp->return_value(pp->data, &pp->children[i].process,
> +				 &pp->children[i].err, code);
>  
>  		argv_array_clear(&pp->children[i].process.args);
>  		argv_array_clear(&pp->children[i].process.env_array);
> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
>  			   return_value_fn return_value)
>  {
>  	struct parallel_processes pp;
> -	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>  
> +	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>  	while (1) {
> -		while (pp.nr_processes < pp.max_processes &&
> -		       !pp_start_one(&pp))
> -			; /* nothing */
> -		if (!pp.nr_processes)
> +		int no_more_task, cnt;
> +		int output_timeout = 100;
> +		int spawn_cap = 4;
> +
> +		for (cnt = spawn_cap, no_more_task = 0;
> +		     cnt && pp.nr_processes < pp.max_processes;
> +		     cnt--) {
> +			if (!pp_start_one(&pp)) {
> +				no_more_task = 1;
> +				break;
> +			}
> +		}
> +
> +		if (no_more_task && !pp.nr_processes)
>  			break;
> -		pp_buffer_stderr(&pp);
> +		pp_buffer_stderr(&pp, output_timeout);
> +
>  		pp_output(&pp);
>  		pp_collect_finished(&pp);
>  	}
> diff --git a/run-command.h b/run-command.h
> index 3807fd1..f7035cb 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,
>  
>  typedef void (*return_value_fn)(void *data,
>  				struct child_process *cp,
> +				struct strbuf *err,
>  				int result);
>  
>  /**

Re: [PATCH 1/2] SQUASH???

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think that is a sensible change.  Don't we want the same for the
> other failure handler, though.  Capture any message from it and
> append it to the output of the process that just finished, or
> something?

Ah, that is already done.  Scratch that "don't we want" part.

>
> By the way, I understand that these two are solely for early review
> and I'll be getting them as either new patches or part of updated
> patches in the next reroll (i.e. you are not expecting me to split
> these apart and do "rebase -i" for you to the last-posted version).
> Asking only to make sure we are on the same wavelength.
>
> Thanks.
>
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  run-command.c | 43 ++++++++++++++++++++++++++++++-------------
>>  run-command.h |  1 +
>>  2 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 494e1f8..0d22291 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -907,6 +907,7 @@ void default_start_failure(void *data,
>>  
>>  void default_return_value(void *data,
>>  			  struct child_process *cp,
>> +			  struct strbuf *err,
>>  			  int result)
>>  {
>>  	int i;
>> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
>>  			"output will be degraded");
>>  }
>>  
>> -/* returns 1 if a process was started, 0 otherwise */
>> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
>>  static int pp_start_one(struct parallel_processes *pp)
>>  {
>>  	int i;
>> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
>>  	if (!pp->get_next_task(pp->data,
>>  			       &pp->children[i].process,
>>  			       &pp->children[i].err))
>> -		return 1;
>> +		return 0;
>>  
>> -	if (start_command(&pp->children[i].process))
>> +	if (start_command(&pp->children[i].process)) {
>>  		pp->start_failure(pp->data,
>>  				  &pp->children[i].process,
>>  				  &pp->children[i].err);
>> +		strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
>> +		strbuf_reset(&pp->children[i].err);
>> +		return -1;
>> +	}
>>  
>>  	set_nonblocking(pp->children[i].process.err);
>>  
>>  	pp->nr_processes++;
>>  	pp->children[i].in_use = 1;
>>  	pp->pfd[i].fd = pp->children[i].process.err;
>> -	return 0;
>> +	return 1;
>>  }
>>  
>> -static void pp_buffer_stderr(struct parallel_processes *pp)
>> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>>  {
>>  	int i;
>>  
>> -	while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
>> +	while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
>>  		if (errno == EINTR)
>>  			continue;
>>  		pp_cleanup(pp);
>> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)
>>  			error("waitpid is confused (%s)",
>>  			      pp->children[i].process.argv[0]);
>>  
>> -		pp->return_value(pp->data, &pp->children[i].process, code);
>> +		pp->return_value(pp->data, &pp->children[i].process,
>> +				 &pp->children[i].err, code);
>>  
>>  		argv_array_clear(&pp->children[i].process.args);
>>  		argv_array_clear(&pp->children[i].process.env_array);
>> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
>>  			   return_value_fn return_value)
>>  {
>>  	struct parallel_processes pp;
>> -	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>>  
>> +	pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>>  	while (1) {
>> -		while (pp.nr_processes < pp.max_processes &&
>> -		       !pp_start_one(&pp))
>> -			; /* nothing */
>> -		if (!pp.nr_processes)
>> +		int no_more_task, cnt;
>> +		int output_timeout = 100;
>> +		int spawn_cap = 4;
>> +
>> +		for (cnt = spawn_cap, no_more_task = 0;
>> +		     cnt && pp.nr_processes < pp.max_processes;
>> +		     cnt--) {
>> +			if (!pp_start_one(&pp)) {
>> +				no_more_task = 1;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (no_more_task && !pp.nr_processes)
>>  			break;
>> -		pp_buffer_stderr(&pp);
>> +		pp_buffer_stderr(&pp, output_timeout);
>> +
>>  		pp_output(&pp);
>>  		pp_collect_finished(&pp);
>>  	}
>> diff --git a/run-command.h b/run-command.h
>> index 3807fd1..f7035cb 100644
>> --- a/run-command.h
>> +++ b/run-command.h
>> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,
>>  
>>  typedef void (*return_value_fn)(void *data,
>>  				struct child_process *cp,
>> +				struct strbuf *err,
>>  				int result);
>>  
>>  /**

Re: [PATCH 1/2] SQUASH???

[Stefan Beller]

On Thu, Sep 24, 2015 at 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>  * If you do not die() in start_failure_fn or return_value_fn, you
>>    don't want to write to stderr directly as you would destroy the fine
>>    ordering of the processes output. So make the err strbuf available in
>>    both these functions, and make sure the strbuf is appended to the
>>    buffered output in both cases
>
> I think that is a sensible change.  Don't we want the same for the
> other failure handler, though.  Capture any message from it and
> append it to the output of the process that just finished, or
> something?
>
> By the way, I understand that these two are solely for early review
> and I'll be getting them as either new patches or part of updated
> patches in the next reroll (i.e. you are not expecting me to split
> these apart and do "rebase -i" for you to the last-posted version).
> Asking only to make sure we are on the same wavelength.

Sure. I just wanted to point out the details instead of resending the series.
I'll do a resend later today, hoping to get all issues addressed.

Thanks,
Stefan


>
> Thanks.
>
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  run-command.c | 43 ++++++++++++++++++++++++++++++-------------
>>  run-command.h |  1 +
>>  2 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 494e1f8..0d22291 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -907,6 +907,7 @@ void default_start_failure(void *data,
>>
>>  void default_return_value(void *data,
>>                         struct child_process *cp,
>> +                       struct strbuf *err,
>>                         int result)
>>  {
>>       int i;
>> @@ -977,7 +978,7 @@ static void set_nonblocking(int fd)
>>                       "output will be degraded");
>>  }
>>
>> -/* returns 1 if a process was started, 0 otherwise */
>> +/* return 0 if get_next_task() ran out of things to do, non-zero otherwise */
>>  static int pp_start_one(struct parallel_processes *pp)
>>  {
>>       int i;
>> @@ -991,26 +992,30 @@ static int pp_start_one(struct parallel_processes *pp)
>>       if (!pp->get_next_task(pp->data,
>>                              &pp->children[i].process,
>>                              &pp->children[i].err))
>> -             return 1;
>> +             return 0;
>>
>> -     if (start_command(&pp->children[i].process))
>> +     if (start_command(&pp->children[i].process)) {
>>               pp->start_failure(pp->data,
>>                                 &pp->children[i].process,
>>                                 &pp->children[i].err);
>> +             strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
>> +             strbuf_reset(&pp->children[i].err);
>> +             return -1;
>> +     }
>>
>>       set_nonblocking(pp->children[i].process.err);
>>
>>       pp->nr_processes++;
>>       pp->children[i].in_use = 1;
>>       pp->pfd[i].fd = pp->children[i].process.err;
>> -     return 0;
>> +     return 1;
>>  }
>>
>> -static void pp_buffer_stderr(struct parallel_processes *pp)
>> +static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>>  {
>>       int i;
>>
>> -     while ((i = poll(pp->pfd, pp->max_processes, 100)) < 0) {
>> +     while ((i = poll(pp->pfd, pp->max_processes, output_timeout)) < 0) {
>>               if (errno == EINTR)
>>                       continue;
>>               pp_cleanup(pp);
>> @@ -1069,7 +1074,8 @@ static void pp_collect_finished(struct parallel_processes *pp)
>>                       error("waitpid is confused (%s)",
>>                             pp->children[i].process.argv[0]);
>>
>> -             pp->return_value(pp->data, &pp->children[i].process, code);
>> +             pp->return_value(pp->data, &pp->children[i].process,
>> +                              &pp->children[i].err, code);
>>
>>               argv_array_clear(&pp->children[i].process.args);
>>               argv_array_clear(&pp->children[i].process.env_array);
>> @@ -1111,15 +1117,26 @@ int run_processes_parallel(int n, void *data,
>>                          return_value_fn return_value)
>>  {
>>       struct parallel_processes pp;
>> -     pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>>
>> +     pp_init(&pp, n, data, get_next_task, start_failure, return_value);
>>       while (1) {
>> -             while (pp.nr_processes < pp.max_processes &&
>> -                    !pp_start_one(&pp))
>> -                     ; /* nothing */
>> -             if (!pp.nr_processes)
>> +             int no_more_task, cnt;
>> +             int output_timeout = 100;
>> +             int spawn_cap = 4;
>> +
>> +             for (cnt = spawn_cap, no_more_task = 0;
>> +                  cnt && pp.nr_processes < pp.max_processes;
>> +                  cnt--) {
>> +                     if (!pp_start_one(&pp)) {
>> +                             no_more_task = 1;
>> +                             break;
>> +                     }
>> +             }
>> +
>> +             if (no_more_task && !pp.nr_processes)
>>                       break;
>> -             pp_buffer_stderr(&pp);
>> +             pp_buffer_stderr(&pp, output_timeout);
>> +
>>               pp_output(&pp);
>>               pp_collect_finished(&pp);
>>       }
>> diff --git a/run-command.h b/run-command.h
>> index 3807fd1..f7035cb 100644
>> --- a/run-command.h
>> +++ b/run-command.h
>> @@ -138,6 +138,7 @@ typedef void (*start_failure_fn)(void *data,
>>
>>  typedef void (*return_value_fn)(void *data,
>>                               struct child_process *cp,
>> +                             struct strbuf *err,
>>                               int result);
>>
>>  /**

Re: [PATCH 1/2] SQUASH???

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Sure. I just wanted to point out the details instead of resending the series.
> I'll do a resend later today, hoping to get all issues addressed.

Thanks.  For something this small, unless there are many small
pieces that need to be picked apart, I do not terribly mind to do
the squashing myself (after all that is what I do every day, prepare
a squash while queuing, do the integration testing and send the
squash out to make sure the original author agrees with it).

I just wanted to make sure that we won't be in a situation where
I am waiting forever expecting you to reroll while you are expecting
me to do the squashing, resulting in no progress getting made until
either one of us starts wondering what is going on.