[9front] git/fetch: be more robust

[9front] git/fetch: be more robust

[ori]

currently, git/fetch prints the refs
to update before it fully fetches the
pack files; this can lead to updates
to the refs before we're 100% certain
that the objects are present.

This change prints the updates after
the packfile has been successfully
indexed.

I've been running with this for a bit,
and it passes the tests in

	gits://shithub.us/ori/regress

so I'm reasonably confident it works,
but considering that bugs in the past
have caused major pain -- test reports
wanted:

--- //.git/fs/object/28f76455d39d990b47c6e46e18158f0a9ba09d25/tree/sys/src/cmd/git/fetch.c
+++ sys/src/cmd/git/fetch.c
@@ -181,7 +181,7 @@
 fetchpack(Conn *c)
 {
 	char buf[Pktmax], *sp[3];
-	char *packtmp, *idxtmp;
+	char *packtmp, *idxtmp, **ref;
 	Hash h, *have, *want;
 	int nref, refsz, first;
 	int i, n, req, pfd;
@@ -193,6 +193,7 @@
 	first = 1;
 	have = eamalloc(refsz, sizeof(have[0]));
 	want = eamalloc(refsz, sizeof(want[0]));
+	ref = eamalloc(refsz, sizeof(ref[0]));
 	while(1){
 		n = readpkt(c, buf, sizeof(buf));
 		if(n == -1)
@@ -213,14 +214,15 @@
 			continue;
 		if(refsz == nref + 1){
 			refsz *= 2;
-			have = erealloc(have, refsz * sizeof(have[0]));
-			want = erealloc(want, refsz * sizeof(want[0]));
+			have = earealloc(have, refsz, sizeof(have[0]));
+			want = earealloc(want, refsz, sizeof(want[0]));
+			ref = earealloc(ref, refsz, sizeof(ref[0]));
 		}
+		ref[nref] = estrdup(sp[1]);
 		if(hparse(&want[nref], sp[0]) == -1)
 			sysfatal("invalid hash %s", sp[0]);
-		if (resolveremote(&have[nref], sp[1]) == -1)
+		if (resolveremote(&have[nref], ref[nref]) == -1)
 			memset(&have[nref], 0, sizeof(have[nref]));
-		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
 		nref++;
 	}
 	if(listonly){
@@ -295,6 +297,14 @@
 		fail(packtmp, idxtmp, "could not index fetched pack: %r");
 	if(rename(packtmp, idxtmp, h) == -1)
 		fail(packtmp, idxtmp, "could not rename indexed pack: %r");
+
+	for(i = 0; i < nref; i++){
+		print("remote %s %H local %H\n", ref[i], want[i], have[i]);
+		free(ref[i]);
+	}
+	free(ref);
+	free(want);
+	free(have);
 	return 0;
 }
 

[9front] Re: git/fetch: be more robust

[Anthony Martin]

>  			memset(&have[nref], 0, sizeof(have[nref]));
> -		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
>  		nref++;
>  	}
>  	if(listonly){

This will cause git/fetch -l to output nothing.

Cheers,
  Anthony

Re: [9front] git/fetch: be more robust

[cinap_lenrek]

so this is like a race condition?

depending how fast the reader is able to process the message?

the other side of the pipeline is reading the hash and then fails to locate
it in git/fs because it has not been indexed yet when it reads too fast?

is this just a theory or is there proof that the problem is
well understood?

 		if(hparse(&want[nref], sp[0]) == -1)
 			sysfatal("invalid hash %s", sp[0]);
-		if (resolveremote(&have[nref], sp[1]) == -1)
+		if (resolveremote(&have[nref], ref[nref]) == -1)
 			memset(&have[nref], 0, sizeof(have[nref]));
-		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
 		nref++;


wouldnt it be better to just estrdup() at the print(), then it seems
cleaner and makes the diff smaller... like:

		if (resolveremote(&have[nref], sp[1]) == -1)
 			memset(&have[nref], 0, sizeof(have[nref]));
-		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
+		ref[nref] = estrdup(sp[1]);
		nref++

--
cinap

Re: [9front] Re: git/fetch: be more robust

[ori]

Quoth Anthony Martin <ality@pbrane.org>:
> >  			memset(&have[nref], 0, sizeof(have[nref]));
> > -		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
> >  		nref++;
> >  	}
> >  	if(listonly){
> 
> This will cause git/fetch -l to output nothing.
> 
> Cheers,
>   Anthony
> 

Ah, you're right -- question, does anyone use '-l'?
Can I just kill it?

Re: [9front] git/fetch: be more robust

[ori]

Quoth cinap_lenrek@felloff.net:
> so this is like a race condition?
> 
> depending how fast the reader is able to process the message?
> 
> the other side of the pipeline is reading the hash and then fails to locate
> it in git/fs because it has not been indexed yet when it reads too fast?

Kind of.

git/pull consumes the output of git/fetch, and
uses it to update the refs. The problem is that
we can consume the output of git/fetch and update
the refs, then *fail* to update the repo.

For some reason, it's never bitten me (fast internet,
small deltas when I pull), but it makes me uneasy
that we *can* write invalid refs.

