Re: [PATCH 2/2] Rename lineno_width to decimal_width and export it

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2012, 23:29 UTC
Message-ID: <7vhayub9d3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329056180-29796-1-git-send-email-zbyszek@in.waw.pl>

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> This function will be used in calculating diff --stat graph width.
> The name is changed because the function works for any number.
> The function is moved from builtins/blame.c to pager.c because it
> will be used not only in builtins/blame.c.

I'd prefer to see description of preliminary changes phrased without depending too heavily on things that hasn't happened when possible. Making a generic helper function to count digits necessary to print a cardinal number available to future callers is a good thing by itself, even if the "dynamic --stat width computation" turned out to be a bad idea for whatever reason (I am not saying it is a bad idea here).

Perhaps like this.

	Subject: make lineno_width() from blame reusable for others

	builtin/blame.c has a helper function to compute how many columns
	we need to show a line-number, whose implementation is reusable as
	a more generic helper function to count the number of columns
	necessary to show any cardinal number.

	Rename it to decimal_width(), move it to pager.c and export it for
        use by future callers.

And you can say something like "I'll be using this in 'diff --stat' in later patches" after the three-dash line.

> ---

Sign-off before the three-dash line?

> +/*
> + * How many columns do we need to show numbers in decimal?

s/numbers/this number/;

> + */
> +int decimal_width(int number)

Don't we want to make the argument "unsigned number" instead?

Show 7 quoted lines

> +{
> +	int i, width;
> +
> +	for (width = 1, i = 10; i <= number; width++)
> +		i *= 10;
> +	return width;
> +}