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

Re: [PATCH 07/10] color: delay auto-color decision until point of use

From
Jeff King <peff@peff.net>
Date
Aug 18, 2011, 22:28 UTC
Message-ID
<20110818222817.GA8668@sigill.intra.peff.net>
In-Reply-To
<7vvctu7402.fsf@alter.siamese.dyndns.org>
On Thu, Aug 18, 2011 at 02:59:37PM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/color.h b/color.h
> > index a190a25..d715fd5 100644
> > --- a/color.h
> > +++ b/color.h
> > @@ -49,6 +49,16 @@ struct strbuf;
> >  #define GIT_COLOR_NIL "NIL"
> >  
> >  /*
> > + * The first three are chosen to match common usage in the code, and what is
> > + * returned from git_config_colorbool. The "auto" value can be returned from
> > + * config_colorbool, and will be converted by want_color() into either 0 or 1.
> > + */
> > +#define GIT_COLOR_UNKNOWN -1
> > +#define GIT_COLOR_ALWAYS 0
> > +#define GIT_COLOR_NEVER  1
> > +#define GIT_COLOR_AUTO   2
> 
> The ALWAYS/NEVER somehow go against my intuition. Let me trace one
> codepath starting from git_branch_config().
> 
>     branch_use_color is set from git_config_colorbool("color.branch");
>     -> given "never", git_config_colorbool() returns 0;
>     branch_get_color() asks want_color(branch_use_color);
>     -> want_color() returns if the given value is positive.
> 
> Because git_config_colorbool() does not use the above symbolic constants,
> everything goes well, but aren't these two swapped?

Oooops. Yes, they are completely swapped and I'm an idiot. But as you noticed, we don't actually _use_ them anywhere. I started on replacing every "0" with NEVER, every "1" with ALWAYS, and every "-1" with UNKNOWN. But it really bloated the patch, and didn't actually make the code any more readable.

The only symbolic constant that is really necessary is the AUTO one. I just felt odd randomly defining "2" as GIT_COLOR_AUTO, but not defining the other possible values of the enumeration. So definitely they should be swapped. I'm also fine with just dropping all of them except AUTO.

-Peff
Previous: Junio C HamanoNext: Jeff King
Message 12 of 37 in “color and pager improvements”
  1. 0/10 color and pager improvementsJeff King, Aug 18, 2011
  2. 01/10 t7006: modernize calls to unsetJeff King, Aug 18, 2011
  3. Junio C HamanoAug 18, 2011
  4. 02/10 test-lib: add helper functions for configJeff King, Aug 18, 2011
  5. Junio C HamanoAug 18, 2011
  6. 03/10 t7006: use test_config helpersJeff King, Aug 18, 2011
  7. 04/10 setup_pager: set GIT_PAGER_IN_USEJeff King, Aug 18, 2011
  8. 05/10 diff: refactor COLOR_DIFF from a flag into an intJeff King, Aug 18, 2011
  9. 06/10 git_config_colorbool: refactor stdout_is_tty handlingJeff King, Aug 18, 2011
  10. 07/10 color: delay auto-color decision until point of useJeff King, Aug 18, 2011
  11. Junio C HamanoAug 18, 2011
  12. Jeff KingAug 18, 2011
  13. 08/10 config: refactor get_colorbool functionJeff King, Aug 18, 2011
  14. 09/10 diff: don't load color config in plumbingJeff King, Aug 18, 2011
  15. 10/10 want_color: automatically fallback to color.uiJeff King, Aug 18, 2011
  16. Martin von ZweigbergkSep 4, 2011
  17. Jeff KingSep 4, 2011
  18. Steffen Daode NurpmesoSep 5, 2011
  19. Jeff KingAug 18, 2011
  20. 11/10 support pager.* for aliasesJeff King, Aug 18, 2011
  21. Junio C HamanoAug 18, 2011
  22. Jeff KingAug 19, 2011
  23. Junio C HamanoAug 19, 2011
  24. Jeff KingAug 19, 2011
  25. Junio C HamanoAug 19, 2011
  26. Junio C HamanoAug 19, 2011
  27. Jeff KingAug 19, 2011
  28. 12/10 support pager.* for external commandsJeff King, Aug 18, 2011
  29. Junio C HamanoAug 18, 2011
  30. Ævar Arnfjörð BjarmasonFeb 12, 2012
  31. Jeff KingFeb 14, 2012
  32. Ingo BrücklAug 18, 2011
  33. Jeff KingAug 18, 2011
  34. Ingo BrücklAug 19, 2011
  35. Jeff KingAug 25, 2011
  36. Steffen Daode NurpmesoAug 18, 2011
  37. Junio C HamanoAug 18, 2011

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.