git status in clean working dir

git status in clean working dir

[David Bremner]

According to the manual page for git-status (version 1.5.6.3)

       If there is no path that is different between the index file
       and the current HEAD commit (i.e., there is nothing to commit
       by running git-commit), the command exits with non-zero status.

But it doesn't seem to work that way for me.

git status -a 

exits with 0

but 

git commit -a 

exits with 1

Is the man page wrong, or is this a bug? Or, option #3 ?

David

P.S. Please CC me. I'm not on the list.

Re: git status in clean working dir

[Junio C Hamano]

David Bremner <bremner@unb.ca> writes:

> According to the manual page for git-status (version 1.5.6.3)
>
>        If there is no path that is different between the index file
>        and the current HEAD commit (i.e., there is nothing to commit
>        by running git-commit), the command exits with non-zero status.
>
> But it doesn't seem to work that way for me.
>
> git status -a 
>
> exits with 0
> ...

Try "git status -a >/dev/null" or "git --no-pager status -a".

I think this is an instance of the c8af1de (make git-status use a pager,
2008-04-23) stupidity raising its ugly head again.

Do people mind reverting that patch?

Re: git status in clean working dir

[Abhijit Menon-Sen]

At 2008-07-21 19:30:16 -0700, gitster@pobox.com wrote:
>
> I think this is an instance of the c8af1de (make git-status use a
> pager, 2008-04-23) stupidity raising its ugly head again.
> 
> Do people mind reverting that patch?

I think that is an excellent idea. I've found myself annoyed time and
again by "git status" starting the pager for just a couple of lines of
output.

-- ams

Re: git status in clean working dir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> git status -a 
>>
>> exits with 0
>> ...
>
> Try "git status -a >/dev/null" or "git --no-pager status -a".
>
> I think this is an instance of the c8af1de (make git-status use a pager,
> 2008-04-23) stupidity raising its ugly head again.
>
> Do people mind reverting that patch?

Actually, the situation is now even worse than I originally thought
especially with Jeff's pager.<cmd> patch on 'master' recently.  For
example, you can screw yourself quite badly by forcing diff-files used in
the scripts you run to page, defeating --exit-code option.  Which means

 (1) It hurts?  Don't do it then; but

 (2) Then why are we even allowing to configure the plumbing to page?

Should we maintain a table of commands that we allow paging to be
customized, and ignore pager.<cmd> for commands that are not in the list?

Which codepath should issue error messages when the user tries to break
the system by saying "pager.diff-files = true"?

Re: git status in clean working dir

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Actually, the situation is now even worse than I originally thought
> especially with Jeff's pager.<cmd> patch on 'master' recently.  For
> example, you can screw yourself quite badly by forcing diff-files used in
> the scripts you run to page, defeating --exit-code option.  Which means
>
>  (1) It hurts?  Don't do it then; but
>
>  (2) Then why are we even allowing to configure the plumbing to page?
>
> Should we maintain a table of commands that we allow paging to be
> customized, and ignore pager.<cmd> for commands that are not in the list?
>
> Which codepath should issue error messages when the user tries to break
> the system by saying "pager.diff-files = true"?

Another possibility is to set up an extra process whose sole purpose is to
wait for the main process that feeds the pager pipe and relay its exit
status to the outside world.  But I do not think we would want to go
there...

Re: git status in clean working dir

[Jeff King]

On Mon, Jul 21, 2008 at 07:48:02PM -0700, Junio C Hamano wrote:

> Another possibility is to set up an extra process whose sole purpose is to
> wait for the main process that feeds the pager pipe and relay its exit
> status to the outside world.  But I do not think we would want to go
> there...

We could also swap the parent/child relationship, and have the pager as
child. But I assume that it is done the way we have it because otherwise
the shell gets confused about when the command ends (i.e., we want it to
run until pager completion). I didn't test, though.

-Peff

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 12:44:00AM -0400, Jeff King wrote:

> We could also swap the parent/child relationship, and have the pager as
> child. But I assume that it is done the way we have it because otherwise
> the shell gets confused about when the command ends (i.e., we want it to
> run until pager completion). I didn't test, though.

Hmm, it looks like the MINGW32 codepath already _does_ spawn in that
order, but has a "wait_for_child" atexit handler. I wonder if there is a
reason all platforms can't use that trick (though the mingw approach
uses run_command, which makes it harder to do the "wait for input before
starting less" trick).

-Peff

Re: git status in clean working dir

[Johannes Schindelin]

Hi,

On Tue, 22 Jul 2008, Jeff King wrote:

> On Tue, Jul 22, 2008 at 12:44:00AM -0400, Jeff King wrote:
> 
> > We could also swap the parent/child relationship, and have the pager 
> > as child. But I assume that it is done the way we have it because 
> > otherwise the shell gets confused about when the command ends (i.e., 
> > we want it to run until pager completion). I didn't test, though.
> 
> Hmm, it looks like the MINGW32 codepath already _does_ spawn in that 
> order, but has a "wait_for_child" atexit handler. I wonder if there is a 
> reason all platforms can't use that trick (though the mingw approach 
> uses run_command, which makes it harder to do the "wait for input before 
> starting less" trick).

I recently suggested exactly that already, to catch SIGSEGVs in the paged 
process, amongst other things.

Ciao,
Dscho

Re: git status in clean working dir

[Jeff King]

On Mon, Jul 21, 2008 at 07:40:28PM -0700, Junio C Hamano wrote:

> Actually, the situation is now even worse than I originally thought
> especially with Jeff's pager.<cmd> patch on 'master' recently.  For
> example, you can screw yourself quite badly by forcing diff-files used in
> the scripts you run to page, defeating --exit-code option.  Which means

Actually, you could _always_ do that with "git -p diff-files". Which is
obviously stupid, just as setting pager.diff-files is. In the reported
case, though, "status" is broken, which we now do by default. So no
stupidity required.

>  (2) Then why are we even allowing to configure the plumbing to page?

  1. Laziness. We just never marked which shouldn't be allowed to page.
     But again, in this case, we have explicitly marked status as "this
     should page" so I don't think this is a plumbing / porcelain thing.
     Status fulfills both roles here (some people want it paged, because
     they use it as porcelain, and some people want the exit code).

  2. We don't always know all git commands. We execute user scripts as
     "git foo", but we don't know what they do. Worse than that, we have
     to commit our pager choice early because we might be exec'ing (but
     this is somewhat of an artifact of the way the code is structured,
     and not necessarily an impossible obstacle).

> Should we maintain a table of commands that we allow paging to be
> customized, and ignore pager.<cmd> for commands that are not in the list?

The patch below sets up the infrastructure, which is trivial. Note that
this _doesn't_ handle the case of "git -p status", because we have to
commit that choice at a different time (again, we might be able to
overcome that with a little code restructuring).

This marks diff-files as FORBID_PAGER; I will leave it to others to
fight about which commands should have it. But it doesn't make sense to
mark "status" since some people obviously _want_ the paging there.

> Which codepath should issue error messages when the user tries to break
> the system by saying "pager.diff-files = true"?

No error, but it is silently ignored. :)

---
diff --git a/git.c b/git.c
index 74ea0e6..72cadb5 100644
--- a/git.c
+++ b/git.c
@@ -210,6 +210,7 @@ const char git_version_string[] = GIT_VERSION;
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE	(1<<2)
+#define FORBID_PAGER	(1<<3)
 
 struct cmd_struct {
 	const char *cmd;
@@ -231,6 +232,8 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 		use_pager = check_pager_config(p->cmd);
 	if (use_pager == -1 && p->option & USE_PAGER)
 		use_pager = 1;
+	if (use_pager ==  1 && p->option & FORBID_PAGER)
+		use_pager = 0;
 	commit_pager_choice();
 
 	if (p->option & NEED_WORK_TREE)
@@ -286,7 +289,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files, RUN_SETUP },
+		{ "diff-files", cmd_diff_files, RUN_SETUP | FORBID_PAGER },
 		{ "diff-index", cmd_diff_index, RUN_SETUP },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
 		{ "fast-export", cmd_fast_export, RUN_SETUP },

