Re: [PATCH v4] add --summary option to git-push and git-fetch

[Junio C Hamano <gitster@pobox.com>]

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.

>> > @@ -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?

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

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