Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Shouldn't we be doing this? I think if quiet is set,
then a failed rename will go undetected since we
won't enter print_summary to have lookup_commit fail.

rerere() die()'s on a failure, but also returns zero
if it can't create a lock file.

run_hook also is not checked for failure. I guess it
should at least print an error message on failure, but
I've never used hooks.

-brandon


 builtin-commit.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index a764053..d7db7b3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -122,19 +122,23 @@ static void rollback_index_files(void)
 	}
 }
 
-static void commit_index_files(void)
+static int commit_index_files(void)
 {
+	int err = 0;
+
 	switch (commit_style) {
 	case COMMIT_AS_IS:
 		break; /* nothing to do */
 	case COMMIT_NORMAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		break;
 	case COMMIT_PARTIAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		rollback_lock_file(&false_lock);
 		break;
 	}
+
+	return err;
 }
 
 /*
@@ -912,7 +916,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 
-	commit_index_files();
+	if (commit_index_files())
+		die("unable to write new_index file");
 
 	rerere();
 	run_hook(get_index_file(), "post-commit", NULL);
-- 
1.5.4.rc3.17.gb63a4

-

But then it's a bit too late, isn't it?  We already have
successfully made the commit and updated the HEAD to point at
it.  We would need to tell the user that the index is not where

I think you mean the final post-commit one, but that is
deliberate.  post-commit is not meant to affect the outcome of
the command.
-

Ok, so the commit has been made, but the index (since the rename

The new index we are trying to rename will be deleted.
Are you saying we should 
  warn the user that the index is now out of sync?
  Or prevent the deletion of the updated index?
  or just ignore this case which I now see as very unlikely to occur?

-brandon

-

Yeah, something like that.  But I think that once this happens
there is no easy and sane recovery path for the user, as the
most likely cause of the failure there would be the user running
out of quota, so "git reset HEAD" which may be the way to
recover from that failure would not have enough room to create a
new index file anyway.
-

---



If you're interested, here's a patch.

-brandon


 builtin-commit.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..d8deb1a 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -122,19 +122,23 @@ static void rollback_index_files(void)
 	}
 }
 
-static void commit_index_files(void)
+static int commit_index_files(void)
 {
+	int err = 0;
+
 	switch (commit_style) {
 	case COMMIT_AS_IS:
 		break; /* nothing to do */
 	case COMMIT_NORMAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		break;
 	case COMMIT_PARTIAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		rollback_lock_file(&false_lock);
 		break;
 	}
+
+	return err;
 }
 
 /*
@@ -926,7 +930,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 
-	commit_index_files();
+	if (commit_index_files())
+		die ("Repository has been updated, but unable to write\n"
+		     "new_index file. Check that disk is not full or quota is\n"
+		     "not exceeded, and then \"git reset HEAD\" to recover.");
 
 	rerere();
 	run_hook(get_index_file(), "post-commit", NULL);
-- 
1.5.4.rc4.16.gdd591

-

Looks Ok from a quick glance.  I am mired at day job this week
so it may take a while for me to come up with a commit log
message though.
-

In certain rare cases, the creation of the commit object
and update of HEAD can succeed, but then installing the
updated index will fail. This is most likely caused by a
full disk or exceeded disk quota. When this happens the
new index file will be removed, and the repository will
be left with the original now-out-of-sync index. The
user can recover with a "git reset HEAD" once the disk
space issue is resolved.

We should detect this failure and offer the user some
helpful guidance.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---



Oh, I had /ASS/u/ME/d this was simple enough that the one-liner
was sufficient.

This patch includes a commit message that hopefully provides a better
base for you to modify.

-brandon


 builtin-commit.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..d8deb1a 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -122,19 +122,23 @@ static void rollback_index_files(void)
 	}
 }
 
-static void commit_index_files(void)
+static int commit_index_files(void)
 {
+	int err = 0;
+
 	switch (commit_style) {
 	case COMMIT_AS_IS:
 		break; /* nothing to do */
 	case COMMIT_NORMAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		break;
 	case COMMIT_PARTIAL:
-		commit_lock_file(&index_lock);
+		err = commit_lock_file(&index_lock);
 		rollback_lock_file(&false_lock);
 		break;
 	}
+
+	return err;
 }
 
 /*
@@ -926,7 +930,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 
-	commit_index_files();
+	if (commit_index_files())
+		die ("Repository has been updated, but unable to write\n"
+		     "new_index file. Check that disk is not full or quota is\n"
+		     "not exceeded, and then \"git reset HEAD\" to recover.");
 
 	rerere();
 	run_hook(get_index_file(), "post-commit", NULL);
-- 
1.5.4...
[ message continues ]

I do not get the funny punctuation, sorry.

Anyway, here is a bit more detailed reason behind my request.

 (1) The subject is primarily to help people who look at
     shortlog (or "gitk") to get the overview of recent changes,
     or in general "changes within a given range".

     Readers are most interested in what areas are affected
     (e.g. the command from the end-user's point of view, or the
     internal implementation) and what the nature of the change
     was (e.g. bugfix vs enhancement).  To help them, the
     Subject: line summarizes "what the change is about".

     Your Subject: line is _perfect_.  It identifies the area as
     "git-commit" instead of "builtin-commit.c", because it is
     not about fixing internal implementation of that file, but
     about the end-user experience interacting with the command.
     It also makes it clear that it is a fix by saying that we
     failed to exit with non-zero status code upon some failure.

 (2) The body of the commit log message is primarily to help
     people who look at this particular commit 6 months down the
     road to see why things got there that way.  

     Reason behind the logic in the code _after_ the change can
     be left in in-code comments.  The reason behind the change
     itself (why the logic behind the code _before_ the change
     was faulty or insufficient, and the logic behind the new
     code is better) is not captured well in such a comment (and
     we do not want to clutter the code comments with a long "in
     ancient versions we used to do this but then we updated it
     to do that but now we do it this way instead." --- I made
     that mistake earlier and I suspect some of the older source
     files still have them).

     The commit log message should describe why the change was
     needed (e.g. "The earlier code assumed X because it knew Y
     won't happen, but that is not the case anymore since commit
     Z, so this code stops relying on that assumption and
...
[ message continues ]

An old joke. I made an assumption and when you _assume_ you make an


-brandon

-