On Wed, Jun 11, 2025 at 7:26 PM Timofei Zhakov <t...@chemodax.net> wrote:
> (snip) > > Branko, I'm so confused from the entire thread and all those points you > are mentioning. > > 1. I would like to clarify this change. This wasn't made to fix that > problem in the first place. I noticed two unrelated things which were wrong > (in my opinion) in the utf header file/implementation. First - I wanted to > make SVN_APR_LOCALE_CHARSET and SVN_APR_DEFAULT_CHARSET not just aliases > to APR, but a complete part of the Subversion API. And then I noticed we > can also remove that include (now I understand this was a breaking change > for backward compatibility). > > 2. Initially, you've vetoed the whole revision (plus follow-ups). This is > fine. I respect vetoes. But your reasoning was that "apr headers may > change". Fine, but I've clarified that it would still work even if APR's > and SVN's constants will be different in the future. > > 3. It seemed that you agreed with this. However, backward compatibility > was broken by removing apr_xlate.h from svn_utf.h. > > 4. I thought those were the only mistakes in that commit, affected by the > veto. I reverted this change without changing the remaining part. > > 5. Eventually you are saying you are still standing with your veto even > though I think I've clearly reasoned it and reverted the part you weren't > agreeing with (see 4). > > I can't get it. What exactly am I doing wrong? Just because you don't > like it? > > Again, I respect vetoes and, of course, your opinion. I just want our > developing process and communication to be... like more straight-forward, > you know. I would like the new features to be more appreciated and > discussed, rather than be completely rejected due to minor and fixable > mistakes. It's fine to make mistakes. And those should be discussed and > fixed without blaming either authors and their opinions or anything else > like this. This is my understanding of the commit-then-review policy that > we use in svn. > > ps: I've drafted a patch that adds the same alias as for those two > (default and current locale) for utf8 as well, which would be also really > useful for utf8-cmdline-prototype, since I had to specify "UTF-8" constant > for multiple times. Initially I wanted to discuss it in a separate thread, > but I don't think it's worthed at this moment, so I'll just attach it > here, just to have it. > > -- > Timofei Zhakov > Hi Timofei, Regarding the patch: IMHO it makes sense to have a #define, rather than hardcode the string "UTF-8" in a bunch of places. It's more greppable, improves clarity regarding the intended uses, and won't get mixed up with unrelated instances of the text "UTF-8" when grepping for use sites. But, wouldn't it be safer to define it as: #define SVN_UTF8_CHARSET "UTF-8" rather than (const char *)2 ? I understand the downside of a string: comparisons must be made with strcmp. But it is safer in the sense that it doesn't depend on the current values of the APR constants. This was Brane's earlier feedback about not relying on them never changing. I agree with that part: APR is a separate library. Our implementation should be agnostic of the actual values behind defines. Also, in response to my earlier lengthy email, Brane clarified that users of APR are supposed to use only those two names (APR_LOCALE_CHARSET or APR_DEFAULT_CHARSET), or a string. So, I recommend that we work with APR's intention rather than against it and define it as a string. Thoughts? Thanks, Nathan
