sort order regression

sort order regression

[Sebastian Spaeth]

jkr and I noticed that patch series are shown in reverse order now, in
fact threads seem to display messages at the same depth in reverse
chronological order now.

Here is my monologue from IRC:

Is that supposed to show the whole thread in reverse chronological order?:
notmuch show --entire-thread id:"1271927251-19867-3-git-send-email-Sebastian@SSpaeth.de"

doh, http://git.notmuchmail.org/git/notmuch/commitdiff/f43990ce134d838cdb2cdd5d0752a602e81cfdd9
sort order for within threads used to be SORT_OLDEST_FIRST
with that commit it was changed to be inherited from whatever it was (and by default notmuch does NEWEST_FIRST)
so that would cause the change IMHO.
someone does not seem to have run a test suite there :

query.cc sets to NEWEST-FIRST by default. So now we get messages at each depth NEWEST-FIRST. So the code is working as expected.
the problem is imho, that we previously had NEWEST_FIRST for top-level and an explicit OLDEST_FIRST within threads
< jkr> spaetz: yep.
and that explicit OLDEST_FIRST got removed

Re: sort order regression

[Jesse Rosenthal]

On Thu, 22 Apr 2010 16:06:27 +0200, Sebastian Spaeth <Sebastian@SSpaeth.de> wrote:
> 
> jkr and I noticed that patch series are shown in reverse order now, in
> fact threads seem to display messages at the same depth in reverse
> chronological order now.

Just to follow up on this, it seems that the regression comes from the
fix Carl introduced in 2a1a4f0551 to make his simplification of my patch
(simplification = 36e4459a3 , my patch = 4971b85641) pass tests. The
question is whether my original, more complicated version would have
passed the tests without the regressing fix. I do know that it showed
the search order as expected.

Unfortunately, I'm not quite skilled enough at git to turn back some
files to a certain state, tests to another state, and so on.

Best,
Jesse

Re: sort order regression

[Jesse Rosenthal]

On Thu, 22 Apr 2010 10:14:16 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> Just to follow up on this, it seems that the regression comes from the
> fix Carl introduced in 2a1a4f0551 to make his simplification of my patch
> (simplification = 36e4459a3 , my patch = 4971b85641) pass tests. The
> question is whether my original, more complicated version would have
> passed the tests without the regressing fix.

Okay, I just tested using the current test suite: my patch (4971b85641)
passes all tests, while Carl's simplification fails.

My suggestion would be to revert both the simplification and the fix to
enable the simplification to pass: (36e4459a3, 2a1a4f0551).

Should we send reverts as patches?

Best,
Jesse

Re: sort order regression

[Jesse Rosenthal]

On Thu, 22 Apr 2010 10:21:11 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> My suggestion would be to revert both the simplification and the fix to
> enable the simplification to pass: (36e4459a3, 2a1a4f0551).

Sorry, got that slightly wrong. The following commits need to be
reverted:
 
36e4459a328b8449b3e9d510be81a332a9b35aaa
f43990ce134d838cdb2cdd5d0752a602e81cfdd9
7fb56f9dc5d8e66f717f5e48ecbfbc11c8190182

When I do this, search order is right, and all tests are passed.

Best,
Jesse

Re: sort order regression

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Thu, 22 Apr 2010 16:06:27 +0200, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> 
> jkr and I noticed that patch series are shown in reverse order now, in
> fact threads seem to display messages at the same depth in reverse
> chronological order now.

My fault! Sorry about that.

On Thu, 22 Apr 2010 10:14:16 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> Just to follow up on this, it seems that the regression comes from the
> fix Carl introduced in 2a1a4f0551 to make his simplification of my patch

Indeed that was the problem. And your original code *did* pass the tests
I wrote. There are still a couple of cleanups I'd like to make, (like
assigning the subject only in _add_matched_message and not uselessly in
_add_message as well). But I can do those now without breaking the test
suite!

> Unfortunately, I'm not quite skilled enough at git to turn back some
> files to a certain state, tests to another state, and so on.

For now, since the test suite is just a single file, it's easy enough to
do:

	cp test/notmuch-test latest-notmuch-test
        git checkout HEAD~5	# or whatever
	make
	./latest-notmuch-test

Though that doesn't answer the question of how to do this with
git. Probably something like:

	git checkout HEAD~5
	git checkout master -- test
	make test

On Thu, 22 Apr 2010 10:31:18 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> Sorry, got that slightly wrong. The following commits need to be
> reverted:
>  
> 36e4459a328b8449b3e9d510be81a332a9b35aaa
> f43990ce134d838cdb2cdd5d0752a602e81cfdd9
> 7fb56f9dc5d8e66f717f5e48ecbfbc11c8190182
> 
> When I do this, search order is right, and all tests are passed.

