{"thread":{"id":"36377","subject":"[PATCH] Remove the close_ref function.","startedAt":"2014-04-08T21:17:09Z","lastAt":"2014-04-08T22:23:47Z","messageCount":5,"participants":["Ronnie Sahlberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238566","messageId":"1396991830-20938-1-git-send-email-sahlberg@google.com","threadId":"36377","inReplyTo":null,"subject":"[PATCH] Remove redundant close_ref function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-08T21:17:09Z","receivedAt":"2014-04-08T21:17:09Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"\nList,\n\nThis is a trivial patch that removes the function close_ref() from refs.c.\nThis function was only called from two codepaths and can be removed since both codepaths shortly afterwards\nboth call unlock_ref() which implicitely closes the file anyway.\n\nBy removing this function we simplify the api to refs slightly.\nThis also means that the lifetime of the filedescriptor becomes the same as the lifetime for the 'struct ref_lock' object.\nThe filedescriptor is opened at the same time ref_lock is allocated and the descriptor is closed when ref_lock is released.\n\n\nregards\nronnie sahlberg\n"},{"id":"238565","messageId":"1396991830-20938-2-git-send-email-sahlberg@google.com","threadId":"36377","inReplyTo":"1396991830-20938-1-git-send-email-sahlberg@google.com","subject":"[PATCH] Remove the close_ref function.","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-08T21:17:10Z","receivedAt":"2014-04-08T21:17:10Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"close_ref() is only called from two codepaths semi-immediately before unlock_ref() is called.\nSince unlock_ref() will also close the file if it is still open, we can delete close_ref() and\nlet the file become closed during unlock_ref().\nThis simplifies the refs.c api by removing a redundant function.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/reflog.c |  3 +--\n refs.c           | 11 +----------\n refs.h           |  3 ---\n 3 files changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c12a9784..b67fbe6 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -428,8 +428,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t} else if (cmd->updateref &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n-\t\t\t close_ref(lock) < 0)) {\n+\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n \t\t\t\tlock->lk->filename);\n \t\t\tunlink(newlog_path);\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..094b047 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2665,14 +2665,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 1;\n }\n \n-int close_ref(struct ref_lock *lock)\n-{\n-\tif (close_lock_file(lock->lk))\n-\t\treturn -1;\n-\tlock->lock_fd = -1;\n-\treturn 0;\n-}\n-\n int commit_ref(struct ref_lock *lock)\n {\n \tif (commit_lock_file(lock->lk))\n@@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\treturn -1;\n \t}\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n-\t    write_in_full(lock->lock_fd, &term, 1) != 1\n-\t\t|| close_ref(lock) < 0) {\n+\t    write_in_full(lock->lock_fd, &term, 1) != 1) {\n \t\terror(\"Couldn't write %s\", lock->lk->filename);\n \t\tunlock_ref(lock);\n \t\treturn -1;\ndiff --git a/refs.h b/refs.h\nindex 87a1a79..d8732a5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -153,9 +153,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n \t\t\t\t\t\tint flags, int *type_p);\n \n-/** Close the file descriptor owned by a lock and return the status */\n-extern int close_ref(struct ref_lock *lock);\n-\n /** Close and commit the ref locked by the lock */\n extern int commit_ref(struct ref_lock *lock);\n \n-- \n1.9.1.423.g4596e3a\n"},{"id":"238568","messageId":"xmqqioqj33tu.fsf@gitster.dls.corp.google.com","threadId":"36377","inReplyTo":"1396991830-20938-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH] Remove redundant close_ref function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-08T21:32:13Z","receivedAt":"2014-04-08T21:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\n\n> List,\n>\n> This is a trivial patch that removes the function close_ref() from refs.c.\n> This function was only called from two codepaths and can be removed since both codepaths shortly afterwards\n> both call unlock_ref() which implicitely closes the file anyway.\n>\n> By removing this function we simplify the api to refs slightly.\n> This also means that the lifetime of the filedescriptor becomes the same as the lifetime for the 'struct ref_lock' object.\n> The filedescriptor is opened at the same time ref_lock is allocated and the descriptor is closed when ref_lock is released.\n>\n>\n> regards\n> ronnie sahlberg\n\nThanks.  A few tips:\n\n - wrap your lines at around 72 columns.\n\n - \"git format-patch --cover-letter\" will give you a skeletal\n   message with \"Subject: [PATCH 0/n]\" with list of individual\n   patches and diffstat to show the overall damage, with two\n   placeholders \"*** SUBJECT HERE ***\" and \"*** BLURB HERE ***\"\n   for you to fill in the remainder.\n\n - For a short/single patch, you do not have to add a cover letter\n   (it is not a crime to add one, though).\n"},{"id":"238569","messageId":"xmqqeh173301.fsf@gitster.dls.corp.google.com","threadId":"36377","inReplyTo":"1396991830-20938-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH] Remove the close_ref function.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-08T21:50:06Z","receivedAt":"2014-04-08T21:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\n\n> @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock,\n>  \t\treturn -1;\n>  \t}\n>  \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n> -\t    write_in_full(lock->lock_fd, &term, 1) != 1\n> -\t\t|| close_ref(lock) < 0) {\n> +\t    write_in_full(lock->lock_fd, &term, 1) != 1) {\n\nIn the original code, we try to write the new object name and the\nline terminator, and also try to make sure that the file descriptor\nis successfully closed here.  Only when all of that is done\nsuccessfully we go update the reflog and then after that, we commit\nthe lockfile by renaming.\n\nWith the updated code, these two write(2)s may succeed, but we would\nnot know if the close(2) would succeed, until commit_lock_file() is\ncalled much later in this codepath.\n\nWe would end up updating the reflog even when the close(2) of the\nref fails.\n\nTo be really paranoid, we should probably be doing an fsync(2)\nto make sure that the bytes written hit the disk platter, not just\nclose(2), and such a change may be a good step in the direction to\nmake the code more robust; in that light, the patch goes in the\nopposite direction \"what it does is not robust enough anyway, so\nlet's loosen it further\".\n\nHmm...\n"},{"id":"238570","messageId":"CAL=YDWk3CkLG5P18GFhUKKQ9bEqOeJayNxKCiTMRtg-d4K_smw@mail.gmail.com","threadId":"36377","inReplyTo":"xmqqeh173301.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Remove the close_ref function.","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-08T22:23:47Z","receivedAt":"2014-04-08T22:23:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 8, 2014 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock,\n>>               return -1;\n>>       }\n>>       if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n>> -         write_in_full(lock->lock_fd, &term, 1) != 1\n>> -             || close_ref(lock) < 0) {\n>> +         write_in_full(lock->lock_fd, &term, 1) != 1) {\n>\n> In the original code, we try to write the new object name and the\n> line terminator, and also try to make sure that the file descriptor\n> is successfully closed here.  Only when all of that is done\n> successfully we go update the reflog and then after that, we commit\n> the lockfile by renaming.\n>\n> With the updated code, these two write(2)s may succeed, but we would\n> not know if the close(2) would succeed, until commit_lock_file() is\n> called much later in this codepath.\n>\n> We would end up updating the reflog even when the close(2) of the\n> ref fails.\n>\n> To be really paranoid, we should probably be doing an fsync(2)\n> to make sure that the bytes written hit the disk platter, not just\n> close(2), and such a change may be a good step in the direction to\n> make the code more robust; in that light, the patch goes in the\n> opposite direction \"what it does is not robust enough anyway, so\n> let's loosen it further\".\n>\n> Hmm...\n>\n\nGood point. Thanks.\nPlease consider this patch abandoned.\n"}]}