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

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

[Peter Krefting]

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/

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

[Peter Krefting]

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/

[PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

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 <johan@herland.net>
Tested-by: Peter Krefting <peter@softwolves.pp.se>
---

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

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

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

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Wednesday 08 July 2009, Johannes Sixt wrote:
> 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.

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, <johan@herland.net>
www.herland.net

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

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

You should be OK (you could try this).

-- >8 --
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <signal.h>
#include <string.h>

int main(int ac, char **av)
{
	int pipefd[2];
	int child;

	if (pipe(pipefd) < 0) {
		fprintf(stderr, "pipe failed: %s\n", strerror(errno));
		exit(1);
	}
	child = fork();
	if (child < 0) {
		fprintf(stderr, "fork failed: %s\n", strerror(errno));
		exit(1);
	} else if (child == 0) {
		char buf[1024];
		ssize_t sz;

		/* the child reads from the parent but does not talk back */
		close(pipefd[1]);

		/* emulate reading a bit, then dying without cleaning up */
		sz = read(pipefd[0], buf, sizeof(buf));
		fprintf(stderr, "read %lu bytes, and will die\n",
			(unsigned long) sz);
		exit(1);
	} else {
		const char data[] = "abcdefg";
		size_t len = sizeof(data);
		size_t written = 0;

		/* the parent writes to the child but does not listen */
		close(pipefd[0]);

		/* we will rite to the pipe even after the child is gone */
		signal(SIGPIPE, SIG_IGN);

		/* write, write, write, ... */
		while (1) {
			ssize_t sz = write(pipefd[1], data, len);
			if (sz < 0) {
				/* error */
				fprintf(stderr,
					"write failed (%s) after writing"
					" %lu bytes\n",
					strerror(errno),
					(unsigned long) written);
				break;
			}
			written += sz;
		}
		errno = 0;
		if (close(pipefd[1]))
			fprintf(stderr, "close failed: %s\n", strerror(errno));
		else
			fprintf(stderr, "close ok\n");
	}
	exit(0);
}

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Wednesday 08 July 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > Maybe I need to do something to the close() call as well? What happens
> > on close() after EPIPE?
>
> You should be OK (you could try this).
>
> -- >8 --

Thanks! The programs works well on my Linux box (close() succeeds), but it 
does not run at all in Windows/MSYS (lacks pipe() and fork()).

Does anybody with Windows/MSYS experience know how this scenario (write() to 
a terminated process, followed by close()) would play out in msysGit?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Alex Riesen]

On Thu, Jul 9, 2009 at 10:43, Johan Herland<johan@herland.net> wrote:
> On Wednesday 08 July 2009, Junio C Hamano wrote:
>> Johan Herland <johan@herland.net> writes:
>> > Maybe I need to do something to the close() call as well? What happens
>> > on close() after EPIPE?
>
> Does anybody with Windows/MSYS experience know how this scenario (write() to
> a terminated process, followed by close()) would play out in msysGit?
>

It fails with ERROR_BROKEN_PIPE. See MSDN for WriteFile:

 http://msdn.microsoft.com/en-us/library/aa365747%28VS.85%29.aspx

(look for the error above).

Well, sometimes it just fails, so you can hardly use the error code to detect
if the other process is truly gone or something broke in Windows.

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> Does anybody with Windows/MSYS experience know how this scenario (write() to 
> a terminated process, followed by close()) would play out in msysGit?

The first write() sometimes fails with EPIPE, otherwise it fails with
EINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE
is irrelevant because it is unknown to Windows.

There's precedent already in write_or_die.c. You should not write the
error message for both EPIPE and EINVAL.

-- Hannes

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Thursday 09 July 2009, Johannes Sixt wrote:
> Johan Herland schrieb:
> > Does anybody with Windows/MSYS experience know how this scenario
> > (write() to a terminated process, followed by close()) would play out
> > in msysGit?
>
> The first write() sometimes fails with EPIPE, otherwise it fails with
> EINVAL. All subsequent write()s fail with EINVAL. The setting of SIGPIPE
> is irrelevant because it is unknown to Windows.
>
> There's precedent already in write_or_die.c. You should not write the
> error message for both EPIPE and EINVAL.

