Re: [PATCH 1/3 v3] Add support for external programs for handling native fetches
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 5, 2009, 10:01 UTC
- Message-ID
- <alpine.DEB.1.00.0908051143000.8306@pacific.mpi-cbg.de>
- In-Reply-To
- <alpine.LNX.2.00.0907310109130.2147@iabervon.org>
Hi,
On Fri, 31 Jul 2009, Daniel Barkalow wrote:
> dscho: it turned out that disconnect...() is a method defined to return > int.
Okay, thanks for the clarification.
Show 17 quoted lines
> +static int fetch(struct transport *transport,
> + int nr_heads, const struct ref **to_fetch)
> +{
> + struct helper_data *data = transport->data;
> + struct child_process *helper;
> + const struct ref *posn;
> + struct strbuf buf = STRBUF_INIT;
> + int i, count;
> + FILE *file;
> +
> + count = 0;
> + for (i = 0; i < nr_heads; i++) {
> + posn = to_fetch[i];
> + if (posn->status & REF_STATUS_UPTODATE)
> + continue;
> + count++;
> + }I still do not like it, but I guess you refuse to please me here.
Show 19 quoted lines
> +
> + if (!count)
> + return 0;
> +
> + helper = get_helper(transport);
> +
> + if (!data->fetch)
> + return -1;
> +
> + file = xfdopen(helper->out, "r");
> + for (i = 0; i < nr_heads; i++) {
> + posn = to_fetch[i];
> + if (posn->status & REF_STATUS_UPTODATE)
> + continue;
> + write_in_full(helper->in, "fetch ", 6);
> + write_in_full(helper->in, sha1_to_hex(posn->old_sha1), 40);
> + write_in_full(helper->in, " ", 1);
> + write_in_full(helper->in, posn->name, strlen(posn->name));
> + write_in_full(helper->in, "\n", 1);How about
strbuf_addf(&buf, "fetch %s %s\n", sha1_to_hex(posn->old_sha1), posn->name); write_in_full(helper->in, buf.buf, buf.len);
instead? It is not only much more readable, but also less error-prone.
Show 11 quoted lines
> +static struct ref *get_refs_list(struct transport *transport, int for_push)
> +{
> + struct child_process *helper;
> + struct ref *ret = NULL;
> + struct ref **tail = &ret;
> + struct ref *posn;
> + struct strbuf buf = STRBUF_INIT;
> + FILE *file;
> +
> + helper = get_helper(transport);
> + write_in_full(helper->in, "list\n", 5);Speaking of error-prone: we introduced e.g. prefixcmp() for the sole purpose of avoiding errors due to hard-coded constants. I would strongly suggest to introduce a helper here instead of having all those constants that every reviewer has to check (which is no fun).
Show 12 quoted lines
> + if (strbuf_getline(&buf, file, '\n') == EOF)
> + exit(128); /* child died, message supplied already */
> +
> + if (!*buf.buf)
> + break;
> +
> + eov = strchr(buf.buf, ' ');
> + if (!eov)
> + die("Malformed response in ref list: %s", buf.buf);
> + eon = strchr(eov + 1, ' ');
> + *eov = '\0';
> + if (eon)Funny indentation?
> + *eon = '\0';
Please register a complaint about your naming ("posn", "eov", "eon"), and also please register a complaint about the rather confusing order of your statements.
Show 7 quoted lines
> + *tail = alloc_ref(eov + 1); > + if (buf.buf[0] == '@') > + (*tail)->symref = xstrdup(buf.buf + 1); > + else if (buf.buf[0] != '?') > + get_sha1_hex(buf.buf, (*tail)->old_sha1); > + tail = &((*tail)->next); > + strbuf_reset(&buf);
For clarity's sake, this should be done _before_ the strbuf is filled, not after it (because then everybody has to think harder why the code is still correct, but the performance is _exactly_ the same).
Show 13 quoted lines
> + }
> + strbuf_release(&buf);
> +
> + for (posn = ret; posn; posn = posn->next)
> + resolve_remote_symref(posn, ret);
> +
> + return ret;
> +}
> +
> +int transport_native_helper_init(struct transport *transport)
> +{
> + struct helper_data *data = xmalloc(sizeof(*data));
> + char *eom = strchr(transport->url, ':');"End of message"?
All in all, I like this patch much better, thank you!
Ciao, Dscho