Re: Jens, please try to get more peer review

On Thu, May 29, 2008 at 04:25:00PM +0200, Jens Seidel wrote:
> Hi Stefan,
>
> On Thu, May 29, 2008 at 11:19:48AM +0200, Stefan Sperling wrote:
> > you have made a few commits outside the area that you have
> > been nominated a partial committer for (German translations).
>
> ahm, yes. But really not far outside :-)
>
> > This is great, I am very happy to see you contributing to
> > Subversion even more than by translating -- which is
> > already quite some job! I've tried updating the German
> > translation before, and it took me ages, so I was very glad
> > when you sent another update in time for 1.5 :)
>
> Now I expect that you find and report errors in it :-)

I could run svn with LANG=de_DE for a while I guess :)

> > But I think that some of your commits made outside the German
> > translation should have gotten more peer review. Specifically,
> > you have never put an 'Approved-by:' line in any of your commits
> > outside of your area (r31457, r31459, r31460, r31461, r31484,
> > r31491, and r31496).
>
> As you noticed, most are just trivial 'obvious fixes' ...
> r31457: TRANSLATING: Removed broken links.
>
> r31459: Added svn:executable flags to various Python files.
> r31460: Added svn:executable flags to various Shell files.

These are obvious fixes.

> Daniel Shahaf added +1 in
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=139124
> and mentioned it a "obvious fix" which doesn't require approval.

If anyone expressed approval in any way, say so in your commit!
This shows people looking at your commits that you have indeed
discussed this with others.

> Should I add "Approved-by:" to this kind of patches?

Add it whenever someone says '+1' or otherwise blesses your changes.

> To be honest, what could I made wrong with such a commit? I even
> did not add additional Shebang lines to qualify for more executable
> flags ... Are there volunteers to do this?
>
> r31461: Removed a useless space in front of \n in a message.

Any change in output could cause tests to break, as you have seen.
So while this may be an obvious fix, it may not be trivial. In this
case, please make sure all tests pass in your environment (of course,
we cannot expect everybody to test in all possible environments).

> r31484: Use workaround for an xgettext limitation.
> r31496: Marked a further string explicitly translatable which is also
> missed by xgettext.
>
> It affected my translations stats and I posted it to the list first :-)
> In summary it just added a macro which expands to an empty string
> and few comments. In the 1.5.x branch I just modified all PO files
> to get this fixed.
>
> r31491: Fixed a build error for Windows introduced in r31484.
>
> If I make an error I have to correct it :-)

Yeah, that was one that your environment could not catch.
I couldn't have caught that, either (I don't run Windows).

But we forgot "r31506: Added a missing comma", which is the one
that essentially triggered my original mail. You could have
caught this one by running regression tests, I believe.
But don't worry, mistakes happen :)

> Erik Huelsmann explained how to fix it but gave no explicit approval.

You can say 'Suggested by: <name>' in such a case.

For full/partial committers, <name> is their tigris.org login in
COMMITTERS. Otherwise, use 'Full Name <email>'.

> > Why not?
>
> Most of the time because the 'obvious fix'. If there was any doubt
> I also wrote to this list. I also just forgot adding this flag at
> least once.

OK. Maybe we should require obvious fixes to be designated as such?
Such as: "Obvious fix: Remove trailing whitespace"

This would make it more obvious that the obvious fix rule was applied.
Should we update HACKING accordingly?

> > Please try to get your commits outside the German translation
> > approved in the future.
>
> OK, will do so.

Great, thank you!
Stefan