I did this. And before doing it I added a new test to show the
regression, (which the reverts then make pass).

So in my defense, it wasn't my fault[*] I didn't notice the
regression. It was the test suite's fault for not testing "notmuch show"
at all.

-Carl

[*] Except that I'm the one that wrote the test suite and didn't put any
"notmuch show" tests into it. Oops!

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: sort order regression

[Sebastian Spaeth]

On 2010-04-22, Carl Worth wrote:
> On 22 Apr 2010, Sebastian Spaeth wrote:
> > 
> > jkr and I noticed that patch series are shown in reverse order now, in
> > fact threads seem to display messages at the same depth in reverse
> > chronological order now.
> 
> My fault! Sorry about that.

No harm done, no need to be sorry. We are pre-1.0 in a development
branch, what should we expect :) ? 

> So in my defense, it wasn't my fault[*] I didn't notice the
> regression. It was the test suite's fault for not testing "notmuch show"
> at all.

Ohh, that is a good point. Maybe I should write some :-). Is the test
suite going to be changed any day now or does it still make sense to
write tests for the "monolitic" test suite?

Sebastian

Re: sort order regression

[Michal Sojka]

On Fri, 23 Apr 2010, Sebastian Spaeth wrote:
> Ohh, that is a good point. Maybe I should write some :-). Is the test
> suite going to be changed any day now or does it still make sense to
> write tests for the "monolitic" test suite?

I do not have a plan to modularize the test suite in a near future.

I guess that currently we are not able to relicense the test library
from git. There is still one missing ack. I've just sent a mail to Junio
about his opinion.

Michal

Re: sort order regression

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

On Fri, 23 Apr 2010 10:15:22 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Fri, 23 Apr 2010, Sebastian Spaeth wrote:
> > Ohh, that is a good point. Maybe I should write some :-). Is the test
> > suite going to be changed any day now or does it still make sense to
> > write tests for the "monolitic" test suite?

I would still like to make things more modular in the future. That is,
I'd like the tests to be categorized and grouped so that one can run a
single group of tests, (or even a single test). I'd also like to have
some dependencies between tests so that when an early test fails, later
tests that depend on that functionality will simply report UNTESTED due
to the earlier failure.

But there's certainly no need to wait for that before adding additional
tests.

> I do not have a plan to modularize the test suite in a near future.

Michal, thanks for all your effort to modularize based on the git test
suite. I know you've had to put continual effort to keep your patches up
to date as things have changed. Sorry that you got blocked by some
relicensing that hasn't quite worked out.

> I guess that currently we are not able to relicense the test library
> from git. There is still one missing ack. I've just sent a mail to Junio
> about his opinion.

Want to replay all the git test-suite commits other than any from the
person with the missing ack? That should get us pretty close to the
current state in git, would give us code we could use, and might even
make it possible for us to submit improvements directly to the upstream
git repository.

Should be a pretty simple "git rebase -i" or so and edit out the lines
From the author of interest. (Simple, assuming it doesn't fall over
completely due to conflicts.)

Just an idea anyway. I also understand if you don't have motivation to
pursue this further. And we can just do our own "modularization" as we
want new features.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: sort order regression

[Michal Sojka]

> Want to replay all the git test-suite commits other than any from the
> person with the missing ack? That should get us pretty close to the
> current state in git, would give us code we could use, and might even
> make it possible for us to submit improvements directly to the upstream
> git repository.
> 
> Should be a pretty simple "git rebase -i" or so and edit out the lines
> From the author of interest. (Simple, assuming it doesn't fall over
> completely due to conflicts.)

I do not fully understand what you propose here. You mean to reply the
commits from git repository into notmuch repository and then put your
relicensing patch on top of it?

We do not need the contribution with missing ack in notmuch test suite
since it is git specific.
 
> Just an idea anyway. I also understand if you don't have motivation to
> pursue this further. And we can just do our own "modularization" as we
> want new features.

The motivation is not the problem. I will only have less time for
notmuch in the upcomming two months.

-Michal

Re: sort order regression

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

On Mon, 26 Apr 2010 11:59:12 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > Want to replay all the git test-suite commits other than any from the
> > person with the missing ack?
> 
> I do not fully understand what you propose here. You mean to reply the
> commits from git repository into notmuch repository and then put your
> relicensing patch on top of it?

I just meant that we could get to a state where there's no code that
hasn't been acked for the license change. I didn't mean that we'd need
all the history inside our repository.

> We do not need the contribution with missing ack in notmuch test suite
> since it is git specific.

That was my suspicion---that the missing contribution wouldn't cause us
any problem.

> The motivation is not the problem. I will only have less time for
> notmuch in the upcomming two months.

Ah well. We've definitely appreciated all of your contributions so far,
and we'll look forward to anything else you might be able to do in the
future.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]