re: [PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

Hi Frédéric,
your patch series seems to have failed the cmd tests:

batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301
(got 'Syntax error', wanted '1')
batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302
(got '1', wanted '3')
batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303
(got '3', wanted '5')

See http://buildbot.kegel.com/builders/runtests/builds/86

I'll change the buildbot so it tests each subset of a patch series,
and pinpoint the one that broke things.

Incidentally, that 'Syntax error' probably should go to stderr;
and since the cmd tests ignore stderr, that might fix your failure.

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/19 Dan Kegel :
> Hi Frédéric,
> your patch series seems to have failed the cmd tests:
>
> batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301
> (got 'Syntax error', wanted '1')
> batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302
> (got '1', wanted '3')
> batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303
> (got '3', wanted '5')
>
> See http://buildbot.kegel.com/builders/runtests/builds/86
>
> I'll change the buildbot so it tests each subset of a patch series,
> and pinpoint the one that broke things.
>
> Incidentally, that 'Syntax error' probably should go to stderr;
> and since the cmd tests ignore stderr, that might fix your failure.

They should eventually go to stderr, but I wouldn't change them right now.
The current tests can run successfully only if the # lines match in
the output / expected files, so putting them to stderr instead might/will break
things.
Also, they are (only?) generated for perfectly valid constructs due to
parsing bugs in current code.

I don't know how your code works, and it's probably done already, but
in certain cases if tests fail, some test files/dirs can be left in
the test directory, so a preliminary cleanup is probably a wise idea.

Frédéric

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/19 Frédéric Delanoy :
> > Incidentally, that 'Syntax error' probably should go to stderr;
> > and since the cmd tests ignore stderr, that might fix your failure.
>
> They should eventually go to stderr, but I wouldn't change them right now.
> The current tests can run successfully only if the # lines match in
> the output / expected files, so putting them to stderr instead might/will break
> things.

Do our tests pass on windows cmd?  That outputs errors to stderr, I think.
> I don't know how your code works, and it's probably done already, but
> in certain cases if tests fail, some test files/dirs can be left in
> the test directory, so a preliminary cleanup is probably a wise idea.

The buildbot always does clean builds/tests.
- Dan

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/19 Dan Kegel :
> Hi Frédéric,
> your patch series seems to have failed the cmd tests:
>
> batch.c:259: Test failed: unexpected char 0x53 position 0 in line 301
> (got 'Syntax error', wanted '1')
> batch.c:259: Test failed: unexpected char 0x31 position 0 in line 302
> (got '1', wanted '3')
> batch.c:259: Test failed: unexpected char 0x33 position 0 in line 303
> (got '3', wanted '5')
>
> See http://buildbot.kegel.com/builders/runtests/builds/86
>
> I'll change the buildbot so it tests each subset of a patch series,
> and pinpoint the one that broke things.

The last [10/10] patch was the cause. I sent an updated version.

I wonder how/whether the various (resend) or (try N) are handled for
revised patch series

Frédéric

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/19 Frédéric Delanoy :
> The last [10/10] patch was the cause. I sent an updated version.

Hmm.  You sent 10/10 before 1/10.  My patch series recognizer
rejected the series.  It would be hard to fix.  Let's see if we can
live with the rule "patch series must be sent in order".
> I wonder how/whether the various (resend) or (try N) are handled for
> revised patch series

My recognizer does not notice those designations, nor the time or
order of arrival.
It only looks at the 'Date:' and 'From:' fields, and the '[%d/%d]'
part of the subject line.
(The exact regular expression I use is [\D*(\d+)/(\d+)\D*] .)

Humans prefer (resend) for unchanged resends, and (try N+1) for
changed resends, I think.
- Dan

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/20 Dan Kegel :
> 2011/8/19 Frédéric Delanoy :
> > The last [10/10] patch was the cause. I sent an updated version.
>
> Hmm.  You sent 10/10 before 1/10.

Yeah, I thought it was sufficient, but testbot choked on it and waited
for the remaining patches in the series.
> My patch series recognizer
> rejected the series.  It would be hard to fix.  Let's see if we can
> live with the rule "patch series must be sent in order".
> > I wonder how/whether the various (resend) or (try N) are handled for
> > revised patch series
>
> My recognizer does not notice those designations, nor the time or
> order of arrival.
> It only looks at the 'Date:' and 'From:' fields, and the '[%d/%d]'
> part of the subject line.
> (The exact regular expression I use is [\D*(\d+)/(\d+)\D*] .)

Using the dates of the git format-patches can be problematic, e.g.
when you local git branch has been modified before submission (think
rebase -i)
> Humans prefer (resend) for unchanged resends, and (try N+1) for
> changed resends, I think.

As I said, I have sent an updated/fixed version of the [10/10]. Then
(for the unchanged one) [1/10] <...> (resend) (try 2) ... [9/10] <...>
(resend) (try 2) once I saw testbot was waiting for the rest of the
series, which it processed afterwards

IIRC someone told me some time ago to resent the whole series with
(try N+1) for tests processed by testbot. The (resend) is just a hint
to AJ that that very patch hasn't been revised, just regenerated.

IMHO your patch series recognizer should use the same strategy as
testbot, for consistency, and wait for the whole series to be
available before processing can start.

Frédéric

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/20 Frédéric Delanoy :
> IIRC someone told me some time ago to resent the whole series with
> (try N+1) for tests processed by testbot. The (resend) is just a hint
> to AJ that that very patch hasn't been revised, just regenerated.

Yeah.  And sometimes the body says "No changes since last try",
in which case maybe even the (resend) hint isn't needed.
> IMHO your patch series recognizer should use the same strategy as
> testbot, for consistency

I see his code at
http://wine.git.sourceforge.net/git/gitweb.cgi?p=wine/winetestbot;a=blob;f=l...
http://wine.git.sourceforge.net/git/gitweb.cgi?p=wine/winetestbot;a=blob;f=l...
and can see a few details, but his overall strategy is hard to discern
in a quick read.
> and wait for the whole series to be available before processing can start.

That I do.
- Dan

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/20 Dan Kegel :
> 2011/8/20 Frédéric Delanoy :
> > IIRC someone told me some time ago to resent the whole series with
> > (try N+1) for tests processed by testbot. The (resend) is just a hint
> > to AJ that that very patch hasn't been revised, just regenerated.
>
> Yeah.  And sometimes the body says "No changes since last try",
> in which case maybe even the (resend) hint isn't needed.

Probably more straightforward to use the (resend) in the subject, so
you don't have to check the message contents

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Michael Stefaniuc]