Thanks, but what about the subsequent close()? Will it fail with EINVAL? 
EBADF? or will is succeed (like on Linux)?

I will send an updated patch with all fixes, as soon as I know what to do 
about close().


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> Thanks, but what about the subsequent close()? Will it fail with EINVAL? 
> EBADF? or will is succeed (like on Linux)?

I guess it succeeds, but I don't have a quick test. Send your patch, and
we'll see ;)

-- Hannes

[PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

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 <johan@herland.net>
Improved-by: Johannes Sixt <j.sixt@viscovery.net>
Improved-by: Alex Riesen <raa.lkml@gmail.com>
Tested-by: Peter Krefting <peter@softwolves.pp.se>
---
 builtin-fetch.c |   62 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..20bcbdd 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,8 +416,9 @@ 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
@@ -429,34 +430,41 @@ 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) {
+			err = errno;
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list");
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		err = errno;
+		error("failed to close rev-list's stdin");
+	}
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
-- 
1.6.3.rc0.1.gf800

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> 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.

I removed the "signal(SIGPIPE, SIG_IGN)", but the test suite still passes.
IOW, there is no test case that has the configuration that you describe
here. Would you please add such a test (perhaps in t5502)? It would also
help me verify the patch works as intended on Windows.

> Signed-off-by: Johan Herland <johan@herland.net>
> Improved-by: Johannes Sixt <j.sixt@viscovery.net>

Please make this <j6t@kdbg.org> despite the email address I'm using right now.

> Improved-by: Alex Riesen <raa.lkml@gmail.com>
> Tested-by: Peter Krefting <peter@softwolves.pp.se>

> +	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) {
> +			err = errno;
> +			if (err != EPIPE && err != EINVAL)
> +				error("failed write to rev-list");
> +			break;
> +		}
> +	}
> +
> +	if (close(revlist.in)) {
> +		err = errno;
> +		error("failed to close rev-list's stdin");
> +	}
> +	return finish_command(&revlist) || err;

