Need feedback on first four UTF-7 patches

[Alex Henrie]

Alexandre,

I submitted four UTF-7 patches on October 19 and am still waiting for
feedback. I was hoping that these patches would not be controversial
because they only add encoding tests. I have six additional patches
waiting in https://github.com/alexhenrie/wine/commits/master - four
patches for adding decoding tests and two patches to make all the
tests pass - but I will not submit them until I get feedback about the
first four that I have already submitted.

-Alex

[Ruslan Kabatsayev]

Hi Alex,

If you look at http://source.winehq.org/patches/ , you'll see that
there're some patches which are a month old, but still having "New"
status. So please be patient, your ones are not that old yet :)

Regards,
Ruslan

On Tue, Oct 28, 2014 at 11:13 PM, Alex Henrie  wrote:
> Alexandre,
>
> I submitted four UTF-7 patches on October 19 and am still waiting for
> feedback. I was hoping that these patches would not be controversial
> because they only add encoding tests. I have six additional patches
> waiting in https://github.com/alexhenrie/wine/commits/master - four
> patches for adding decoding tests and two patches to make all the
> tests pass - but I will not submit them until I get feedback about the
> first four that I have already submitted.
>
> -Alex

[Indrek Altpere]

Well, indeed, but is this really a good target to strive for and bring out
as an example?
The 4 patches add only a number of tests that verify difference between wine
and windows UTF-7 encoding, missing stuff is marked as wine_todo, testbot
has marked them as passing (meaning todo_wine marked tests are expected to
fail on current wine and do so and same tests are supposed to pass on
windows and do so), no actual implementation that would change the behavior
of wine itself.
So unless there is something wrong with the coding style (which hasn't been
commented on after the last patches), shouldn't such patches (only tests,
verified as valid by testbot) get included a lot faster than 10-30 days??

Regards,
Indrek Altpere
-----Original Message-----
From: wine-devel [mailto:[e-mail]] On Behalf Of Ruslan
Kabatsayev
Sent: Wednesday, October 29, 2014 9:48 AM
To: Alex Henrie; Alex Henrie
Cc: Alexandre Julliard; Wine Devel; Alexandre Julliard; Wine Devel
Subject: Re: Need feedback on first four UTF-7 patches

Hi Alex,

If you look at http://source.winehq.org/patches/ , you'll see that there're
some patches which are a month old, but still having "New"
status. So please be patient, your ones are not that old yet :)

Regards,
Ruslan

On Tue, Oct 28, 2014 at 11:13 PM, Alex Henrie 
wrote:
> Alexandre,
>
> I submitted four UTF-7 patches on October 19 and am still waiting for 
> feedback. I was hoping that these patches would not be controversial 
> because they only add encoding tests. I have six additional patches 
> waiting in https://github.com/alexhenrie/wine/commits/master - four 
> patches for adding decoding tests and two patches to make all the 
> tests pass - but I will not submit them until I get feedback about the 
> first four that I have already submitted.
>
> -Alex

[GOUJON Alexandre]

On 10/29/2014 09:47 AM, Indrek Altpere wrote:
> Well, indeed, but is this really a good target to strive for and bring out
> as an example?
> The 4 patches add only a number of tests that verify difference between wine
> and windows UTF-7 encoding, missing stuff is marked as wine_todo, testbot
> has marked them as passing (meaning todo_wine marked tests are expected to
> fail on current wine and do so and same tests are supposed to pass on
> windows and do so), no actual implementation that would change the behavior
> of wine itself.
> So unless there is something wrong with the coding style (which hasn't been
> commented on after the last patches), shouldn't such patches (only tests,
> verified as valid by testbot) get included a lot faster than 10-30 days??
>
> Regards,
> Indrek Altpere

Keep in mind that developers time is very scarce because they (amongst 
many other tasks) :
- read bugzilla reports
- install the app, trigger the bug
- scratch some patch to aid debugging
- test expected behavior on windows machines (real or virtual)
- refine their patch
- check for non-regression with the testbot
- sometimes, ask for comments on wine-devel
but they also
- read patches from the patch queue when they are confident in this wine 
area (check coding style, variable names, logic, error handling...)
- test and comment them
- read technical specifications
- write lengthy e-mails when things are complicated

AJ is doing all of that *plus* he manages his team and manages *all the* 
patches queue

They are doing that every day for many years and even if it's 
perfectible, don't waste their precious time and stop whining

Please be patient, if nothing happens, try doing a more convincing 
argument (add tests, reference bug id you solve...) and finally, resend 
your patch

[Henri Verbeet]

