Re: [PATCH 0/2] teach git pull to handle --log=<n>

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2015, 17:11 UTC
Message-ID: <xmqqd22384om.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1431508661-21729-1-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 12 quoted lines

> Since efb779f (merge, pull: add '--(no-)log' command line option,
> 2008-04-06) git-pull supported the (--no-)log switch and would pass it
> to git-merge.
>
> 96e9420 (merge: Make '--log' an integer option for number of shortlog
> entries, 2010-09-08) implemented support for the --log=<n> switch, which
> would explicitly set the number of shortlog entries. However, git-pull
> does not recognize this option, and will instead pass it to git-fetch,
> leading to "unknown option" errors.
>
> This patch series implements a failing test that demonstrates the above,
> and teaches git-pull to handle the switch --log=<n>.

Looks good.

One advice; for a small patch like this one (and the "pull.ff vs merge.ff" one, too), it is not necessary or even desirable to do a two-step "first add a failure test and then another patch to fix and flip the expectation" series. Just do the fix and add a test to expect success.

After all, the primary reason why we add test is *not* for you to demonstrate that what you did works as expected. It is to catch other people breaking what you did in the future.

Thanks.