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

Re: [PATCH 0/7] tag: more fine-grained pager-configuration

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 10, 2017, 22:42 UTC
Message-ID
<xmqqpod752yh.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<cover.1499723297.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 27 quoted lines
> Using, e.g., `git -c pager.tag tag -a new-tag` results in errors
> such as "Vim: Warning: Output is not to a terminal" and a garbled
> terminal. A user who makes use of `git tag -a` and `git tag -l`
> will probably choose not to configure `pager.tag` or to set it to
> "no", so that `git tag -a` will actually work, at the cost of not
> getting the pager with `git tag -l`.
>
> In the discussion at [1], it was brought up that 1) the individual
> builtins should be in charge of setting up the paging (as opposed
> to git.c which has no knowledge about what the command is about to
> do) and that 2) there could then be a configuration
> `pager.tag.list` to address the specific problem of `git tag`.
>
> This is an attempt to implement something like that. I decided to
> let `pager.tag.list` fall back to `pager.tag` before falling back
> to "on". The default for `pager.tag` is still "off". I can see how
> that might seem confusing. However, my argument is that it would
> be awkward for `git tag -l` to ignore `pager.tag` -- we are after
> all running a subcommand of `git tag`. Also, this avoids a
> regression for someone who has set `pager.tag` and uses `git tag
> -l`.
>
> I am not moving all builtins to handling the pager on their own,
> but instead introduce a flag IGNORE_PAGER_CONFIG and use it only
> with the tag builtin. That means there's another flag to reason
> about, but it avoids moving all builtins to handling the paging
> themselves just to make one of them do something more "clever".

All of the above smells like taking us in a sensible direction. I agree with these design decisions described in the above, i.e. 'pager.tag.list falling back to pager.tag', making this an opt-in to make code transition easier.

Even though it is purely internal thing, IGNORE_PAGER_CONFIG probably is a bit confusion-inducing name. After all, the subcommand specific configuration is not being ignored---we are merely delaying our reaction to it---instead of acting on it inside git.c without giving the subcommand a chance to make a decision, we are still letting (and we do expect) the subcommand to react to it.

But that is a fairly minor thing we can fix.
> A review would be much appreciated. Comments on the way I
> structured the series would be just as welcome as ones on the
> final result.

I see you CC'ed Peff who's passionate in this area, so these patches are in good hands already ;-) I briefly skimmed your patches myself, and did not spot anything glaringly wrong.

Thanks.
Previous: Jeff KingNext: Jeff King
Message 21 of 69 in “tag: more fine-grained pager-configuration”
  1. 0/7 tag: more fine-grained pager-configurationMartin Ågren, Jul 10, 2017
  2. 1/7 api-builtin.txt: document SUPPORT_SUPER_PREFIXMartin Ågren, Jul 10, 2017
  3. Brandon WilliamsJul 10, 2017
  4. Martin ÅgrenJul 11, 2017
  5. Brandon WilliamsJul 11, 2017
  6. 2/7 git.c: let builtins opt for handling `pager.foo` themselvesMartin Ågren, Jul 10, 2017
  7. Jeff KingJul 11, 2017
  8. Martin ÅgrenJul 11, 2017
  9. Jeff KingJul 11, 2017
  10. 3/7 git.c: provide setup_auto_pager()Martin Ågren, Jul 10, 2017
  11. Jeff KingJul 11, 2017
  12. Martin ÅgrenJul 11, 2017
  13. 4/7 t7006: add tests for how git tag paginatesMartin Ågren, Jul 10, 2017
  14. 6/7 tag: make git tag -l consider new config `pager.tag.list`Martin Ågren, Jul 10, 2017
  15. Jeff KingJul 11, 2017
  16. Martin ÅgrenJul 11, 2017
  17. 5/7 tag: handle `pager.tag`-configuration within the builtinMartin Ågren, Jul 10, 2017
  18. Jeff KingJul 11, 2017
  19. 7/7 tag: make git tag -l use pager by defaultMartin Ågren, Jul 10, 2017
  20. Jeff KingJul 11, 2017
  21. Junio C HamanoJul 10, 2017
  22. Jeff KingJul 11, 2017
  23. Junio C HamanoJul 11, 2017
  24. Martin ÅgrenJul 12, 2017
  25. Jeff KingJul 11, 2017
  26. Martin ÅgrenJul 11, 2017
  27. Jeff KingJul 11, 2017
  28. 00/10 tag: only respect `pager.tag` in list-modeMartin Ågren, Jul 17, 2017
  29. 01/10 builtin.h: take over documentation from api-builtin.txtMartin Ågren, Jul 17, 2017
  30. 02/10 builtin.h: format documentation-comment properlyMartin Ågren, Jul 17, 2017
  31. 03/10 builtin.h: document SUPPORT_SUPER_PREFIXMartin Ågren, Jul 17, 2017
  32. 04/10 git.c: let builtins opt for handling `pager.foo` themselvesMartin Ågren, Jul 17, 2017
  33. 05/10 git.c: provide setup_auto_pager()Martin Ågren, Jul 17, 2017
  34. Jeff KingJul 31, 2017
  35. Junio C HamanoJul 31, 2017
  36. 06/10 t7006: add tests for how git tag paginatesMartin Ågren, Jul 17, 2017
  37. Jeff KingJul 31, 2017
  38. Junio C HamanoJul 31, 2017
  39. Martin ÅgrenJul 31, 2017
  40. 07/10 tag: handle `pager.tag`-configuration within the builtinMartin Ågren, Jul 17, 2017
  41. 08/10 tag: respect `pager.tag` in list-mode onlyMartin Ågren, Jul 17, 2017
  42. Jeff KingJul 31, 2017
  43. 09/10 tag: change default of `pager.tag` to "on"Martin Ågren, Jul 17, 2017
  44. 10/10 git.c: ignore pager.* when launching builtin as dashed externalMartin Ågren, Jul 17, 2017
  45. Jeff KingJul 31, 2017
  46. Martin ÅgrenJul 31, 2017
  47. Junio C HamanoJul 18, 2017
  48. Junio C HamanoJul 20, 2017
  49. Martin ÅgrenJul 23, 2017
  50. Jeff KingJul 31, 2017
  51. Martin ÅgrenJul 31, 2017
  52. Jeff KingJul 31, 2017
  53. Junio C HamanoJul 31, 2017
  54. 0/7 tag: only respect `pager.tag` in list-modeMartin Ågren, Aug 2, 2017
  55. 1/7 builtin.h: take over documentation from api-builtin.txtMartin Ågren, Aug 2, 2017
  56. Junio C HamanoAug 3, 2017
  57. Martin ÅgrenAug 4, 2017
  58. Junio C HamanoAug 4, 2017
  59. Martin ÅgrenAug 4, 2017
  60. 2/7 git.c: let builtins opt for handling `pager.foo` themselvesMartin Ågren, Aug 2, 2017
  61. 3/7 git.c: provide setup_auto_pager()Martin Ågren, Aug 2, 2017
  62. 4/7 t7006: add tests for how git tag paginatesMartin Ågren, Aug 2, 2017
  63. 5/7 tag: respect `pager.tag` in list-mode onlyMartin Ågren, Aug 2, 2017
  64. 6/7 tag: change default of `pager.tag` to "on"Martin Ågren, Aug 2, 2017
  65. 7/7 git.c: ignore pager.* when launching builtin as dashed externalMartin Ågren, Aug 2, 2017
  66. Junio C HamanoAug 3, 2017
  67. Jeff KingAug 3, 2017
  68. Martin ÅgrenAug 4, 2017
  69. Jeff KingAug 4, 2017

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.