{"thread":{"id":"38424","subject":"[PATCHv2 1/5] update-ref: test handling large transactions properly","startedAt":"2015-01-22T02:32:04Z","lastAt":"2015-01-28T21:35:23Z","messageCount":45,"participants":["Stefan Beller","Michael Haggerty","Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255059","messageId":"1421893929-7447-1-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":null,"subject":"[PATCHv2 0/5] Fix bug in large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T02:32:04Z","receivedAt":"2015-01-22T02:32:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"version2:\n\n* This applies on top of origin/sb/atomic-push though it will result in a one\n line merge conflict with origin/jk/lock-ref-sha1-basic-return-errors when\n merging to origin/next.\n\n* It now uses the FILE* pointer instead of file descriptors. This\n results in a combination of the 2 former patches \"refs.c: have\n a write_in_full_to_lock_file wrapper\" and \"refs.c: write to a\n lock file only once\" as the wrapper function is more adapted to\n its consumers\n\n* no need to dance around with char *pointers which may leak.\n\n* another new patch sneaked into the series: Renaming ULIMIT in t7004\n to ULIMIT_STACK_SIZE\n\nThat said, only the first and third patch are updated from the first version\nof the patches. The others are new in the sense that rewriting them was cheaper\nthan keeping notes in between.\n\nversion1:\n\n(reported as: git update-ref --stdin : too many open files, 2014-12-20)\n\nFirst a test case is introduced to demonstrate the failure,\nthe patches 2-6 are little refactoring and the last patch\nfixes the bug and also marks the bugs as resolved in the\ntest suite.\n\nUnfortunately this applies on top of origin/next.\n\nAny feedback would be welcome!\n\nThanks,\nStefan\n\nStefan Beller (5):\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 refs.c: have a write_sha1_to_lock_file wrapper\n refs.c: enable large transactions\n\n refs.c | 34 ++++++++++++++++++++++------------\n t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++\n t/t7004-tag.sh | 4 ++--\n 3 files changed, 52 insertions(+), 14 deletions(-)\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"255054","messageId":"1421893929-7447-2-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421893929-7447-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 1/5] update-ref: test handling large transactions properly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T02:32:05Z","receivedAt":"2015-01-22T02:32:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \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 \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.2.1.62.g3f15098\n"},{"id":"255058","messageId":"1421893929-7447-4-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421893929-7447-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 3/5] refs.c: remove lock_fd from struct ref_lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T02:32:07Z","receivedAt":"2015-01-22T02:32:07Z","isPatch":false,"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 \n---\n refs.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 14e52ca..2472e61 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,7 @@ 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\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2904,7 +2901,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 +2908,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 +3085,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 +4042,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.2.1.62.g3f15098\n"},{"id":"255057","messageId":"1421893929-7447-5-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421893929-7447-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 4/5] refs.c: have a write_sha1_to_lock_file wrapper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T02:32:08Z","receivedAt":"2015-01-22T02:32:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Now we only have one place where we need to touch the internals of\nthe lock_file struct.\n\nNo functional changes intended.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2472e61..2013d37 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3052,6 +3052,16 @@ int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n+static int write_sha1_to_lock_file(struct ref_lock *lock,\n+\t\t\t\t const unsigned char *sha1)\n+{\n+\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n+\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n+\t\treturn -1;\n+\telse\n+\t\treturn 0;\n+}\n+\n /*\n * Write sha1 into the ref specified by the lock. Make sure that errno\n * is sane on error.\n@@ -3059,7 +3069,6 @@ int is_branch(const char *refname)\n static int write_ref_sha1(struct ref_lock *lock,\n \tconst unsigned char *sha1, const char *logmsg)\n {\n-\tstatic char term = '\\n';\n \tstruct object *o;\n \n \tif (!lock) {\n@@ -3085,8 +3094,7 @@ 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->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n-\t write_in_full(lock->lk->fd, &term, 1) != 1 ||\n+\tif (write_sha1_to_lock_file(lock, sha1) ||\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n@@ -4042,9 +4050,7 @@ 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->lk->fd,\n-\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_str_in_full(lock->lk->fd, \"\\n\") != 1 ||\n+\t\t\t(write_sha1_to_lock_file(lock, cb.last_kept_sha1) ||\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.2.1.62.g3f15098\n"},{"id":"255056","messageId":"1421893929-7447-6-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421893929-7447-1-git-send-email-sbeller@google.com","subject":"[PATCHv2 5/5] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T02:32:09Z","receivedAt":"2015-01-22T02:32:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By closing the file descriptors after creating the lock file we are not\nlimiting the size of the transaction by the number of available file\ndescriptors.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 17 +++++++++++++----\n t/t1400-update-ref.sh | 4 ++--\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2013d37..9d01102 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3055,11 +3055,18 @@ int is_branch(const char *refname)\n static int write_sha1_to_lock_file(struct ref_lock *lock,\n \t\t\t\t const unsigned char *sha1)\n {\n-\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n-\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n+\tif (lock->lk->fd == -1) {\n+\t\tif (reopen_lock_file(lock->lk) < 0\n+\t\t || fdopen_lock_file(lock->lk, \"w\") < 0\n+\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41\n+\t\t || close_lock_file(lock->lk) < 0)\n+\t\t return -1;\n+\t} else {\n+\t\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n+\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n \t\treturn -1;\n-\telse\n-\t\treturn 0;\n+\t}\n+\treturn 0;\n }\n \n /*\n@@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n+\t\t/* Do not keep all lock files open at the same time. */\n+\t\tclose_lock_file(update->lock->lk);\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 47d2fe9..c593a1d 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -979,7 +979,7 @@ run_with_limited_open_files () {\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+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n )\n '\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n-- \n2.2.1.62.g3f15098\n"},{"id":"255078","messageId":"54C0D6F9.1000808@alum.mit.edu","threadId":"38424","inReplyTo":"1421893929-7447-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 1/5] update-ref: test handling large transactions properly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-01-22T10:54:49Z","receivedAt":"2015-01-22T10:54:49Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/22/2015 03:32 AM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++\n> 1 file changed, 28 insertions(+)\n> \n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 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\nRegarding the choice of \"32\", I wonder what is the worst-case number of\nopen file descriptors that are needed *before* counting the ones that\nare currently wasted on open loose-reference locks. On Linux it seems to\nbe only 4 with my setup:\n\n $ (ulimit -n 3 && git update-ref --stdin [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255079","messageId":"54C0DDE7.8030708@alum.mit.edu","threadId":"38424","inReplyTo":"1421893929-7447-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-01-22T11:24:23Z","receivedAt":"2015-01-22T11:24:23Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/22/2015 03:32 AM, Stefan Beller wrote:\n> By closing the file descriptors after creating the lock file we are not\n> limiting the size of the transaction by the number of available file\n> descriptors.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> refs.c | 17 +++++++++++++----\n> t/t1400-update-ref.sh | 4 ++--\n> 2 files changed, 15 insertions(+), 6 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 2013d37..9d01102 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)\n> static int write_sha1_to_lock_file(struct ref_lock *lock,\n> \t\t\t\t const unsigned char *sha1)\n> {\n> -\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n> -\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n> +\tif (lock->lk->fd == -1) {\n> +\t\tif (reopen_lock_file(lock->lk) < 0\n> +\t\t || fdopen_lock_file(lock->lk, \"w\") < 0\n> +\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41\n> +\t\t || close_lock_file(lock->lk) < 0)\n> +\t\t return -1;\n> +\t} else {\n> +\t\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n> +\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n> \t\treturn -1;\n> -\telse\n> -\t\treturn 0;\n> +\t}\n> +\treturn 0;\n> }\n\nI can't figure out where to apply this series or where to fetch it from,\nso I can't see these changes in context, so maybe I'm misunderstanding\nsomething. It looks like this code is doing\n\n open(), close(), open(), fdopen(), write(), fclose(), rename()\n\non each lockfile. But don't we have enough information to write the\nSHA-1 into the lockfile the first time we touch it? I.e., couldn't we\nreduce this to\n\n open(), fdopen(), write(), fclose(), rename()\n\n, where the first four calls all happen in the initial loop? If a\nproblem is discovered when writing a later reference, we would roll back\nthe transaction anyway.\n\nI understand that this would require a bigger rewrite, so maybe it is\nnot worth it.\n\n> /*\n> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t\t update->refname);\n> \t\t\tgoto cleanup;\n> \t\t}\n> +\t\t/* Do not keep all lock files open at the same time. */\n> +\t\tclose_lock_file(update->lock->lk);\n> \t}\n> \n> \t/* Perform updates first so live commits remain referenced */\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255083","messageId":"54C0E76D.5070104@alum.mit.edu","threadId":"38424","inReplyTo":"1421893929-7447-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/5] Fix bug in large transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-01-22T12:05:01Z","receivedAt":"2015-01-22T12:05:01Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/22/2015 03:32 AM, Stefan Beller wrote:\n> version2:\n\nSummary: patches 1-4 look good to me. I sent a separate comment about\npatch 5, which seems to do more system calls than necessary.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255090","messageId":"54C0F41F.2080705@ramsay1.demon.co.uk","threadId":"38424","inReplyTo":"1421893929-7447-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2015-01-22T12:59:11Z","receivedAt":"2015-01-22T12:59:11Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 22/01/15 02:32, Stefan Beller wrote:\n> By closing the file descriptors after creating the lock file we are not\n> limiting the size of the transaction by the number of available file\n> descriptors.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> refs.c | 17 +++++++++++++----\n> t/t1400-update-ref.sh | 4 ++--\n> 2 files changed, 15 insertions(+), 6 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 2013d37..9d01102 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)\n> static int write_sha1_to_lock_file(struct ref_lock *lock,\n> \t\t\t\t const unsigned char *sha1)\n> {\n> -\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n> -\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n> +\tif (lock->lk->fd == -1) {\n> +\t\tif (reopen_lock_file(lock->lk) < 0\n> +\t\t || fdopen_lock_file(lock->lk, \"w\") < 0\n\nfdopen_lock_file() returns a 'FILE *', so this causes sparse to bark:\n\n refs.c:3105:56: error: incompatible types for operation (<)\n refs.c:3105:56: left side has type struct _IO_FILE [usertype] *\n refs.c:3105:56: right side has type int\n\n> +\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41\n> +\t\t || close_lock_file(lock->lk) < 0)\n> +\t\t return -1;\n> +\t} else {\n> +\t\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n\nSimilarly, sparse barks:\n\n refs.c:3110:53: error: incompatible types for operation (<)\n refs.c:3110:53: left side has type struct _IO_FILE [usertype] *\n refs.c:3110:53: right side has type int\n\n> +\t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n> \t\treturn -1;\n> -\telse\n> -\t\treturn 0;\n> +\t}\n> +\treturn 0;\n> }\n> \n> /*\n> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t\t update->refname);\n> \t\t\tgoto cleanup;\n> \t\t}\n> +\t\t/* Do not keep all lock files open at the same time. */\n> +\t\tclose_lock_file(update->lock->lk);\n> \t}\n> \n> \t/* Perform updates first so live commits remain referenced */\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 47d2fe9..c593a1d 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -979,7 +979,7 @@ run_with_limited_open_files () {\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> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n> (\n> \tfor i in $(test_seq 33)\n> \tdo\n> @@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n> )\n> '\n> \n> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n> (\n> \tfor i in $(test_seq 33)\n> \tdo\n> \n\nATB,\nRamsay Jones\n"},{"id":"255092","messageId":"20150122130746.GD19681@peff.net","threadId":"38424","inReplyTo":"54C0D6F9.1000808@alum.mit.edu","subject":"Re: [PATCHv2 1/5] update-ref: test handling large transactions properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-22T13:07:47Z","receivedAt":"2015-01-22T13:07:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 22, 2015 at 11:54:49AM +0100, Michael Haggerty wrote:\n\n> > +run_with_limited_open_files () {\n> > +\t(ulimit -n 32 && \"$@\")\n> > +}\n> \n> Regarding the choice of \"32\", I wonder what is the worst-case number of\n> open file descriptors that are needed *before* counting the ones that\n> are currently wasted on open loose-reference locks. On Linux it seems to\n> be only 4 with my setup:\n> \n> $ (ulimit -n 3 && git update-ref --stdin bash: /dev/null: Too many open files\n> $ (ulimit -n 4 && git update-ref --stdin $\n> \n> This number might depend a little bit on details of the repository, like\n> whether config files import other config files. But as long as the\n> \"background\" number of fds required is at least a few less than 32, then\n> your number should be safe.\n> \n> Does anybody know of a platform where file descriptors are eaten up\n> gluttonously by, for example, each shared library that is in use or\n> something? That's the only think I can think of that could potentially\n> make your choice of 32 problematic.\n\nIt's not just choice of platform. There could be inherited descriptors\nin the environment (e.g., the test suite is being run by a shell that\nkeeps a pipe to CI server open, or something). And the test suite itself\nuses several extra descriptors for hiding and showing output.\n\nI think this is the sort of thing that we have to determine with a mix\nof guessing and empiricism. 4 is almost certainly too low. 32 looks\n\"pretty big\" in practice but not so big that it will make the test slow.\nI think our best bet is probably to ship it and see if anybody reports\nproblems while the patch cooks. Then we can bump the number (or find a\nnew approach) as necessary.\n\n-Peff\n"},{"id":"255093","messageId":"20150122131018.GE19681@peff.net","threadId":"38424","inReplyTo":"54C0DDE7.8030708@alum.mit.edu","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-22T13:10:18Z","receivedAt":"2015-01-22T13:10:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:\n\n> I can't figure out where to apply this series or where to fetch it from,\n> so I can't see these changes in context, so maybe I'm misunderstanding\n> something. It looks like this code is doing\n> \n> open(), close(), open(), fdopen(), write(), fclose(), rename()\n> \n> on each lockfile. But don't we have enough information to write the\n> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we\n> reduce this to\n> \n> open(), fdopen(), write(), fclose(), rename()\n> \n> , where the first four calls all happen in the initial loop? If a\n> problem is discovered when writing a later reference, we would roll back\n> the transaction anyway.\n> \n> I understand that this would require a bigger rewrite, so maybe it is\n> not worth it.\n\nI had a nagging feeling on the multiple-open thing, too, and would much\nprefer to just write out the contents early (since we know what they\nare). It looks like we would just need to split write_ref_sha1() into\nits two halves:\n\n 1. Write out the lockfile\n\n 2. Commit the change\n\nAnd then call them at the appropriate spots from ref_transaction_commit().\n\nI guess that is maybe a step backwards for abstracted ref backends,\nthough.\n\n-Peff\n"},{"id":"255106","messageId":"54C12644.1000406@alum.mit.edu","threadId":"38424","inReplyTo":"20150122131018.GE19681@peff.net","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-01-22T16:33:08Z","receivedAt":"2015-01-22T16:33:08Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/22/2015 02:10 PM, Jeff King wrote:\n> On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:\n> \n>> I can't figure out where to apply this series or where to fetch it from,\n>> so I can't see these changes in context, so maybe I'm misunderstanding\n>> something. It looks like this code is doing\n>>\n>> open(), close(), open(), fdopen(), write(), fclose(), rename()\n>>\n>> on each lockfile. But don't we have enough information to write the\n>> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we\n>> reduce this to\n>>\n>> open(), fdopen(), write(), fclose(), rename()\n>>\n>> , where the first four calls all happen in the initial loop? If a\n>> problem is discovered when writing a later reference, we would roll back\n>> the transaction anyway.\n>>\n>> I understand that this would require a bigger rewrite, so maybe it is\n>> not worth it.\n> \n> I had a nagging feeling on the multiple-open thing, too, and would much\n> prefer to just write out the contents early (since we know what they\n> are). It looks like we would just need to split write_ref_sha1() into\n> its two halves:\n> \n> 1. Write out the lockfile\n> \n> 2. Commit the change\n> \n> And then call them at the appropriate spots from ref_transaction_commit().\n> \n> I guess that is maybe a step backwards for abstracted ref backends,\n> though.\n\nNah, the implementation of ref_transaction_commit() will have to differ\nbetween backends anyway. I don't think this would be a step backwards.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255118","messageId":"CAGZ79kZnECcGOWKr4JLpF5zh68+L8XX7yYntyZDJTbZ5cLD50w@mail.gmail.com","threadId":"38424","inReplyTo":"54C0F41F.2080705@ramsay1.demon.co.uk","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T19:16:32Z","receivedAt":"2015-01-22T19:16:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"How do you run sparse on git?\n\nI noticed there is 'make sparse' though I cannot get it working\nhere in the corporate world as I have problems with openssl\nheaders not being found.\n\nAlso the line numbers seem to bit off compared to what I have\nhere, did you need to modify/preprocess files to get sparse running?\n\nAs for the fix, would it be sufficient to check != NULL instead of < 0?\n\nThanks,\nStefan\n\n\nOn Thu, Jan 22, 2015 at 4:59 AM, Ramsay Jones\n wrote:\n> On 22/01/15 02:32, Stefan Beller wrote:\n>> By closing the file descriptors after creating the lock file we are not\n>> limiting the size of the transaction by the number of available file\n>> descriptors.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> refs.c | 17 +++++++++++++----\n>> t/t1400-update-ref.sh | 4 ++--\n>> 2 files changed, 15 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 2013d37..9d01102 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3055,11 +3055,18 @@ int is_branch(const char *refname)\n>> static int write_sha1_to_lock_file(struct ref_lock *lock,\n>> const unsigned char *sha1)\n>> {\n>> - if (fdopen_lock_file(lock->lk, \"w\") < 0\n>> - || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n>> + if (lock->lk->fd == -1) {\n>> + if (reopen_lock_file(lock->lk) < 0\n>> + || fdopen_lock_file(lock->lk, \"w\") < 0\n>\n> fdopen_lock_file() returns a 'FILE *', so this causes sparse to bark:\n>\n> refs.c:3105:56: error: incompatible types for operation (<)\n> refs.c:3105:56: left side has type struct _IO_FILE [usertype] *\n> refs.c:3105:56: right side has type int\n>\n>> + || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41\n>> + || close_lock_file(lock->lk) < 0)\n>> + return -1;\n>> + } else {\n>> + if (fdopen_lock_file(lock->lk, \"w\") < 0\n>\n> Similarly, sparse barks:\n>\n> refs.c:3110:53: error: incompatible types for operation (<)\n> refs.c:3110:53: left side has type struct _IO_FILE [usertype] *\n> refs.c:3110:53: right side has type int\n>\n>> + || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n>> return -1;\n>> - else\n>> - return 0;\n>> + }\n>> + return 0;\n>> }\n>>\n>> /*\n>> @@ -3761,6 +3768,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>> update->refname);\n>> goto cleanup;\n>> }\n>> + /* Do not keep all lock files open at the same time. */\n>> + close_lock_file(update->lock->lk);\n>> }\n>>\n>> /* Perform updates first so live commits remain referenced */\n>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n>> index 47d2fe9..c593a1d 100755\n>> --- a/t/t1400-update-ref.sh\n>> +++ b/t/t1400-update-ref.sh\n>> @@ -979,7 +979,7 @@ run_with_limited_open_files () {\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>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n>> (\n>> for i in $(test_seq 33)\n>> do\n>> @@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n>> )\n>> '\n>>\n>> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n>> (\n>> for i in $(test_seq 33)\n>> do\n>>\n>\n> ATB,\n> Ramsay Jones\n>\n>\n>\n"},{"id":"255120","messageId":"CAGZ79kbrnu7ZH7T6WR5j0_nhS+Ann0LcEmXfQYMRqdSzK0xW9Q@mail.gmail.com","threadId":"38424","inReplyTo":"54C12644.1000406@alum.mit.edu","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T19:24:57Z","receivedAt":"2015-01-22T19:24:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 22, 2015 at 8:33 AM, Michael Haggerty wrote:\n> On 01/22/2015 02:10 PM, Jeff King wrote:\n>> On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:\n>>\n>>> I can't figure out where to apply this series or where to fetch it from,\n>>> so I can't see these changes in context, so maybe I'm misunderstanding\n>>> something. It looks like this code is doing\n>>>\n>>> open(), close(), open(), fdopen(), write(), fclose(), rename()\n>>>\n>>> on each lockfile. But don't we have enough information to write the\n>>> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we\n>>> reduce this to\n>>>\n>>> open(), fdopen(), write(), fclose(), rename()\n>>>\n>>> , where the first four calls all happen in the initial loop? If a\n>>> problem is discovered when writing a later reference, we would roll back\n>>> the transaction anyway.\n>>>\n>>> I understand that this would require a bigger rewrite, so maybe it is\n>>> not worth it.\n>>\n>> I had a nagging feeling on the multiple-open thing, too, and would much\n>> prefer to just write out the contents early (since we know what they\n>> are). It looks like we would just need to split write_ref_sha1() into\n>> its two halves:\n>>\n>> 1. Write out the lockfile\n>>\n>> 2. Commit the change\n>>\n>> And then call them at the appropriate spots from ref_transaction_commit().\n>>\n>> I guess that is maybe a step backwards for abstracted ref backends,\n>> though.\n>\n> Nah, the implementation of ref_transaction_commit() will have to differ\n> between backends anyway. I don't think this would be a step backwards.\n>\n> Michael\n>\n\nI also dislike the double open/close thing, but I just wanted to come up with\na quick and unobtrusive fix which doesn't rewrite the whole refs backend as\nwe have some code churn in the refs lately.\n\nMichael, I forgot your short term intentions on the refs backend, so I tried to\nbe shy with that bug fix. What huge changes are you planning in the next few\nweeks w.r.t. the refs handling? I would look more into that if there are no code\nconflicts likely to arise.\n\nThanks,\nStefan\n"},{"id":"255123","messageId":"54C154C0.9020708@ramsay1.demon.co.uk","threadId":"38424","inReplyTo":"CAGZ79kZnECcGOWKr4JLpF5zh68+L8XX7yYntyZDJTbZ5cLD50w@mail.gmail.com","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2015-01-22T19:51:28Z","receivedAt":"2015-01-22T19:51:28Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 22/01/15 19:16, Stefan Beller wrote:\n> How do you run sparse on git?\n\n $ make sparse >sp-out 2>&1\n\n> \n> I noticed there is 'make sparse' though I cannot get it working\n> here in the corporate world as I have problems with openssl\n> headers not being found.\n\nIf you can build git with gcc, you should be able to run 'make sparse'\n(modulo bugs, of course!). Having said that, I build sparse from source\nand (on Linux) I'm running:\n\n $ sparse --version\n v0.5.0-30-gca3309e\n $\n\nThe most up-to-date (from git://git.kernel.org/pub/scm/devel/sparse/sparse.git)\nis actually:\n\n $ sparse --version\n v0.5.0-41-g6c2d743\n $ \n\nwhich should work just fine. (I also run sparse on cygwin and MinGW).\n\n> \n> Also the line numbers seem to bit off compared to what I have\n> here, did you need to modify/preprocess files to get sparse running?\n\nNo, I am simply building the 'pu' branch (currently @ 028c360).\n\n> \n> As for the fix, would it be sufficient to check != NULL instead of < 0?\n\nHmm, I didn't give it any thought, but don't you want that to be '== NULL'?\n(you don't want to use a NULL lock->lk->fp in the following fprintf()).\nOr simply '!fdopen_lock_file(lock->lk, \"w\")' I suppose.\n\nATB,\nRamsay Jones\n"},{"id":"255127","messageId":"54C15A00.9000706@ramsay1.demon.co.uk","threadId":"38424","inReplyTo":"54C154C0.9020708@ramsay1.demon.co.uk","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2015-01-22T20:13:52Z","receivedAt":"2015-01-22T20:13:52Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 22/01/15 19:51, Ramsay Jones wrote:\n> On 22/01/15 19:16, Stefan Beller wrote:\n>> How do you run sparse on git?\n> \n> $ make sparse >sp-out 2>&1\n> \n\nBTW, you can get gcc to warn about this also:\n\n $ rm refs.o\n $ make CFLAGS='-Wall -Wextra' refs.o\n * new build flags\n CC refs.o\nIn file included from cache.h:4:0,\n from refs.c:1:\ngit-compat-util.h: In function ‘git_has_dos_drive_prefix’:\ngit-compat-util.h:277:56: warning: unused parameter ‘path’ [-Wunused-parameter]\n static inline int git_has_dos_drive_prefix(const char *path)\n ^\nIn file included from cache.h:4:0,\n from refs.c:1:\ngit-compat-util.h: In function ‘xsize_t’:\ngit-compat-util.h:689:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n if (len > (size_t) len)\n ^\nrefs.c: In function ‘check_refname_component’:\nrefs.c:54:61: warning: unused parameter ‘flags’ [-Wunused-parameter]\n static int check_refname_component(const char *refname, int flags)\n ^\nrefs.c: In function ‘warn_if_dangling_symref’:\nrefs.c:1814:78: warning: unused parameter ‘sha1’ [-Wunused-parameter]\n static int warn_if_dangling_symref(const char *refname, const unsigned char *sha1,\n ^\nrefs.c: In function ‘log_ref_write_fd’:\nrefs.c:3042:14: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n if (written != len)\n ^\nrefs.c: In function ‘write_sha1_to_lock_file’:\nrefs.c:3105:42: warning: ordered comparison of pointer with integer zero [-Wextra]\n || fdopen_lock_file(lock->lk, \"w\") < 0\n ^\nrefs.c:3110:39: warning: ordered comparison of pointer with integer zero [-Wextra]\n if (fdopen_lock_file(lock->lk, \"w\") < 0\n ^\nrefs.c: In function ‘create_symref’:\nrefs.c:3220:18: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n if (sizeof(ref) <= len) {\n ^\nrefs.c: In function ‘read_ref_at_ent’:\nrefs.c:3277:15: warning: unused parameter ‘email’ [-Wunused-parameter]\n const char *email, unsigned long timestamp, int tz,\n ^\nrefs.c: In function ‘read_ref_at_ent_oldest’:\nrefs.c:3324:19: warning: unused parameter ‘email’ [-Wunused-parameter]\n const char *email, unsigned long timestamp,\n ^\nrefs.c: In function ‘for_each_reflog_ent_reverse’:\nrefs.c:3451:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;\n ^\nrefs.c:3451:43: warning: signed and unsigned type in conditional expression [-Wsign-compare]\n cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;\n ^\nrefs.c: In function ‘ref_transaction_free’:\nrefs.c:3659:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n for (i = 0; i < transaction->nr; i++) {\n ^\n $ \n\nNotice the [-Wextra] warnings above. ;-)\n\nATB,\nRamsay Jones\n"},{"id":"255129","messageId":"CAGZ79kaq25tD5y3r6FY_5X6xUz9b7Hb8h1KBbVdhsX5zPN_7Dw@mail.gmail.com","threadId":"38424","inReplyTo":"54C15A00.9000706@ramsay1.demon.co.uk","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T20:20:11Z","receivedAt":"2015-01-22T20:20:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 22, 2015 at 12:13 PM, Ramsay Jones\n wrote:\n>\n> Notice the [-Wextra] warnings above. ;-)\n>\n> ATB,\n> Ramsay Jones\n>\n\nThanks, I put that into my config.mak\nThough recompiling the whole project yields\n\n 4 [-Wempty-body]\n 477 [-Wmissing-field-initializers]\n 966 [-Wsign-compare]\n 899 [-Wunused-parameter]\n\nso maybe I'll disable it again when I think it's too much output.\n"},{"id":"255132","messageId":"54C1649A.4030705@ramsay1.demon.co.uk","threadId":"38424","inReplyTo":"CAGZ79kaq25tD5y3r6FY_5X6xUz9b7Hb8h1KBbVdhsX5zPN_7Dw@mail.gmail.com","subject":"Re: [PATCHv2 5/5] refs.c: enable large transactions","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2015-01-22T20:59:06Z","receivedAt":"2015-01-22T20:59:06Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 22/01/15 20:20, Stefan Beller wrote:\n> On Thu, Jan 22, 2015 at 12:13 PM, Ramsay Jones\n> wrote:\n>>\n>> Notice the [-Wextra] warnings above. ;-)\n>>\n>> ATB,\n>> Ramsay Jones\n>>\n> \n> Thanks, I put that into my config.mak\n> Though recompiling the whole project yields\n> \n> 4 [-Wempty-body]\n> 477 [-Wmissing-field-initializers]\n> 966 [-Wsign-compare]\n> 899 [-Wunused-parameter]\n> \n> so maybe I'll disable it again when I think it's too much output.\n> \n\nYes, you don't want to use -Wextra for everyday usage! :-D\n\n[BTW, I don't know if sparse works on OS X, so ...]\n\nATB,\nRamsay Jones\n"},{"id":"255148","messageId":"1421968297-25407-1-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"54C0DDE7.8030708@alum.mit.edu","subject":"[RFC PATCH 0/5] So you dislike the sequence of system calls?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T23:11:32Z","receivedAt":"2015-01-22T23:11:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This series goes on top of origin/sb/atomic-push-fix for now.\n\nI am inclined to squash the first patch into the last commit of \norigin/sb/atomic-push-fix when rerolling that series as it just \nfixes the warning Ramsay pointed out. I'd also like to combine\nthis series with the origin/sb/atomic-push-fix in a reroll of\neither series such that it becomes one larger series.\n\nThe patches 2,3,4 are just preparations (no change intended)\nfor the patch 5 which then corrects the sequence of system calls\nsuch that we don't close and reopen the lock file.\n\n(Background: Why am I spending time to fix that bug the way I do?)\nI think writing out the sha1 early to the .lock file helps in\nfurther patch series for cross repository atomic pushes, because\nif we can split the transaction_commit into two parts, where the\nlatter part only has lock file pathes in memory, dealing with\ncross repository ref updates becomes easy in such a way:\n(compare discussion [RFC] Introducing different handling \nfor small/large transactions)\n\n cat << EOF > stdin_pipe\n delete topic2 2345\n update master 4567 2378\n repository ../API-consumer # this switches to another repository\n delete topic2 3459\n update master 6787 9878\n EOF\n git update-ref --stdin \n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9fc0e60..aae3b66 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3058,12 +3058,12 @@ static int write_sha1_to_lock_file(struct ref_lock *lock,\n {\n \tif (lock->lk->fd == -1) {\n \t\tif (reopen_lock_file(lock->lk) < 0\n-\t\t || fdopen_lock_file(lock->lk, \"w\") < 0\n+\t\t || fdopen_lock_file(lock->lk, \"w\") == NULL\n \t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41\n \t\t || close_lock_file(lock->lk) < 0)\n \t\t return -1;\n \t} else {\n-\t\tif (fdopen_lock_file(lock->lk, \"w\") < 0\n+\t\tif (fdopen_lock_file(lock->lk, \"w\") == NULL\n \t\t || fprintf(lock->lk->fp, \"%s\\n\", sha1_to_hex(sha1)) != 41)\n \t\treturn -1;\n \t}\n-- \n2.2.1.62.g3f15098\n"},{"id":"255149","messageId":"1421968297-25407-3-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421968297-25407-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/5] refs.c: remove unlock_ref from write_ref_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T23:11:34Z","receivedAt":"2015-01-22T23:11:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of calling unlock_ref before each return in write_ref_sha1\nwe can call this after the call. This is a first step to split up\nwrite_ref_sha1 into the write and commit phase which is done in a\nlater patch.\n\nThere is a call in each code path after write_ref_sha1 now. Even in\nthe last hunk in the error case, the 'goto cleanup;' will make sure\nthere is a call to unlock_ref.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 20 ++++++++------------\n 1 file changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex aae3b66..4580919 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2866,9 +2866,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n \tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\t\tunlock_ref(lock);\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n \t\tgoto rollback;\n \t}\n+\tunlock_ref(lock);\n \n \treturn 0;\n \n@@ -2884,6 +2886,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlog_all_ref_updates = 0;\n \tif (write_ref_sha1(lock, orig_sha1, NULL))\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n+\tunlock_ref(lock);\n \tlog_all_ref_updates = flag;\n \n rollbacklog:\n@@ -3083,22 +3086,19 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n-\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n-\t\tunlock_ref(lock);\n+\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n \t\treturn 0;\n-\t}\n+\n \to = parse_object(sha1);\n \tif (!o) {\n \t\terror(\"Trying to write ref %s with nonexistent object %s\",\n \t\t\tlock->ref_name, sha1_to_hex(sha1));\n-\t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\terror(\"Trying to write non-commit object %s to branch %s\",\n \t\t\tsha1_to_hex(sha1), lock->ref_name);\n-\t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n@@ -3106,7 +3106,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n-\t\tunlock_ref(lock);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n@@ -3114,7 +3113,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n@@ -3141,10 +3139,8 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t}\n \tif (commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tunlock_ref(lock);\n \treturn 0;\n }\n \n@@ -3770,7 +3766,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tgoto cleanup;\n \t\t}\n \t\t/* Do not keep all lock files open at the same time. */\n-\t\tclose_lock_file(update->lock->lk);\n+\t\tclose_ref(update->lock);\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n@@ -3780,13 +3776,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t update->msg)) {\n-\t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n+\t\t\tunlock_ref(update->lock);\n+\t\t\tupdate->lock = NULL;\n \t\t}\n \t}\n \n-- \n2.2.1.62.g3f15098\n"},{"id":"255152","messageId":"1421968297-25407-4-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421968297-25407-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/5] refs.c: move static functions to close and commit refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T23:11:35Z","receivedAt":"2015-01-22T23:11:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By moving the functions up we don't need to have to declare them first\nwhen using them in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4580919..6f3cd7b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2808,6 +2808,20 @@ static int rename_ref_available(const char *oldname, const char *newname)\n static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n \t\t\t const char *logmsg);\n \n+static int close_ref(struct ref_lock *lock)\n+{\n+\tif (close_lock_file(lock->lk))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int commit_ref(struct ref_lock *lock)\n+{\n+\tif (commit_lock_file(lock->lk))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n \tunsigned char sha1[20], orig_sha1[20];\n@@ -2901,20 +2915,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 1;\n }\n \n-static int close_ref(struct ref_lock *lock)\n-{\n-\tif (close_lock_file(lock->lk))\n-\t\treturn -1;\n-\treturn 0;\n-}\n-\n-static int commit_ref(struct ref_lock *lock)\n-{\n-\tif (commit_lock_file(lock->lk))\n-\t\treturn -1;\n-\treturn 0;\n-}\n-\n /*\n * copy the reflog message msg to buf, which has been allocated sufficiently\n * large, while cleaning up the whitespaces. Especially, convert LF to space,\n-- \n2.2.1.62.g3f15098\n"},{"id":"255153","messageId":"1421968297-25407-5-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421968297-25407-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/5] refs.c: remove committing the ref from write_ref_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T23:11:36Z","receivedAt":"2015-01-22T23:11:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes write_ref_sha1 only write the the lock file, committing\nneeds to be done outside of that function. This will help us change\nthe ref_transaction_commit in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6f3cd7b..c108c95 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)\n \treturn 0;\n }\n \n-static int commit_ref(struct ref_lock *lock)\n+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n {\n+\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n+\t\treturn 0;\n+\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n \treturn 0;\n@@ -2879,7 +2882,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t}\n \tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n-\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\tif (write_ref_sha1(lock, orig_sha1, logmsg)\n+\t || commit_ref(lock, orig_sha1)) {\n \t\tunlock_ref(lock);\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n \t\tgoto rollback;\n@@ -2898,8 +2902,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlock->force_write = 1;\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n-\tif (write_ref_sha1(lock, orig_sha1, NULL))\n+\tif (write_ref_sha1(lock, orig_sha1, NULL)\n+\t || commit_ref(lock, orig_sha1))\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n+\n \tunlock_ref(lock);\n \tlog_all_ref_updates = flag;\n \n@@ -3137,10 +3143,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\t !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n \t}\n-\tif (commit_ref(lock)) {\n-\t\terror(\"Couldn't set %s\", lock->ref_name);\n-\t\treturn -1;\n-\t}\n \treturn 0;\n }\n \n@@ -3775,7 +3777,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t update->msg)) {\n+\t\t\t\t\t update->msg)\n+\t\t\t || commit_ref(update->lock, update->new_sha1)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n@@ -4064,7 +4067,8 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"unable to commit reflog '%s' (%s)\",\n \t\t\t\t\tlog_file, strerror(errno));\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF)\n+\t\t\t && commit_ref(lock, cb.last_kept_sha1)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n \t\t}\n \t}\n-- \n2.2.1.62.g3f15098\n"},{"id":"255151","messageId":"1421968297-25407-6-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1421968297-25407-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/5] refs.c: write values to lock files early for committing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T23:11:37Z","receivedAt":"2015-01-22T23:11:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By writing the values to the lock file early we have a better sequence\nof system calls. Before this commit we had\n\n open(), close(), open(), fdopen(), write(), fclose(), rename()\n\nNow there is:\n\n open(), fdopen(), write(), fclose(), rename()\n\nThe first four operations are performed in the first loop of\nref_transaction_commit (\"/* Acquire all locks while verifying\nold values */\"), while the renameing is left to the next loop.\n\nSigned-off-by: Stefan Beller \n---\n refs.c | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c108c95..3d1890f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3767,6 +3767,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n+\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n+\t\t\t\t\t update->msg)) {\n+\t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n+\t\t\t\t\t update->refname);\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n \t\t/* Do not keep all lock files open at the same time. */\n \t\tclose_ref(update->lock);\n \t}\n@@ -3776,9 +3785,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n-\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t update->msg)\n-\t\t\t || commit_ref(update->lock, update->new_sha1)) {\n+\t\t\tif (commit_ref(update->lock, update->new_sha1)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-- \n2.2.1.62.g3f15098\n"},{"id":"255215","messageId":"1422043442-30676-1-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"54C0E76D.5070104@alum.mit.edu","subject":"[PATCHv3 0/6] Fix bug in large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:03:56Z","receivedAt":"2015-01-23T20:03:56Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"version3:\n patches 1,2,3 stayed completely as is, while patches 4,5 are new, patch 6 is \n rewritten to first write the contents of the lock files before closing them.\n \n This combines the series \"Enable large transactions v2\" as sent out yesterday\n with the follow up series \"[RFC PATCH 0/5] So you dislike the sequence of \n system calls?\"\n \n There is no write_in_full_to_lock_file wrapper any more, but write_ref_sha1\n was reduced in functionality in patch 5.\n \n This applies on top of origin/sb/atomic-push and results in a merge conflict\n when merging it to origin/jk/lock-ref-sha1-basic-return-errors which looks like\n \n $git checkout origin/jk/lock-ref-sha1-basic-return-errors\n $git merge enable_large_transactions\n CONFLICT (content): Merge conflict in refs.c\n $git diff\n++<<<<<<< HEAD\n + lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n + if (lock->lock_fd < 0) {\n++=======\n+ if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n++>>>>>>> enable_large_transactions\n\nwhich is best resolved as:\n@@@ -2316,8 -2333,7 +2333,12 @@@ static struct ref_lock *lock_ref_sha1_b\n goto error_return;\n }\n \n+ if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n last_errno = errno;\n if (errno == ENOENT && --attempts_remaining > 0)\n /*\n\n\nversion2:\n\n* This applies on top of origin/sb/atomic-push though it will result in a one\n line merge conflict with origin/jk/lock-ref-sha1-basic-return-errors when\n merging to origin/next.\n\n* It now uses the FILE* pointer instead of file descriptors. This\n results in a combination of the 2 former patches \"refs.c: have\n a write_in_full_to_lock_file wrapper\" and \"refs.c: write to a\n lock file only once\" as the wrapper function is more adapted to\n its consumers\n\n* no need to dance around with char *pointers which may leak.\n\n* another new patch sneaked into the series: Renaming ULIMIT in t7004\n to ULIMIT_STACK_SIZE\n\nThat said, only the first and third patch are updated from the first version\nof the patches. The others are new in the sense that rewriting them was cheaper\nthan keeping notes in between.\n\nversion1:\n\n(reported as: git update-ref --stdin : too many open files, 2014-12-20)\n\nFirst a test case is introduced to demonstrate the failure,\nthe patches 2-6 are little refactoring and the last patch\nfixes the bug and also marks the bugs as resolved in the\ntest suite.\n\nUnfortunately this applies on top of origin/next.\n\nAny feedback would be welcome!\n\nThanks,\nStefan\n\nStefan Beller (6):\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 refs.c: move static functions to close and commit refs\n refs.c: remove unlock_ref and commit_ref from write_ref_sha1\n refs.c: enable large transactions\n\n refs.c | 93 +++++++++++++++++++++++++++------------------------\n t/t1400-update-ref.sh | 28 ++++++++++++++++\n t/t7004-tag.sh | 4 +--\n 3 files changed, 79 insertions(+), 46 deletions(-)\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"255218","messageId":"1422043442-30676-2-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1422043442-30676-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 1/6] update-ref: test handling large transactions properly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:03:57Z","receivedAt":"2015-01-23T20:03:57Z","isPatch":false,"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\nNotes:\n v2->v3:\n no changes\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\nNotes:\n v2->v3:\n no changes\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.2.1.62.g3f15098\n"},{"id":"255217","messageId":"1422043442-30676-4-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1422043442-30676-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 3/6] refs.c: remove lock_fd from struct ref_lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:03:59Z","receivedAt":"2015-01-23T20:03:59Z","isPatch":false,"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\nNotes:\n v2->v3:\n no changes\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.2.1.62.g3f15098\n"},{"id":"255219","messageId":"1422043442-30676-5-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1422043442-30676-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 4/6] refs.c: move static functions to close and commit refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:04:00Z","receivedAt":"2015-01-23T20:04:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By moving the functions up we don't need to have to declare them first\nwhen using them in a later patch.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n new in v3\n\n refs.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4066752..f1eefc7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2808,6 +2808,20 @@ static int rename_ref_available(const char *oldname, const char *newname)\n static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n \t\t\t const char *logmsg);\n \n+static int close_ref(struct ref_lock *lock)\n+{\n+\tif (close_lock_file(lock->lk))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n+static int commit_ref(struct ref_lock *lock)\n+{\n+\tif (commit_lock_file(lock->lk))\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n \tunsigned char sha1[20], orig_sha1[20];\n@@ -2898,20 +2912,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 1;\n }\n \n-static int close_ref(struct ref_lock *lock)\n-{\n-\tif (close_lock_file(lock->lk))\n-\t\treturn -1;\n-\treturn 0;\n-}\n-\n-static int commit_ref(struct ref_lock *lock)\n-{\n-\tif (commit_lock_file(lock->lk))\n-\t\treturn -1;\n-\treturn 0;\n-}\n-\n /*\n * copy the reflog message msg to buf, which has been allocated sufficiently\n * large, while cleaning up the whitespaces. Especially, convert LF to space,\n-- \n2.2.1.62.g3f15098\n"},{"id":"255221","messageId":"1422043442-30676-6-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1422043442-30676-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:04:01Z","receivedAt":"2015-01-23T20:04:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes write_ref_sha1 only write the the lock file, committing\nneeds to be done outside of that function. This will help us change\nthe ref_transaction_commit in a later patch.\n\nAlso instead of calling unlock_ref before each return in write_ref_sha1\nwe can call this after the call. This is a first step to split up\nwrite_ref_sha1 into the write and commit phase which is done in a\nlater patch.\n\nThere is a call in each code path after write_ref_sha1 now. Even in\nthe last hunk in the error case, the 'goto cleanup;' will make sure\nthere is a call to unlock_ref.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n new in v3\n\n refs.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f1eefc7..1bfc84b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)\n \treturn 0;\n }\n \n-static int commit_ref(struct ref_lock *lock)\n+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n {\n+\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n+\t\treturn 0;\n+\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n \treturn 0;\n@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t}\n \tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n-\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\tif (write_ref_sha1(lock, orig_sha1, logmsg)\n+\t || commit_ref(lock, orig_sha1)) {\n+\t\tunlock_ref(lock);\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n \t\tgoto rollback;\n \t}\n+\tunlock_ref(lock);\n \n \treturn 0;\n \n@@ -2896,8 +2902,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlock->force_write = 1;\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n-\tif (write_ref_sha1(lock, orig_sha1, NULL))\n+\tif (write_ref_sha1(lock, orig_sha1, NULL)\n+\t || commit_ref(lock, orig_sha1))\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n+\n+\tunlock_ref(lock);\n \tlog_all_ref_updates = flag;\n \n rollbacklog:\n@@ -3067,22 +3076,19 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n-\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n-\t\tunlock_ref(lock);\n+\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n \t\treturn 0;\n-\t}\n+\n \to = parse_object(sha1);\n \tif (!o) {\n \t\terror(\"Trying to write ref %s with nonexistent object %s\",\n \t\t\tlock->ref_name, sha1_to_hex(sha1));\n-\t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\terror(\"Trying to write non-commit object %s to branch %s\",\n \t\t\tsha1_to_hex(sha1), lock->ref_name);\n-\t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n@@ -3091,7 +3097,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n-\t\tunlock_ref(lock);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n@@ -3099,7 +3104,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n@@ -3124,12 +3128,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\t !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n \t}\n-\tif (commit_ref(lock)) {\n-\t\terror(\"Couldn't set %s\", lock->ref_name);\n-\t\tunlock_ref(lock);\n-\t\treturn -1;\n-\t}\n-\tunlock_ref(lock);\n \treturn 0;\n }\n \n@@ -3762,14 +3760,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t update->msg)) {\n-\t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n+\t\t\t\t\t update->msg)\n+\t\t\t || commit_ref(update->lock, update->new_sha1)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n+\t\t\tunlock_ref(update->lock);\n+\t\t\tupdate->lock = NULL;\n \t\t}\n \t}\n \n@@ -4053,7 +4052,8 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t} else if (commit_lock_file(&reflog_lock)) {\n \t\t\tstatus |= error(\"unable to commit reflog '%s' (%s)\",\n \t\t\t\t\tlog_file, strerror(errno));\n-\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n+\t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF)\n+\t\t\t && commit_ref(lock, cb.last_kept_sha1)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n \t\t}\n \t}\n-- \n2.2.1.62.g3f15098\n"},{"id":"255220","messageId":"1422043442-30676-7-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"1422043442-30676-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 6/6] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-23T20:04:02Z","receivedAt":"2015-01-23T20:04:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By closing the file descriptors after creating the lock file we are not\nlimiting the size of the transaction by the number of available file\ndescriptors.\n\nWhen closing the file descriptors early, we also need to write the values\nin early, if we don't want to reopen the files.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n version3:\n * Do not reopen the files after closing them. Make sure we have\n written all necessary information before closing the file.\n Doing it that way enabled us to drop the patch \"[PATCH 4/6]\n refs.c: Have a write_in_full_to_lock_file wrapping write_in_full\"\n\n refs.c | 15 ++++++++++++---\n t/t1400-update-ref.sh | 4 ++--\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1bfc84b..2594b23 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3752,6 +3752,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n+\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n+\t\t\t\t\t update->msg)) {\n+\t\t\t\tstrbuf_addf(err, \"Cannot write to the ref lock '%s'.\",\n+\t\t\t\t\t update->refname);\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t\t/* Do not keep all lock files open at the same time. */\n+\t\tclose_ref(update->lock);\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n@@ -3759,9 +3770,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n-\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t update->msg)\n-\t\t\t || commit_ref(update->lock, update->new_sha1)) {\n+\t\t\tif (commit_ref(update->lock, update->new_sha1)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 47d2fe9..c593a1d 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -979,7 +979,7 @@ run_with_limited_open_files () {\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+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n )\n '\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n-- \n2.2.1.62.g3f15098\n"},{"id":"255238","messageId":"xmqq61bxoxs8.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"1422043442-30676-6-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-23T23:57:43Z","receivedAt":"2015-01-23T23:57:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> -static int commit_ref(struct ref_lock *lock)\n> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n> {\n> +\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n> +\t\treturn 0;\n> \tif (commit_lock_file(lock->lk))\n> \t\treturn -1;\n> \treturn 0;\n> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n> \t}\n> \tlock->force_write = 1;\n> \thashcpy(lock->old_sha1, orig_sha1);\n> -\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n> +\tif (write_ref_sha1(lock, orig_sha1, logmsg)\n> +\t || commit_ref(lock, orig_sha1)) {\n> +\t\tunlock_ref(lock);\n\nThis is not a new problem, but the two lines in pre-context of this\npatch look strange. When the code is renaming into some ref, the\nref either would have no original SHA-1 (i.e. we are renaming to a\nnon-existing name) or have unrelated SHA-1 (i.e. we are overwriting\nan existing one). For some (unknown to me) reason, however, the\ncode pretends that lock->old_sha1 has the new SHA-1 already before\nwe start to do the write or commit.\n\nAnd because both write and commit tries to pretend to be no-op when\nthe caller tries to update a ref with the same SHA-1, but in this\ncodepath it does want the write to happen, it needs to set the\nforce_write bit set, which look like an unnecessary workaround.\n\nRegardless of what this particular caller does, I am not sure if the\nearly-return codepath in commit_ref() is correct. From the callers'\npoint of view, it sometimes unlocks the ref (i.e. when a different\nSHA-1 is written or force_write is set) and sometimes keeps the ref\nlocked (i.e. when early-return is taken). Shouldn't these two cases\nbehave identically? Or am I wrong to assume that the early return\nusing \"hashcmp(lock->old_sha1, sha1)\" is a mere optimization?\n"},{"id":"255239","messageId":"xmqqy4otnigf.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"1422043442-30676-7-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 6/6] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-24T00:14:08Z","receivedAt":"2015-01-24T00:14:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> By closing the file descriptors after creating the lock file we are not\n> limiting the size of the transaction by the number of available file\n> descriptors.\n>\n> When closing the file descriptors early, we also need to write the values\n> in early, if we don't want to reopen the files.\n\n\nI am not sure if an early return in write_ref_sha1() is entirely\ncorrect. The unlock step was split out of write and commit\nfunctions in the previous step because you eventually want to be\nable to close the file descriptor that is open to $ref.lock early,\nwhile still keeping the $ref.lock file around to avoid others\ncompeting with you, so that you can limit the number of open file\ndescriptors, no?\n\nIf so, shouldn't the write function at least close the file\ndescriptor even when it knows that the $ref.lock already has the\ncorrect object name? The call to close_ref() is never made when the\nearly-return codepath is taken as far as I can see.\n\nPuzzled...\n"},{"id":"255240","messageId":"CAGZ79kYPCFBNNkYV0C_XeCEaqfz=Mh3Zy=L-vAWsmZBFgebqNQ@mail.gmail.com","threadId":"38424","inReplyTo":"xmqq61bxoxs8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-24T00:22:57Z","receivedAt":"2015-01-24T00:22:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> -static int commit_ref(struct ref_lock *lock)\n>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n>> {\n>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n>> + return 0;\n>> if (commit_lock_file(lock->lk))\n>> return -1;\n>> return 0;\n>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>> }\n>> lock->force_write = 1;\n>> hashcpy(lock->old_sha1, orig_sha1);\n>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {\n>> + if (write_ref_sha1(lock, orig_sha1, logmsg)\n>> + || commit_ref(lock, orig_sha1)) {\n>> + unlock_ref(lock);\n>\n> This is not a new problem, but the two lines in pre-context of this\n> patch look strange.\n\nWhich (not new) problem are you talking about here? Do you have\na reference?\n\n> Regardless of what this particular caller does, I am not sure if the\n> early-return codepath in commit_ref() is correct. From the callers'\n> point of view, it sometimes unlocks the ref (i.e. when a different\n> SHA-1 is written or force_write is set) and sometimes keeps the ref\n> locked (i.e. when early-return is taken). Shouldn't these two cases\n> behave identically? Or am I wrong to assume that the early return\n> using \"hashcmp(lock->old_sha1, sha1)\" is a mere optimization?\n>\n\nI assumed it was not just an optimization as the test suite fails if I\ntouch that line. I'll look into cleaning it up in a more obvious fashion.\n"},{"id":"255241","messageId":"CAGZ79kYbCBHVhL+FvKu-yKMinxGCMwGfqG96rSp+0-7frgO2wQ@mail.gmail.com","threadId":"38424","inReplyTo":"xmqqy4otnigf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 6/6] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-24T00:24:25Z","receivedAt":"2015-01-24T00:24:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> By closing the file descriptors after creating the lock file we are not\n>> limiting the size of the transaction by the number of available file\n>> descriptors.\n>>\n>> When closing the file descriptors early, we also need to write the values\n>> in early, if we don't want to reopen the files.\n>\n>\n> I am not sure if an early return in write_ref_sha1() is entirely\n> correct. The unlock step was split out of write and commit\n> functions in the previous step because you eventually want to be\n> able to close the file descriptor that is open to $ref.lock early,\n> while still keeping the $ref.lock file around to avoid others\n> competing with you, so that you can limit the number of open file\n> descriptors, no?\n\nyeah that's the goal. Though as we're in one transaction, as soon\nas we have an early exit, the transaction will abort.\n\n>\n> If so, shouldn't the write function at least close the file\n> descriptor even when it knows that the $ref.lock already has the\n> correct object name? The call to close_ref() is never made when the\n> early-return codepath is taken as far as I can see.\n>\n\nThe goto cleanup; will take care of unlocking (and closing fds of) all refs\n"},{"id":"255242","messageId":"xmqqtwzhnhbl.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"CAGZ79kYbCBHVhL+FvKu-yKMinxGCMwGfqG96rSp+0-7frgO2wQ@mail.gmail.com","subject":"Re: [PATCHv3 6/6] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-24T00:38:38Z","receivedAt":"2015-01-24T00:38:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano wrote:\n>\n> yeah that's the goal. Though as we're in one transaction, as soon\n> as we have an early exit, the transaction will abort.\n\nAn early exit I am talking about is this:\n\nstatic int write_ref_sha1(struct ref_lock *lock,\n\tconst unsigned char *sha1, const char *logmsg)\n{\n\tstatic char term = '\\n';\n\tstruct object *o;\n\n\tif (!lock) {\n\t\terrno = EINVAL;\n\t\treturn -1;\n\t}\n\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n\t\treturn 0;\n\nIt returns 0 and then the transaction side has this call in a loop:\n\n\t\tif (!is_null_sha1(update->new_sha1)) {\n\t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n\t\t\t\t\t update->msg)) {\n\t\t\t\tstrbuf_addf(err, \"Cannot write to the ref lock '%s'.\",\n\t\t\t\t\t update->refname);\n\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n\t\t\t\tgoto cleanup;\n\t\t\t}\n\t\t}\n\n>> If so, shouldn't the write function at least close the file\n>> descriptor even when it knows that the $ref.lock already has the\n>> correct object name? The call to close_ref() is never made when the\n>> early-return codepath is taken as far as I can see.\n>\n> The goto cleanup; will take care of unlocking (and closing fds of) all refs\n\nYes, if write_ref_sha1() returns with non-zero signaling an error,\nthen the goto will trigger.\n\nBut if it short-cuts and returns zero, that goto will not be\nreached.\n"},{"id":"255243","messageId":"xmqqppa5nhab.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"CAGZ79kYPCFBNNkYV0C_XeCEaqfz=Mh3Zy=L-vAWsmZBFgebqNQ@mail.gmail.com","subject":"Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-24T00:39:24Z","receivedAt":"2015-01-24T00:39:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> -static int commit_ref(struct ref_lock *lock)\n>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n>>> {\n>>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n>>> + return 0;\n>>> if (commit_lock_file(lock->lk))\n>>> return -1;\n>>> return 0;\n>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>>> }\n>>> lock->force_write = 1;\n>>> hashcpy(lock->old_sha1, orig_sha1);\n>>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {\n>>> + if (write_ref_sha1(lock, orig_sha1, logmsg)\n>>> + || commit_ref(lock, orig_sha1)) {\n>>> + unlock_ref(lock);\n>>\n>> This is not a new problem, but the two lines in pre-context of this\n>> patch look strange.\n>\n> Which (not new) problem are you talking about here? Do you have\n> a reference?\n\nThese two lines in pre-context of the hunk:\n\n>>> lock->force_write = 1;\n>>> hashcpy(lock->old_sha1, orig_sha1);\n"},{"id":"255245","messageId":"CAGZ79kZ0EBRgSuc4Wupaa9t85xcF79SizvGbjD9OC3+Jq=XbFA@mail.gmail.com","threadId":"38424","inReplyTo":"xmqqppa5nhab.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-24T01:04:14Z","receivedAt":"2015-01-24T01:04:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>>\n>>>> -static int commit_ref(struct ref_lock *lock)\n>>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n>>>> {\n>>>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n>>>> + return 0;\n>>>> if (commit_lock_file(lock->lk))\n>>>> return -1;\n>>>> return 0;\n>>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>>>> }\n>>>> lock->force_write = 1;\n>>>> hashcpy(lock->old_sha1, orig_sha1);\n>>>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {\n>>>> + if (write_ref_sha1(lock, orig_sha1, logmsg)\n>>>> + || commit_ref(lock, orig_sha1)) {\n>>>> + unlock_ref(lock);\n>>>\n>>> This is not a new problem, but the two lines in pre-context of this\n>>> patch look strange.\n>>\n>> Which (not new) problem are you talking about here? Do you have\n>> a reference?\n>\n> These two lines in pre-context of the hunk:\n>\n>>>> lock->force_write = 1;\n>>>> hashcpy(lock->old_sha1, orig_sha1);\n>\n\nSo these 2 lines (specially the force_write=1 line) is just there to trigger\nthe valid early exit path as you sent in the other mail :\n\n> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n> return 0;\n\nwhen we have the same sha1?\nand you're saying that's a problem because hard to understand?\n\nI am confused as well by now.\n"},{"id":"255246","messageId":"xmqqiofxneys.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"CAGZ79kZ0EBRgSuc4Wupaa9t85xcF79SizvGbjD9OC3+Jq=XbFA@mail.gmail.com","subject":"Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-24T01:29:31Z","receivedAt":"2015-01-24T01:29:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:\n>>>> Stefan Beller writes:\n>>>>\n>>>>> -static int commit_ref(struct ref_lock *lock)\n>>>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n>>>>> {\n>>>>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n>>>>> + return 0;\n>>>>> if (commit_lock_file(lock->lk))\n>>>>> return -1;\n>>>>> return 0;\n>>>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>>>>> }\n>>>>> lock->force_write = 1;\n>>>>> hashcpy(lock->old_sha1, orig_sha1);\n>>>>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {\n>>>>> + if (write_ref_sha1(lock, orig_sha1, logmsg)\n>>>>> + || commit_ref(lock, orig_sha1)) {\n>>>>> + unlock_ref(lock);\n>>>>\n>>>> This is not a new problem, but the two lines in pre-context of this\n>>>> patch look strange.\n>>>\n>>> Which (not new) problem are you talking about here? Do you have\n>>> a reference?\n>>\n>> These two lines in pre-context of the hunk:\n>>\n>>>>> lock->force_write = 1;\n>>>>> hashcpy(lock->old_sha1, orig_sha1);\n>>\n>\n> So these 2 lines (specially the force_write=1 line) is just there to trigger\n> the valid early exit path as you sent in the other mail :\n>\n>> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n>> return 0;\n>\n> when we have the same sha1?\n> and you're saying that's a problem because hard to understand?\n\nWhat is the point of that hashcmp() in the first place? My\nunderstanding is that \n\n (1) lock->old_sha1 is to hold the original SHA-1 in the ref we took\n the lock on.\n\n (2) when not forcing, and when the two SHA-1 are the same, we\n bypass and return early because overwriting the ref with the\n same value is no-op.\n\nNow, in that codepath, when the code is renaming into some ref, the\nref either would have no original SHA-1 (i.e. we are renaming to a\nnon-existing name) or have unrelated SHA-1 (i.e. we are overwriting\nan existing one). For some (unknown to me) reason, however, the\ncode pretends that lock->old_sha1 has the new SHA-1 already before\nwe start to do the write or commit.\n\nAnd because both write and commit tries to pretend to be no-op when\nthe caller tries to update a ref with the same SHA-1, but in this\ncodepath it does want the write to happen, it needs to set the\nforce_write bit set, which look like an unnecessary workaround.\n\nLet me rephrase.\n\nA natural way to write that caller, I think, would be more like\nthis:\n\n\t... lock is given by the caller, probably with ->old_sha1\n\t... filled in; it is not likely to be orig_sha1, as it is\n ... either null (if locking to create a new ref) or some\n ... unrelated value read from the ref being overwritten by\n ... this rename_ref() operation.\n\n\twrite_ref_sha1(lock, orig_sha1, logmsg);\n\t# which may do the \"don't write the same value\" optmization\n # if we are renaming another ref that happens to have the\n # same value, in which case it is OK. Otherwise this will\n # overwrite without being forced.\n\n\t... *IF* and only if there is some reason lock->old_sha1\n ... needs to match what is in the filesystem ref right now,\n ... then do\n\thashcpy(lock->old_sha1, orig_sha1);\n\t... but probably there is no reason to do so.\n\nThe two lines hashcpy() and ->force_write = 1 that appear before the\nwrite_ref_sha1() do not conceptually make sense. The former does\nnot make sense because ->old_sha1 is supposed to be the original\nvalue that is already stored in the ref, to allow us optimize \"write\nthe same value\" case, and you are breaking that invariant by doing\nhashcpy() before write_ref_sha1(). The lock->old_sha1 value does\nnot have anything to do with the (original) value of the ref we took\nthe lock on.\n\nAnd ->force_write = 1 becomes necessary only because the effect of\nthis nonsensical hashcpy() is to make the !hashcmp() used by the\nshort-cut logic trigger. Since the code needs to override that\nlogic, you need to say \"force\" before calling write_ref_sha1(). If\nyou didn't do the hashcpy(), you wouldn't have to say \"force\", no?\n"},{"id":"255301","messageId":"CAGZ79kbQiVQJyZC8mKaSUnOpY6YJc0TYdX=msuZDXLd7DxmTmQ@mail.gmail.com","threadId":"38424","inReplyTo":"xmqqtwzhnhbl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 6/6] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-26T19:30:56Z","receivedAt":"2015-01-26T19:30:56Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 23, 2015 at 4:38 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano wrote:\n>>\n>> yeah that's the goal. Though as we're in one transaction, as soon\n>> as we have an early exit, the transaction will abort.\n>\n> An early exit I am talking about is this:\n>\n> static int write_ref_sha1(struct ref_lock *lock,\n> const unsigned char *sha1, const char *logmsg)\n> {\n> static char term = '\\n';\n> struct object *o;\n>\n> if (!lock) {\n> errno = EINVAL;\n> return -1;\n> }\n> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n> return 0;\n>\n> It returns 0 and then the transaction side has this call in a loop:\n>\n> if (!is_null_sha1(update->new_sha1)) {\n> if (write_ref_sha1(update->lock, update->new_sha1,\n> update->msg)) {\n> strbuf_addf(err, \"Cannot write to the ref lock '%s'.\",\n> update->refname);\n> ret = TRANSACTION_GENERIC_ERROR;\n> goto cleanup;\n> }\n> }\n\nAnd just after this code path there is the new\n+ /* Do not keep all lock files open at the same time. */\n+ close_ref(update->lock);\n\nSo in case we go the zero return path of write_ref_sha1 the loop looks like\n /* Acquire all locks while verifying old values */\n for (all updates) {\n write_ref_sha1(...)\n close_ref(...)\n }\n\nIn case we do go the non zero return path, it is\n if (write_ref_sha1(update->lock, update->new_sha1, ..)\n goto cleanup;\n...\ncleanup:\n for (i = 0; i < n; i++)\n if (updates[i]->lock)\n unlock_ref(updates[i]->lock);\n\nI do not see the problem in the code itself, but rather in understanding\nthe code. I will send a follow up patch which makes it easier to follow\nby removing the early exit with no problem away.\n\n>\n>>> If so, shouldn't the write function at least close the file\n>>> descriptor even when it knows that the $ref.lock already has the\n>>> correct object name? The call to close_ref() is never made when the\n>>> early-return codepath is taken as far as I can see.\n>>\n>> The goto cleanup; will take care of unlocking (and closing fds of) all refs\n>\n> Yes, if write_ref_sha1() returns with non-zero signaling an error,\n> then the goto will trigger.\n>\n> But if it short-cuts and returns zero, that goto will not be\n> reached.\n\nYes, if the goto is not reached we just drop down to\nclose_ref(update->lock); which should take care of\nthe open fd.\n\nThanks,\nStefan\n"},{"id":"255305","messageId":"1422306638-23785-1-git-send-email-sbeller@google.com","threadId":"38424","inReplyTo":"CAGZ79kbQiVQJyZC8mKaSUnOpY6YJc0TYdX=msuZDXLd7DxmTmQ@mail.gmail.com","subject":"[PATCH] refs.c: clean up write_ref_sha1 returns","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-26T21:10:38Z","receivedAt":"2015-01-26T21:10:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"write_ref_sha1 now either returns 0 for a successful write or !=0 if an\nerror occurred. This cleanup results in cleaning the code at other places\nas well where we had to set force_write to make the write_ref_sha1(...)\n|| commit_ref(...) combination work. Also the checks for the optimisation\nof old and new sha1 values being the same has been moved to a helper\nfunction so that check is not part of write_ref_sha1 or commit_ref any\nmore.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n v1:\n \n applies on top of origin/sb/atomic-push-fix (79370dd9656)\n \n This undoes some of the changes of previous patches\n such as removing the check if old and new sha1 are equal\n and not being forced to rewrite the value.\n \n Junio, I think this patch adresses the concerns you raised.\n I can redo the atomic-push-fix series with this cleanup merged\n into the appropriate patches or you could just queue it on top \n of said series.\n\n refs.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2594b23..c8fd4a4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2817,9 +2817,6 @@ static int close_ref(struct ref_lock *lock)\n \n static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n {\n-\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n-\t\treturn 0;\n-\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n \treturn 0;\n@@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n \t}\n-\tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n \tif (write_ref_sha1(lock, orig_sha1, logmsg)\n \t || commit_ref(lock, orig_sha1)) {\n@@ -2899,7 +2895,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollbacklog;\n \t}\n \n-\tlock->force_write = 1;\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n \tif (write_ref_sha1(lock, orig_sha1, NULL)\n@@ -3062,12 +3057,22 @@ int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n+static int should_write_ref_sha1(struct ref_lock *lock,\n+\t\t\t\t const unsigned char *new_sha1)\n+{\n+\t/* If the old and new sha1 are equal only write if forced to. */\n+\tif (!lock->force_write && !hashcmp(lock->old_sha1, new_sha1))\n+\t\treturn 0;\n+\t/* null sha indicates deletion, so don't write it */\n+\treturn !is_null_sha1(new_sha1);\n+}\n+\n /*\n * Write sha1 into the ref specified by the lock. Make sure that errno\n * is sane on error.\n */\n-static int write_ref_sha1(struct ref_lock *lock,\n-\tconst unsigned char *sha1, const char *logmsg)\n+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n+\t\t\t const char *logmsg)\n {\n \tstatic char term = '\\n';\n \tstruct object *o;\n@@ -3076,8 +3081,6 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n-\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n-\t\treturn 0;\n \n \to = parse_object(sha1);\n \tif (!o) {\n@@ -3752,7 +3755,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n-\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\tif (should_write_ref_sha1(update->lock, update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t update->msg)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot write to the ref lock '%s'.\",\n@@ -3769,7 +3772,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\tif (should_write_ref_sha1(update->lock, update->new_sha1)) {\n \t\t\tif (commit_ref(update->lock, update->new_sha1)) {\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t update->refname);\n@@ -3785,7 +3788,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif (update->lock) {\n+\t\tif (is_null_sha1(update->new_sha1)) {\n \t\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n-- \n2.2.1.62.g3f15098\n"},{"id":"255315","messageId":"xmqqd260nc98.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"CAGZ79kbQiVQJyZC8mKaSUnOpY6YJc0TYdX=msuZDXLd7DxmTmQ@mail.gmail.com","subject":"Re: [PATCHv3 6/6] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T03:17:07Z","receivedAt":"2015-01-27T03:17:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I do not see the problem in the code itself, but rather in understanding\n> the code. I will send a follow up patch which makes it easier to follow\n> by removing the early exit with no problem away.\n\n\nTaken as a whole the code may function correctly but the division of\nroles of individual functions seems screwed up. write_ref_sha1()\nsometimes unlocks, and sometimes leaves the unlocking to the caller,\nand the caller cannot even tell if it is expected to do the unlocking\nfor it from the return value because both cases return 0 (success).\n\nI am not sure if it is sensible to call that \"correct but hard to\nunderstand\". I'd rather see us admit that its behaviour is screwey\nand needs fixing for better code health longer term.\n"},{"id":"255316","messageId":"xmqq8ugonc0h.fsf@gitster.dls.corp.google.com","threadId":"38424","inReplyTo":"1422306638-23785-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: clean up write_ref_sha1 returns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T03:22:22Z","receivedAt":"2015-01-27T03:22:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I can redo the atomic-push-fix series with this cleanup merged\n> into the appropriate patches or you could just queue it on top \n> of said series.\n\nYeah, I do not think we are expecting to fast track these two series\nthrough 'next' to 'master' before 2.3 final, so I think it would be\nbetter to use this patch _only_ to see if the final shape of the\ncode this patch represents makes sense, so that we can expedite the\nfinal submission in the next development cycle, at which time we\nwill have a chance to refresh 'next', hence a chance to clean-up\natomic-push series in place.\n\nThanks.\n\n> refs.c | 27 +++++++++++++++------------\n> 1 file changed, 15 insertions(+), 12 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 2594b23..c8fd4a4 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2817,9 +2817,6 @@ static int close_ref(struct ref_lock *lock)\n> \n> static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)\n> {\n> -\tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1))\n> -\t\treturn 0;\n> -\n> \tif (commit_lock_file(lock->lk))\n> \t\treturn -1;\n> \treturn 0;\n> @@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n> \t\terror(\"unable to lock %s for update\", newrefname);\n> \t\tgoto rollback;\n> \t}\n> -\tlock->force_write = 1;\n> \thashcpy(lock->old_sha1, orig_sha1);\n\nIs this hashcpy() still necessary?\n\n> \tif (write_ref_sha1(lock, orig_sha1, logmsg)\n> \t || commit_ref(lock, orig_sha1)) {\n"},{"id":"255394","messageId":"CAGZ79kY1Hy7xAZ4JSRev2EUoORFNbdhSeNysBwo0jhmskKgvvw@mail.gmail.com","threadId":"38424","inReplyTo":"xmqq8ugonc0h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] refs.c: clean up write_ref_sha1 returns","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-28T21:35:23Z","receivedAt":"2015-01-28T21:35:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jan 26, 2015 at 7:22 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> I can redo the atomic-push-fix series with this cleanup merged\n>> into the appropriate patches or you could just queue it on top\n>> of said series.\n>\n> Yeah, I do not think we are expecting to fast track these two series\n> through 'next' to 'master' before 2.3 final, so I think it would be\n> better to use this patch _only_ to see if the final shape of the\n> code this patch represents makes sense, so that we can expedite the\n> final submission in the next development cycle, at which time we\n> will have a chance to refresh 'next', hence a chance to clean-up\n> atomic-push series in place.\n\nI tried to rip this patch and its 3 previous patches apart to see if it could be\ndone another way. The outcome was to actually sqquash this patch\ncompletely into b1c6da0a13 (refs.c: remove unlock_ref and commit_ref\nfrom write_ref_sha1).\n\nLooking at the end result the write_ref_sha1 function it has a good\ndesign contract. Either you get 0 returned and all is good, or -1 is returned\nand errno is set to a meaningful value which seems to adress your concerns\non that patch:\n\n> I am not sure if it is sensible to call that \"correct but hard to\n> understand\". I'd rather see us admit that its behaviour is screwey\n> and needs fixing for better code health longer term.\n\n\n>> @@ -2880,7 +2877,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>> error(\"unable to lock %s for update\", newrefname);\n>> goto rollback;\n>> }\n>> - lock->force_write = 1;\n>> hashcpy(lock->old_sha1, orig_sha1);\n>\n> Is this hashcpy() still necessary?\n\nThanks for catching that! It is not necessary any more and will be\nremoved in a reroll.\nI think I'll wait for rerolling the atomic-push-fix series until 2.3\nis out then?\n"}]}