[PATCH] show git tag output in pager

[PATCH] show git tag output in pager

[Michal Vyskocil]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

---
 builtin/tag.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 667515e..9f70fa8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -147,6 +147,8 @@ static int list_tags(const char **patterns, int lines,
 			struct commit_list *with_commit)
 {
 	struct tag_filter filter;
+        
+        setup_pager();
 
 	filter.patterns = patterns;
 	filter.lines = lines;
-- 
1.7.6.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] show git tag output in pager

[Matthieu Moy]

The commit message should explain why this is needed, and in particular
why you prefer this to setting pager.tag in your ~/.gitconfig.

Michal Vyskocil <mvyskocil@suse.cz> writes:

> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -147,6 +147,8 @@ static int list_tags(const char **patterns, int lines,
>  			struct commit_list *with_commit)
>  {
>  	struct tag_filter filter;
> +        
> +        setup_pager();

Git indents with tabs, not spaces, and does not leave trailing
whitespaces.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] show git tag output in pager

[Michal Vyskocil]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On Tue, Sep 27, 2011 at 04:19:39PM +0200, Matthieu Moy wrote:
> The commit message should explain why this is needed, and in particular
> why you prefer this to setting pager.tag in your ~/.gitconfig.

Opps! I read a documentation, but I did not realize this works for all
commands and not only for them calling setup_pager(). Then sorry, no
change is needed.

> 
> Michal Vyskocil <mvyskocil@suse.cz> writes:
> 
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -147,6 +147,8 @@ static int list_tags(const char **patterns, int lines,
> >  			struct commit_list *with_commit)
> >  {
> >  	struct tag_filter filter;
> > +        
> > +        setup_pager();
> 
> Git indents with tabs, not spaces, and does not leave trailing
> whitespaces.

OK, thanks for the feedbac. I will try to be more carefull in the
future.

Regards
Michal Vyskocil

> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] show git tag output in pager

[Jeff King]

On Thu, Sep 29, 2011 at 11:37:49AM +0200, Michal Vyskocil wrote:

> On Tue, Sep 27, 2011 at 04:19:39PM +0200, Matthieu Moy wrote:
> > The commit message should explain why this is needed, and in particular
> > why you prefer this to setting pager.tag in your ~/.gitconfig.
> 
> Opps! I read a documentation, but I did not realize this works for all
> commands and not only for them calling setup_pager(). Then sorry, no
> change is needed.

I don't think you want to set pager.tag. It will invoke the pager for
all tag subcommands, including tag creation and deletion. It's not a
huge deal if your pager exits immediately when the input is less than a
page (which I think our default LESS settings will do). But I wouldn't
be surprised if it ends up confusing some program at some point.

I think instead, you want some way for commands to say "OK, I'm in a
subcommand that might or might not want a pager now".

Something like the (thoroughly not tested) patch below, which you can
use like:

  git config pager.tag.list

diff --git a/builtin.h b/builtin.h
index 0e9da90..d0fe971 100644
--- a/builtin.h
+++ b/builtin.h
@@ -35,6 +35,7 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
 void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
 
 extern int check_pager_config(const char *cmd);
+extern void try_subcommand_pager(const char *subcommand);
 
 extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 667515e..bc45fe6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -152,6 +152,7 @@ static int list_tags(const char **patterns, int lines,
 	filter.lines = lines;
 	filter.with_commit = with_commit;
 
+	try_subcommand_pager("tag.list");
 	for_each_tag_ref(show_reference, (void *) &filter);
 
 	return 0;
diff --git a/git.c b/git.c
index 8e34903..60fbf1e 100644
--- a/git.c
+++ b/git.c
@@ -64,6 +64,16 @@ static void commit_pager_choice(void) {
 	}
 }
 
