v2pull: handle --log=<n>

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.

Fix this by matching --log=* in addition to --log and --no-log.

Implement a test for this use case.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
This is a re-roll of [1].

The only change is the removal of "verbose".

Thanks Matthieu for the review last round.

[1] http://thread.gmane.org/gmane.comp.version-control.git/268961

 git-pull.sh         |  4 ++--
 t/t5524-pull-msg.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

Hi Paul,

On 2015-05-18 15:39, Paul Tan wrote:

I completely forgot to send a mail last time where I wanted to mention the test_commit function. I *think* that above invocation is equivalent to

    test_commit "second commit" afile second

with the additional benefit that `test_tick` is called automatically. Maybe you want to use that more concise function call?

Ciao, Dscho

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Sounds like a good idea especially when you are testing commands that does not do things to tags implicitly.

I have been in love-hate relation with test_commit for a long time. It is very nice that it gives us ticks (i.e. reproducible object names) automatically and reduces the chance for new tests to forget to do so, but I found it extremely annoying that it wants to always add a light-weight tag to every commit it creates.

I suspect that this design decision largely comes from the fact that back then we did not exactly trust the syntax like "master~3" and wanted to have a set of fixed points, but often these auto-generated tags get in the way by either contaminating the refs namespaces (and we are testing pull here, which worries me that test authors need to be aware that these tags may be transferred to the destination) or anchoring the objects in the object database (e.g. when writing "reset --hard branch branch~3 && prune" tests, you need to remember to kill these tags).

So I dunno. I really wish test_commit didn't create tags and either left the tagging to the calling script.

Hi Junio,

On 2015-05-18 20:18, Junio C Hamano wrote:

Yeah, light-weight tags with `test_commit` can be very annoying. IIRC the reason was not because we did not trust master~3, but that it is really unreadable. You have to be a Git nerd to find those tests intuitive that refer to master~3 which later is master~17 and then after a `git reset HEAD~5^2~3` is master~1 instead. The idea was that you could refer to the commits in question by their commit message.

I guess that I should clean up this mess and remove the tagging facility. We could use the :/<subject prefix> syntax instead, for example.

Or maybe just add a --tag flag to `test_commit` and use that in all cases where the tags were actually needed. Yeah, I think I like that option best.

Ciao, Dscho

On Tue, May 19, 2015 at 6:35 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Thanks for inferring what I wanted to say but left unsaid due to my stupidity and lack of proofreading. I meant to follow "either left to the caller" with "or with an option" ;-)

On di, 2015-05-19 at 06:57 -0700, Junio C Hamano wrote:

