Re: [PATCH 3/2] Enhance --early-output format

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 5, 2007, 20:24 UTC
Message-ID: <7vsl3kphjp.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.0.999.0711041124050.15101@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

> It wasn't totally trivial, but it doesn't seem to be excessively subtle 
> either. About half the patch is moving around some code to look at whether 
> the commit is interesting or not and rewriting the parents, so that it can 
> be shared with the revision walker.

Very nicely done.

Show 11 quoted lines

> +	while (list) {
> +		struct commit *commit = list->item;
> +		unsigned int flags = commit->object.flags;
> +
> +		list = list->next;
> +		if (flags & UNINTERESTING)
> +			continue;
> +		if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> +			if (commit->parents && !commit->parents->next)
> +				continue;
> +		}

When looking at:

	if (A && B && C) {
        	if (D && E)
                	continue;
	}

an uninitiated might say "Huh? Why use nested 'if'?", but to somebody who knows how revision traversal works, the above split is a more logical way to test this condition. Maybe one liner comment is in order?

Show 9 quoted lines

> +static void show_early_header(struct rev_info *rev, const char *stage, int nr)
> +{
> +	if (rev->shown_one) {
> +		rev->shown_one = 0;
> +		if (rev->commit_format != CMIT_FMT_ONELINE)
> +			putchar(rev->diffopt.line_termination);
> +	}
> +	printf("Final output: %d %s\n", nr, stage);
> +}

As you noted, this is more like "Partial output" now. How about painting the bikeshed pink by saying:

	Partial output: 20
        Partial output: 70
        Final output: 70000

Show 14 quoted lines

> +	/* Did we already get enough commits for the early output? */
> +	if (!i)
> +		return;
> +
> +	/*
> +	 * ..if no, then repeat it twice a second until we
> +	 * do.
> +	 *
> +	 * NOTE! We don't use "it_interval", because if the
> +	 * reader isn't listening, we want our output to be
> +	 * throttled by the writing, and not have the timer
> +	 * trigger every second even if we're blocked on a
> +	 * reader!
> +	 */

A comment like this is very much appreciated.

> +	early_output_timer.it_value.tv_sec = 0;
> +	early_output_timer.it_value.tv_usec = 500000;
> +	setitimer(ITIMER_REAL, &early_output_timer, NULL);
>  }