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

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

[Paul Tan]

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(-)

diff --git a/git-pull.sh b/git-pull.sh
index 9ed01fd..5ff4545 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -81,8 +81,8 @@ do
 		diffstat=--no-stat ;;
 	--stat|--summary)
 		diffstat=--stat ;;
-	--log|--no-log)
-		log_arg=$1 ;;
+	--log|--log=*|--no-log)
+		log_arg="$1" ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
 		no_commit=--no-commit ;;
 	--c|--co|--com|--comm|--commi|--commit)
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..eebb8c9 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -17,6 +17,9 @@ test_expect_success setup '
 		git commit -m "add bfile"
 	) &&
 	test_tick && test_tick &&
+	echo "second" >afile &&
+	git add afile &&
+	git commit -m "second commit" &&
 	echo "original $dollar" >afile &&
 	git add afile &&
 	git commit -m "do not clobber $dollar signs"
@@ -32,4 +35,18 @@ test_expect_success pull '
 )
 '
 
+test_expect_failure '--log=1 limits shortlog length' '
+(
+	cd cloned &&
+	git reset --hard HEAD^ &&
+	test "$(cat afile)" = original &&
+	test "$(cat bfile)" = added &&
+	git pull --log=1 &&
+	git log -3 &&
+	git cat-file commit HEAD >result &&
+	grep Dollar result &&
+	! grep "second commit" result
+)
+'
+
 test_done
-- 
2.1.4

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

[Johannes Schindelin]

Hi Paul,

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

> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
> index 8cccecc..eebb8c9 100755
> --- a/t/t5524-pull-msg.sh
> +++ b/t/t5524-pull-msg.sh
> @@ -17,6 +17,9 @@ test_expect_success setup '
>  		git commit -m "add bfile"
>  	) &&
>  	test_tick && test_tick &&
> +	echo "second" >afile &&
> +	git add afile &&
> +	git commit -m "second commit" &&
>  	echo "original $dollar" >afile &&
>  	git add afile &&

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

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

[Paul Tan]

Just a mental note to myself,

On Mon, May 18, 2015 at 9:39 PM, Paul Tan <pyokagan@gmail.com> wrote:
> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
> index 8cccecc..eebb8c9 100755
> --- a/t/t5524-pull-msg.sh
> +++ b/t/t5524-pull-msg.sh
> @@ -17,6 +17,9 @@ test_expect_success setup '
>                 git commit -m "add bfile"
>         ) &&
>         test_tick && test_tick &&
> +       echo "second" >afile &&
> +       git add afile &&
> +       git commit -m "second commit" &&
>         echo "original $dollar" >afile &&
>         git add afile &&
>         git commit -m "do not clobber $dollar signs"
> @@ -32,4 +35,18 @@ test_expect_success pull '
>  )
>  '
>
> +test_expect_failure '--log=1 limits shortlog length' '

Ugh ><

s/test_expect_failure/test_expect_success/

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

> +(
> +       cd cloned &&
> +       git reset --hard HEAD^ &&
> +       test "$(cat afile)" = original &&
> +       test "$(cat bfile)" = added &&
> +       git pull --log=1 &&
> +       git log -3 &&
> +       git cat-file commit HEAD >result &&
> +       grep Dollar result &&
> +       ! grep "second commit" result
> +)
> +'
> +
>  test_done
> --
> 2.1.4

Thanks,
Paul

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

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 17:15, Paul Tan wrote:
> Just a mental note to myself,
> 
> On Mon, May 18, 2015 at 9:39 PM, Paul Tan <pyokagan@gmail.com> wrote:
>> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
>> index 8cccecc..eebb8c9 100755
>> --- a/t/t5524-pull-msg.sh
>> +++ b/t/t5524-pull-msg.sh
>> @@ -17,6 +17,9 @@ test_expect_success setup '
>>                 git commit -m "add bfile"
>>         ) &&
>>         test_tick && test_tick &&
>> +       echo "second" >afile &&
>> +       git add afile &&
>> +       git commit -m "second commit" &&
>>         echo "original $dollar" >afile &&
>>         git add afile &&
>>         git commit -m "do not clobber $dollar signs"
>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>  )
>>  '
>>
>> +test_expect_failure '--log=1 limits shortlog length' '
> 
> Ugh ><
> 
> s/test_expect_failure/test_expect_success/

Heh, I missed that one myself.

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

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

Ciao,
Dscho

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

[Junio C Hamano]

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

> Hi Paul,
>
> On 2015-05-18 15:39, Paul Tan wrote:
>
>> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
>> index 8cccecc..eebb8c9 100755
>> --- a/t/t5524-pull-msg.sh
>> +++ b/t/t5524-pull-msg.sh
>> @@ -17,6 +17,9 @@ test_expect_success setup '
>>  		git commit -m "add bfile"
>>  	) &&
>>  	test_tick && test_tick &&
>> +	echo "second" >afile &&
>> +	git add afile &&
>> +	git commit -m "second commit" &&
>>  	echo "original $dollar" >afile &&
>>  	git add afile &&
>
> 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?

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.

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

[Johannes Schindelin]

Hi Junio,

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

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

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

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

[Junio C Hamano]