The call site of quickfetch() is not interested in the errno, only on
whether the return value is non-zero: You can just assign -1 to err
(that's our convention for failure). OTOH, it would be helpful to include
strerror(errno) in the error message.

Shouldn't you reset signal(SIGPIPE) to its previous value?

-- Hannes

[PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

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 temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <johan@herland.net>
Improved-by: Johannes Sixt <j6t@kdbg.org>
Improved-by: Alex Riesen <raa.lkml@gmail.com>
Tested-by: Peter Krefting <peter@softwolves.pp.se>
---

On Thursday 09 July 2009, Johannes Sixt wrote:
> Would you please add such a test (perhaps in t5502)? It
> would also help me verify the patch works as intended on Windows.

Done (although somewhat naively). I don't have an msysgit setup to test
this, but faking the failure condition in quickfetch() (return -1 if
#refs > 800) does trigger the selftest (the second git fetch fails).

I could add a separate pre-patch introducing the selftest with
test_expect_failure, but that would only confuse non-msysgit users
where the test succeeds both before and after the fix.

> Please make this <j6t@kdbg.org> despite the email address I'm using right
> now.

Ok.

> The call site of quickfetch() is not interested in the errno, only on
> whether the return value is non-zero: You can just assign -1 to err
> (that's our convention for failure). OTOH, it would be helpful to include
> strerror(errno) in the error message.

Fixed.

> Shouldn't you reset signal(SIGPIPE) to its previous value?

Done (provided that the sigchain_push/pop infrastructure works the way
I expect).

Thanks a lot for your review and suggestions.


Have fun! :)

...Johan


 builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------
 t/t5502-quickfetch.sh |   20 +++++++++++++++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..2e3c609 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,8 +416,9 @@ 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
@@ -429,34 +430,44 @@ 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. */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		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) {
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh
@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done
-- 
1.6.3.rc0.1.gf800

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> On Thursday 09 July 2009, Johannes Sixt wrote:
>> Shouldn't you reset signal(SIGPIPE) to its previous value?
> 
> Done (provided that the sigchain_push/pop infrastructure works the way
> I expect).

I'm not sure, either. Peff?

> +test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
> +
> +	git gc &&
> +	head=$(git rev-parse HEAD) &&
> +	branchprefix="$head refs/heads/branch" &&
> +	for i in 0 1 2 3 4 5 6 7 8 9; do
> +		for j in 0 1 2 3 4 5 6 7 8 9; do
> +			for k in 0 1 2 3 4 5 6 7 8 9; do
> +				echo "$branchprefix$i$j$k" >> .git/packed-refs
> +			done
> +		done
> +	done &&
> +	(
> +		cd cloned &&
> +		git fetch &&
> +		git fetch
> +	)
> +
> +'

This test fails on Windows without the code change and passes with the
code change. So, it's a good test.

But actually I meant you to make a test that triggers the SIGPIPE that
would kill git-fetch if it were not ignored. This one doesn't trigger it,
either.

-- Hannes

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Jeff King]

On Thu, Jul 09, 2009 at 04:21:09PM +0200, Johannes Sixt wrote:

> Johan Herland schrieb:
> > On Thursday 09 July 2009, Johannes Sixt wrote:
> >> Shouldn't you reset signal(SIGPIPE) to its previous value?
> > 
> > Done (provided that the sigchain_push/pop infrastructure works the way
> > I expect).
> 
> I'm not sure, either. Peff?

I don't think I ever tried explicitly pushing SIG_IGN, but the
infrastructure was designed so that it would Just Work. So yes, I think
it's right, but you may want to test it. :)

That being said, in the patch in question there is an early return after
the push that misses the corresponding pop. That should be fixed.

-Peff

[PATCH v4] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

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 temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <johan@herland.net>
Improved-by: Johannes Sixt <j6t@kdbg.org>
Improved-by: Alex Riesen <raa.lkml@gmail.com>
Tested-by: Peter Krefting <peter@softwolves.pp.se>
---

On Thursday 09 July 2009, Jeff King wrote:
> That being said, in the patch in question there is an early return after
> the push that misses the corresponding pop. That should be fixed.

Thanks, fixed.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

 builtin-fetch.c       |   65 ++++++++++++++++++++++++++++--------------------
 t/t5502-quickfetch.sh |   20 +++++++++++++++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..34ba878 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,8 +416,9 @@ 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
@@ -429,34 +430,44 @@ 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;
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
+
+	/* If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below. */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	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) {
+			if (err != EPIPE && err != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh
@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done
-- 
1.6.3.rc0.1.gf800

Re: [PATCH v4] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

With this fixup the patch is good, I think.

-- Hannes

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 34ba878..7547631 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -446,14 +446,16 @@ static int quickfetch(struct ref *ref_map)
 		return err;
 	}

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

 	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) {
-			if (err != EPIPE && err != EINVAL)
+			if (errno != EPIPE && errno != EINVAL)
 				error("failed write to rev-list: %s", strerror(errno));
 			err = -1;
 			break;

[PATCH v5] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

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 temporarily ignore SIGPIPE so that the fetch process is not
terminated.

The patch also contains a testcase to verify the fix (note that before
the patch, the testcase would only fail on msysGit).

Signed-off-by: Johan Herland <johan@herland.net>
Improved-by: Johannes Sixt <j6t@kdbg.org>
Improved-by: Alex Riesen <raa.lkml@gmail.com>
Tested-by: Peter Krefting <peter@softwolves.pp.se>
---

On Thursday 09 July 2009, Johannes Sixt wrote:
> With this fixup the patch is good, I think.

Again, thanks for all your help!

Here's the final (*crossing fingers*) iteration of the patch.


Have fun! :)

...Johan


 builtin-fetch.c       |   67 +++++++++++++++++++++++++++++-------------------
 t/t5502-quickfetch.sh |   20 ++++++++++++++
 2 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cd5eb9a..817dd6b 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,8 +416,9 @@ 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
@@ -429,34 +430,46 @@ 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;
+
+	err = start_command(&revlist);
+	if (err) {
+		error("could not run rev-list");
+		return err;
+	}
+
+	/*
+	 * If rev-list --stdin encounters an unknown commit, it terminates,
+	 * which will cause SIGPIPE in the write loop below.
+	 */
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	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) {
+			if (errno != EPIPE && errno != EINVAL)
+				error("failed write to rev-list: %s", strerror(errno));
+			err = -1;
+			break;
+		}
+	}
+
+	if (close(revlist.in)) {
+		error("failed to close rev-list's stdin: %s", strerror(errno));
+		err = -1;
+	}
+
+	sigchain_pop(SIGPIPE);
 