I took a stab at this, adding a --tag option to test_commit and adding the option to the test_commit calls that need it (or removing tests' reliance on these tags where appropriate, or removing tests' workarounds for dealing with these tags when they don't want them), and the result is 59 files changed, 280 insertions(+), 281 deletions(-)

A test run on master with GIT_TEST_LONG set causes 1138 calls to test_commit on my system, of which 255 now use the --tag option (measured with a really crude hack that INCR's some keys in redis at appropriate points in test_commit).

Is this interesting enough to turn into a proper patch series?

On Tue, May 19, 2015 at 2:24 PM, Dennis Kaarsemaker <dennis@kaarsemaker.net> wrote:

I guess most of the line changes are just adding the --tag? And I'd guess (281-280) that there is no huge code inside of test_commit either, so I'd assume it doesn't add clutter, but rather cleans up....

... 255 out of 1138 is awesome IMHO! Do you see an improvement in time as well (as in "time make test" is X% faster overall) ?

I'd think this is worth making a real patch, definitely!

Thanks, Stefan

On di, 2015-05-19 at 14:33 -0700, Stefan Beller wrote:

Full stat:

 t/t0100-previous.sh                        |  6 +++---
 t/t1011-read-tree-sparse-checkout.sh       |  2 +-
 t/t1403-show-ref.sh                        |  6 +++---
 t/t1430-bad-ref-name.sh                    |  2 +-
 t/t1450-fsck.sh                            |  4 ++--
 t/t2018-checkout-branch.sh                 |  4 ++--
 t/t2020-checkout-detach.sh                 |  8 ++++----
 t/t2030-unresolve-info.sh                  | 10 +++++-----
 t/t3200-branch.sh                          |  2 +-
 t/t3211-peel-ref.sh                        |  2 +-
 t/t3307-notes-man.sh                       |  4 ++--
 t/t3308-notes-merge.sh                     | 10 +++++-----
 t/t3309-notes-merge-auto-resolve.sh        | 30
+++++++++++++++---------------
 t/t3310-notes-merge-manual-resolve.sh      | 10 +++++-----
 t/t3311-notes-merge-fanout.sh              |  2 +-
 t/t3400-rebase.sh                          |  6 +++---
 t/t3404-rebase-interactive.sh              | 22 +++++++++++-----------
 t/t3406-rebase-message.sh                  |  6 +++---
 t/t3410-rebase-preserve-dropped-merges.sh  | 14 +++++++-------
 t/t3411-rebase-preserve-around-merges.sh   | 14 +++++++-------
 t/t3414-rebase-preserve-onto.sh            | 14 +++++++-------
 t/t3416-rebase-onto-threedots.sh           | 12 ++++++------
 t/t3418-rebase-continue.sh                 |  4 ++--
 t/t3421-rebase-topology-linear.sh          | 22 +++++++++++-----------
 t/t3425-rebase-topology-merges.sh          | 20 ++++++++++----------
 t/t3507-cherry-pick-conflict.sh            |  8 ++++----
 t/t3508-cherry-pick-many-commits.sh        |  6 +++---
 t/t3510-cherry-pick-sequence.sh            | 16 ++++++++--------
 t/t3511-cherry-pick-x.sh                   |  2 +-
 t/t4131-apply-fake-ancestor.sh             |  8 ++++----
 t/t4152-am-subjects.sh                     |  2 +-
 t/t4202-log.sh                             | 14 +++++++-------
 t/t4207-log-decoration-colors.sh           |  8 +++-----
 t/t4300-merge-tree.sh                      | 62 +++++++++++++++++++++++++++++---------------------------------
 t/t5407-post-rewrite-hook.sh               | 12 ++++++------
 t/t5509-fetch-push-namespaces.sh           |  4 ++--
 t/t5510-fetch.sh                           |  4 ++--
 t/t5514-fetch-multiple.sh                  |  2 +-
 t/t5571-pre-push-hook.sh                   |  2 +-
 t/t5704-bundle.sh                          |  5 +----
 t/t6009-rev-list-parent.sh                 | 18 +++++++++---------
 t/t6010-merge-base.sh                      | 24 ++++++++++++------------
 t/t6016-rev-list-graph-simplify-history.sh | 22 +++++++++++-----------
 t/t6019-rev-list-ancestry-path.sh          | 18 +++++++++---------
 t/t6101-rev-parse-parents.sh               |  8 ++++----
 t/t6111-rev-list-treesame.sh               |  2 +-
 t/t6300-for-each-ref.sh                    |  1 -
 t/t7003-filter-branch.sh                   | 16 ++++++++--------
 t/t7007-show.sh                            |  8 ++++----
 t/t7012-skip-worktree-writing.sh           |  2 +-
 t/t7060-wtstatus.sh                        |  6 +++---
 t/t7111-reset-table.sh                     |  6 +++---
 t/t7407-submodule-foreach.sh               |  2 +-
 t/t7512-status-help.sh                     |  6 +++---
 t/t7606-merge-custom.sh                    |  8 ++++----
 t/t7607-merge-overwrite.sh                 |  6 +++---
 t/t7608-merge-messages.sh                  |  4 ++--
 t/t9502-gitweb-standalone-parse-output.sh  |  2 +-
 t/test-lib-functions.sh                    |  9 ++++++++-
 59 files changed, 278 insertions(+), 281 deletions(-)

I have not measured, but will.

OK.

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

Wow.

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.

Perhaps if we see two sample patches to see how it looks like, would that help us decide?

That is, the mechanical [1/N] and [2/N] for one of the test script that can do without --tag, and a sample "do not apply" patch to show "if we change 'test_commit --tag' to 'test_commit', the script t1234 needs this many manual tagging by the caller, and it is not worth doing"? I dunno.

Thanks.

On di, 2015-05-19 at 14:37 -0700, Junio C Hamano wrote:

I've actually done it differently while implementing:

1) Make test_commit recognize --tags and stop creating tags unless
   specified
2) while ! prove --state=save,failed {
       Find and fix tests that now need --tags
   }

For the actual patch series I'll add -p the changes slightly differently:

1/N: Make test_commit recognize a --tags parameter but not change behaviour. 2/N - N-1/N: Add --tags where necesary (or other fixes as appropriate) N/N: Only write tags when --tags is passed to test_commit.

That way 'make test' will pass at every step.

I'll just send an entire patch series. It's not that much more work.

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