On Tue, May 19, 2015 at 6:35 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
>
> On 2015-05-18 20:18, Junio C Hamano wrote:
>>
>> So I dunno.  I really wish test_commit didn't create tags and either
>> left the tagging to the calling script.
>
> 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.

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" ;-)

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 06:57 -0700, Junio C Hamano wrote:
> On Tue, May 19, 2015 at 6:35 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> >
> > On 2015-05-18 20:18, Junio C Hamano wrote:
> >>
> >> So I dunno.  I really wish test_commit didn't create tags and either
> >> left the tagging to the calling script.
> >
> > 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.
> 
> 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" ;-)

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?
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Stefan Beller]

On Tue, May 19, 2015 at 2:24 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On di, 2015-05-19 at 06:57 -0700, Junio C Hamano wrote:
>> On Tue, May 19, 2015 at 6:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> >
>> > On 2015-05-18 20:18, Junio C Hamano wrote:
>> >>
>> >> So I dunno.  I really wish test_commit didn't create tags and either
>> >> left the tagging to the calling script.
>> >
>> > 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.
>>
>> 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" ;-)
>
> 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(-)

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

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

... 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) ?

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

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

Thanks,
Stefan

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

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> 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?

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.

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 14:33 -0700, Stefan Beller wrote:
> On Tue, May 19, 2015 at 2:24 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
> > On di, 2015-05-19 at 06:57 -0700, Junio C Hamano wrote:
> >> On Tue, May 19, 2015 at 6:35 AM, Johannes Schindelin
> >> <johannes.schindelin@gmx.de> wrote:
> >> >
> >> > On 2015-05-18 20:18, Junio C Hamano wrote:
> >> >>
> >> >> So I dunno.  I really wish test_commit didn't create tags and either
> >> >> left the tagging to the calling script.
> >> >
> >> > 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.
> >>
> >> 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" ;-)
> >
> > 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(-)
> 
> 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....

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(-)

> > 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).
> 
> ... 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 have not measured, but will.

> > Is this interesting enough to turn into a proper patch series?
> 
> I'd think this is worth making a real patch, definitely!

OK.
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 14:37 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > 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?
> 
> 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.

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.

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

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

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> 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
>    }

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

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

Make test will pass after this one.

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

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.

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > 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
> >    }
> 
> That was what I feared.  The result of that process is the hardest
> to reason about and review.
> 
> > 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.
> 
> Make test will pass after this one.
> 
> > 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.
> 
> 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.

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.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Junio C Hamano]

On Tue, May 19, 2015 at 3:30 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>
>> > 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
>> >    }
>>
>> That was what I feared.  The result of that process is the hardest
>> to reason about and review.
>>
>
> 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.

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.

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

[Junio C Hamano]

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

> On Tue, May 19, 2015 at 3:30 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
>> On di, 2015-05-19 at 15:10 -0700, Junio C Hamano wrote:
>>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>>
>>> > 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
>>> >    }
>>>
>>> That was what I feared.  The result of that process is the hardest
>>> to reason about and review.
>>
>> 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.
>
> 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.

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.

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

[Junio C Hamano]

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

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

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 19:19 -0700, Junio C Hamano wrote:
> Hopefully now you have some idea how your approach is problematic.

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

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Dennis Kaarsemaker]

On di, 2015-05-19 at 22:10 -0700, Junio C Hamano wrote:
> 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.

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

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

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

[Paul Tan]

On Mon, May 18, 2015 at 10:53 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> On 2015-05-18 15:39, Paul Tan wrote:
>> diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
>> index 8cccecc..eebb8c9 100755
>> --- a/t/t5524-pull-msg.sh
>> +++ b/t/t5524-pull-msg.sh
>> @@ -17,6 +17,9 @@ test_expect_success setup '
>>               git commit -m "add bfile"
>>       ) &&
>>       test_tick && test_tick &&
>> +     echo "second" >afile &&
>> +     git add afile &&
>> +     git commit -m "second commit" &&
>>       echo "original $dollar" >afile &&
>>       git add afile &&
>
> 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?

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(-)

diff --git a/git-pull.sh b/git-pull.sh
index 9ed01fd..5ff4545 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -81,8 +81,8 @@ do
 		diffstat=--no-stat ;;
 	--stat|--summary)
 		diffstat=--stat ;;
-	--log|--no-log)
-		log_arg=$1 ;;
+	--log|--log=*|--no-log)
+		log_arg="$1" ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
 		no_commit=--no-commit ;;
 	--c|--co|--com|--comm|--commi|--commit)
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..9bc7821 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -16,7 +16,9 @@ test_expect_success setup '
 		test_tick &&
 		git commit -m "add bfile"
 	) &&
-	test_tick && test_tick &&
+	test_tick &&
+	test_commit second afile &&
+	test_tick &&
 	echo "original $dollar" >afile &&
 	git add afile &&
 	git commit -m "do not clobber $dollar signs"
@@ -32,4 +34,18 @@ test_expect_success pull '
 )
 '
 
+test_expect_success '--log=1 limits shortlog length' '
+(
+	cd cloned &&
+	git reset --hard HEAD^ &&
+	test "$(cat afile)" = original &&
+	test "$(cat bfile)" = added &&
+	git pull --log=1 &&
+	git log -3 &&
+	git cat-file commit HEAD >result &&
+	grep Dollar result &&
+	! grep second result
+)
+'
+
 test_done
-- 
2.1.4

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> So, here's the re-rolled patch.

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.

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

[Paul Tan]

Hi Junio,

On Fri, May 22, 2015 at 5:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> So, here's the re-rolled patch.
>
> 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.

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