patch, 4 partsfix regression that "git status" doesn't refresh the index

Patches 1 and 2 are minor unrelated fixes, noticed while working on the regression. Patches 3 and 4 add a test and fix the regression.

Markus Heidelberg (4):
  builtin/commit: fix duplicated sentence in a comment
  builtin/commit: remove unnecessary variable definition
  t7508: add test for "git status" refreshing the index
  git status: refresh the index

 builtin/commit.c  |   11 ++++++++---
 t/t7508-status.sh |   10 ++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin/commit.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

The file descriptor is already defined at the beginning of the function.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin/commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 t/t7508-status.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

This was already the case before commit 9e4b7ab6 (git status: not "commit --dry-run" anymore, 2009-08-15) and got lost during the conversion, which was meant to only change behaviour when invoked with arguments.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin/commit.c  |    5 +++++
 t/t7508-status.sh |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

On Fri, Apr 02, 2010 at 02:27:21PM +0200, Markus Heidelberg wrote:

Does this mean we will fail to run in a read-only repository? I think that status, like diff, should refresh the index on disk if it _can_, but as that refresh is a side effect of the main purpose (which is to output information), it should not be fatal if it cannot do so.

-Peff

Jeff King, 2010-04-02 18:57:

You're right. But that was already the case when "status" was "commit --dry-run". I have to admit, I didn't think about this scenario, but simply looked for the differences between these two commands.

Sounds sensible.

Markus

On Fri, Apr 02, 2010 at 09:37:03PM +0100, Markus Heidelberg wrote:

Sort of. See ab68545 (status: don't require the repository to be writable, 2010-01-19), which went onto maint while the "status is no longer commit --dry-run" topic was cooking elsewhere. So I think it would be a regression from 1.6.6.2 onwards.

At any rate, I think we all agree on what it _should_ do, so if you're willing to do an updated patch, that would be great. The patch from ab68545 may be helpful, as the code it changed is quite similar to what you posted in this series.

-Peff

Markus Heidelberg <markus.heidelberg@web.de> writes:

This is a regression, I think.

The first two patches are trivially correct and I'll queue them to 'master'.

Thanks.

Junio C Hamano, 2010-04-02 20:56:

A regressions in comparison with the current behaviour, but not with the former "commit --dry-run".

I'll send a second attempt without regression.

Markus

This was already the case before commit 9e4b7ab6 (git status: not "commit --dry-run" anymore, 2009-08-15) with the difference that it died at failure. It got lost during the new implementation of "git status", which was meant to only change behaviour when invoked with arguments.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---

v2: Doesn't die when writing the index fails and so works for read-only repositories. Is rollback_lock_file(&index_lock) necessary? It isn't used in "git commit --dry-run" when commit_style is COMMIT_AS_IS.

 builtin/commit.c  |    9 +++++++++
 t/t7508-status.sh |    2 +-
 2 files changed, 10 insertions(+), 1 deletions(-)

Markus Heidelberg <markus.heidelberg@web.de> writes:

That is because AS_IS commit does not even lock anything for writing, as AS_IS means just that: "git commit" does not touch the index but just writes tree out of the index.

Upon program exit (unless you get an uncontrolled crash), the lockfile API arranges atexit(3) to roll back the lockfiles, so it probably may not make much of a difference if you omitted rollback_lock_file(&index_lock) yourself, but it is a good idea to clean up the mess you made after you are done, especially if the mess is not something the operating system will clean up for us (e.g. open file descriptors, malloc'ed region of memory etc.)

To make sure that the failure case is covered, you may also want to add a test case where you run "chmod a-w $GIT_DIR" and then run status (but that test needs to be conditional on POSIXPERM).

Thanks.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 t/t7508-status.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

Thanks, will queue with a minor tweak.

Junio C Hamano, 2010-04-03 06:33:

Hmm, it does lock and write the index, doesn't it?

	/*
	 * As-is commit.
	 *
	 * (1) return the name of the real index file.
	 *
	 * The caller should run hooks on the real index,
	 * and create commit from the_index.
	 * We still need to refresh the index here.
	 */
	if (!pathspec || !*pathspec) {
		fd = hold_locked_index(&index_lock, 1);
		refresh_cache_or_die(refresh_flags);
		if (write_cache(fd, active_cache, active_nr) ||
		    commit_locked_index(&index_lock))
			die("unable to write new_index file");
		commit_style = COMMIT_AS_IS;
		return get_index_file();
	}

$ stat .git/index
Access: 2010-04-03 12:31:11.000000000 +0200
Modify: 2010-04-03 12:31:11.000000000 +0200
Change: 2010-04-03 12:31:11.000000000 +0200
$ git commit --dry-run
$ stat .git/index
Access: 2010-04-03 12:31:52.000000000 +0200
Modify: 2010-04-03 12:31:52.000000000 +0200
Change: 2010-04-03 12:31:52.000000000 +0200

$ chmod a-w .git $ git commit --dry-run fatal: Unable to create '/home/markus/git/git/.git/index.lock': Permission denied

Thanks for the explanation!

Patch has been sent.

Markus