Re: [PATCH v4] add --summary option to git-push and git-fetch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 30, 2010, 01:17 UTC
- Message-ID
- <7vzl3w9yst.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20100130005948.GA14938@cthulhu>
Larry D'Anna <larry@elder-gods.org> writes:
Please don't use M-F-T to deflect a direct response meant to you away from you. I also do not want people wasting _my_ time by following your M-F-T and sending their comments meant to _you_ coming to me with my address on To: line, as I prioritize incoming messages based on where my address is in the header, and do not want you waste _other's_ time by making them correct their "To:" like to avoid wasting my time.
Show 14 quoted lines
>> > @@ -373,12 +379,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>> > fputc(url[i], fp);
>> > fputc('\n', fp);
>> >
>> > - if (ref)
>> > - rc |= update_local_ref(ref, what, note);
>> > - else
>> > + if (ref) {
>> > + *quickref = 0;
>> > + rc |= update_local_ref(ref, what, note, quickref);
>>
>> Makes me wonder why update_local_ref() does not put that NUL upon entry.
>
> I'm not sure what you mean. Could you elaborate?Why is it necessary for the caller to do "*quickref = '\0'" before calling update_local_ref()? Shouldn't your updated u-l-r be doing that clearing, so that the callers don't have to worry about it?
Show 14 quoted lines
>> > + init_revisions(&rev, NULL);
>> > + rev.prune = 0;
>> > + assert(!handle_revision_arg(quickref, &rev, 0, 1));
>> > + assert(!prepare_revision_walk(&rev));
>> > +
>> > + while ((commit = get_revision(&rev)) != NULL) {
>> > + struct strbuf buf = STRBUF_INIT;
>> > + if (limit == 0) {
>> > + fprintf(stderr, " ...\n");
>>
>> How would you know, when you asked 20 and you showed 20 here, that there
>> is no more to come?
>
> If there's more it will print the "...", if there isn't then it won't.If your limit is 20 and if you unconditionally say "..." after pulling 20 from the pool, the consumer of your output would think "Ah, I see 20 but that is only I asked for 20, and the ... means there are more". But that is incorrect because your 21st call to get_revision() might have yielded NULL in which case you had only 20 after all.
You cannot do "..." correctly without pulling one more than the limit from the pool.
Show 15 quoted lines
>> > +test_expect_success 'fetch --summary forced update' ' >> > + mk_empty && >> > + ( >> > ... >> > + ) >> > + >> > +' >> >> There are at least two missing combinations. (1) "fetch --summary" to >> fetch a new branch, and (2) "fetch --summary" does not try segfaulting by >> accessing unavailable information after a failed fetch. >> >> The same comment applies to the push side of the tests. > > What would be a good way to induce a failed fetch for this test?
Not having a valid ref or repo, perhaps. I dunno---it's been quite a while since I saw the patch.