-	for (i = 0; argv[i]; i++)
-		free(argv[i]);
-	free(argv);
-	return err;
+	return finish_command(&revlist) || err;
 }
 
 static int fetch_refs(struct transport *transport, struct ref *ref_map)
diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index 16eadd6..1037a72 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh
@@ -119,4 +119,24 @@ test_expect_success 'quickfetch should not copy from alternate' '
 
 '
 
+test_expect_success 'quickfetch should handle ~1000 refs (on Windows)' '
+
+	git gc &&
+	head=$(git rev-parse HEAD) &&
+	branchprefix="$head refs/heads/branch" &&
+	for i in 0 1 2 3 4 5 6 7 8 9; do
+		for j in 0 1 2 3 4 5 6 7 8 9; do
+			for k in 0 1 2 3 4 5 6 7 8 9; do
+				echo "$branchprefix$i$j$k" >> .git/packed-refs
+			done
+		done
+	done &&
+	(
+		cd cloned &&
+		git fetch &&
+		git fetch
+	)
+
+'
+
 test_done
-- 
1.6.3.rc0.1.gf800

Re: [PATCH v5] quickfetch(): Prevent overflow of the rev-list command line

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

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

I feel uneasy with that "somehow" at the end of the first paragraph, but
nevertheless this is the right thing to do.  Since it is a very isolated
change, I'd queue this directly on 'master' and see if anybody notices a
breakage, as it would be relatively pain-free to revert if it turns out to
be necessary.

Thanks.

Re: [PATCH v5] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Saturday 11 July 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > 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.
>
> I feel uneasy with that "somehow" at the end of the first paragraph, but
> nevertheless this is the right thing to do.

