From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 01 Mar 2008 07:29:32 GMT
Subject: Re: [PATCH v2 3/3] git-submodule: New subcommand 'summary' (3) - limit summary size
Message-ID: <7v1w6u7vhf.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <18af168b52a735c33612c9c9e4778d8b8bef1cbc.1204306070.git.pkufranky@gmail.com>

Ping Yin <pkufranky@gmail.com> writes:

> This patches teaches git-submodule an option '--summary-limit|-n <number>'
> to limit number of commits for the summary. Number 0 will disable summary
> and minus number will not limit the summary size.

"Negative means unlimited" feels unnecessary.  Didn't you make "unlimited"
the default anyway?
>
> For beauty and clarification, the last commit for each section (backward
> and forward) will always be shown disregarding the given limit. So actual
> summary size may be greater than the given limit.
>
> In the same super project of these patch series, 'git submodule -n 2
> summary sm1' and 'git submodule -n 3 summary sm1' will show the same.

This description is unclear.  Does "-n 2" tell "show 2 commits from both
side", or "show 2 in total"?

> ---------------------------------------
>  $ git submodule -n 2 summary sm1
>  # Submodules modifiled: sm1
>  #
>  # * sm1 354cd45...3f751e5:
>  #   <one line message for C
>  #   <one line message for B
>  #   >... (1 more)
>  #   >one line message for E
>  #

When you have room only for N lines, you might have to say (X more), but
you never need to say (1 more).  You can fit that omitted one item on that
line instead of wasting that line to say (1 more).

It is unclear from the above illustration as the (1 more) is pointing at
right without having anything newer than that, but I think you meant to do
something like this:

    <top
    <second
    <... (N more)
    <bottom
    >top
    >second
    >... (M more)
    >bottom

I guess fork-point may be interesting enough that showing the bottom one
like you did might make sense.  I am not convinced but we'll see.

> +		-n|--summary-limit)
> +			if test -z "$2" || echo "$2" | grep --quiet -v '^-\?[0-9]\+$'

\?\+?????

	summary_limit=$(expr "$2" : '[0-9][0-9]*$')

or even

	if summary_limit=$(( $2 + 0 )) 2>/dev/null ||
           test "$2" != "$summary_limit"
	then
		usage
	fi

perhaps.

> +			if (( $summary_limit < 0 ))

Don't.  The first line of this script says "#!/bin/sh", not bash.