{"thread":{"id":"11652","subject":"[PATCH] git-commit: exit non-zero if we fail to commit the index","startedAt":"2008-01-17T00:07:53Z","lastAt":"2008-01-23T20:47:26Z","messageCount":9,"participants":["Brandon Casey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"65639","messageId":"478E9C59.9070008@nrlssc.navy.mil","threadId":"11652","inReplyTo":null,"subject":"[PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-17T00:07:53Z","receivedAt":"2008-01-17T00:07:53Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Signed-off-by: Brandon Casey \n---\n\n\nShouldn't we be doing this? I think if quiet is set,\nthen a failed rename will go undetected since we\nwon't enter print_summary to have lookup_commit fail.\n\nrerere() die()'s on a failure, but also returns zero\nif it can't create a lock file.\n\nrun_hook also is not checked for failure. I guess it\nshould at least print an error message on failure, but\nI've never used hooks.\n\n-brandon\n\n\n builtin-commit.c | 13 +++++++++----\n 1 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex a764053..d7db7b3 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -122,19 +122,23 @@ static void rollback_index_files(void)\n \t}\n }\n \n-static void commit_index_files(void)\n+static int commit_index_files(void)\n {\n+\tint err = 0;\n+\n \tswitch (commit_style) {\n \tcase COMMIT_AS_IS:\n \t\tbreak; /* nothing to do */\n \tcase COMMIT_NORMAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\tbreak;\n \tcase COMMIT_PARTIAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\trollback_lock_file(&false_lock);\n \t\tbreak;\n \t}\n+\n+\treturn err;\n }\n \n /*\n@@ -912,7 +916,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n \n-\tcommit_index_files();\n+\tif (commit_index_files())\n+\t\tdie(\"unable to write new_index file\");\n \n \trerere();\n \trun_hook(get_index_file(), \"post-commit\", NULL);\n-- \n1.5.4.rc3.17.gb63a4\n"},{"id":"65658","messageId":"7vir1tmey8.fsf@gitster.siamese.dyndns.org","threadId":"11652","inReplyTo":"478E9C59.9070008@nrlssc.navy.mil","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-17T01:13:51Z","receivedAt":"2008-01-17T01:13:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Signed-off-by: Brandon Casey \n> ---\n>\n>\n> Shouldn't we be doing this? I think if quiet is set,\n> then a failed rename will go undetected since we\n> won't enter print_summary to have lookup_commit fail.\n\nBut then it's a bit too late, isn't it? We already have\nsuccessfully made the commit and updated the HEAD to point at\nit. We would need to tell the user that the index is not where\nit is when we detect the error, though.\n\n> run_hook also is not checked for failure.\n\nI think you mean the final post-commit one, but that is\ndeliberate. post-commit is not meant to affect the outcome of\nthe command.\n"},{"id":"65667","messageId":"478EB430.1040604@nrlssc.navy.mil","threadId":"11652","inReplyTo":"7vir1tmey8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-17T01:49:36Z","receivedAt":"2008-01-17T01:49:36Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> Signed-off-by: Brandon Casey \n>> ---\n>>\n>>\n>> Shouldn't we be doing this? I think if quiet is set,\n>> then a failed rename will go undetected since we\n>> won't enter print_summary to have lookup_commit fail.\n> \n> But then it's a bit too late, isn't it? We already have\n> successfully made the commit and updated the HEAD to point at\n> it.\n\nOk, so the commit has been made, but the index (since the rename\nfailed), is out of sync?\n\n> We would need to tell the user that the index is not where\n> it is when we detect the error, though.\n\nThe new index we are trying to rename will be deleted.\nAre you saying we should \n warn the user that the index is now out of sync?\n Or prevent the deletion of the updated index?\n or just ignore this case which I now see as very unlikely to occur?\n\n-brandon\n"},{"id":"65670","messageId":"7v63xtmc9z.fsf@gitster.siamese.dyndns.org","threadId":"11652","inReplyTo":"478EB430.1040604@nrlssc.navy.mil","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-17T02:11:36Z","receivedAt":"2008-01-17T02:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n>> We would need to tell the user that the index is not where\n>> it is when we detect the error, though.\n>\n> The new index we are trying to rename will be deleted.\n> Are you saying we should \n> warn the user that the index is now out of sync?\n\nYeah, something like that. But I think that once this happens\nthere is no easy and sane recovery path for the user, as the\nmost likely cause of the failure there would be the user running\nout of quota, so \"git reset HEAD\" which may be the way to\nrecover from that failure would not have enough room to create a\nnew index file anyway.\n"},{"id":"66324","messageId":"47964370.8010300@nrlssc.navy.mil","threadId":"11652","inReplyTo":"7v63xtmc9z.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-22T19:26:40Z","receivedAt":"2008-01-22T19:26:40Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"---\n\n\nJunio C Hamano wrote:\n> Brandon Casey writes:\n> \n>>> We would need to tell the user that the index is not where\n>>> it is when we detect the error, though.\n>> The new index we are trying to rename will be deleted.\n>> Are you saying we should \n>> warn the user that the index is now out of sync?\n> \n> Yeah, something like that. But I think that once this happens\n> there is no easy and sane recovery path for the user, as the\n> most likely cause of the failure there would be the user running\n> out of quota, so \"git reset HEAD\" which may be the way to\n> recover from that failure would not have enough room to create a\n> new index file anyway.\n\nIf you're interested, here's a patch.\n\n-brandon\n\n\n builtin-commit.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 0227936..d8deb1a 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -122,19 +122,23 @@ static void rollback_index_files(void)\n \t}\n }\n \n-static void commit_index_files(void)\n+static int commit_index_files(void)\n {\n+\tint err = 0;\n+\n \tswitch (commit_style) {\n \tcase COMMIT_AS_IS:\n \t\tbreak; /* nothing to do */\n \tcase COMMIT_NORMAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\tbreak;\n \tcase COMMIT_PARTIAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\trollback_lock_file(&false_lock);\n \t\tbreak;\n \t}\n+\n+\treturn err;\n }\n \n /*\n@@ -926,7 +930,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n \n-\tcommit_index_files();\n+\tif (commit_index_files())\n+\t\tdie (\"Repository has been updated, but unable to write\\n\"\n+\t\t \"new_index file. Check that disk is not full or quota is\\n\"\n+\t\t \"not exceeded, and then \\\"git reset HEAD\\\" to recover.\");\n \n \trerere();\n \trun_hook(get_index_file(), \"post-commit\", NULL);\n-- \n1.5.4.rc4.16.gdd591\n"},{"id":"66337","messageId":"7vfxwpqvfg.fsf@gitster.siamese.dyndns.org","threadId":"11652","inReplyTo":"47964370.8010300@nrlssc.navy.mil","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-22T23:42:27Z","receivedAt":"2008-01-22T23:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> ---\n>\n>\n> Junio C Hamano wrote:\n>> Brandon Casey writes:\n>> \n>>>> We would need to tell the user that the index is not where\n>>>> it is when we detect the error, though.\n>>> The new index we are trying to rename will be deleted.\n>>> Are you saying we should \n>>> warn the user that the index is now out of sync?\n>> \n>> Yeah, something like that. But I think that once this happens\n>> there is no easy and sane recovery path for the user, as the\n>> most likely cause of the failure there would be the user running\n>> out of quota, so \"git reset HEAD\" which may be the way to\n>> recover from that failure would not have enough room to create a\n>> new index file anyway.\n>\n> If you're interested, here's a patch.\n\nLooks Ok from a quick glance. I am mired at day job this week\nso it may take a while for me to come up with a commit log\nmessage though.\n"},{"id":"66418","messageId":"47977792.5080001@nrlssc.navy.mil","threadId":"11652","inReplyTo":"7vfxwpqvfg.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-23T17:21:22Z","receivedAt":"2008-01-23T17:21:22Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"In certain rare cases, the creation of the commit object\nand update of HEAD can succeed, but then installing the\nupdated index will fail. This is most likely caused by a\nfull disk or exceeded disk quota. When this happens the\nnew index file will be removed, and the repository will\nbe left with the original now-out-of-sync index. The\nuser can recover with a \"git reset HEAD\" once the disk\nspace issue is resolved.\n\nWe should detect this failure and offer the user some\nhelpful guidance.\n\nSigned-off-by: Brandon Casey \n---\n\n\nJunio C Hamano wrote:\n> Brandon Casey writes:\n>> If you're interested, here's a patch.\n> \n> Looks Ok from a quick glance. I am mired at day job this week\n> so it may take a while for me to come up with a commit log\n> message though.\n\nOh, I had /ASS/u/ME/d this was simple enough that the one-liner\nwas sufficient.\n\nThis patch includes a commit message that hopefully provides a better\nbase for you to modify.\n\n-brandon\n\n\n builtin-commit.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 0227936..d8deb1a 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -122,19 +122,23 @@ static void rollback_index_files(void)\n \t}\n }\n \n-static void commit_index_files(void)\n+static int commit_index_files(void)\n {\n+\tint err = 0;\n+\n \tswitch (commit_style) {\n \tcase COMMIT_AS_IS:\n \t\tbreak; /* nothing to do */\n \tcase COMMIT_NORMAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\tbreak;\n \tcase COMMIT_PARTIAL:\n-\t\tcommit_lock_file(&index_lock);\n+\t\terr = commit_lock_file(&index_lock);\n \t\trollback_lock_file(&false_lock);\n \t\tbreak;\n \t}\n+\n+\treturn err;\n }\n \n /*\n@@ -926,7 +930,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tunlink(git_path(\"MERGE_HEAD\"));\n \tunlink(git_path(\"MERGE_MSG\"));\n \n-\tcommit_index_files();\n+\tif (commit_index_files())\n+\t\tdie (\"Repository has been updated, but unable to write\\n\"\n+\t\t \"new_index file. Check that disk is not full or quota is\\n\"\n+\t\t \"not exceeded, and then \\\"git reset HEAD\\\" to recover.\");\n \n \trerere();\n \trun_hook(get_index_file(), \"post-commit\", NULL);\n-- \n1.5.4.rc4.17.g0830c\n"},{"id":"66424","messageId":"7vzluwgvls.fsf@gitster.siamese.dyndns.org","threadId":"11652","inReplyTo":"47977792.5080001@nrlssc.navy.mil","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-23T20:01:03Z","receivedAt":"2008-01-23T20:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> In certain rare cases, the creation of the commit object\n> and update of HEAD can succeed, but then installing the\n> updated index will fail. This is most likely caused by a\n> full disk or exceeded disk quota. When this happens the\n> new index file will be removed, and the repository will\n> be left with the original now-out-of-sync index. The\n> user can recover with a \"git reset HEAD\" once the disk\n> space issue is resolved.\n>\n> We should detect this failure and offer the user some\n> helpful guidance.\n>\n> Signed-off-by: Brandon Casey \n> ---\n\nThanks, looks much better, especially with S-o-b: line.\n\n>> Brandon Casey writes:\n>>> If you're interested, here's a patch.\n>> \n>> Looks Ok from a quick glance. I am mired at day job this week\n>> so it may take a while for me to come up with a commit log\n>> message though.\n>\n> Oh, I had /ASS/u/ME/d this was simple enough that the one-liner\n> was sufficient.\n\nI do not get the funny punctuation, sorry.\n\nAnyway, here is a bit more detailed reason behind my request.\n\n (1) The subject is primarily to help people who look at\n shortlog (or \"gitk\") to get the overview of recent changes,\n or in general \"changes within a given range\".\n\n Readers are most interested in what areas are affected\n (e.g. the command from the end-user's point of view, or the\n internal implementation) and what the nature of the change\n was (e.g. bugfix vs enhancement). To help them, the\n Subject: line summarizes \"what the change is about\".\n\n Your Subject: line is _perfect_. It identifies the area as\n \"git-commit\" instead of \"builtin-commit.c\", because it is\n not about fixing internal implementation of that file, but\n about the end-user experience interacting with the command.\n It also makes it clear that it is a fix by saying that we\n failed to exit with non-zero status code upon some failure.\n\n (2) The body of the commit log message is primarily to help\n people who look at this particular commit 6 months down the\n road to see why things got there that way. \n\n Reason behind the logic in the code _after_ the change can\n be left in in-code comments. The reason behind the change\n itself (why the logic behind the code _before_ the change\n was faulty or insufficient, and the logic behind the new\n code is better) is not captured well in such a comment (and\n we do not want to clutter the code comments with a long \"in\n ancient versions we used to do this but then we updated it\n to do that but now we do it this way instead.\" --- I made\n that mistake earlier and I suspect some of the older source\n files still have them).\n\n The commit log message should describe why the change was\n needed (e.g. \"The earlier code assumed X because it knew Y\n won't happen, but that is not the case anymore since commit\n Z, so this code stops relying on that assumption and\n implements the logic this way instead\"), why the proposed\n implementation was thought to be the best one to choose\n (e.g. \"We alternatively could do W and it may have some\n performance edge, but this way the code is simpler and in\n my benchmark with real life data I did not see significant\n gain from the added complexity\").\n\n How the code was changed in this commit does not need to be\n described; that can be seen in \"git show $this_commit\"\n output easily.\n\nIn this particular case, I think it is probably sufficient to\nbriefly describe what \"failure to commit the index\", mentioned\nin the summary line, means. For more complex fixes and\nenhancements, it would make a good log message to also describe\nwhat the plausible cases the updated codepath is triggered, from\nthe point of view of the committer/author when making the commit\n(IOW, what scenarios the updated behaviour intends to handle),\nlike you did in this version.\n\nSuch a description will help the person who finds the change was\nfaulty or insufficient 6 months down the road by allowing him to\nsay \"Aha, the change considered these cases but forgot to\nconsider this case, and missed the fact that this part of the\ncode needs to work differently in that particular case\" while\nmaking further fixes. Otherwise the person will be left\nwondering if the omission of handing that case he encountered\nwas deliberate or a simple oversight. With the comment, he can\nmake his fix with more confidence.\n\nIn addition, at the end of the body, there is expected to be\nyour S-o-b: line, so it will never be \"1-liner\".\n\nIn any case, thanks for a fix. Will apply.\n"},{"id":"66426","messageId":"4797A7DE.7000604@nrlssc.navy.mil","threadId":"11652","inReplyTo":"7vzluwgvls.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-commit: exit non-zero if we fail to commit the index","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-23T20:47:26Z","receivedAt":"2008-01-23T20:47:26Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n\n>> Oh, I had /ASS/u/ME/d this was simple enough that the one-liner\n>> was sufficient.\n> \n> I do not get the funny punctuation, sorry.\n\nAn old joke. I made an assumption and when you _assume_ you make an\n\n _ASS_ out of _U_ and _ME_\n\n:)\n\n-brandon\n"}]}