Re: git status in clean working dir

[Mike Hommey]

On Tue, Jul 22, 2008 at 12:41:57AM -0400, Jeff King wrote:
> On Mon, Jul 21, 2008 at 07:40:28PM -0700, Junio C Hamano wrote:
> 
> > Actually, the situation is now even worse than I originally thought
> > especially with Jeff's pager.<cmd> patch on 'master' recently.  For
> > example, you can screw yourself quite badly by forcing diff-files used in
> > the scripts you run to page, defeating --exit-code option.  Which means
> 
> Actually, you could _always_ do that with "git -p diff-files". Which is
> obviously stupid, just as setting pager.diff-files is. In the reported
> case, though, "status" is broken, which we now do by default. So no
> stupidity required.
> 
> >  (2) Then why are we even allowing to configure the plumbing to page?
> 
>   1. Laziness. We just never marked which shouldn't be allowed to page.
>      But again, in this case, we have explicitly marked status as "this
>      should page" so I don't think this is a plumbing / porcelain thing.
>      Status fulfills both roles here (some people want it paged, because
>      they use it as porcelain, and some people want the exit code).
> 
>   2. We don't always know all git commands. We execute user scripts as
>      "git foo", but we don't know what they do. Worse than that, we have
>      to commit our pager choice early because we might be exec'ing (but
>      this is somewhat of an artifact of the way the code is structured,
>      and not necessarily an impossible obstacle).
> 
> > Should we maintain a table of commands that we allow paging to be
> > customized, and ignore pager.<cmd> for commands that are not in the list?
> 
> The patch below sets up the infrastructure, which is trivial. Note that
> this _doesn't_ handle the case of "git -p status", because we have to
> commit that choice at a different time (again, we might be able to
> overcome that with a little code restructuring).
> 
> This marks diff-files as FORBID_PAGER; I will leave it to others to
> fight about which commands should have it. But it doesn't make sense to
> mark "status" since some people obviously _want_ the paging there.

Why not "simply" forbid the pager when output is not a terminal ?

Mike

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 07:39:21AM +0200, Mike Hommey wrote:

> > This marks diff-files as FORBID_PAGER; I will leave it to others to
> > fight about which commands should have it. But it doesn't make sense to
> > mark "status" since some people obviously _want_ the paging there.
> 
> Why not "simply" forbid the pager when output is not a terminal ?

We already do that (see pager.c:53). The original poster still had a
problem, but I don't know if it was for actual usage or simply a toy

  $ git status
  $ echo $?
  $ echo "why don't exit codes work in status?" | mail git@vger

question.

-Peff

Re: git status in clean working dir

[Mike Hommey]

On Tue, Jul 22, 2008 at 02:06:43AM -0400, Jeff King wrote:
> On Tue, Jul 22, 2008 at 07:39:21AM +0200, Mike Hommey wrote:
> 
> > > This marks diff-files as FORBID_PAGER; I will leave it to others to
> > > fight about which commands should have it. But it doesn't make sense to
> > > mark "status" since some people obviously _want_ the paging there.
> > 
> > Why not "simply" forbid the pager when output is not a terminal ?
> 
> We already do that (see pager.c:53). The original poster still had a
> problem, but I don't know if it was for actual usage or simply a toy
> 
>   $ git status
>   $ echo $?
>   $ echo "why don't exit codes work in status?" | mail git@vger
> 
> question.

As you said in another branch of the thread, this part would be solved by
having parent/child being reverted.

Now, for the case where diff-files can have a pager if the user shoots
himself in the foot, if the output is not a terminal and pager.c already
does the right thing, I don't see where diff-files having a pager will
be a problem.

