Re: 'git commit --short' without touching index?

Jeff King [off-list ref] writes:

On Mon, Jul 05, 2010 at 01:10:43PM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

It would be nice if the index-refreshing code only wrote to the index if
there was something worth writing. I'm not sure how hard that would be
to implement, though.

Hmm, don't we already do that with "istate->cache_changed"?

Apparently not:

  $ stat .git/index | grep -i modify
  Modify: 2010-07-05 16:52:11.000000000 -0400
  $ git status
  # On branch master
  nothing to commit
  $ stat .git/index | grep -i modify
  Modify: 2010-07-05 16:53:09.000000000 -0400

and it is not just updating some stat-dirtiness. Doing it over and over
will keep updating the index. It looks like we unconditionally do the
lock and write in cmd_status, but I haven't looked further.

Something like this, plus possibly a similar fix to "git commit $path"
codepath, perhaps?

We may want to audit all uses of write_cache() and write_index() that are
not protected with active_cache_changed (or istate->cache_changed); I am
reluctant to suggest placing that logic into write_index() at this point,
though, as we may be updating the index in bulk, without marking
active_cache_changed bit, exactly because we know we will unconditionally
write the result out.

 builtin/commit.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c101f00..86d3926 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -343,9 +343,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
 	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");
+		if (active_cache_changed) {
+			if (write_cache(fd, active_cache, active_nr) ||
+			    commit_locked_index(&index_lock))
+				die("unable to write new_index file");
+		} else {
+			rollback_lock_file(&index_lock);
+		}
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
 	}

On Mon, Jul 05, 2010 at 02:28:16PM -0700, Junio C Hamano wrote:

quoted

Apparently not:

  $ stat .git/index | grep -i modify
  Modify: 2010-07-05 16:52:11.000000000 -0400
  $ git status
  # On branch master
  nothing to commit
  $ stat .git/index | grep -i modify
  Modify: 2010-07-05 16:53:09.000000000 -0400

and it is not just updating some stat-dirtiness. Doing it over and over
will keep updating the index. It looks like we unconditionally do the
lock and write in cmd_status, but I haven't looked further.

Something like this, plus possibly a similar fix to "git commit $path"
codepath, perhaps?

Yeah, that looks sane, though the one for "git status" is actually:

diff --git a/builtin/commit.c b/builtin/commit.c
index 8258a16..5e578af 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1090,9 +1090,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	fd = hold_locked_index(&index_lock, 0);
 	if (0 <= fd) {
-		if (!write_cache(fd, active_cache, active_nr))
+		if (active_cache_changed &&
+		    !write_cache(fd, active_cache, active_nr))
 			commit_locked_index(&index_lock);
-		rollback_lock_file(&index_lock);
+		else
+			rollback_lock_file(&index_lock);
 	}
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;

and I did confirm that it works (does not touch the index when nothing
changes, and does write out the index even for a stat-dirty entry).

We may want to audit all uses of write_cache() and write_index() that are
not protected with active_cache_changed (or istate->cache_changed); I am
reluctant to suggest placing that logic into write_index() at this point,
though, as we may be updating the index in bulk, without marking
active_cache_changed bit, exactly because we know we will unconditionally
write the result out.

I don't think changing write_index() is a good idea. There are a lot of
calls, and I would rather have each one opt into this optimization than
break them in a mass change.

I was thinking that we might be able to simplify all of these lines into
a "lock, write, and commit" function that would either rollback or die
on failure, depending on a parameter. But there's often more complex
logic going on between those functions, so I don't think it would really
save us much.

-Peff