msysgit git-submodule: "Unable to fetch in submodule path ..."

Hi!

This is with msysGit 1.6.3.2.1299.gee46c

I am having some problem when checking out a large project with many 
submodules. After cloning the superproject, doing a git submodule --init 
will fail with a "Unable to fetch in submodule path '...'" for one specific 
module (does this on all machines that we have tried it on). I cannot see 
any error messages from git-fetch itself:

   [...]
   Initialized empty Git repository in c:/Users/peter/src/foo/bar/modules/foo/.git/
   remote: Counting objects: 14752, done.
   remote: Compressing objects: 100% (5036/5036), done.
   remote: Total 14752 (delta 9278), reused 14752 (delta 9278)Receiving objects:  9

   Receiving objects: 100% (14752/14752), 5.07 MiB | 3501 KiB/s, done.
   Resolving deltas: 100% (9278/9278), done.
   Unable to fetch in submodule path 'modules/foo'

If I enter the modules/foo manually and enter

   git reset --hard

or similar, everything works fine, and I do have the complete history.

Looking at the code for git-submodule, it seems to suggest that git 
submodule is calling git-fetch without parameters, and checking the return 
value from it. It does, as indicated above, not seem to return any errors.

I tried adding a "-v -v" to the git-fetch command line in git-submodule, and I 
see that it does terminate early. With the other modules, I get a list of 
all the cloned branches and tags. But for this module, it stops as above. I 
get some additional debug output, but can't quite say it helps me much:

   Initialized empty Git repository in c:/Users/peter/src/foo/baz/modules/foo/.git/
   remote: Counting objects: 14752, done.
   remote: Compressing objects: 100% (5036/5036), done.
   emote: Total 14752 (delta 9278), reused 14752 (delta 9278)
   Receiving objects: 100% (14752/14752), 5.07 MiB | 3629 KiB/s, done.
   Resolving deltas: 100% (9278/9278), done.
   Server supports multi_ack
   Server supports side-band-64k
   Server supports ofs-delta
   Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete
   Unable to fetch in submodule path 'modules/foo'

The string "Marking %s as complete" seems to stem from 
mark_recent_complete_commits() in builtin-fetch-pack.c. The other messages 
seems to stem from do_fetch_pack() in the same file, so it gets there and 
not further. I cannot seem to find any exit point.

Does anyone know how to continue debugging, or know what might be going wrong?

-- 
\\// Peter - http://www.softwolves.pp.se/

Peter Krefting:

 Receiving objects: 100% (14752/14752), 5.07 MiB | 3629 KiB/s, done.
 Resolving deltas: 100% (9278/9278), done.
 Server supports multi_ack
 Server supports side-band-64k
 Server supports ofs-delta
 Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete
 Unable to fetch in submodule path 'modules/foo'

I added some extra debugging output to the code in builtin-fetch.c and 
builtin-fetch-pack.c, and ended up with this:

   Receiving objects: 100% (14752/14752), 5.07 MiB | 3383 KiB/s, done.
   Resolving deltas: 100% (9278/9278), done.
   cmd_fetch(): calling do_fetch()
   do_fetch(): calling get_ref_map()
   do_fetch(): get_ref_map() done
   do_fetch(): check_not_current_branch() done
   do_fetch(): read_ref() loop done
   fetch_refs(): quickfetch() returned -10001
   Server supports multi_ack
   Server supports side-band-64k
   Server supports ofs-delta
   Entering everything_local()
   everything_local() after 1st ref loop
   Marking 76b96bfecc0d47013dd1fca1a555f12074eca814 as complete
   everything_local() after mark_recent_complete_commits()
   everything_local() after 2nd ref loop
   everything_local() after filter_refs()
   everything_local() done with retval = 1
   everything_local() returned true
   do_fetch_pack() done
   fetch_pack(): calling reprepare_packed_git()
   fetch_pack(): done
   fetch_refs(): transport_fetch_refs() returned -1
   do_fetch(): fetch_refs(transport, ref_map) returned non-zero
   cmd_fetch(): do_fetch() return with exit_code = 1
   Unable to fetch in submodule path 'modules/foo'

