* [9front] git/fetch: be more robust @ 2021-07-26 0:23 ori 2021-07-26 0:36 ` [9front] " Anthony Martin 2021-07-26 0:45 ` [9front] " cinap_lenrek 0 siblings, 2 replies; 5+ messages in thread From: ori @ 2021-07-26 0:23 UTC (permalink / raw) To: 9front 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* [9front] Re: git/fetch: be more robust 2021-07-26 0:23 [9front] git/fetch: be more robust ori @ 2021-07-26 0:36 ` Anthony Martin 2021-07-26 1:39 ` ori 2021-07-26 0:45 ` [9front] " cinap_lenrek 1 sibling, 1 reply; 5+ messages in thread From: Anthony Martin @ 2021-07-26 0:36 UTC (permalink / raw) To: 9front > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] Re: git/fetch: be more robust 2021-07-26 0:36 ` [9front] " Anthony Martin @ 2021-07-26 1:39 ` ori 0 siblings, 0 replies; 5+ messages in thread From: ori @ 2021-07-26 1:39 UTC (permalink / raw) To: 9front 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/fetch: be more robust 2021-07-26 0:23 [9front] git/fetch: be more robust ori 2021-07-26 0:36 ` [9front] " Anthony Martin @ 2021-07-26 0:45 ` cinap_lenrek 2021-07-26 1:42 ` ori 1 sibling, 1 reply; 5+ messages in thread From: cinap_lenrek @ 2021-07-26 0:45 UTC (permalink / raw) To: 9front 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [9front] git/fetch: be more robust 2021-07-26 0:45 ` [9front] " cinap_lenrek @ 2021-07-26 1:42 ` ori 0 siblings, 0 replies; 5+ messages in thread From: ori @ 2021-07-26 1:42 UTC (permalink / raw) To: 9front 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; } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-26 1:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-26 0:23 [9front] git/fetch: be more robust ori 2021-07-26 0:36 ` [9front] " Anthony Martin 2021-07-26 1:39 ` ori 2021-07-26 0:45 ` [9front] " cinap_lenrek 2021-07-26 1:42 ` ori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).