{"thread":{"id":"39079","subject":"[PATCH 3/3] refs.c: remove lock_fd from struct ref_lock","startedAt":"2015-04-14T22:25:05Z","lastAt":"2015-04-15T18:35:06Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"259379","messageId":"1429050308-9617-1-git-send-email-sbeller@google.com","threadId":"39079","inReplyTo":null,"subject":"[PATCH 0/3] Large transactions in git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T22:25:05Z","receivedAt":"2015-04-14T22:25:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Git Merge Wilhelm Bierbaum from Twitter made clear that we'd have problems\nwith large transactions in git. As I have been working on that series a few\nmonths ago and it still bugs me, I thought about reviving the series.\n\nHowever the series got stale a few months ago because we were not sure how\nto do it right. The first few patches were deemed uncontroversial though,\nwhich I am resending now. \n\nThe first patch contains tests telling us what needs fixing.\nThe second patch renames the precondition for other tests, as ULIMIT is\na bit generic. The third patch removes the `lock_fd` field from struct\nref_lock which was duplicate information.\n\nThat said, this series is not fixing the actual issue, but I'd guess\nhaving these patches is step forward actually.\n\nThanks,\nStefan\n\nStefan Beller (3):\n update-ref: test handling large transactions properly\n t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE\n refs.c: remove lock_fd from struct ref_lock\n\n refs.c | 16 ++++++----------\n t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++\n t/t7004-tag.sh | 4 ++--\n 3 files changed, 36 insertions(+), 12 deletions(-)\n\n-- \n2.3.0.81.gc37f363\n"},{"id":"259377","messageId":"1429050308-9617-2-git-send-email-sbeller@google.com","threadId":"39079","inReplyTo":"1429050308-9617-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/3] update-ref: test handling large transactions properly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T22:25:06Z","receivedAt":"2015-04-14T22:25:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 7b4707b..47d2fe9 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n+run_with_limited_open_files () {\n+\t(ulimit -n 32 && \"$@\")\n+}\n+\n+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'\n+\n+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n+(\n+\tfor i in $(test_seq 33)\n+\tdo\n+\t\techo \"create refs/heads/$i HEAD\"\n+\tdone >large_input &&\n+\trun_with_limited_open_files git update-ref --stdin large_input &&\n+\trun_with_limited_open_files git update-ref --stdin \nSigned-off-by: Junio C Hamano \n---\n t/t7004-tag.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 796e9f7..06b8e0d 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1463,10 +1463,10 @@ run_with_limited_stack () {\n \t(ulimit -s 128 && \"$@\")\n }\n \n-test_lazy_prereq ULIMIT 'run_with_limited_stack true'\n+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'\n \n # we require ulimit, this excludes Windows\n-test_expect_success ULIMIT '--contains works in a deep repo' '\n+test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '\n \t>expect &&\n \ti=1 &&\n \twhile test $i -lt 8000\n-- \n2.3.0.81.gc37f363\n"},{"id":"259376","messageId":"1429050308-9617-4-git-send-email-sbeller@google.com","threadId":"39079","inReplyTo":"1429050308-9617-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/3] refs.c: remove lock_fd from struct ref_lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-14T22:25:08Z","receivedAt":"2015-04-14T22:25:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove\nit. You may argue this introduces more coupling as we need to know more\nabout the internals of the lock file mechanism, but this will be solved in\na later patch.\n\nNo functional changes intended.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n refs.c | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 14e52ca..4066752 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -11,7 +11,6 @@ struct ref_lock {\n \tchar *orig_ref_name;\n \tstruct lock_file *lk;\n \tunsigned char old_sha1[20];\n-\tint lock_fd;\n \tint force_write;\n };\n \n@@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint attempts_remaining = 3;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n-\tlock->lock_fd = -1;\n \n \tif (mustexist)\n \t\tresolve_flags |= RESOLVE_REF_READING;\n@@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tgoto error_return;\n \t}\n \n-\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n-\tif (lock->lock_fd < 0) {\n+\tif (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n+\t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2904,7 +2902,6 @@ static 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@@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)\n {\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n-\tlock->lock_fd = -1;\n \treturn 0;\n }\n \n@@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\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+\tif (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n+\t write_in_full(lock->lk->fd, &term, 1) != 1 ||\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n@@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n \t\t\t\t\tstrerror(errno));\n \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n-\t\t\t(write_in_full(lock->lock_fd,\n+\t\t\t(write_in_full(lock->lk->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 write_str_in_full(lock->lk->fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n \t\t\t\t\tlock->lk->filename.buf);\n-- \n2.3.0.81.gc37f363\n"},{"id":"259384","messageId":"xmqq8udu1f0c.fsf@gitster.dls.corp.google.com","threadId":"39079","inReplyTo":"1429050308-9617-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-14T23:12:03Z","receivedAt":"2015-04-14T23:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove\n> it. You may argue this introduces more coupling as we need to know more\n> about the internals of the lock file mechanism, but this will be solved in\n> a later patch.\n>\n> No functional changes intended.\n\nIt is somewhat strange to hear \"in a later patch\" in [PATCH 3/3] of\na 3-patch series ;-), but I think this makes sense. Whenever\nwe take a ref-lock, and we are going to actually write something\ninto the filesystem, we would go thru the lock_file API, so we can\ndepend on lk to have its own file descriptor field.\n\n\n\n\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> refs.c | 16 ++++++----------\n> 1 file changed, 6 insertions(+), 10 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 14e52ca..4066752 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -11,7 +11,6 @@ struct ref_lock {\n> \tchar *orig_ref_name;\n> \tstruct lock_file *lk;\n> \tunsigned char old_sha1[20];\n> -\tint lock_fd;\n> \tint force_write;\n> };\n> \n> @@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \tint attempts_remaining = 3;\n> \n> \tlock = xcalloc(1, sizeof(struct ref_lock));\n> -\tlock->lock_fd = -1;\n> \n> \tif (mustexist)\n> \t\tresolve_flags |= RESOLVE_REF_READING;\n> @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \t\tgoto error_return;\n> \t}\n> \n> -\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n> -\tif (lock->lock_fd < 0) {\n> +\tif (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n> +\t\tlast_errno = errno;\n> \t\tif (errno == ENOENT && --attempts_remaining > 0)\n> \t\t\t/*\n> \t\t\t * Maybe somebody just deleted one of the\n> @@ -2904,7 +2902,6 @@ static 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> @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)\n> {\n> \tif (commit_lock_file(lock->lk))\n> \t\treturn -1;\n> -\tlock->lock_fd = -1;\n> \treturn 0;\n> }\n> \n> @@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,\n> \t\terrno = EINVAL;\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> +\tif (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n> +\t write_in_full(lock->lk->fd, &term, 1) != 1 ||\n> \t close_ref(lock) < 0) {\n> \t\tint save_errno = errno;\n> \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n> @@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n> \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n> \t\t\t\t\tstrerror(errno));\n> \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n> -\t\t\t(write_in_full(lock->lock_fd,\n> +\t\t\t(write_in_full(lock->lk->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 write_str_in_full(lock->lk->fd, \"\\n\") != 1 ||\n> \t\t\t close_ref(lock) < 0)) {\n> \t\t\tstatus |= error(\"couldn't write %s\",\n> \t\t\t\t\tlock->lk->filename.buf);\n"},{"id":"259399","messageId":"552E2608.7020104@alum.mit.edu","threadId":"39079","inReplyTo":"1429050308-9617-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-15T08:49:12Z","receivedAt":"2015-04-15T08:49:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2015 12:25 AM, Stefan Beller wrote:\n> The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove\n> it. You may argue this introduces more coupling as we need to know more\n> about the internals of the lock file mechanism, but this will be solved in\n> a later patch.\n> \n> No functional changes intended.\n\nThis whole series LGTM; however, I suggest that this patch be split up.\nSee below.\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> refs.c | 16 ++++++----------\n> 1 file changed, 6 insertions(+), 10 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 14e52ca..4066752 100644\n> --- a/refs.c\n> +++ b/refs.c\n> [...]\n> @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \t\tgoto error_return;\n> \t}\n> \n> -\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n> -\tif (lock->lock_fd < 0) {\n> +\tif (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n> +\t\tlast_errno = errno;\n> \t\tif (errno == ENOENT && --attempts_remaining > 0)\n> \t\t\t/*\n> \t\t\t * Maybe somebody just deleted one of the\n> [...]\n\nHere you add the line \"last_errno = errno\". It is a good change, but it\nis not part of removing ref_lock::lock_fd.\n\nI suggest that you move this change to a separate commit. You might also\nconsider moving the new line to the \"else\" clause, because it's really\nabout preserving errno around the call to error() and preparing for\n\"goto error_return\".\n\nWith or without this split, this patch is\n\nReviewed-by: Michael Haggerty \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"259436","messageId":"xmqqbnipkzol.fsf@gitster.dls.corp.google.com","threadId":"39079","inReplyTo":"552E2608.7020104@alum.mit.edu","subject":"Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T18:35:06Z","receivedAt":"2015-04-15T18:35:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This whole series LGTM; however, I suggest that this patch be split up.\n> See below.\n>\n>> Signed-off-by: Stefan Beller \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> refs.c | 16 ++++++----------\n>> 1 file changed, 6 insertions(+), 10 deletions(-)\n>> \n>> diff --git a/refs.c b/refs.c\n>> index 14e52ca..4066752 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> [...]\n>> @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>> \t\tgoto error_return;\n>> \t}\n>> \n>> -\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n>> -\tif (lock->lock_fd < 0) {\n>> +\tif (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n>> +\t\tlast_errno = errno;\n>> \t\tif (errno == ENOENT && --attempts_remaining > 0)\n>> \t\t\t/*\n>> \t\t\t * Maybe somebody just deleted one of the\n>> [...]\n>\n> Here you add the line \"last_errno = errno\". It is a good change, but it\n> is not part of removing ref_lock::lock_fd.\n\nI think this patch came from an ancient codebase before 06839515\n(lock_ref_sha1_basic: do not die on locking errors, 2014-11-19),\nwhich added the \"last_errno = errno\", and was not rebased to match\nmore recent codebase.\n\nI am planning to apply these on top of v2.4.0-rc, so there will be\nno new \"save to last_errno\" in the end.\n\nThanks.\n"}]}