This seems to indicate that fetch_refs() seems to think that the fetch 
(which is done over ssh) fails, whereas the regular trace output ("Receiving 
objects", etc.) indicates that it succeeds.

Is there anything obvious that I should have a look at here?

My next step otherwise is adding trace output to the transport_fetch_refs() 
and whatever it is it calls and that calls the code in builtin-fetch-pack.c. 
Anywhere in particular I should have a look at?

-- 
\\// Peter - http://www.softwolves.pp.se/

quickfetch() calls rev-list to check whether the objects we are about to
fetch are already present in the repo (if so, we can skip the object fetch).
However, when there are many (~1000) refs to be fetched, the rev-list
command line grows larger than the maximum command line size on some systems
(32K in Windows). This causes rev-list to fail, making quickfetch() return
non-zero, which unnecessarily triggers the transport machinery. This somehow
causes fetch to fail with an exit code.

By using the --stdin option to rev-list (and feeding the object list to its
standard input), we prevent the overflow of the rev-list command line,
which causes quickfetch(), and subsequently the overall fetch, to succeed.

However, using rev-list --stdin is not entirely straightforward: rev-list
terminates immediately when encountering an unknown object, which can
trigger SIGPIPE if we are still writing object's to its standard input.
We therefore ignore SIGPIPE so that the fetch process is not terminated.

Signed-off-by: Johan Herland <redacted>
Tested-by: Peter Krefting <redacted>
---

Hi,

It seems the git fetch failure described by Peter earlier in this thread
is caused by a long ref list overflowing the command line buffer on
Windows (32K I am told), when calling rev-list from quickfetch(). AFAICS
this overflow will trigger on any fetch from msysgit with more than ~800
(32K / 40) refs.

According to Peter, this patch fixes the submodule update failure.

CC-ing Shawn since he is the original author of quickfetch().


Have fun! :)

...Johan

 builtin-fetch.c |   63 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..52febc6 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -400,14 +400,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 /*
  * We would want to bypass the object transfer altogether if
- * everything we are going to fetch already exists and connected
+ * everything we are going to fetch already exists and is connected
  * locally.
  *
- * The refs we are going to fetch are in to_fetch (nr_heads in
- * total).  If running
+ * The refs we are going to fetch are in ref_map.  If running
  *
- *  $ git rev-list --objects to_fetch[0] to_fetch[1] ... --not --all
+ *  $ git rev-list --objects --stdin --not --all
  *
+ * (feeding all the refs in ref_map on its standard input)
  * does not error out, that means everything reachable from the
  * refs we are going to fetch exists and is connected to some of
  * our existing refs.

@@ -416,9 +416,10 @@ static int quickfetch(struct ref *ref_map)
 {
 	struct child_process revlist;
 	struct ref *ref;
-	char **argv;
-	int i, err;
-
+	int err;
+	const char *argv[] = {
+		"rev-list", "--quiet", "--objects", "--stdin", "--not", "--all", NULL
+	};
 	/*
 	 * If we are deepening a shallow clone we already have these
 	 * objects reachable.  Running rev-list here will return with

@@ -429,34 +430,40 @@ static int quickfetch(struct ref *ref_map)
 	if (depth)
 		return -1;
 
-	for (i = 0, ref = ref_map; ref; ref = ref->next)
-		i++;
-	if (!i)
+	if (!ref_map)
 		return 0;
 
-	argv = xmalloc(sizeof(*argv) * (i + 6));
-	i = 0;
-	argv[i++] = xstrdup("rev-list");
-	argv[i++] = xstrdup("--quiet");
-	argv[i++] = xstrdup("--objects");
-	for (ref = ref_map; ref; ref = ref->next)
-		argv[i++] = xstrdup(sha1_to_hex(ref->old_sha1));
-	argv[i++] = xstrdup("--not");
-	argv[i++] = xstrdup("--all");
-	argv[i++] = NULL;
-
 	memset(&revlist, 0, sizeof(revlist));
-	revlist.argv = (const char**)argv;
+	revlist.argv = argv;
 	revlist.git_cmd = 1;
-	revlist.no_stdin = 1;
 	revlist.no_stdout = 1;
 	revlist.no_stderr = 1;
-	err = run_command(&revlist);
+	revlist.in = -1;
+
+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */
+	signal(SIGPIPE, SIG_IGN);
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			error("failed write to rev-list");
+			err = errno;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin");
+		err = errno;
+	}
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)