On 08/20/2011 05:06 AM, Dan Kegel wrote:
> 2011/8/19 Frédéric Delanoy :
> > The last [10/10] patch was the cause. I sent an updated version.
>
> Hmm.  You sent 10/10 before 1/10.  My patch series recognizer
> rejected the series.  It would be hard to fix.  Let's see if we can
> live with the rule "patch series must be sent in order".

You cannot rely on that. git send-email of a patch series pushes the
emails very fast out and with MX server clusters you run into race
conditions.

bye
    michael

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc  wrote:
> On 08/20/2011 05:06 AM, Dan Kegel wrote:
> > Hmm.  You sent 10/10 before 1/10.  My patch series recognizer
> > rejected the series.  It would be hard to fix.  Let's see if we can
> > live with the rule "patch series must be sent in order".
>
> You cannot rely on that. git send-email of a patch series pushes the
> emails very fast out and with MX server clusters you run into race
> conditions.

I thought the Date: field was set by the client, so server
races shouldn't matter.

If I run into messages with identical dates, I could sort
first by date, then by patch number within the series.
- Dan

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Michael Stefaniuc]

On 08/20/2011 04:28 PM, Dan Kegel wrote:
> On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc  wrote:
> > On 08/20/2011 05:06 AM, Dan Kegel wrote:
> > > Hmm.  You sent 10/10 before 1/10.  My patch series recognizer
> > > rejected the series.  It would be hard to fix.  Let's see if we can
> > > live with the rule "patch series must be sent in order".
> >
> > You cannot rely on that. git send-email of a patch series pushes the
> > emails very fast out and with MX server clusters you run into race
> > conditions.
>
> I thought the Date: field was set by the client, so server
> races shouldn't matter.

Sorry, missed that you use that and not the order you receive it.
> If I run into messages with identical dates, I could sort
> first by date, then by patch number within the series.

bye
    michael

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

On Sat, Aug 20, 2011 at 16:34, Michael Stefaniuc  wrote:
> On 08/20/2011 04:28 PM, Dan Kegel wrote:
> > On Sat, Aug 20, 2011 at 3:43 AM, Michael Stefaniuc  wrote:
> > > On 08/20/2011 05:06 AM, Dan Kegel wrote:
> > > > Hmm.  You sent 10/10 before 1/10.  My patch series recognizer
> > > > rejected the series.  It would be hard to fix.  Let's see if we can
> > > > live with the rule "patch series must be sent in order".
> > >
> > > You cannot rely on that. git send-email of a patch series pushes the
> > > emails very fast out and with MX server clusters you run into race
> > > conditions.
> >
> > I thought the Date: field was set by the client, so server
> > races shouldn't matter.
>
> Sorry, missed that you use that and not the order you receive it.

That's not always reliable: say you commit locally patches [1-2/3] on
day D and patch [3/3] on day D+1
P3 - baz - D+1
P2 - bar - D
P1 - foo - D

For whatever reason, before submission, you decide patch [3/3] should
be the first one, so you use "git rebase" do to that:
P3 - bar - D
P2 - foo - D
P1 - baz - D+1

and you git-send the mails.