+void try_subcommand_pager(const char *subcommand)
+{
+	/* it's too late to turn off a running pager */
+	if (pager_in_use())
+		return;
+
+	use_pager = check_pager_config(subcommand);
+	commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;

Other programs like "git branch list" would probably want similar hooks.

For commands like "tag" and "branch" where there's really only one
operation that you would want to be paged (i.e., "list"), it's tempting
to also (or instead) make "pager.tag" only affect the useful command.

That's not too hard for internal commands like "tag" (patch below). But
I'm not sure how you would do it for something external like "stash";
from git.c's perspective, we don't know anything about stash or whether
it would want to respect pager config or not.

diff --git a/builtin/tag.c b/builtin/tag.c
index bc45fe6..bbdb135 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -152,7 +152,7 @@ static int list_tags(const char **patterns, int lines,
 	filter.lines = lines;
 	filter.with_commit = with_commit;
 
-	try_subcommand_pager("tag.list");
+	try_subcommand_pager("tag");
 	for_each_tag_ref(show_reference, (void *) &filter);
 
 	return 0;
diff --git a/git.c b/git.c
index 60fbf1e..64a078d 100644
--- a/git.c
+++ b/git.c
@@ -276,6 +276,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define NO_PAGER_CONFIG		(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -299,7 +300,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 			prefix = setup_git_directory_gently(&nongit_ok);
 		}
 
-		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
+		if (use_pager == -1 &&
+		    p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
+		    !(p->option & NO_PAGER_CONFIG))
 			use_pager = check_pager_config(p->cmd);
 		if (use_pager == -1 && p->option & USE_PAGER)
 			use_pager = 1;
@@ -436,7 +439,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 		{ "stripspace", cmd_stripspace },
 		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-		{ "tag", cmd_tag, RUN_SETUP },
+		{ "tag", cmd_tag, RUN_SETUP | NO_PAGER_CONFIG },
 		{ "tar-tree", cmd_tar_tree },
 		{ "unpack-file", cmd_unpack_file, RUN_SETUP },
 		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },

-Peff

Re: [PATCH] show git tag output in pager

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 29, 2011 at 11:37:49AM +0200, Michal Vyskocil wrote:
>
>> On Tue, Sep 27, 2011 at 04:19:39PM +0200, Matthieu Moy wrote:
>> > The commit message should explain why this is needed, and in particular
>> > why you prefer this to setting pager.tag in your ~/.gitconfig.
>> 
>> Opps! I read a documentation, but I did not realize this works for all
>> commands and not only for them calling setup_pager(). Then sorry, no
>> change is needed.
>
> I don't think you want to set pager.tag. It will invoke the pager for
> all tag subcommands, including tag creation and deletion.

That's the kind of argument/discussion I was expecting in the commit
message.

> I think instead, you want some way for commands to say "OK, I'm in a
> subcommand that might or might not want a pager now".

Right.

I like the try_subcommand_pager idea. Ideally, there would also be a
nice mechanism to set defaults for subcommands, so that "git tag
<whatever>" does the right thing without configuration.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] show git tag output in pager

[Jeff King]

On Mon, Oct 03, 2011 at 02:57:09PM +0200, Matthieu Moy wrote:

> I like the try_subcommand_pager idea. Ideally, there would also be a
> nice mechanism to set defaults for subcommands, so that "git tag
> <whatever>" does the right thing without configuration.

That's easy enough. Something like the patch below?

Still thoroughly untested, but it looks Obviously Correct to me. :)

---
 builtin.h     |    1 +
 builtin/tag.c |    1 +
 git.c         |   13 +++++++++++++
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/builtin.h b/builtin.h
index 0e9da90..b2badf8 100644
--- a/builtin.h
+++ b/builtin.h
@@ -35,6 +35,7 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
 void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
 
 extern int check_pager_config(const char *cmd);
+extern void try_subcommand_pager(const char *subcommand, int fallback);
 
 extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 9d89616..c77adc4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -152,6 +152,7 @@ static int list_tags(const char **patterns, int lines,
 	filter.lines = lines;
 	filter.with_commit = with_commit;
 
+	try_subcommand_pager("tag.list", 1);
 	for_each_tag_ref(show_reference, (void *) &filter);
 
 	return 0;
diff --git a/git.c b/git.c
index 8e34903..11ee1a8 100644
--- a/git.c
+++ b/git.c
@@ -64,6 +64,19 @@ static void commit_pager_choice(void) {
 	}
 }
 
+void try_subcommand_pager(const char *subcommand, int fallback)
+{
+	/* it's too late to turn off a running pager */
+	if (pager_in_use())
+		return;
+
+	if (use_pager == -1)
+		use_pager = check_pager_config(subcommand);
+	if (use_pager == -1)
+		use_pager = fallback;
+	commit_pager_choice();
+}
+
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
 	const char **orig_argv = *argv;
-- 
1.7.7.rc2.7.gdfc92

Re: [PATCH] show git tag output in pager

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 03, 2011 at 02:57:09PM +0200, Matthieu Moy wrote:
>
>> I like the try_subcommand_pager idea. Ideally, there would also be a
>> nice mechanism to set defaults for subcommands, so that "git tag
>> <whatever>" does the right thing without configuration.
>
> That's easy enough. Something like the patch below?

It may have been better with a big centralized array of configurations
like

	static struct cmd_struct commands[] = {
		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
                ...

in git.c, but if we have only a few instances of this, your system is
probably fine. I like it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] show git tag output in pager

[Jeff King]

On Fri, Oct 07, 2011 at 04:48:35PM +0200, Matthieu Moy wrote:

> > On Mon, Oct 03, 2011 at 02:57:09PM +0200, Matthieu Moy wrote:
> >
> >> I like the try_subcommand_pager idea. Ideally, there would also be a
> >> nice mechanism to set defaults for subcommands, so that "git tag
> >> <whatever>" does the right thing without configuration.
> >
> > That's easy enough. Something like the patch below?
> 
> It may have been better with a big centralized array of configurations
> like
> 
> 	static struct cmd_struct commands[] = {
> 		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
>                 ...
> 
> in git.c, but if we have only a few instances of this, your system is
> probably fine. I like it.

I don't think you can centralize it in the same way, because some of it
will have to be implemented in shell script (unlike the full-command
ones, which we can always trigger at the git.c wrapper layer).

So you could have:

  static struct pager_default subcommands[] = {
          { "tag.list", 1 },
          { "branch.list", 1 },
  };

but you'll never be able to put "stash.list" into that structure (and as
you probably guessed, my patch isn't enough to implement stash.list,
either; it would need a shell implementation of try_subcommand_pager).

-Peff