-- 
1.6.3.2.316.gda4e

Johan Herland schrieb:

+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */

Under the conditions you describe here...

+	signal(SIGPIPE, SIG_IGN);

... and SIGPIPE being ignored...

+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
+		    write_in_full(revlist.in, "\n", 1) < 0) {
+			error("failed write to rev-list");
+			err = errno;

... don't you get this error message with errno set to EPIPE? Previously,
there was no error message.

-- Hannes

On Wednesday 08 July 2009, Johannes Sixt wrote:

Johan Herland schrieb:

quoted

+	/* If rev-list --stdin encounters an unknown commit, it
terminates, +	 * which will cause SIGPIPE in the write loop below.
*/

Under the conditions you describe here...

quoted

+	signal(SIGPIPE, SIG_IGN);

... and SIGPIPE being ignored...

quoted

+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}

-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	for (ref = ref_map; ref; ref = ref->next) {
+		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <
0 || +		    write_in_full(revlist.in, "\n", 1) < 0) {
+			error("failed write to rev-list");
+			err = errno;

... don't you get this error message with errno set to EPIPE?
Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

 	if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
 	    write_in_full(revlist.in, "\n", 1) < 0) {
-		error("failed write to rev-list");
-		err = errno;
+		if (errno != EPIPE) {
+			error("failed write to rev-list");
+			err = errno;
+		}
 		break;
 	}

Maybe I need to do something to the close() call as well? What happens on close() after EPIPE?


Thanks,

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Wed, Jul 8, 2009 at 18:01, Johan Herland[off-list ref] wrote:

On Wednesday 08 July 2009, Johannes Sixt wrote:

quoted

... don't you get this error message with errno set to EPIPE?
Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

       if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
           write_in_full(revlist.in, "\n", 1) < 0) {
-               error("failed write to rev-list");
-               err = errno;
+               if (errno != EPIPE) {
+                       error("failed write to rev-list");
+                       err = errno;

You'll loose errno this way: error() does not save it.

On Thursday 09 July 2009, Alex Riesen wrote:

On Wed, Jul 8, 2009 at 18:01, Johan Herland[off-list ref] wrote:

quoted

On Wednesday 08 July 2009, Johannes Sixt wrote:

quoted

... don't you get this error message with errno set to EPIPE?
Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

       if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <
0 || write_in_full(revlist.in, "\n", 1) < 0) {
-               error("failed write to rev-list");
-               err = errno;
+               if (errno != EPIPE) {
+                       error("failed write to rev-list");
+                       err = errno;

You'll loose errno this way: error() does not save it.

Not sure what you mean here. Should I move "err = errno;" outside the 
innermost "if"?

From my POV, if errno != EPIPE, we save it into err, and return that 

(overridden by finish_command()'s return value, if non-zero). If errno == 
EPIPE, we're not interested in saving it, because we expect finish_command() 
to return non-zero in any case.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Thu, Jul 9, 2009 at 10:37, Johan Herland[off-list ref] wrote:

On Thursday 09 July 2009, Alex Riesen wrote:

quoted

On Wed, Jul 8, 2009 at 18:01, Johan Herland[off-list ref] wrote:

quoted

On Wednesday 08 July 2009, Johannes Sixt wrote:

quoted

... don't you get this error message with errno set to EPIPE?
Previously, there was no error message.

Indeed, you are correct. I guess the following should be added to the
patch:

       if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) <
0 || write_in_full(revlist.in, "\n", 1) < 0) {
-               error("failed write to rev-list");
-               err = errno;
+               if (errno != EPIPE) {
+                       error("failed write to rev-list");
+                       err = errno;

You'll loose errno this way: error() does not save it.

Not sure what you mean here. Should I move "err = errno;" outside the
innermost "if"?

put it before error("failed write to rev-list"); or even before the
"if (err != EPIPE)".
Otherwise it is 0 after fprintf to stderr (which is the error() call).

From my POV, if errno != EPIPE, we save it into err, and return that
(overridden by finish_command()'s return value, if non-zero). If errno ==
EPIPE, we're not interested in saving it, because we expect finish_command()
to return non-zero in any case.

And you think this expectation makes the code simpler to understand?