On 29 October 2014 09:47, Indrek Altpere  wrote:
> So unless there is something wrong with the coding style (which hasn't been
> commented on after the last patches), shouldn't such patches (only tests,
> verified as valid by testbot) get included a lot faster than 10-30 days??

In general, yes, if your patches are still "New" after about a week,
they're probably going to stay that way. That usually only happens
when not reviewing the patches is likely to end up being more of a
benefit to the project than reviewing them.

[Indrek Altpere]

My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases.
To which I pointed out that the 4 patches Alex asked about are seemingly simple patches, that only add tests and those patches have already been verified to run correctly by testbot.

To Goujons's reply: "don't waste their precious time and stop whining"
As per Alex's comment from https://www.winehq.org/pipermail/wine-devel/2014-October/105419.html
"slackner, puk, and Andre_H gave me a ton of help over IRC over the past couple of days"
Time has already been spent (wasted?) by other active Wine developers who are also constantly contributing to Wine.

Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status?

Regards,
Indrek Altpere

-----Original Message-----
From: Henri Verbeet [mailto:[e-mail]] 
Sent: Wednesday, October 29, 2014 1:58 PM
To: Indrek Altpere
Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre Julliard; Wine Devel
Subject: Re: Need feedback on first four UTF-7 patches

On 29 October 2014 09:47, Indrek Altpere  wrote:
> So unless there is something wrong with the coding style (which hasn't 
> been commented on after the last patches), shouldn't such patches 
> (only tests, verified as valid by testbot) get included a lot faster than 10-30 days??

In general, yes, if your patches are still "New" after about a week, they're probably going to stay that way. That usually only happens when not reviewing the patches is likely to end up being more of a benefit to the project than reviewing them.

[Henri Verbeet]

On 29 October 2014 13:21, Indrek Altpere  wrote:
> My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases.

Yeah, it's not.
> Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status?

The patch status page seems pretty clear to me. Put a different way,
suppose you e.g. had additional information about why a patch is still
"New", what would you do with that information?

[Indrek Altpere]

Well, looking at the list, there are patches that are marked as rejected: those are pretty clear that there is some issue with the patch/patchset.

But seeing a patch row marked as New and for which testbot runs have completed successfully gives no definitive feedback about the actual status.
There is no way to differentiate if a patch has been forgotten accidentally, or ignored on purpose (for the benefit of the project) or postponed for later review (in a week or two).

For me personally, in this case, I wouldn't do much with the information since I'm not the author of those patches nor did I help review them. But it would conserve time for both the developers and reviewers if there is a clearer marker of status, instead of guessing it via calendar and/or polling the wine-devel list.

Regards,
Indrek Altpere

-----Original Message-----
From: Henri Verbeet [mailto:[e-mail]] 
Sent: Wednesday, October 29, 2014 2:42 PM
To: Indrek Altpere
Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre Julliard; Wine Devel
Subject: Re: Need feedback on first four UTF-7 patches

On 29 October 2014 13:21, Indrek Altpere  wrote:
> My previous response was mainly to Ruslan's reply, that 30 days waiting time could/should be considered the norm in some cases.

Yeah, it's not.
> Perhaps there should be some more clear way to let the developers know that their patches are still not good enough (even to be reviewed and commented on) instead of checking the calendar to guess the status?

The patch status page seems pretty clear to me. Put a different way, suppose you e.g. had additional information about why a patch is still "New", what would you do with that information?

[Henri Verbeet]

On 29 October 2014 14:05, Indrek Altpere  wrote:
> But seeing a patch row marked as New and for which testbot runs have completed successfully gives no definitive feedback about the actual status.
> There is no way to differentiate if a patch has been forgotten accidentally, or ignored on purpose (for the benefit of the project) or postponed for later review (in a week or two).

I get that, but what I'm saying is that in either case you're not
going to make the patch go in quicker by e.g. sending another three
versions of the same patch to the patches list.

The first case is pretty rare, and I don't think having a separate
"Forgotten" state would really help there. The second and third case
are perhaps more interesting, but not necessarily distinct. It's not
so much that when reviewing patches you arbitrarily decide to not look
at a patch for two weeks, but rather that if it's e.g. a slow day you
may review a couple of patches that you normally wouldn't have
bothered with.

[Michael Stefaniuc]

On 10/29/2014 01:21 PM, Indrek Altpere wrote:
> My previous response was mainly to Ruslan's reply, that 30 days
> waiting time could/should be considered the norm in some cases. To
> which I pointed out that the 4 patches Alex asked about are
> seemingly simple patches, that only add tests and those patches
> have already been verified to run correctly by testbot.
>
> To Goujons's reply: "don't waste their precious time and stop
> whining" As per Alex's comment from
> https://www.winehq.org/pipermail/wine-devel/2014-October/105419.html