If diff-files' output is a terminal, it's obviously intended to be
displayed, be it in a script or not. But most of the time, its output
will be piped, thus not triggering the pager anyways.

And for diff-files' exit code, well, see the first paragraph.

Mike

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 08:18:07AM +0200, Mike Hommey wrote:

> > We already do that (see pager.c:53). The original poster still had a
> > problem, but I don't know if it was for actual usage or simply a toy
> > 
> >   $ git status
> >   $ echo $?
> >   $ echo "why don't exit codes work in status?" | mail git@vger
> > 
> > question.
> 
> As you said in another branch of the thread, this part would be solved by
> having parent/child being reverted.
> 
> Now, for the case where diff-files can have a pager if the user shoots
> himself in the foot, if the output is not a terminal and pager.c already
> does the right thing, I don't see where diff-files having a pager will
> be a problem.

Ah, OK. I misunderstood your original post. Yes, there are two ways
paging can screw you: munging the data in a pipeline and munging the
exit code. We already deal with former, so it is really just the latter
that is posing a problem in this thread.

I am tempted by the "order switching" I mentioned, but that would entail
the git process waiting to clean the pager, during which time it may be
consuming memory. But maybe that isn't worth worrying about.

-Peff

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 02:46:04AM -0400, Jeff King wrote:

> I am tempted by the "order switching" I mentioned, but that would entail
> the git process waiting to clean the pager, during which time it may be
> consuming memory. But maybe that isn't worth worrying about.

It feels very wrong proposing this during release freeze, but here is
the "pager is child of git" implementation.

Patch 1/1 adds a bit of necessary infrastructure to run-command, and
patch 2/2 does the deed. The nice thing is that it unifies the Windows
and Unix implementations of setup_pager, so we get a nice line
reduction.

 pager.c       |   49 ++++++++-----------------------------------------
 run-command.c |    2 ++
 run-command.h |    1 +
 3 files changed, 11 insertions(+), 41 deletions(-)

[PATCH 1/2] run-command: add pre-exec callback

[Jeff King]

This is a function provided by the caller which is called
_after_ the process is forked, but before the spawned
program is executed. On platforms (like mingw) where
subprocesses are forked and executed in a single call, the
preexec callback is simply ignored.

This will be used in the following patch to do some setup
for 'less' that must happen in the forked child.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c |    2 ++
 run-command.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/run-command.c b/run-command.c
index 6e29fdf..73d0c31 100644
--- a/run-command.c
+++ b/run-command.c
@@ -110,6 +110,8 @@ int start_command(struct child_process *cmd)
 					unsetenv(*cmd->env);
 			}
 		}
+		if (cmd->preexec_cb)
+			cmd->preexec_cb();
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
diff --git a/run-command.h b/run-command.h
index 5203a9e..4f2b7d7 100644
--- a/run-command.h
+++ b/run-command.h
@@ -42,6 +42,7 @@ struct child_process {
 	unsigned no_stderr:1;
 	unsigned git_cmd:1; /* if this is to be git sub-command */
 	unsigned stdout_to_stderr:1;
+	void (*preexec_cb)(void);
 };
 
 int start_command(struct child_process *);
-- 
1.6.0.rc0.1.g9291f.dirty

[PATCH 2/2] spawn pager via run_command interface

[Jeff King]

This has two important effects:

 1. The pager is now the _child_ process, instead of the
    parent. This means that whatever spawned git (e.g., the
    shell) will see the exit code of the git process, and
    not the pager.

 2. The mingw and regular code are now unified, which makes
    the setup_pager function much simpler.

There are two caveats:

 1. We used to call execlp directly on the pager, followed
    by trying to exec it via the shall. We now just use the
    shell (which is what mingw has always done). This may
    have different results for pager names which contain
    shell metacharacters.

    It is also slightly less efficient because we
    unnecessarily run the shell; however, pager spawning is
    by definition an interactive task, so it shouldn't be
    a huge problem.

 2. The git process will remain in memory while the user
    looks through the pager. This is potentially wasteful.
    We could get around this by turning the parent into a
    meta-process which spawns _both_ git and the pager,
    collects the exit status from git, waits for both to
    end, and then exits with git's exit code.