Yes, it feels wrong that transport_fetch_refs() returns error when there are 
no objects to be fetched. After all, the quickfetch() routine is only meant 
to be an optimization (to skip the object fetching when not needed). Only if 
quickfetch() returned a false positive (indicating that all objects are 
present when they're really not) would I expect it to have adverse effects 
on the result of the fetch. But even then, as you indicate, fixing 
quickfetch() itself is the right thing to do, and looking into 
transport_fetch_refs() is a separate issue.

> Since it is a very isolated
> change, I'd queue this directly on 'master' and see if anybody notices a
> breakage, as it would be relatively pain-free to revert if it turns out
> to be necessary.

Thanks.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Thursday 09 July 2009, Johannes Sixt wrote:
> But actually I meant you to make a test that triggers the SIGPIPE that
> would kill git-fetch if it were not ignored. This one doesn't trigger it,
> either.

AFAIU from earlier in this thread (and a mail from Peter linking to 
http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on 
Windows, thus ignoring SIGPIPE is not needed for the fix per se. However, as 
a side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX 
platforms), so although it never triggers on Windows, it's still needed.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> On Thursday 09 July 2009, Johannes Sixt wrote:
>> But actually I meant you to make a test that triggers the SIGPIPE that
>> would kill git-fetch if it were not ignored. This one doesn't trigger it,
>> either.
> 
> AFAIU from earlier in this thread (and a mail from Peter linking to 
> http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers on 
> Windows, thus ignoring SIGPIPE is not needed for the fix per se. However, as 
> a side-effect of the fix, we may now get SIGPIPE on Linux (and other POSIX 
> platforms), so although it never triggers on Windows, it's still needed.

I know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)
and run the test suite. There is not a single failure. IOW, we don't have
a single test case that verifies that the signal(SIGPIPE, SIG_IGN) is
needed. I would like to have that test case, and you seem to know how to
construct it (otherwise there wouldn't be so much buzz about it).

-- Hannes

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Thursday 09 July 2009, Johannes Sixt wrote:
> Johan Herland schrieb:
> > On Thursday 09 July 2009, Johannes Sixt wrote:
> >> But actually I meant you to make a test that triggers the SIGPIPE that
> >> would kill git-fetch if it were not ignored. This one doesn't trigger
> >> it, either.
> >
> > AFAIU from earlier in this thread (and a mail from Peter linking to
> > http://markmail.org/message/dbgdj4csafen65ye), SIGPIPE _never_ triggers
> > on Windows, thus ignoring SIGPIPE is not needed for the fix per se.
> > However, as a side-effect of the fix, we may now get SIGPIPE on Linux
> > (and other POSIX platforms), so although it never triggers on Windows,
> > it's still needed.
>
> I know that, of course. But try this: Remove the signal(SIGPIPE, SIG_IGN)
> and run the test suite. There is not a single failure.

That's not what I'm seeing. When I don't ignore the signal, the testsuite 
fails intermittently for me (on Linux). I see the following tests fail:

- t3409-rebase-preserve-merges.sh (subtest #2)
- t5503-tagfollow.sh (subtests #4, #6, #7)
- t5505-remote.sh (subtests #10, #12, #14 - #20, #27)
- t5510-fetch.sh (subtest #6 or #25)
- probably more (I seldom get this far...)

I assume the intermittent failures are caused by git rev-list sometimes 
terminate before git fetch is finished writing objects to its standard input 
(because of scheduling differences).

When i enable the signal handling, all selftests pass every time.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH v3] quickfetch(): Prevent overflow of the rev-list command line

[Johannes Sixt]

Johan Herland schrieb:
> On Thursday 09 July 2009, Johannes Sixt wrote:
>> But try this: Remove the signal(SIGPIPE, SIG_IGN)
>> and run the test suite. There is not a single failure.
> 
> That's not what I'm seeing. When I don't ignore the signal, the testsuite 
> fails intermittently for me (on Linux). I see the following tests fail:
> 
> - t3409-rebase-preserve-merges.sh (subtest #2)
> - t5503-tagfollow.sh (subtests #4, #6, #7)
> - t5505-remote.sh (subtests #10, #12, #14 - #20, #27)
> - t5510-fetch.sh (subtest #6 or #25)

I see. If I insert sched_yield() in the for loop, I see many failures (not
100% reproducible, but almost). On Windows, I have to insert Sleep(10) so
that the error exit is taken, and the error code is EINVAL :-/

-- Hannes

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Alex Riesen]

On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:
> On Wednesday 08 July 2009, Johannes Sixt wrote:
>> ... 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.

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Johan Herland]

On Thursday 09 July 2009, Alex Riesen wrote:
> On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:
> > On Wednesday 08 July 2009, Johannes Sixt wrote:
> >> ... 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, <johan@herland.net>
www.herland.net

Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

[Alex Riesen]

On Thu, Jul 9, 2009 at 10:37, Johan Herland<johan@herland.net> wrote:
> On Thursday 09 July 2009, Alex Riesen wrote:
>> On Wed, Jul 8, 2009 at 18:01, Johan Herland<johan@herland.net> wrote:
>> > On Wednesday 08 July 2009, Johannes Sixt wrote:
>> >> ... 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?