That was what I feared. The result of that process is the hardest to reason about and review.

Make test will pass after this one.

And untouched ones will continue passing.

Only when you remove --tags from ones that need (i.e. the caller needs to create the necessary tags), we will see a patch and unless there is mistake in the conversion, the result should pass.

On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:

I'm not quite sure I understand what you're trying to say. You seem to be worried that there will be silent successes for tests that should fail after N/N if I take the proposed approach. I have no idea how that could happen though.

On Tue, May 19, 2015 at 3:30 PM, Dennis Kaarsemaker <dennis@kaarsemaker.net> wrote:

You are placing too much faith in the tests. We would want to see a conversion that is reasonable and indeed reasoned by eyeballing the change for correctness.

Besides, there are different build options and test configurations. Even if you run with EXPENSIVE defined, the only thing we can say without validating the changes is the test happened to have passed. There is no assurance that the result of your change will catch the same breakage, if inttroduced later to the code (not tests), as the current unmodified tests will. Two mistakes may happen to hide the breakage in tests. Taking to the extreme, if you replace all the test bodies in test_expect_success with "true", the result of such a rewrite will still pass all the existting tests, and the only way you can say "That's absurd" is because you compare what the existing tests do and a single "true" would do and realize that they do vastly different things. Your "while ! prove ..." method does not give me much more faith than such a "rewrite everything to true without looking" method.

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

To put it in another way, imagine this scenario.

Six months ago, you found a bug in "git frotz" command. The bug triggered only when a commit had an extra tag pointing at it. You fixed the bug, and in order to protect your fix from getting broken by other people, added a new test that would have failed with the bug present, perhaps like this:

    test_expect_success 'commit with an extra tag' '
	test_commit sample &&
        git frotz HEAD
    '

Fast forward to today. You changed "test_commit" and it no longer creates a tag by default with your step 1.

You run this test again.

It will pass.

Why? "git frotz" command used to break when you had tag, but did not break if you didn't have the tag even without your bugfix six months ago.

Next week, somebody will accidentally break the bugfix you did six months ago. "git frotz" again does not work correctly when the commit is pointed at by a tag. But with the test will not catch this breakage.

The problem was that your step 1 broke this test, and your step 2 of running the test did not catch the fact that you broke it.

Hopefully now you have some idea how your approach is problematic.

On di, 2015-05-19 at 19:19 -0700, Junio C Hamano wrote:

Yes, thanks for the thorough explanation!

Two more bad sideeffects, so two more reasons not to take this approach:

- test_must_fail tests might now fail for the wrong reason, undetectedly
  ("ref does not exist" instead of "can't handle ref")
- same for TODO tests

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

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.

On di, 2015-05-19 at 22:10 -0700, Junio C Hamano wrote:

Agreed, and in combination with the very marginal (if any) time saving, I'll just drop the whole thing.

On Mon, May 18, 2015 at 10:53 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Hmm, it says that 'second commit' is not a valid tag name ;-) Okay, I can work around it by making the commit message "second".

On another note though, I think I need to add a "test_tick" for the commit just below as well:

        echo "original $dollar" >afile &&
        git add afile &&c
        git commit -m "do not clobber $dollar signs"

So, here's the re-rolled patch.

--- >8 --- 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.

Fix this by matching --log=* in addition to --log and --no-log.

Implement a test for this use case.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-pull.sh         |  4 ++--
 t/t5524-pull-msg.sh | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

Paul Tan <pyokagan@gmail.com> writes:

Sigh, too late.

I thought the previous round was good enough and the patch is already on 'next'.

If the incremental change is still worth doing on top, please do so.

Thanks.

Hi Junio,

On Fri, May 22, 2015 at 5:24 AM, Junio C Hamano <gitster@pobox.com> wrote:

My bad, I should have checked 'next'.

But it's okay, I re-rolled the patch because it seemed that people preferred "test_commit". I personally don't have a strong opinion on this, so if you're fine with it then I'm okay as well.

Thanks, Paul

Just a mental note to myself,

On Mon, May 18, 2015 at 9:39 PM, Paul Tan <pyokagan@gmail.com> wrote:

Ugh ><

s/test_expect_failure/test_expect_success/

Will respond to the other reviews tomorrow (it's bedtime in SGT).

Thanks, Paul

Hi Paul,

On 2015-05-18 17:15, Paul Tan wrote:

Heh, I missed that one myself.

That was quite some stuff you sent to the list, indeed. Good work!

Ciao, Dscho