So, you should always use the numbering specified by the author IMHO
> > If I run into messages with identical dates, I could sort
> > first by date, then by patch number within the series.

Frédéric

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/20 Frédéric Delanoy :
> > > I thought the Date: field was set by the client, so server
> > > races shouldn't matter.
> >
> > Sorry, missed that you use that and not the order you receive it.
>
> That's not always reliable: say you commit locally patches [1-2/3] on
> day D and patch [3/3] on day D+1
> P3 - baz - D+1
> P2 - bar - D
> P1 - foo - D
>
> For whatever reason, before submission, you decide patch [3/3] should
> be the first one, so you use "git rebase" do to that:
> P3 - bar - D
> P2 - foo - D
> P1 - baz - D+1
>
> and you git-send the mails.

git-send's messages' timestamps use current time, not commit time:
http://git.kernel.org/?p=git/git.git;a=blob;f=git-send-email.perl#l98
And it is careful to increment the time after each message!
So I think my usage is compatible with git-send.
> So, you should always use the numbering specified by the author IMHO

I wish it were so easy.  It is very difficult to recognize patch series
without relying on them being sent in order.
I've tried:
  take all unprocessed messages
  divide them by sender
  further divide them into groups based on length of patch series in
subject line
  sort each group by patch number from subject line
but that breaks down if the developer sends two patch series with
the same length, which happens very often, e.g. when somebody
retries a patch series.  Maybe one could further subdivide messages
by retry number, but that sounds hard and fragile.
- Dan

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/20 Dan Kegel :
> 2011/8/20 Frédéric Delanoy :
> > So, you should always use the numbering specified by the author IMHO
>
> I wish it were so easy.  It is very difficult to recognize patch series
> without relying on them being sent in order.
> I've tried:
>  take all unprocessed messages
>  divide them by sender
>  further divide them into groups based on length of patch series in
> subject line
>  sort each group by patch number from subject line
> but that breaks down if the developer sends two patch series with
> the same length, which happens very often, e.g. when somebody
> retries a patch series.  Maybe one could further subdivide messages
> by retry number, but that sounds hard and fragile.

Hmm well we/AJ could institute some rules for resending patch series,
so that the whole series is resubmitted with the same (try N)
"marker", even when only one patch is changed, possibly preceded by a
(resend) [i.e. the subdivision by retry number you're talking about],
and document that in the wiki.

That would probably help a bit, but in that case, there should be a
filter on wine-patches/testbot so it refuses ill-formed messages (like
with (resen*t*), etc.) in order to enforce that rule,
e.g. by checking the subject line with a regexp like  ^.*( resend)?( try \d+)?$

Just my 2 ¢

Frédéric

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/19 Dan Kegel :
> I'll change the buildbot so it tests each subset of a patch series,
> and pinpoint the one that broke things.

For the record, this is implemented now, e.g.
http://buildbot.kegel.com/builders/runtests/builds/99  is 9/10, and is ok
http://buildbot.kegel.com/builders/runtests/builds/100 is 10/10, and fails

And it was handy already:
http://buildbot.kegel.com/builders/runtests/builds/103  was 1/3, and was ok
http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and
had problems.

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Frédéric Delanoy]

2011/8/20 Dan Kegel :
> 2011/8/19 Dan Kegel :
> > I'll change the buildbot so it tests each subset of a patch series,
> > and pinpoint the one that broke things.
>
> For the record, this is implemented now, e.g.
> http://buildbot.kegel.com/builders/runtests/builds/99  is 9/10, and is ok
> http://buildbot.kegel.com/builders/runtests/builds/100 is 10/10, and fails
>
> And it was handy already:
> http://buildbot.kegel.com/builders/runtests/builds/103  was 1/3, and was ok
> http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and
> had problems.

I like the "blamelist" section ;)
But seriously you could probably remove it altogether since the email
address is already in the "Reason" (unless I misunderstand its
function)

Frédéric

NB: the time to process each patch seems rather high (14 mins). Maybe
you could use ccache to speed up things

[PATCH 01/10] cmd: Use CSTR_* instead of hardcoded values as result of CompareStringW

[Dan Kegel]

2011/8/20 Frédéric Delanoy :
> > And it was handy already:
> > http://buildbot.kegel.com/builders/runtests/builds/103  was 1/3, and was ok
> > http://buildbot.kegel.com/builders/runtests/builds/104 was 2/3, and
> > had problems.
>
> I like the "blamelist" section ;)
> But seriously you could probably remove it altogether since the email
> address is already in the "Reason" (unless I misunderstand its
> function)

I would have to change buildbot's code to make it only show
up in one of those places, I think.
> NB: the time to process each patch seems rather high (14 mins). Maybe
> you could use ccache to speed up things

True, I should try that.

And patch series could be sped up even more by doing
incremental builds for the 2nd, 3rd, etc. patches in the series.
I haven't looked into how easy/hard that would be.
- Dan