---
 pager.c |   49 ++++++++-----------------------------------------
 1 files changed, 8 insertions(+), 41 deletions(-)

diff --git a/pager.c b/pager.c
index 6b5c9e4..7743742 100644
--- a/pager.c
+++ b/pager.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 
 /*
  * This is split up from the rest of git so that we can do
@@ -8,7 +9,7 @@
 static int spawned_pager;
 
 #ifndef __MINGW32__
-static void run_pager(const char *pager)
+static void pager_preexec(void)
 {
 	/*
 	 * Work around bug in "less" by not starting it until we
@@ -20,16 +21,17 @@ static void run_pager(const char *pager)
 	FD_SET(0, &in);
 	select(1, &in, NULL, &in, NULL);
 
-	execlp(pager, pager, NULL);
-	execl("/bin/sh", "sh", "-c", pager, NULL);
+	setenv("LESS", "FRSX", 0);
 }
-#else
-#include "run-command.h"
+#endif
 
 static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
 static struct child_process pager_process = {
 	.argv = pager_argv,
-	.in = -1
+	.in = -1,
+#ifndef __MINGW32__
+	.preexec_cb = pager_preexec,
+#endif
 };
 static void wait_for_pager(void)
 {
@@ -40,14 +42,9 @@ static void wait_for_pager(void)
 	close(2);
 	finish_command(&pager_process);
 }
-#endif
 
 void setup_pager(void)
 {
-#ifndef __MINGW32__
-	pid_t pid;
-	int fd[2];
-#endif
 	const char *pager = getenv("GIT_PAGER");
 
 	if (!isatty(1))
@@ -66,35 +63,6 @@ void setup_pager(void)
 
 	spawned_pager = 1; /* means we are emitting to terminal */
 
-#ifndef __MINGW32__
-	if (pipe(fd) < 0)
-		return;
-	pid = fork();
-	if (pid < 0) {
-		close(fd[0]);
-		close(fd[1]);
-		return;
-	}
-
-	/* return in the child */
-	if (!pid) {
-		dup2(fd[1], 1);
-		dup2(fd[1], 2);
-		close(fd[0]);
-		close(fd[1]);
-		return;
-	}
-
-	/* The original process turns into the PAGER */
-	dup2(fd[0], 0);
-	close(fd[0]);
-	close(fd[1]);
-
-	setenv("LESS", "FRSX", 0);
-	run_pager(pager);
-	die("unable to execute pager '%s'", pager);
-	exit(255);
-#else
 	/* spawn the pager */
 	pager_argv[2] = pager;
 	if (start_command(&pager_process))
@@ -107,7 +75,6 @@ void setup_pager(void)
 
 	/* this makes sure that the parent terminates after the pager */
 	atexit(wait_for_pager);
-#endif
 }
 
 int pager_in_use(void)
-- 
1.6.0.rc0.1.g9291f.dirty

Re: [PATCH 2/2] spawn pager via run_command interface

[Jeff King]

On Tue, Jul 22, 2008 at 03:14:12AM -0400, Jeff King wrote:

>  static struct child_process pager_process = {
>  	.argv = pager_argv,
> -	.in = -1
> +	.in = -1,
> +#ifndef __MINGW32__
> +	.preexec_cb = pager_preexec,
> +#endif

I couldn't recall if this initializer style is portable enough for us.
It was already there wrapped in ifdefs, but perhaps it was only ok
because the mingw version always uses the same compiler?

-Peff

Re: [PATCH 2/2] spawn pager via run_command interface

[Pierre Habouzit]

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

On Tue, Jul 22, 2008 at 07:16:30AM +0000, Jeff King wrote:
> On Tue, Jul 22, 2008 at 03:14:12AM -0400, Jeff King wrote:
> 
> >  static struct child_process pager_process = {
> >  	.argv = pager_argv,
> > -	.in = -1
> > +	.in = -1,
> > +#ifndef __MINGW32__
> > +	.preexec_cb = pager_preexec,
> > +#endif
> 
> I couldn't recall if this initializer style is portable enough for us.
> It was already there wrapped in ifdefs, but perhaps it was only ok
> because the mingw version always uses the same compiler?

  it's not, I asked long time ago, and it's C99, which mingw supports
indeed, and we don't want to require a C99 compiler.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH 2/2] spawn pager via run_command interface

[Jeff King]

On Tue, Jul 22, 2008 at 09:31:08AM +0200, Pierre Habouzit wrote:

> > I couldn't recall if this initializer style is portable enough for us.
> > It was already there wrapped in ifdefs, but perhaps it was only ok
> > because the mingw version always uses the same compiler?
> 
>   it's not, I asked long time ago, and it's C99, which mingw supports
> indeed, and we don't want to require a C99 compiler.

OK, then this should be squashed in.

diff --git a/pager.c b/pager.c
index 7743742..aa0966c 100644
--- a/pager.c
+++ b/pager.c
@@ -26,13 +26,8 @@ static void pager_preexec(void)
 #endif
 
 static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
-static struct child_process pager_process = {
-	.argv = pager_argv,
-	.in = -1,
-#ifndef __MINGW32__
-	.preexec_cb = pager_preexec,
-#endif
-};
+static struct child_process pager_process;
+
 static void wait_for_pager(void)
 {
 	fflush(stdout);
@@ -65,6 +60,11 @@ void setup_pager(void)
 
 	/* spawn the pager */
 	pager_argv[2] = pager;
+	pager_process.argv = pager_argv;
+	pager_process.in = -1;
+#ifndef __MINGW32__
+	pager_process.preexec_cb = pager_preexec;
+#endif
 	if (start_command(&pager_process))
 		return;
 

Re: [PATCH 2/2] spawn pager via run_command interface

[Johannes Sixt]

Jeff King schrieb:
> On Tue, Jul 22, 2008 at 03:14:12AM -0400, Jeff King wrote:
> 
>>  static struct child_process pager_process = {
>>  	.argv = pager_argv,
>> -	.in = -1
>> +	.in = -1,
>> +#ifndef __MINGW32__
>> +	.preexec_cb = pager_preexec,
>> +#endif
> 
> I couldn't recall if this initializer style is portable enough for us.
> It was already there wrapped in ifdefs, but perhaps it was only ok
> because the mingw version always uses the same compiler?

Yes, that's because on mingw we know that we use gcc. This really must be
changed for portability.

BTW, you could remove the #ifndef __MINGW32__ around both the definition
and the use of pager_preexec. We have everything on mingw to compile and
link this function.

-- Hannes

Re: [PATCH 2/2] spawn pager via run_command interface

[Jeff King]

On Tue, Jul 22, 2008 at 09:48:09AM +0200, Johannes Sixt wrote:

> BTW, you could remove the #ifndef __MINGW32__ around both the definition
> and the use of pager_preexec. We have everything on mingw to compile and
> link this function.

Ah, OK. I left it around the function because I was worried about fd_set
needing some magic for compilation.

However, it still won't be _used_ on Windows, because there is no
opportunity to use the pre-exec callback (it is silently ignored).

-Peff

Re: [PATCH 2/2] spawn pager via run_command interface

[Johannes Sixt]

Jeff King schrieb:
> On Tue, Jul 22, 2008 at 09:48:09AM +0200, Johannes Sixt wrote:
> 
>> BTW, you could remove the #ifndef __MINGW32__ around both the definition
>> and the use of pager_preexec. We have everything on mingw to compile and
>> link this function.
> 
> Ah, OK. I left it around the function because I was worried about fd_set
> needing some magic for compilation.

I do not expect you to know the intricacies of the mingw port. Hence,
staying on the safe side, like you did, is of course highly appreciated.

> However, it still won't be _used_ on Windows, because there is no
> opportunity to use the pre-exec callback (it is silently ignored).

That's fine.

-- Hannes

Re: git status in clean working dir

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 22, 2008 at 02:46:04AM -0400, Jeff King wrote:
>
>> I am tempted by the "order switching" I mentioned, but that would entail
>> the git process waiting to clean the pager, during which time it may be
>> consuming memory. But maybe that isn't worth worrying about.
>
> It feels very wrong proposing this during release freeze, but here is
> the "pager is child of git" implementation.

Another slight worry I have is if the now-parent git process does the
right thing when the user kills the pager without viewing the output to
the end.  git itself will get stuck with write() while the user is
reading, and then notice that the pipe does not have any more reader when
the pager is killed.  This fact itself won't change by swapping the
parent-child relationship, but would we get a sensible behaviour after
that, or have we been ignoring what happens afterwards only because our
exit status has been hidden behind the pager?  Running "git log" and
killing it by "q" (my pager is "less") makes it exit with 141.

I shouldn't worry, if everything is written correctly in the other parts
of the system, this swap should not have much ill effect.

By the way [2/2] was not signed-off.  Just forgotten?

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 02:17:43AM -0700, Junio C Hamano wrote:

> Another slight worry I have is if the now-parent git process does the
> right thing when the user kills the pager without viewing the output to
> the end.  git itself will get stuck with write() while the user is
> reading, and then notice that the pipe does not have any more reader when
> the pager is killed.  This fact itself won't change by swapping the
> parent-child relationship, but would we get a sensible behaviour after
> that, or have we been ignoring what happens afterwards only because our
> exit status has been hidden behind the pager?  Running "git log" and
> killing it by "q" (my pager is "less") makes it exit with 141.

Hmm, good point. Though previously in this case, we were getting
whatever code the pager provided. Which means nobody probably cared that
much. Though I suppose that people who use "$?" in their prompt might
see scariness.

> I shouldn't worry, if everything is written correctly in the other parts
> of the system, this swap should not have much ill effect.

I am a little unhappy about the git process hanging around, but I don't
know if it is worth making a meta-process just to manage the pager.

Also, I think people with a pager that has spaces in in it will now need
to quote it (e.g., PAGER="/path with space/less" used to work, but now
is passed to the shell). Arguably, this brings it in line with other
spawned programs, like EDITOR, but it is a difference, and we are in
release freeze. That could be fixed with some magic in run_command.
(Note that it has always been run by the shell under Windows, so again,
this is making things more consistent).

> By the way [2/2] was not signed-off.  Just forgotten?

Yes, forgotten. If you are planning on applying, please forge (and
squash the portability fix).

-Peff

Re: git status in clean working dir

[David Bremner]

>>>>> "Jeff" == Jeff King <peff@peff.net> writes:
    Jeff> We already do that (see pager.c:53). The original poster
    Jeff> still had a problem, but I don't know if it was for actual
    Jeff> usage or simply a toy

    Jeff>   $ git status 
    Jeff>   $ echo $?  
    Jeff>   $ echo "why don't exit codes work in status?" | mail git@vger

Well, I wanted to know if git status was the right tool to detect a
working directory with no changes to commit. So I tried it in the
shell, and it failed.  I then read the man page. For me, it would be
fine to document the current behaviour (i.e. in practice "git status
-a > /dev/null" would be what I would use in scripts anyway).  To be
honest the current behaviour was not something I would have guessed.

Thanks to all for paying attention to my complaint,

David

Re: git status in clean working dir

[Johannes Sixt]

Jeff King schrieb:
> @@ -231,6 +232,8 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
>  		use_pager = check_pager_config(p->cmd);
>  	if (use_pager == -1 && p->option & USE_PAGER)
>  		use_pager = 1;
> +	if (use_pager ==  1 && p->option & FORBID_PAGER)
> +		use_pager = 0;
>  	commit_pager_choice();
>  
>  	if (p->option & NEED_WORK_TREE)
> @@ -286,7 +289,7 @@ static void handle_internal_command(int argc, const char **argv)
>  		{ "count-objects", cmd_count_objects, RUN_SETUP },
>  		{ "describe", cmd_describe, RUN_SETUP },
>  		{ "diff", cmd_diff },
> -		{ "diff-files", cmd_diff_files, RUN_SETUP },
> +		{ "diff-files", cmd_diff_files, RUN_SETUP | FORBID_PAGER },

Every now and then I want to use 'git -p diff-files', and I think that is
a valid use-case. But your suggested patch seems to forbid the pager even
in this case. :-(

-- Hannes

Re: git status in clean working dir

[Jeff King]

On Tue, Jul 22, 2008 at 09:34:45AM +0200, Johannes Sixt wrote:

> > -		{ "diff-files", cmd_diff_files, RUN_SETUP },
> > +		{ "diff-files", cmd_diff_files, RUN_SETUP | FORBID_PAGER },
> 
> Every now and then I want to use 'git -p diff-files', and I think that is
> a valid use-case. But your suggested patch seems to forbid the pager even
> in this case. :-(

Actually, it doesn't. If you read earlier in the message, this applies
only to pager.* config. That being said, I think Junio's ultimate goal
was to not allow stupid people to accidentally set the pager on
plumbing, at the expense of any smart people who might want to do it for
a good reason.

Though I have to wonder why "git diff --raw" is not enough for you.

At any rate, I think this isn't the right route. We haven't actually
seen evidence of somebody setting pager.diff-files and complaining about
breakage. We have seen people complaining about the lost exit code from
"git status", which is not something that would be on the "forbid" list
anyway. The real solution is to preserve the exit code when spawning the
pager, which I just posted a patch for.

-Peff

Re: git status in clean working dir

[Johannes Sixt]

Jeff King schrieb:
> On Tue, Jul 22, 2008 at 09:34:45AM +0200, Johannes Sixt wrote:
> 
>>> -		{ "diff-files", cmd_diff_files, RUN_SETUP },
>>> +		{ "diff-files", cmd_diff_files, RUN_SETUP | FORBID_PAGER },
>> Every now and then I want to use 'git -p diff-files', and I think that is
>> a valid use-case. But your suggested patch seems to forbid the pager even
>> in this case. :-(
> 
> Actually, it doesn't. If you read earlier in the message, this applies
> only to pager.* config. That being said, I think Junio's ultimate goal
> was to not allow stupid people to accidentally set the pager on
> plumbing, at the expense of any smart people who might want to do it for
> a good reason.
> 
> Though I have to wonder why "git diff --raw" is not enough for you.

Usually, I use plumbing with a pager while I'm writing or debugging a
script, and I'm studying its output. So, no, I'm not interested in "git
diff --raw". ;)

-- Hannes

Re: git status in clean working dir

[Ask Bjørn Hansen]

On Jul 21, 2008, at 19:40, Junio C Hamano wrote:

> (2) Then why are we even allowing to configure the plumbing to page?


I don't have an opinion on the the appropriateness of paging various  
commands, but to solve the problem of scripts getting tripped up by  
the paging, couldn't the plumping check if STDOUT is a tty and only do  
the paging if so?

(In Perl "page_output(...) if $pager_configured and -t STDOUT")


  - ask

-- 
http://develooper.com/ - http://askask.com/

Re: git status in clean working dir

[Jeff King]

On Wed, Jul 23, 2008 at 11:56:48PM -0700, Ask Bjørn Hansen wrote:

>> (2) Then why are we even allowing to configure the plumbing to page?
>
> I don't have an opinion on the the appropriateness of paging various  
> commands, but to solve the problem of scripts getting tripped up by the 
> paging, couldn't the plumping check if STDOUT is a tty and only do the 
> paging if so?
>
> (In Perl "page_output(...) if $pager_configured and -t STDOUT")

We already do this, so "git status >/dev/null" produces the correct exit
code. But scripts don't necessarily redirect stdout.

-Peff