"slackner, puk, and Andre_H gave me a ton of help over IRC over the past
couple of days"
> Time has already been spent (wasted?) by other active Wine
> developers who are also constantly contributing to Wine.
>
> Perhaps there should be some more clear way to let the developers
> know that their patches are still not good enough (even to be
> reviewed and commented on) instead of checking the calendar to
> guess the status?

Oh, we have provided a ton of feedback. And the patches did improve from
a test coverage point of view and that isn't anymore an issue.

But the code still could look a lot better and some of the previous
feedback wasn't addressed. There is an impedance mismatch between the
code looking good for Alex and looking good for Wine.

bye
    michael
> -----Original Message----- From: Henri Verbeet
> [mailto:[e-mail]] Sent: Wednesday, October 29, 2014 1:58
> PM To: Indrek Altpere Cc: Ruslan Kabatsayev; Alex Henrie; Alexandre
> Julliard; Wine Devel Subject: Re: Need feedback on first four UTF-7
> patches
>
> On 29 October 2014 09:47, Indrek Altpere 
> wrote:
> > So unless there is something wrong with the coding style (which
> > hasn't been commented on after the last patches), shouldn't such
> > patches (only tests, verified as valid by testbot) get included a
> > lot faster than 10-30 days??
>
> In general, yes, if your patches are still "New" after about a
> week, they're probably going to stay that way. That usually only
> happens when not reviewing the patches is likely to end up being
> more of a benefit to the project than reviewing them.

[Alex Henrie]

2014-10-29 6:45 GMT-06:00 Michael Stefaniuc :
> But the code still could look a lot better and some of the previous
> feedback wasn't addressed. There is an impedance mismatch between the
> code looking good for Alex and looking good for Wine.

As far as I know, I only rejected one of your suggestions for these 4
patches: Merging the simple_test and complex_test structs. The 3
simple tests are actually used as inputs for 8 tests each, for a total
of 24 tests. Reformatting the 3 simple tests as complex tests would
force me to manually input the 24 combinations into the complex_tests
array. Is that really what Alexandre wants?

-Alex

[Sebastian Lackner]

On 29.10.2014 20:50, Alex Henrie wrote:
> 2014-10-29 6:45 GMT-06:00 Michael Stefaniuc :
> > But the code still could look a lot better and some of the previous
> > feedback wasn't addressed. There is an impedance mismatch between the
> > code looking good for Alex and looking good for Wine.
>
> As far as I know, I only rejected one of your suggestions for these 4
> patches: Merging the simple_test and complex_test structs. The 3
> simple tests are actually used as inputs for 8 tests each, for a total
> of 24 tests. Reformatting the 3 simple tests as complex tests would
> force me to manually input the 24 combinations into the complex_tests
> array. Is that really what Alexandre wants?
>
> -Alex

It is not really true that this was the only thing we criticized. We also gave you a lot more suggestions for improvements, but from the beginning I had the impression that you want to keep changes as minimal as possible. Thats not a good attitude when trying to upstream code, and will probably be especially a problem for the implementation itself. You always have to consider multiple methods how a specific problem can be solved.

For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.

[Alex Henrie]

2014-10-29 14:37 GMT-06:00 Sebastian Lackner :
> For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.

Again, thanks for the feedback.

For the record, I am holding off on the following suggestions until I
receive feedback from Alexandre:

- Merging the complex and simple structs (this would decrease test coverage)
- Splitting up the ok() statements into pieces not joined by && (this
seems like overkill)
- Rebasing the non-struct-based tests to be ahead of the struct-based
tests in the patch series (this would make it difficult to write a
simple check for the presence or absence of UTF-7 support)

You can see that despite my frustration with this process, I have
incorporated the rest of your suggestions at
https://github.com/alexhenrie/wine/commits/master

-Alex

[Alexandre Julliard]

Alex Henrie  writes:
> 2014-10-29 14:37 GMT-06:00 Sebastian Lackner :
> > For the tests: The main issue (bad test coverage) is fixed at my
> > opinion, but there are still lot of small things which could be
> > improved to make the code more readable. Most of it was already told
> > on IRC, but I will go over patch 1 one more time and tell you again.
>
> Again, thanks for the feedback.
>
> For the record, I am holding off on the following suggestions until I
> receive feedback from Alexandre:

If you want more feedback, you should first address all the issues that
you have already received feedback about.