Re: [PATCH v2] column: show auto columns when pager is active
On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote: > Hi, > > Kevin Daudt wrote: > > > --- a/column.c > > +++ b/column.c > > @@ -5,6 +5,7 @@ > > #include "parse-options.h" > > #include "run-command.h" > > #include "utf8.h" > > +#include "pager.c" > > Should this be pager.h? > > Thanks, > Jonathan Thanks for catching this.
Re: [PATCH v2] column: show auto columns when pager is active
Junio C Hamano wrote: > Subject: column: do not include pager.c > > Everything this file needs from the pager API (e.g. term_columns(), > pager_in_use()) is already declared in the header file it includes. > > Noticed-by: Jonathan Nieder> Signed-off-by: Junio C Hamano > --- > column.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Jonathan Nieder Thanks. > --- a/column.c > +++ b/column.c > @@ -5,7 +5,6 @@ > #include "parse-options.h" > #include "run-command.h" > #include "utf8.h" > -#include "pager.c" > > #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \ > (x) * (d)->rows + (y) : \
Re: [PATCH v2] column: show auto columns when pager is active
Jonathan Niederwrites: > Hi, > > Kevin Daudt wrote: > >> --- a/column.c >> +++ b/column.c >> @@ -5,6 +5,7 @@ >> #include "parse-options.h" >> #include "run-command.h" >> #include "utf8.h" >> +#include "pager.c" > > Should this be pager.h? Ouch. And I was not paying enough attention. Thanks, I'll queue this on top. -- >8 -- Subject: column: do not include pager.c Everything this file needs from the pager API (e.g. term_columns(), pager_in_use()) is already declared in the header file it includes. Noticed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- column.c | 1 - 1 file changed, 1 deletion(-) diff --git a/column.c b/column.c index ded50337f7..49ab85b769 100644 --- a/column.c +++ b/column.c @@ -5,7 +5,6 @@ #include "parse-options.h" #include "run-command.h" #include "utf8.h" -#include "pager.c" #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \ (x) * (d)->rows + (y) : \
Re: [PATCH v2] column: show auto columns when pager is active
Hi, Kevin Daudt wrote: > --- a/column.c > +++ b/column.c > @@ -5,6 +5,7 @@ > #include "parse-options.h" > #include "run-command.h" > #include "utf8.h" > +#include "pager.c" Should this be pager.h? Thanks, Jonathan
Re: [PATCH v2] column: show auto columns when pager is active
Kevin Daudtwrites: > When columns are set to automatic for git tag and the output is > paginated by git, the output is a single column instead of multiple > columns. > > Standard behaviour in git is to honor auto values when the pager is > active, which happens for example with commands like git log showing > colors when being paged. > > Since ff1e72483 (tag: change default of `pager.tag` to "on", > 2017-08-02), the pager has been enabled by default, exposing this > problem to more people. > > finalize_colopts in column.c only checks whether the output is a TTY to > determine if columns should be enabled with columns set to auto. Also > check if the pager is active. > > Adding a test for git column is possible but requires some care to work > around a race on stdin. See commit 18d8c2693 (test_terminal: redirect > child process' stdin to a pty, 2015-08-04). Test git tag instead, since > that does not involve stdin, and since that was the original motivation > for this patch. Nicely done. > +test_expect_success TTY 'git tag with auto-columns ' ' > + test_commit one && > + test_commit two && > + test_commit three && > + test_commit four && > + test_commit five && > + cat >expected <<\EOF && > +initial one two threefour five > +EOF > + test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \ > + git -c column.ui=auto tag --sort=authordate && > + test_cmp expected actual.tag > +' I'd use <<-\EOF so that here document can be intended like other tests, and also use expect vs actual that are used in the other tests in the same script, instead of suddenly becoming creative in only this single test. I can do these clean-ups locally when queuing so no need to resend only to collect these. Thanks, will queue.