git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3] Sanity-check config variable names

From
LPLibor Pechacek <lpechacek@suse.cz>
Date
Jan 28, 2011, 14:53 UTC
Message-ID
<20110128145323.GE1849@fm.suse.cz>
In-Reply-To
<7voc72ge4j.fsf@alter.siamese.dyndns.org>
On Thu 27-01-11 14:45:16, Junio C Hamano wrote:
Show 11 quoted lines
> Libor Pechacek <lpechacek@suse.cz> writes:
> > Fixed the typo and return values from get_value and git_config_set_multivar.
> > We have changed git_config_parse_key to return negative values on error, but
> > forgot to negate the numbers when returning them as exit codes.
> 
> Earlier get_value() returned:
> 
>  -1: when it saw an uncompilable regexp (either in key or value);
>   0: when a value was available (under --all) or unique (without --all);
>   1: when the requested variable is missing; and
>   2: when the requested variable is multivalued under --all.

Fixed one part with the last change and broke the other one. Thanks for catching it. The same return value for "invalid key" and "invalid regex" is OK for me.

BTW is it OK to exit(-1);? The return value of get_value() gets propagated to the process exit status. At the same time shell uses values >128 to indicate that the process was terminated by a signal.

[...]
> When moving an existing code segment around like this, I would not mind to
> see style fixes thrown in to the patch, as long as the moved code is small
> enough.  Perhaps like this:
I've added the style fix into the patch.
[...]
Show 6 quoted lines
> > +/* Auxiliary function to sanity-check and split the key into the section
> 
> 	/*
>        * Style. We write our multi-line comments
> 	 * like this.
>        */
Fixed.
> > +int git_config_parse_key(const char *key, char **store_key, int *baselen_)
[...]
> 
> Does it make sense for this function to be prepared to get called with
> NULL in store_key like this (and in the remainder of your patch)?

No, I wrote it unnecessarily generic. Removed the excess code. Thanks for pointing it out.

[...]
> > +	test_must_fail git -c name=value config core.name
> >  '
> 
> Don't you want to make sure that your sanity check triggers in new tests?
Added a few tests after getting familiar with the test suite.
Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague
Previous: Junio C HamanoNext: Libor Pechacek
Message 12 of 26 in “git-config does not check validity of variable names”
  1. Libor PechacekJan 8, 2011
  2. Jeff KingJan 11, 2011
  3. Libor PechacekJan 19, 2011
  4. Sanity-ckeck config variable namesLibor Pechacek, Jan 19, 2011
  5. Jeff KingJan 20, 2011
  6. Jeff KingJan 21, 2011
  7. Libor PechacekJan 21, 2011
  8. Sanity-ckeck config variable namesLibor Pechacek, Jan 21, 2011
  9. Jeff KingJan 21, 2011
  10. Sanity-check config variable namesLibor Pechacek, Jan 27, 2011
  11. Junio C HamanoJan 27, 2011
  12. Libor PechacekJan 28, 2011
  13. Sanity-check config variable namesLibor Pechacek, Jan 30, 2011
  14. Junio C HamanoFeb 10, 2011
  15. Libor PechacekFeb 11, 2011
  16. Documentation fixes in git-configLibor Pechacek, Jan 19, 2011
  17. Jeff KingJan 21, 2011
  18. Libor PechacekJan 21, 2011
  19. Documentation fixes in git-configLibor Pechacek, Jan 21, 2011
  20. Jeff KingJan 21, 2011
  21. Libor PechacekJan 23, 2011
  22. Documentation fixes in git-configLibor Pechacek, Mar 1, 2012
  23. Jeff KingMar 1, 2012
  24. Libor PechacekMar 1, 2012
  25. Junio C HamanoMar 1, 2012
  26. Documentation fixes in git-configLibor Pechacek, Mar 1, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.