git status in clean working dir

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.

David Bremner <bremner@unb.ca> writes:

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?

At 2008-07-21 19:30:16 -0700, gitster@pobox.com wrote:

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

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"?

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

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...

On Mon, Jul 21, 2008 at 07:48:02PM -0700, Junio C Hamano 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.

-Peff

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

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

Hi,

On Tue, 22 Jul 2008, Jeff King wrote:

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

Ciao, Dscho

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

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.

  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).

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.

No error, but it is silently ignored. :)

---

On Tue, Jul 22, 2008 at 12:41:57AM -0400, Jeff King wrote:

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

Mike

On Tue, Jul 22, 2008 at 07:39:21AM +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.

-Peff

On Tue, Jul 22, 2008 at 02:06:43AM -0400, Jeff King wrote:

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

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

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

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

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(-)

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(-)

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(-)

On Tue, Jul 22, 2008 at 03:14:12AM -0400, Jeff King 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?

-Peff

On Tue, Jul 22, 2008 at 07:16:30AM +0000, Jeff King wrote:

  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.

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

OK, then this should be squashed in.

Jeff King schrieb:

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

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

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

Jeff King schrieb:

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.

That's fine.

-- Hannes

Jeff King <peff@peff.net> writes:

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?

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

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 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).

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

-Peff

    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

Jeff King schrieb:

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

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

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

Jeff King schrieb:

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

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

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

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

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

-Peff