Re: [PATCH v2] pull: handle --log=<n>

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2015, 05:10 UTC
Message-ID: <xmqqzj4zomu6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqfv6s6ygb.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

> A proper patch series would probably be
>
>  [1/N]   Teach "test_commit --tag" and replace existing "test_commit"
>          with "test_commit --tag"
>
>  [2-N/N] For all the test scripts, analyse and judge if they are
>          better off with the auto-generated tags (i.e. no change wrt
>          the result of 1/N) or tags that are created by the script
>          at strategic places only as needed, and convert those that
>          are better read without "test_commit --tag".
>
> [1/N] would be mechanical and easy, but justifying the change in the
> remainder would be a lot of work and reviewing would be, too, and
> would require a good taste.

Actually even the above is not sufficient, as we have to worry about topics in-flight (aka "the rest of the world is not standing still waiting for you"). It must go more like this:

 [1/N]   Introduce a new helper (let's call it "make_commit") that
         by default does not do too much but optionally can do as much
         as the "test_commit" does.  Reimplement "test_commit" in
         terms of "make_commit"

 [2-N/N] Exaine existing uses of "test_commit" and reduce what they
         do by rewriting them with "make_commit" as needed.

So I'd say 1/N may probably be something worth doing, but I am not convinced that the rest is a good use of reviewer's time.

Speaking of "does not do too much", I think the behaviour of "test_commit" that always adds new a file outlived its usefulness.

When a test is primarily interested in the topology of the resulting history (most "log" tests are), we can use "commit --allow-empty" these days (I think "test_commit" predates it and adding a file may have been a handy way to ensure that the new commit's tree is different from parent's). And when a test is primarily about how branches with different contents in various paths interact with each other (e.g. checking out, merging), it does not make a very useful test vector to work on if you can only build your tests with a set of branches whose differences can only be creation and deletion but never in-place modification of paths.

Among various potential uses of the hypothetical "test_commit" replacement that I would imagine, what is common is only that it does the "tick" thing and then it creates a commit, and nothing else. Everything else is optional and needs customization. But then there is not much left, i.e.

    make_commit () {
	test_tick && git commit --allow-empty "$@"
    }

which may be useful to avoid forgetting the tick thing, but does not help much else.

Even if that were found to be useful, this discussion is only for step 1/N; let's refrain from rewriting everything for the sake of rewriting. I do not think it would be a good use of developer time or reviewer time.