> is this just a theory or is there proof that the problem is
> well understood?
> 
>  		if(hparse(&want[nref], sp[0]) == -1)
>  			sysfatal("invalid hash %s", sp[0]);
> -		if (resolveremote(&have[nref], sp[1]) == -1)
> +		if (resolveremote(&have[nref], ref[nref]) == -1)
>  			memset(&have[nref], 0, sizeof(have[nref]));
> -		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
>  		nref++;
> 
> 
> wouldnt it be better to just estrdup() at the print(), then it seems
> cleaner and makes the diff smaller... like:
> 
> 		if (resolveremote(&have[nref], sp[1]) == -1)
>  			memset(&have[nref], 0, sizeof(have[nref]));
> -		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
> +		ref[nref] = estrdup(sp[1]);
> 		nref++

Yeah, that's equivalent, and I've got no
preference. Updated patch attached, with
a fix for '-l':

--- //.git/fs/object/28f76455d39d990b47c6e46e18158f0a9ba09d25/tree/sys/src/cmd/git/fetch.c
+++ sys/src/cmd/git/fetch.c
@@ -181,7 +181,7 @@
 fetchpack(Conn *c)
 {
 	char buf[Pktmax], *sp[3];
-	char *packtmp, *idxtmp;
+	char *packtmp, *idxtmp, **ref;
 	Hash h, *have, *want;
 	int nref, refsz, first;
 	int i, n, req, pfd;
@@ -193,6 +193,7 @@
 	first = 1;
 	have = eamalloc(refsz, sizeof(have[0]));
 	want = eamalloc(refsz, sizeof(want[0]));
+	ref = eamalloc(refsz, sizeof(ref[0]));
 	while(1){
 		n = readpkt(c, buf, sizeof(buf));
 		if(n == -1)
@@ -213,19 +214,20 @@
 			continue;
 		if(refsz == nref + 1){
 			refsz *= 2;
-			have = erealloc(have, refsz * sizeof(have[0]));
-			want = erealloc(want, refsz * sizeof(want[0]));
+			have = earealloc(have, refsz, sizeof(have[0]));
+			want = earealloc(want, refsz, sizeof(want[0]));
+			ref = earealloc(ref, refsz, sizeof(ref[0]));
 		}
 		if(hparse(&want[nref], sp[0]) == -1)
 			sysfatal("invalid hash %s", sp[0]);
 		if (resolveremote(&have[nref], sp[1]) == -1)
 			memset(&have[nref], 0, sizeof(have[nref]));
-		print("remote %s %H local %H\n", sp[1], want[nref], have[nref]);
+		ref[nref] = estrdup(sp[1]);
 		nref++;
 	}
 	if(listonly){
 		flushpkt(c);
-		return 0;
+		goto showrefs;
 	}
 
 	if(writephase(c) == -1)
@@ -295,6 +297,15 @@
 		fail(packtmp, idxtmp, "could not index fetched pack: %r");
 	if(rename(packtmp, idxtmp, h) == -1)
 		fail(packtmp, idxtmp, "could not rename indexed pack: %r");
+
+showrefs:
+	for(i = 0; i < nref; i++){
+		print("remote %s %H local %H\n", ref[i], want[i], have[i]);
+		free(ref[i]);
+	}
+	free(ref);
+	free(want);
+	free(have);
 	return 0;
 }