Mattias Engdegård wrote on Sat, May 04, 2013 at 13:55:25 +0200:
> 3 maj 2013 kl. 16.10 skrev Daniel Shahaf:
>
> >It's a hack. It hardcodes the specific first and third arguments
> >to the
> > (os->errfn)("%s: %s: %c\n", _, "invalid option", _);
> >call in APR's unix/getopt.c. The only benefit we get in return is
> >translating six two-word error messages in option parsing.
>
> This "only" benefit is precisely the one intended: to make a small
> set of important error messages translatable.
> Could you please enumerate the negative effects the patch would
> have, in your opinion?
>
The patch achieves its end in an unmaintainable way. It's liable to
break when APR's implementation details change, and since it ultimately
attempts to parse something which wasn't intended to be parsed, it might
do the _wrong_ thing (as opposed to just not doing anything) someday.
I can't see today what other circumstances APR would call errfn->("%s:
%s: %c\n", _, "invalid option", _) in, but they don't guarantee us that
there won't be other circumstances so I tend not to assume that they
won't. That's how I treat Subversion APIs that I write and call, and
that's how I treat APR too: I rely on promises, not on implementation
details.
> Since the patch:
> 1) solves an annoying problem,
Which no one complained about.
Keep in mind that error message in the libraries are translated, and
'svn help' is translated. Is it really that important to translate
'invalid option'? If it is, wouldn't someone have complained about it
on users@?
> 2) does so by adding a small amount of code in a single place,
> 3) with robust failure modes (if anything breaks, we will just fall
> back to the old behaviour),
> 4) without changing the behaviour in any way whatsoever for those
> living in LANG=C,
> 5) a "proper" solution is likely to be a lot more invasive, involve
> cross-project work, require new library versions and certainly would
> not be done for 1.8.x for any x, and most likely will never happen,
>
> it does not seem to be something that should be very controversial,
> should it?
Feel free to point me at another place in svn's code that ignores API
non-promises or parses something that wasn't intended to be parsed.
We don't do that, and we frown upon users who try to do that to us.
(For example, we advised users not to use rsync with FSFS repository for
years before we actually implemented something that caused that to be a
potentially-corruption-causing workflow.)
Received on 2013-05-06 23:39:52 CEST