{"thread":{"id":"27415","subject":"Better error-handling around revert","startedAt":"2011-05-20T07:16:12Z","lastAt":"2011-05-21T03:47:23Z","messageCount":11,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jonathan Nieder","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168299","messageId":"20110520071609.GA6755@domU-12-31-39-06-A8-0A.compute-1.internal","threadId":"27415","inReplyTo":null,"subject":"Better error-handling around revert","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T07:16:12Z","receivedAt":"2011-05-20T07:16:12Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nInstead of hijacking the Sequencer thread with tons of emails related\nto error-handling, I thought I'd start a new thread. I'm currently\npreparing a large series to fix error-handling issues around revert,\nbut I'm afraid I need a lot of feedback to get it done right. Expect\nlots of RFC patches and diffs, and other discussions related to\nerror-handling in revert on this thread.\n\nRef: http://thread.gmane.org/gmane.comp.version-control.git/173408/focus=173413\n\nAnyway, here's the first:\n\nDoes this look like something callers can use? What should callers do\nwhen an error is returned? Should I also modify callers in the same\npatch?\n\n- do_pick_commit calls write_cherry_pick_head, so I think it should\n propgate an error upwards to revert_or_cherry_pick which should\n ultimately handle the error.\n- do_pick_commit also calls write_message in two places. What should\n happen when the recursive merge fails and the write_message\n succeeds, and viceversa? Again, I think we should propgate the error\n updwards and let revert_or_cherry_pick exit(128).\n\nwrite_cherry_pick_head and write_message used to die when an operation\nsuch as write_in_full failed. Instead, clean up correctly, and return\nan error to be handled by the caller.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 41 +++++++++++++++++++++++++++--------------\n 1 files changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 138485f..bb0db66 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -195,20 +195,29 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n strbuf_addstr(msgbuf, p);\n }\n \n-static void write_cherry_pick_head(void)\n+static int write_cherry_pick_head(void)\n {\n-\tint fd;\n+\tint fd, ret = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n \n\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n \n\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t die_errno(_(\"Could not open '%s' for writing\"),\n-\t \t\t \t git_path(\"CHERRY_PICK_HEAD\"));\n-\t\t\t\t if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-\t\t\t\t die_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n+\t\t\t\t if (fd < 0) {\n+\t\t\t\t \t strbuf_release(&buf);\n+\t\t\t\t\t\treturn error(_(\"Could not open '%s' for writing: %s\"),\n+\t\t\t\t\t\t \t\t git_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+\t\t\t\t\t\t\t\t }\n+\t\t\t\t\t\t\t\t if (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\t\t\t\t\t\t\t \t ret = error(_(\"Could not write to '%s': %s\"),\n+\t\t\t\t\t\t\t\t\t git_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+\t\t\t\t\t\t\t\t\t if (xclose(fd)) {\n+\t\t\t\t\t\t\t\t\t \t ret = error(_(\"Could not close '%s': %s\"),\n+\t\t\t\t\t\t\t\t\t\t \tgit_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t unlink_or_warn(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t }\n \t\t\t\t\t\t\t\t\t\t\t\t\t\t strbuf_release(&buf);\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t return ret;\n }\n \n static void advise(const char *advice, ...)\n@@ -240,17 +249,21 @@ static void print_advice(void)\n advise(\"and commit the result with 'git commit'\");\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int write_message(struct strbuf *msgbuf, const char *filename)\n {\n\tstatic struct lock_file msg_file;\n+\tint ret = 0;\n \n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t \t \t\t\t\t\t LOCK_DIE_ON_ERROR);\n-\t\t\t\t\t\t\t\t if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\t\t\t\t\t\t\t die_errno(_(\"Could not write to %s.\"), filename);\n+\t\t\t\t\t\t\t\t int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n+\t\t\t\t\t\t\t\t if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {\n+\t\t\t\t\t\t\t\t rollback_lock_file(&msg_fd);\n+\t\t\t\t\t\t\t\t\tret = error(_(\"Could not write to %s: %s\"), filename,\n+\t\t\t\t\t\t\t\t\t strerror(errno));\n+\t\t\t\t\t\t\t\t\t }\n+\t\t\t\t\t\t\t\t\t else if (commit_lock_file(&msg_file) < 0)\n+\t\t\t\t\t\t\t\t\t \t ret = error(_(\"Error wrapping up %s\"), filename);\n \t\t\t\t\t\t\t\t\t\t strbuf_release(msgbuf);\n-\t\t\t\t\t\t\t\t\t\t if (commit_lock_file(&msg_file) < 0)\n-\t\t\t\t\t\t\t\t\t\t die(_(\"Error wrapping up %s\"), filename);\n+\t\t\t\t\t\t\t\t\t\t return ret;\n }\n \n static struct tree *empty_tree(void)\n"},{"id":"168300","messageId":"20110520072846.GB6755@domU-12-31-39-06-A8-0A.compute-1.internal","threadId":"27415","inReplyTo":"20110520071609.GA6755@domU-12-31-39-06-A8-0A.compute-1.internal","subject":"Re: Better error-handling around revert","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T07:28:47Z","receivedAt":"2011-05-20T07:28:47Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nUgh, sorry about the whitespace breakage. Here's a fresh diff:\n\n write_cherry_pick_head and write_message used to die when an operation\n such as write_in_full failed. Instead, clean up correctly, and return\n an error to be handled by the caller.\n\nSigned-off-by: Ramkumar Ramachandra \n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 138485f..bb0db66 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -195,20 +195,29 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\nstrbuf_addstr(msgbuf, p);\n }\n \n-static void write_cherry_pick_head(void)\n+static int write_cherry_pick_head(void)\n {\n-int fd;\n+int fd, ret = 0;\nstruct strbuf buf = STRBUF_INIT;\n \nstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n \nfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n-if (fd < 0)\n-die_errno(_(\"Could not open '%s' for writing\"),\n\t - git_path(\"CHERRY_PICK_HEAD\"));\n-if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-die_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n+if (fd < 0) {\n+strbuf_release(&buf);\n+return error(_(\"Could not open '%s' for writing: %s\"),\n\t +git_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+}\n+if (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+ret = error(_(\"Could not write to '%s': %s\"),\n\t +git_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+if (xclose(fd)) {\n+ret = error(_(\"Could not close '%s': %s\"),\n\t +git_path(\"CHERRY_PICK_HEAD\"), strerror(errno));\n+unlink_or_warn(git_path(\"CHERRY_PICK_HEAD\"));\n+}\nstrbuf_release(&buf);\n+return ret;\n }\n \n static void advise(const char *advice, ...)\n@@ -240,17 +249,21 @@ static void print_advice(void)\nadvise(\"and commit the result with 'git commit'\");\n }\n \n-static void write_message(struct strbuf *msgbuf, const char *filename)\n+static int write_message(struct strbuf *msgbuf, const char *filename)\n {\nstatic struct lock_file msg_file;\n+int ret = 0;\n \n-int msg_fd = hold_lock_file_for_update(&msg_file, filename,\n\t\t\t\t\t\t - LOCK_DIE_ON_ERROR);\n-if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-die_errno(_(\"Could not write to %s.\"), filename);\n+int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n+if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {\n+rollback_lock_file(&msg_fd);\n+ret = error(_(\"Could not write to %s: %s\"), filename,\n\t +strerror(errno));\n+}\n+else if (commit_lock_file(&msg_file) < 0)\n+ret = error(_(\"Error wrapping up %s\"), filename);\nstrbuf_release(msgbuf);\n-if (commit_lock_file(&msg_file) < 0)\n-die(_(\"Error wrapping up %s\"), filename);\n+return ret;\n }\n \n static struct tree *empty_tree(void)\n"},{"id":"168306","messageId":"1305880223-7542-1-git-send-email-artagnon@gmail.com","threadId":"27415","inReplyTo":"20110520071609.GA6755@domU-12-31-39-06-A8-0A.compute-1.internal","subject":"[RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T08:30:21Z","receivedAt":"2011-05-20T08:30:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"If the read_cache() call succeeds, the function must call\ndiscard_cache() before returning to the caller.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n cache-tree.c | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..17c5bab 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -573,8 +573,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \n \t\tif (cache_tree_update(active_cache_tree,\n \t\t\t\t active_cache, active_nr,\n-\t\t\t\t missing_ok, 0) < 0)\n+\t\t\t\t missing_ok, 0) < 0) {\n+\t\t\tdiscard_cache();\n \t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n+\t\t}\n \t\tif (0 <= newfd) {\n \t\t\tif (!write_cache(newfd, active_cache, active_nr) &&\n \t\t\t !commit_lock_file(lock_file))\n@@ -591,8 +593,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \tif (prefix) {\n \t\tstruct cache_tree *subtree =\n \t\t\tcache_tree_find(active_cache_tree, prefix);\n-\t\tif (!subtree)\n+\t\tif (!subtree) {\n+\t\t\tdiscard_cache();\n \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n+\t\t}\n \t\thashcpy(sha1, subtree->sha1);\n \t}\n \telse\n@@ -601,6 +605,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \tif (0 <= newfd)\n \t\trollback_lock_file(lock_file);\n \n+\tdiscard_cache();\n \treturn 0;\n }\n \n-- \n1.7.5.GIT\n"},{"id":"168307","messageId":"1305880223-7542-2-git-send-email-artagnon@gmail.com","threadId":"27415","inReplyTo":"1305880223-7542-1-git-send-email-artagnon@gmail.com","subject":"[RFC PATCH] revert: Use assert to catch inherent program bugs","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T08:30:22Z","receivedAt":"2011-05-20T08:30:22Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Instead of returning and error status or calling die, use an assert\nstatement to guard against callers who don't call the functions with\nsane arguments. This situation is hence treated as an inherent bug in\nthe program, rather than a runtime error.\n\nMentored-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 11 +++--------\n 1 files changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex f697e66..8102d77 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)\n \tint abbrev_len, subject_len;\n \tchar *q;\n \n-\tif (!raw_message)\n-\t\treturn -1;\n+\tassert(raw_message);\n \tencoding = get_encoding(raw_message);\n \tif (!encoding)\n \t\tencoding = \"UTF-8\";\n@@ -167,9 +166,7 @@ static char *get_encoding(const char *message)\n {\n \tconst char *p = message, *eol;\n \n-\tif (!p)\n-\t\tdie (_(\"Could not read commit message of %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\tassert(p);\n \twhile (*p && *p != '\\n') {\n \t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n \t\t\t; /* do nothing */\n@@ -444,9 +441,7 @@ static int do_pick_commit(void)\n \t\tdie(_(\"%s: cannot parse parent commit %s\"),\n \t\t me, sha1_to_hex(parent->object.sha1));\n \n-\tif (get_message(commit->buffer, &msg) != 0)\n-\t\tdie(_(\"Cannot get commit message for %s\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\tget_message(commit->buffer, &msg);\n \n \t/*\n \t * \"commit\" is an existing commit. We would want to apply\n-- \n1.7.5.GIT\n"},{"id":"168308","messageId":"1305880223-7542-3-git-send-email-artagnon@gmail.com","threadId":"27415","inReplyTo":"1305880223-7542-1-git-send-email-artagnon@gmail.com","subject":"[RFC PATCH] wrapper: Introduce xclose to restart close on EINTR","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-05-20T08:30:23Z","receivedAt":"2011-05-20T08:30:23Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Suggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n git-compat-util.h | 1 +\n wrapper.c | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 40498b3..6e06ad4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -428,6 +428,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n+extern int xclose(int fd);\n extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\ndiff --git a/wrapper.c b/wrapper.c\nindex 2829000..717e989 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -141,6 +141,21 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+/*\n+ * xclose() is the same a close(), but it automatically restarts close()\n+ * operations with a recoverable error (EINTR).\n+ */\n+int xclose(int fd)\n+{\n+\tint ret;\n+\twhile (1) {\n+\t\tret = close(fd);\n+\t\tif ((ret < 0) && errno == EINTR)\n+\t\t\tcontinue;\n+\t\treturn ret;\n+\t}\n+}\n+\n ssize_t read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n-- \n1.7.5.GIT\n"},{"id":"168321","messageId":"7vboyx48fp.fsf@alter.siamese.dyndns.org","threadId":"27415","inReplyTo":"1305880223-7542-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-20T16:56:42Z","receivedAt":"2011-05-20T16:56:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> If the read_cache() call succeeds, the function must call\n> discard_cache() before returning to the caller.\n\nDoes this mean the callers (current ones and also future ones) cannot\ncontinue using the index after writing it out as a tree?\n\nI would imagine anything that wants to write more than one tree (imagine:\nreimplementation of \"git stash\" in C) would want to do something like:\n\n\tread_cache();\n write_cache_as_tree(index_tree_sha1[]);\n create commit to record that tree, with HEAD as the parent,\n and call that i_commit;\n\n\tfor (all path in work tree different from index)\n\t add_file_to_index();\n write_cache_as_tree(worktree_tree_sha1[]);\n create commit to record that tree, with HEAD as the parent;\n\n create commit to record the same tree, with HEAD and i_commit,\n and return it as the result of \"git stash create\".\n\nOf course you _could_ force the caller to read_cache() what you wrote out\nagain immediately, but I do not see why you want to do so. Especially in\nthe non-error codepath, I do not see why this could be a good change.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index f755590..17c5bab 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -573,8 +573,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n> \n> \t\tif (cache_tree_update(active_cache_tree,\n> \t\t\t\t active_cache, active_nr,\n> -\t\t\t\t missing_ok, 0) < 0)\n> +\t\t\t\t missing_ok, 0) < 0) {\n> +\t\t\tdiscard_cache();\n> \t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n\nAlso this may not be what the callers want, either.\n\nIf your goal is to update the existing code even more reusable by new\ncallers, I would understand if you introduced a \"quiet\" mode to\ncache_tree_update() codepath (especially, verify_cache()), so that the\ncallers can choose to re-inspect the index in the error case and give a\nbetter diagnostics, suggestion, or even auto-correction, depending on the\napplication. That would be a good way to restructure the current API to\ngive more control to the application.\n\nRunning discard_cache() here, however, would forever forbid any such\ncallers to be written, without reverting this change.\n\n> +\t\t}\n> \t\tif (0 <= newfd) {\n> \t\t\tif (!write_cache(newfd, active_cache, active_nr) &&\n> \t\t\t !commit_lock_file(lock_file))\n> @@ -591,8 +593,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n> \tif (prefix) {\n> \t\tstruct cache_tree *subtree =\n> \t\t\tcache_tree_find(active_cache_tree, prefix);\n> -\t\tif (!subtree)\n> +\t\tif (!subtree) {\n> +\t\t\tdiscard_cache();\n> \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n\nSame here.\n\n> +\t\t}\n> \t\thashcpy(sha1, subtree->sha1);\n> \t}\n> \telse\n> @@ -601,6 +605,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n> \tif (0 <= newfd)\n> \t\trollback_lock_file(lock_file);\n> \n> +\tdiscard_cache();\n> \treturn 0;\n> }\n\nI am afraid that I have to say that the approach this patch takes is\ntotally backwards from the point of view of good API design. Perhaps\nthere is an (unstated) other goal you are trying to achieve, but I cannot\nread it from the patch.\n"},{"id":"168322","messageId":"7v7h9l4855.fsf@alter.siamese.dyndns.org","threadId":"27415","inReplyTo":"1305880223-7542-2-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Use assert to catch inherent program bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-20T17:03:02Z","receivedAt":"2011-05-20T17:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Instead of returning and error status or calling die, use an assert\n> statement to guard against callers who don't call the functions with\n> sane arguments.\n\nPlease do not do this. assert() is a mechanism to aid debugging in a\ndevelopment build, and can be stripped away in the production build.\n\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index f697e66..8102d77 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)\n> \tint abbrev_len, subject_len;\n> \tchar *q;\n> \n> -\tif (!raw_message)\n> -\t\treturn -1;\n> +\tassert(raw_message);\n> \tencoding = get_encoding(raw_message);\n\nThis change is wrong, as you could feed a NULL to get_encoding (and you\nalso broke that function in the later hunk).\n\nIf the calling convention of this internal function is that passing NULL\nis a programming bug, please do something like this instead:\n\n\tif (!raw_message)\n\t\tdie(\"BUG: get_message() called with NULL\");\n"},{"id":"168328","messageId":"20110520180745.GC17177@elie","threadId":"27415","inReplyTo":"1305880223-7542-1-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-20T18:07:45Z","receivedAt":"2011-05-20T18:07:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamkumar Ramachandra wrote:\n\n> If the read_cache() call succeeds, the function must call\n> discard_cache() before returning to the caller.\n>\n> Suggested-by: Jonathan Nieder \n\nSorry, I was unclear. What I meant is this: currently all three\ncallers to write_cache_as_tree() call die() or exit() if it fails.\nWhen a new caller wants to carry on after failure, that requires some\ncareful thinking about what the state should be when it resumes.\n\nToward that end, your patch \"[PATCH 1/8] revert: Improve error\nhandling by...\" included\n\n> -\t\tif (write_cache_as_tree(head, 0, NULL))\n> -\t\t\tdie (_(\"Your index file is unmerged.\"));\n> +\t\tif (write_cache_as_tree(head, 0, NULL)) {\n> +\t\t\tdiscard_cache();\n\nbut it doesn't feel right. Why after trying to write-tree do we undo\nthe _reading_ of a tree? It seemed strange.\n\nSo let's think carefully about what the state should be after\nwrite-tree fails. Does it discard_cache() to make valgrind happier?\nOr do we keep the in-core index cached for use in later operations?\nWhichever is the right choice would be likely to apply equally well to\n\"git cherry-pick\" and other write_cache_as_tree callers.\n\nI foolishly did not ask \"do you really want to discard_cache() here?\"\nand instead said something to the effect of \"if you are going to\ndiscard_cache(), won't that apply equally to all callers?\", while the\nformer is a better question. Of course this would all be easier if we\nhad an example to think about that cared about the index state on\nerror one way or another.\n\nIf I understand correctly, the sequencer does not care about the state\nat all, and just wants to write a few files under .git and print a\nmessage. It could do that just as well by keeping the die() and\nsetting up a die handler.\n"},{"id":"168329","messageId":"20110520183513.GE17177@elie","threadId":"27415","inReplyTo":"1305880223-7542-2-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] revert: Use assert to catch inherent program bugs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-20T18:35:13Z","receivedAt":"2011-05-20T18:35:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> This situation is hence treated as an inherent bug in\n> the program, rather than a runtime error.\n\nWell, programming errors can be run-time errors, too. :) But anyway,\nit is not obvious at a glance to me that this assertion would always\nhold, which makes it a bad candidate for an assert().\n\n\"Wait a second,\" you might say. \"You said in a previous message that\nthe check in get_encoding() could be an assert() or die(\"BUG: ...\") to\navoid having to remember the commit id for use in the error message.\"\n\nAnd that's still true:\n\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)\n> \tint abbrev_len, subject_len;\n> \tchar *q;\n> \n> -\tif (!raw_message)\n> -\t\treturn -1;\n> +\tassert(raw_message);\n> \tencoding = get_encoding(raw_message);\n\nget_encoding() only gets called after the check here, so in the\nexisting code, the raw_message parameter to get_encoding() can never\nbe NULL.\n\nBy contrast, get_message() gets called by do_pick_commit() and passed\na buffer that is populated long before then. Can it ever be NULL? I\ndon't know, so the check is certainly doing something useful for my\npeace of mind.\n\n> @@ -444,9 +441,7 @@ static int do_pick_commit(void)\n> \t\tdie(_(\"%s: cannot parse parent commit %s\"),\n> \t\t me, sha1_to_hex(parent->object.sha1));\n> \n> -\tif (get_message(commit->buffer, &msg) != 0)\n> -\t\tdie(_(\"Cannot get commit message for %s\"),\n> -\t\t\t\tsha1_to_hex(commit->object.sha1));\n> +\tget_message(commit->buffer, &msg);\n\nForgetting what's mentioned above, this change would be a regression.\nWhat if get_message() learns to return error() for some other reason?\nIt is only safe to call a function like this if deliberately ignoring\nerrors (in which case a comment might be helpful for explaining why to\nignore the errors) or if the function returns \"void\".\n"},{"id":"168331","messageId":"20110520190538.GG17177@elie","threadId":"27415","inReplyTo":"1305880223-7542-3-git-send-email-artagnon@gmail.com","subject":"Re: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-20T19:05:38Z","receivedAt":"2011-05-20T19:05:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> [Subject: [RFC PATCH] wrapper: Introduce xclose to restart close on EINTR]\n\nclose never returns with errno == EINTR on Linux, but it can happen on\nSolaris, for example.\n\nI can't think of a reason git would want to call close() without this\nloop. Maybe it would make sense to wrap close() unconditionally (see\nbelow).\n\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -141,6 +141,21 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n> \t}\n> }\n> \n> +/*\n> + * xclose() is the same a close(), but it automatically restarts close()\n> + * operations with a recoverable error (EINTR).\n> + */\n\nIf there is to be an xclose, I think I'd say something like\n\n\t/*\n\t * xclose() is like close(), but it retries if interrupted by\n\t * a signal on platforms like Solaris that allow that to avoid\n\t * unnecessarily leaking a file descriptor. It quietly returns\n\t * -1 with errno set appropriately on failure.\n\t */\n\nto avoid confusion with the semantics of xmalloc.\n\n> +int xclose(int fd)\n> +{\n> +\tint ret;\n> +\twhile (1) {\n> +\t\tret = close(fd);\n> +\t\tif ((ret < 0) && errno == EINTR)\n> +\t\t\tcontinue;\n> +\t\treturn ret;\n> +\t}\n> +}\n\nMicronit: close() can only return 0 or -1, so this can be written more\nsimply as\n\n\twhile (close(fd)) {\n\t\tif (errno == EINTR)\n\t\t\tcontinue;\n\t\treturn -1;\n\t}\n\treturn 0;\n\nUntested.\n\n-- >8 --\nSubject: compat: wrap close() to avoid having to worry about EINTR\n\nOn some non-Linux platforms, close() can return EINTR to indicate\ninterruption by a signal. In a poll or select loop, that could be\ngood behavior to prevent hangs, but that doesn't apply anywhere within\ngit. Let close() loop unconditionally in this case to avoid\npreventable file descriptor leaks.\n\nSigned-off-by: Jonathan Nieder \n---\n git-compat-util.h | 12 +++++++++++-\n wrapper.c | 15 ---------------\n 2 files changed, 11 insertions(+), 16 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 6e06ad4..1326edb 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -428,7 +428,6 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n-extern int xclose(int fd);\n extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n@@ -450,6 +449,17 @@ static inline int has_extension(const char *filename, const char *ext)\n \treturn len > extlen && !memcmp(filename + len - extlen, ext, extlen);\n }\n \n+/* Sane close - resumes after interruption by signals */\n+static inline int git_close(int fd)\n+{\n+\twhile (close(fd)) {\n+\t\tif (errno != EINTR)\n+\t\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+#define close git_close\n+\n /* Sane ctype - no locale, and works with signed chars */\n #undef isascii\n #undef isspace\ndiff --git a/wrapper.c b/wrapper.c\nindex 717e989..2829000 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -141,21 +141,6 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n-/*\n- * xclose() is the same a close(), but it automatically restarts close()\n- * operations with a recoverable error (EINTR).\n- */\n-int xclose(int fd)\n-{\n-\tint ret;\n-\twhile (1) {\n-\t\tret = close(fd);\n-\t\tif ((ret < 0) && errno == EINTR)\n-\t\t\tcontinue;\n-\t\treturn ret;\n-\t}\n-}\n-\n ssize_t read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n-- \n1.7.5.1\n"},{"id":"168346","messageId":"201105210547.24831.chriscool@tuxfamily.org","threadId":"27415","inReplyTo":"20110520072846.GB6755@domU-12-31-39-06-A8-0A.compute-1.internal","subject":"Re: Better error-handling around revert","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2011-05-21T03:47:23Z","receivedAt":"2011-05-21T03:47:23Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Friday 20 May 2011 09:28:47 Ramkumar Ramachandra wrote:\n> Hi,\n> \n> Ugh, sorry about the whitespace breakage. Here's a fresh diff:\n\nIt looks like there is still whitespace breakage, because code is not indented \nanymore.\n\n> write_cherry_pick_head and write_message used to die when an operation\n> such as write_in_full failed. Instead, clean up correctly, and return\n> an error to be handled by the caller.\n\nOtherwise it looks good to me.\n\nThanks,\nChristian.\n"}]}