[PATCH v19 00/11] Support translated long option names in getopt and argp

[Adhemerval Zanella Netto]

On 06/12/25 10:19, Vivien Kraus wrote:
> Dear Adhemerval Zanella Netto and other glibc developers,
> 
> Thank you for your feedback.  It is difficult to find the last bits of
> bad indentation, however I have set a dir-local variable in emacs to
> use tabs as much as possible, so the indentation should be correct in
> the future.
> 
> I added a dependency on $(gen-locales) for the tests.
> 
> Le 01/12/2025 à 21:34, Adhemerval Zanella Netto a écrit :
>>> # Test for the glob symbol version that was replaced in glibc 2.27.
>>>  ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
>>>  tests += \
>>> diff --git a/posix/getopt.c b/posix/getopt.c
>>> index 66b43850ee..b49dfb643f 100644
>>> --- a/posix/getopt.c
>>> +++ b/posix/getopt.c
>>> @@ -182,6 +182,21 @@ exchange (char **argv, struct _getopt_data *d)
>>>    d->__last_nonopt = d->optind;
>>>  }
>>>  
>>> +/* Return 1 iff a translation for opt_name has been found and it
>>> +   matches the substring from argument, length argument_length.
>>> +*/
>>> +static const int
>>> +match_translated_option_name (char *(*translate) (const char *msgid),
>>> +			      const char *argument, size_t argument_length,
>>> +			      const char *opt_name)
>>> +{
>>> +  const char *translated = opt_name;
>>> +  if (translate != NULL)
>>> +    translated = translate (opt_name);
>>> +  return (!strncmp (translated, argument, argument_length)
>>> +	  && argument_length == strlen (translated));
>>> +}
>>> +
>> I think that we can optimize it to the following:
>>
>>   if (translated[argument_length] != '\0')
>>     return 0;
>>   return memcmp (translated, argument, argument_length) == 0;
> 
> I’m not sure about that; what if getopt tries to match a very long
> argument against a shorter translation? This would index the
> translation out of bounds.  Am I missing something?

Yeah, you are correct. I was trying to avoid the need to iterate over 'translated'
twice (one for strncmp and another for strlen).  I think we can avoid the last
strlen with:
 
 const char *translated = opt_name;
 if (translate != NULL)
   translated = translate (opt_name);

 if (strncmp (translated, argument, argument_length) != 0)
   return 0;
 /* We know that strlen(translated) >= argument_length.  */
 return translated[argument_length] == '\0';

> 
>>> +	      if (strcmp (translated_option_name, pfound->name) != 0)
>>> +		{
>>> +		  /* Print both names of the option.  */
>>> +		  fprintf (stderr,
>>> +			   _("%s: option '%s%s' / '%s%s' doesn't allow an argument\n"),
>>> +			   argv[0], prefix, translated_option_name, prefix, pfound->name);
>>> +		}
>>> +	      else
>>> +		{
>>> +		  /* Either the option name is not translated, or its
>>> +		     translation is the same as the option name.  */
>>> +		  fprintf (stderr,
>>> +			   _("%s: option '%s%s' doesn't allow an argument\n"),
>>> +			   argv[0], prefix, pfound->name);
>>> +		}
>>
>> I think the usual GNU code style is to omit brackets for one-line directive.
> 
> I thought the braces were required around a block composed of a
> comment and an instruction, but seeing surrounding code, I now
> understand.

I think I saw both ways on glibc code, so I won't fret about it.

> 
>>> diff --git a/posix/tstgetoptl.c b/posix/tstgetoptl.c
>>> [...]
>>> +
>>> +/* This tests that --colour is accepted as a translation of --color.
>>> +   This echoes tstgetopt.c, where --colour was an option name alias
>>> +   for --color, so it had to be listed twice.  */
>>> +
>>> +/* This uses the en_GB locale so that colour means color.  As a
>>> +   special case, we also check that non-translated options have
>>> +   precedence over translated options, by translated "optional" as
>>> +   "required".  */
>>
>> Should we also check when a translation is required, but there is no
>> suitable option to check for the error paths?
> 
> Do you mean: if the user passes --flavour, there is a “flavor” ->
> “flavour” en_GB translation, but flavor is not an option recognized by
> the program, then getopt should detect an unrecognized option?  I
> added a test for this.

Yes.

> 
>>> diff --git a/posix/getopt1.c b/posix/getopt1.c
>>> [...]
>>> +/* FIXME: use pgettext_expr.  */
>>> +static char *
>>> +do_translate (const char *context, const char *msgid)
>>> +{
>>> +  char *full_msgid;
>>> +  const char *translated = msgid;
>>> +  int output_length = 0;
>>> +
>>> +  if (context != NULL)
>>> +    {
>>> +      output_length = __asprintf (&full_msgid, "%s\004%s", context, msgid);
>>> +      if (output_length >= 0)
>>> +	{
>>> +	  translated = __dcgettext (NULL, full_msgid, LC_MESSAGES);
>>> +	  if (strcmp (translated, full_msgid) == 0)
>>> +	    {
>>> +	      /* No translation for this context and message, so drop
>>> +		 the context + ^D prefix.  */
>>> +	      translated = msgid;
>>> +	    }
>>> +	}
>>> +      /* Otherwise, if memory allocation failed, then we won’t accept
>>> +	 translations.  translated remains an alias to msgid.  */
>>> +      free (full_msgid);
>>
>> Wouldn't this possible return a free pointer as translated? On __dcgettext,
>> it calls __dcigettext (NULL, full_msgid, NULL, 0, 0, LC_MESSAGES) and then:
>>
>>  454 char *
>>  455 DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
>>  456             int plural, unsigned long int n, int category)
>>  457 #endif
>>  458 {
>>  [...]
>>  826   __set_errno (saved_errno);
>>  827   return (plural == 0
>>  828           ? (char *) msgid1
>>  829           /* Use the Germanic plural rule.  */
>>  830           : n == 1 ? (char *) msgid1 : (char *) msgid2);
>>  831 }
>>  488 #endif
>>
>> For the unstranslted case? Or am I missing something here?
> 
> It should not happen, because if __dcgettext returns the allocated
> full_msgid, then we detect it in the “No translation” case, and
> replace it with the non-allocated msgid.  In any case, it should only
> return the exact full_msgid pointer, or nothing that aliases it.  I
> agree that it depends a bit too much on the dgettext implementation,
> so this revision adds a pointer to allocated data that must be kept
> around until we are done with the translation.  Do you prefer it that
> way?
> 
I don't have a strong preference, it is just the __dcgettext is quite complex
and full of redirection that I sometime get lost in it.