{"thread":{"id":"38423","subject":"[PATCHv1 0/6] Fix bug in large transactions","startedAt":"2015-01-21T23:23:39Z","lastAt":"2015-01-22T18:29:05Z","messageCount":18,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"255038","messageId":"1421882625-916-1-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":null,"subject":"[PATCHv1 0/6] Fix bug in large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:39Z","receivedAt":"2015-01-21T23:23:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"(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  refs.c: remove lock_fd from struct ref_lock\n  refs.c: replace write_str_in_full by write_in_full\n  refs.c: Have a write_in_full_to_lock_file wrapping write_in_full\n  refs.c: write to a lock file only once\n  refs.c: enable large transactions\n\n refs.c                | 41 +++++++++++++++++++++++++++++------------\n t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+), 12 deletions(-)\n\n-- \n2.2.1.62.g3f15098\n"},{"id":"255039","messageId":"1421882625-916-2-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/6] update-ref: Test handling large transactions properly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:40Z","receivedAt":"2015-01-21T23:23:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\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 6805b9e..ea98b9b 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1065,4 +1065,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 'run_with_limited_open_files true'\n+\n+test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '\n+(\n+\tfor i in $(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+\tgit rev-parse --verify -q refs/heads/33\n+)\n+'\n+\n+test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '\n+(\n+\tfor i in $(seq 33)\n+\tdo\n+\t\techo \"delete refs/heads/$i HEAD\"\n+\tdone >large_input &&\n+\trun_with_limited_open_files git update-ref --stdin <large_input &&\n+\ttest_must_fail git rev-parse --verify -q refs/heads/33\n+)\n+'\n+\n test_done\n-- \n2.2.1.62.g3f15098\n"},{"id":"255043","messageId":"1421882625-916-3-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/6] refs.c: remove lock_fd from struct ref_lock","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:41Z","receivedAt":"2015-01-21T23:23:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove\nit. You may argue this introduces more coupling as we need to know more\nabout the internals of the lock file mechanism, but this will be solved in\na later patch.\n\nNo functional changes intended.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ab2f2a9..e905f51 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@@ -2262,7 +2261,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@@ -2338,8 +2336,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\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n@@ -2916,7 +2913,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@@ -2924,7 +2920,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@@ -3102,8 +3097,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@@ -4083,9 +4078,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":"255040","messageId":"1421882625-916-4-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/6] refs.c: replace write_str_in_full by write_in_full","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:42Z","receivedAt":"2015-01-21T23:23:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There is another occurrence where we could have used write_str_in_full\n(line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state\nis inconsistent. This replaces the only occurrence of write_str_in_full\nby write_in_full, so we only need to wrap write_in_full in the next patch.\n\nNo functional changes intended.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex e905f51..8281bed 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4080,7 +4080,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\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_in_full(lock->lk->fd, \"\\n\", 1) != 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":"255042","messageId":"1421882625-916-5-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 4/6] refs.c: Have a write_in_full_to_lock_file wrapping write_in_full","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:43Z","receivedAt":"2015-01-21T23:23:43Z","isPatch":true,"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 <sbeller@google.com>\n---\n refs.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8281bed..311599b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3064,6 +3064,13 @@ int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n+static ssize_t write_in_full_to_lockfile(struct lock_file *lock,\n+\t\t\t\t\t const void *buf,\n+\t\t\t\t\t size_t count)\n+{\n+\treturn write_in_full(lock->fd, buf, count);\n+}\n+\n /*\n  * Write sha1 into the ref specified by the lock. Make sure that errno\n  * is sane on error.\n@@ -3097,8 +3104,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->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n-\t    write_in_full(lock->lk->fd, &term, 1) != 1 ||\n+\tif (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||\n+\t    write_in_full_to_lockfile(lock->lk, &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@@ -4078,9 +4085,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->lk->fd,\n+\t\t\t(write_in_full_to_lockfile(lock->lk,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_in_full(lock->lk->fd, \"\\n\", 1) != 1 ||\n+\t\t\t write_in_full_to_lockfile(lock->lk, \"\\n\", 1) != 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":"255041","messageId":"1421882625-916-6-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 5/6] refs.c: write to a lock file only once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:44Z","receivedAt":"2015-01-21T23:23:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of having to call write_in_full_to_lock_file twice get a formatted\nstring such that we only need to invoke writing to the lock file once.\n\nThis is helpful for the next patch when we only open the file descriptors\nas needed. The lock file API has a reopen_lock_file which currently\ndoesn't open for appending.\n\nNo functional changes intended.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 311599b..0161667 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3078,8 +3078,8 @@ static ssize_t write_in_full_to_lockfile(struct lock_file *lock,\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+\tconst char *sha1_lf;\n \n \tif (!lock) {\n \t\terrno = EINVAL;\n@@ -3104,8 +3104,9 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n-\tif (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||\n-\t    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||\n+\n+\tsha1_lf = xstrfmt(\"%s\\n\",  sha1_to_hex(sha1));\n+\tif (write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||\n \t    close_ref(lock) < 0) {\n \t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n@@ -3113,6 +3114,7 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n+\tfree((void*)sha1_lf);\n \tclear_loose_ref_cache(&ref_cache);\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@@ -4081,13 +4083,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t(*cleanup_fn)(cb.policy_cb);\n \n \tif (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {\n+\t\tconst char *sha1_lf = xstrfmt(\"%s\\n\",\n+\t\t\t\t\t      sha1_to_hex(cb.last_kept_sha1));\n \t\tif (close_lock_file(&reflog_lock)) {\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_to_lockfile(lock->lk,\n-\t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_in_full_to_lockfile(lock->lk, \"\\n\", 1) != 1 ||\n+\t\t\t(write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||\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@@ -4098,6 +4100,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t\t} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {\n \t\t\tstatus |= error(\"couldn't set %s\", lock->ref_name);\n \t\t}\n+\t\tfree((void*)sha1_lf);\n \t}\n \tfree(log_file);\n \tunlock_ref(lock);\n-- \n2.2.1.62.g3f15098\n"},{"id":"255044","messageId":"1421882625-916-7-git-send-email-sbeller@google.com","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"[PATCH 6/6] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:23:45Z","receivedAt":"2015-01-21T23:23:45Z","isPatch":true,"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 <sbeller@google.com>\n---\n refs.c                | 14 +++++++++++++-\n t/t1400-update-ref.sh |  4 ++--\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0161667..014ef59 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3068,7 +3068,17 @@ static ssize_t write_in_full_to_lockfile(struct lock_file *lock,\n \t\t\t\t\t const void *buf,\n \t\t\t\t\t size_t count)\n {\n-\treturn write_in_full(lock->fd, buf, count);\n+\tif (lock->fd == -1) {\n+\t\tint ret;\n+\t\tif (reopen_lock_file(lock) < 0)\n+\t\t\treturn -1;\n+\t\tret = write_in_full(lock->fd, buf, count);\n+\t\tif (close_lock_file(lock) < 0)\n+\t\t\treturn -1;\n+\t\treturn ret;\n+\t} else {\n+\t\treturn write_in_full(lock->fd, buf, count);\n+\t}\n }\n \n /*\n@@ -3797,6 +3807,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 ea98b9b..751b6dc 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1071,7 +1071,7 @@ run_with_limited_open_files () {\n \n test_lazy_prereq ULIMIT 'run_with_limited_open_files true'\n \n-test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '\n+test_expect_success ULIMIT 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(seq 33)\n \tdo\n@@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT 'large transaction creating branches does not burst o\n )\n '\n \n-test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '\n+test_expect_success ULIMIT 'large transaction deleting branches does not burst open file limit' '\n (\n \tfor i in $(seq 33)\n \tdo\n-- \n2.2.1.62.g3f15098\n"},{"id":"255045","messageId":"20150121233457.GA11115@peff.net","threadId":"38423","inReplyTo":"1421882625-916-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/6] update-ref: Test handling large transactions properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:34:57Z","receivedAt":"2015-01-21T23:34:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:23:40PM -0800, Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\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 6805b9e..ea98b9b 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -1065,4 +1065,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 'run_with_limited_open_files true'\n\nWe already have a ULIMIT prereq in t7004 that does something similar but\ndifferent.  The two do not conflict as long as they are in separate\nscripts, but they would if one gets moved into test-lib.sh. Should we\nmaybe give these more descriptive names? It is not just about \"ulimit\",\nbut the individual limit option. I can imagine a platform where \"ulimit\n-s\" works, but \"ulimit -n\" does not (or the other way around).\n\nI almost also suggested that the two \"ulimit -s\" instances share the\nsame function and lazy prereq, but I think that's probably not a good\nidea. One cares about limiting the stack, and the other care about\nlimiting the cmdline size. The latter _happens_ to be done using \"ulimit\n-s\". That works on Linux, but no clue about elsewhere. I could easily\nimagine a platform where there is some other way, and we add a run-time\nswitch.\n\n> +test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '\n> +(\n> +\tfor i in $(seq 33)\n\nUse test_seq here, for portability.\n\n> +test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '\n> +(\n> +\tfor i in $(seq 33)\n\nDitto here.\n\nThe rest of the tests looked good to me.\n\n-Peff\n"},{"id":"255046","messageId":"20150121233843.GB11115@peff.net","threadId":"38423","inReplyTo":"1421882625-916-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:38:43Z","receivedAt":"2015-01-21T23:38:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:\n\n> There is another occurrence where we could have used write_str_in_full\n> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state\n> is inconsistent. This replaces the only occurrence of write_str_in_full\n> by write_in_full, so we only need to wrap write_in_full in the next patch.\n\nI had to read the first sentence a few times to figure out what you\nmeant. But I am not sure it is even relevant. We do not care about the\ninconsistency. It is just \"we are about to change how callers of\nwrite_in_full in this file behave, the wrapper gets in the way, and it\ndoes not add enough value by itself to merit making our future changes\nin two places\".\n\n-Peff\n"},{"id":"255047","messageId":"20150121234026.GC11115@peff.net","threadId":"38423","inReplyTo":"1421882625-916-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 4/6] refs.c: Have a write_in_full_to_lock_file wrapping write_in_full","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:40:26Z","receivedAt":"2015-01-21T23:40:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:23:43PM -0800, Stefan Beller wrote:\n\n> Now we only have one place where we need to touch the internals of\n> the lock_file struct.\n\nI wonder if the point of the previous patch would be more obvious if it\nwere combined with this one. I am OK leaving them separate, though.\n\n-Peff\n"},{"id":"255048","messageId":"20150121234428.GD11115@peff.net","threadId":"38423","inReplyTo":"1421882625-916-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 5/6] refs.c: write to a lock file only once","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:44:28Z","receivedAt":"2015-01-21T23:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:23:44PM -0800, Stefan Beller wrote:\n\n> +\tconst char *sha1_lf;\n>  \n>  \tif (!lock) {\n>  \t\terrno = EINVAL;\n> @@ -3104,8 +3104,9 @@ static int write_ref_sha1(struct ref_lock *lock,\n>  \t\terrno = EINVAL;\n>  \t\treturn -1;\n>  \t}\n> -\tif (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||\n> -\t    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||\n> +\n> +\tsha1_lf = xstrfmt(\"%s\\n\",  sha1_to_hex(sha1));\n> +\tif (write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||\n>  \t    close_ref(lock) < 0) {\n>  \t\tint save_errno = errno;\n>  \t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n> @@ -3113,6 +3114,7 @@ static int write_ref_sha1(struct ref_lock *lock,\n>  \t\terrno = save_errno;\n>  \t\treturn -1;\n>  \t}\n> +\tfree((void*)sha1_lf);\n\nIt looks like you leak sha1_lf in the error code path here.\n\nIt's a shame that we must allocate at all, when we are really just\npassing through to a buffer. Lockfiles have a \"FILE *\" pointer these\ndays. Could we just allow:\n\n  lockfile_printf(lock->lk, \"%s\\n\", sha1_to_hex(sha1));\n\nor similar?\n\n-Peff\n"},{"id":"255049","messageId":"CAGZ79kaTUraVYc1Th0-8ajw4kMuU-Sir2e9dW1uy90CuQRzgeg@mail.gmail.com","threadId":"38423","inReplyTo":"20150121233843.GB11115@peff.net","subject":"Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-21T23:44:36Z","receivedAt":"2015-01-21T23:44:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 21, 2015 at 3:38 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:\n>\n>> There is another occurrence where we could have used write_str_in_full\n>> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state\n>> is inconsistent. This replaces the only occurrence of write_str_in_full\n>> by write_in_full, so we only need to wrap write_in_full in the next patch.\n>\n> I had to read the first sentence a few times to figure out what you\n> meant. But I am not sure it is even relevant. We do not care about the\n> inconsistency.\n\nYou're not the first who needs to reread my stuff :/\nI have the impression my English worsened since coming into the USA.\n\nWe do not care about the inconsistency, but we may care about the change itself:\n\"write_str_in_full is way better than write_in_full, so why the step backwards?\"\nAnd  I am trying to explain that this is not a huge step backwards but\nrather improves\nconsistency.\n\n> It is just \"we are about to change how callers of\n> write_in_full in this file behave, the wrapper gets in the way, and it\n> does not add enough value by itself to merit making our future changes\n> in two places\".\n\nThat's actually true. Though that sounds as if we'd be lazy (\"we only\nwant to make\none change, so let's bend over here\")\n\nI'll rethink the commit message.\nThanks,\nStefan\n"},{"id":"255050","messageId":"20150121234659.GE11115@peff.net","threadId":"38423","inReplyTo":"1421882625-916-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv1 0/6] Fix bug in large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:47:00Z","receivedAt":"2015-01-21T23:47:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:\n\n> (reported as: git update-ref --stdin : too many open files, 2014-12-20)\n> \n> First a test case is introduced to demonstrate the failure,\n> the patches 2-6 are little refactoring and the last patch \n> fixes the bug and also marks the bugs as resolved in the\n> test suite.\n> \n> Unfortunately this applies on top of origin/next.\n\nSaying \"applies on next\" is not very useful to Junio. He is not going to\nbranch a topic straight from \"next\", as merging it to master would pull\nin all of the topics cooking in \"next\" (not to mention a bunch of merge\ncommits which are generally never part of \"master\").\n\nInstead, figure out which topic in next you actually _need_ to build on,\nand then it can be branched from there. And if there is no such topic,\nthen you should not be building on next, of course. :) But I think you\nknow that part already.\n\n-Peff\n"},{"id":"255051","messageId":"20150121235213.GF11115@peff.net","threadId":"38423","inReplyTo":"CAGZ79kaTUraVYc1Th0-8ajw4kMuU-Sir2e9dW1uy90CuQRzgeg@mail.gmail.com","subject":"Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T23:52:13Z","receivedAt":"2015-01-21T23:52:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 03:44:36PM -0800, Stefan Beller wrote:\n\n> On Wed, Jan 21, 2015 at 3:38 PM, Jeff King <peff@peff.net> wrote:\n> > On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:\n> >\n> >> There is another occurrence where we could have used write_str_in_full\n> >> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state\n> >> is inconsistent. This replaces the only occurrence of write_str_in_full\n> >> by write_in_full, so we only need to wrap write_in_full in the next patch.\n> >\n> > I had to read the first sentence a few times to figure out what you\n> > meant. But I am not sure it is even relevant. We do not care about the\n> > inconsistency.\n> \n> You're not the first who needs to reread my stuff :/\n> I have the impression my English worsened since coming into the USA.\n\nActually, it was my fault in this case. I read it as \"_this_ is another\noccurrence\", and then I scratched my head wondering what the first\noccurrence was (was there a previous change that you should have been\nreferencing?). I finally got it on the third try. :)\n\n> We do not care about the inconsistency, but we may care about the\n> change itself: \"write_str_in_full is way better than write_in_full, so\n> why the step backwards?\" And  I am trying to explain that this is not\n> a huge step backwards but rather improves consistency.\n\nBut you could improve consistency by going the other way, too. :) I\nthink the point is that you should lead in with the _real_ reason for\nthe change, not justifications. You can put in the justifications, too,\nfor the people who say \"wait, but couldn't you do this other thing...\".\n\n> > It is just \"we are about to change how callers of\n> > write_in_full in this file behave, the wrapper gets in the way, and it\n> > does not add enough value by itself to merit making our future changes\n> > in two places\".\n> \n> That's actually true. Though that sounds as if we'd be lazy (\"we only\n> want to make\n> one change, so let's bend over here\")\n\nIt's not laziness. It's avoiding duplicating logic, which would end up\ncosting more lines and providing worse maintainability than just\ndropping the wrapper, which is after all only saving us a few characters\n(and not anything conceptually hard).\n\n> I'll rethink the commit message.\n\nEverything I said above is rather subjective, of course. I do appreciate\nyou breaking your commits apart and explaining each one in the first\nplace. IOW, while I have thoughts on improving them (obviously), the\ncurrent iteration is not so bad that I would be upset to see it go into\ngit. Don't waste too much time on it.\n\n-Peff\n"},{"id":"255069","messageId":"xmqqsif3tfcf.fsf@gitster.dls.corp.google.com","threadId":"38423","inReplyTo":"20150121234659.GE11115@peff.net","subject":"Re: [PATCHv1 0/6] Fix bug in large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T08:00:16Z","receivedAt":"2015-01-22T08:00:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:\n>\n>> (reported as: git update-ref --stdin : too many open files, 2014-12-20)\n>> \n>> First a test case is introduced to demonstrate the failure,\n>> the patches 2-6 are little refactoring and the last patch \n>> fixes the bug and also marks the bugs as resolved in the\n>> test suite.\n>> \n>> Unfortunately this applies on top of origin/next.\n>\n> Saying \"applies on next\" is not very useful to Junio. He is not going to\n> branch a topic straight from \"next\", as merging it to master would pull\n> in all of the topics cooking in \"next\" (not to mention a bunch of merge\n> commits which are generally never part of \"master\").\n>\n> Instead, figure out which topic in next you actually _need_ to build on,\n> and then it can be branched from there. And if there is no such topic,\n> then you should not be building on next, of course. :) But I think you\n> know that part already.\n\nAll very true.\n\nI consider anything new that appears late in the cycle, especially\nduring deep in the pre-release freeze, less for me to apply but more\nfor others to eyeball the preview of a series the submitter plans to\nwork on once the next cycle starts, so basing on 'next' does not\nhurt too much.  For interested others,\n\n\tgit checkout origin/next^0\n\nwould be shorter to type than\n\n\tgit checkout \"origin/next^{/^Merge branch 'sb/atomic-push'}^2\"\n\nso... ;-)\n\nBut what is more troublesome is that neither this or updated v2\napplies to 'next'.\n\nLet me try to wiggle it in first.\n\nThanks.\n"},{"id":"255112","messageId":"CAGZ79kZ5fdDiT=zXhQkt17kxLBbkJnh1F06nyZJN7ta9WZ2dmQ@mail.gmail.com","threadId":"38423","inReplyTo":"xmqqsif3tfcf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv1 0/6] Fix bug in large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T17:52:10Z","receivedAt":"2015-01-22T17:52:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 22, 2015 at 12:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:\n>>\n>>> (reported as: git update-ref --stdin : too many open files, 2014-12-20)\n>>>\n>>> First a test case is introduced to demonstrate the failure,\n>>> the patches 2-6 are little refactoring and the last patch\n>>> fixes the bug and also marks the bugs as resolved in the\n>>> test suite.\n>>>\n>>> Unfortunately this applies on top of origin/next.\n>>\n>> Saying \"applies on next\" is not very useful to Junio. He is not going to\n>> branch a topic straight from \"next\", as merging it to master would pull\n>> in all of the topics cooking in \"next\" (not to mention a bunch of merge\n>> commits which are generally never part of \"master\").\n>>\n>> Instead, figure out which topic in next you actually _need_ to build on,\n>> and then it can be branched from there. And if there is no such topic,\n>> then you should not be building on next, of course. :) But I think you\n>> know that part already.\n>\n> All very true.\n>\n> I consider anything new that appears late in the cycle, especially\n> during deep in the pre-release freeze, less for me to apply but more\n> for others to eyeball the preview of a series the submitter plans to\n> work on once the next cycle starts, so basing on 'next' does not\n> hurt too much.  For interested others,\n>\n>         git checkout origin/next^0\n>\n> would be shorter to type than\n>\n>         git checkout \"origin/next^{/^Merge branch 'sb/atomic-push'}^2\"\n>\n> so... ;-)\n>\n> But what is more troublesome is that neither this or updated v2\n> applies to 'next'.\n\nv2 applies to sb/atomic-push instead of next and will result in a one\nline merge conflict with next.\n\nI know we're late in the cycle, hence I was just sending out for reviews.\nThough as it's not a feature, but a bug fix it may be worth picking it\nup now if it's easy to apply.\n\nThanks,\nStefan\n\n>\n> Let me try to wiggle it in first.\n>\n> Thanks.\n"},{"id":"255114","messageId":"xmqqk30eu271.fsf@gitster.dls.corp.google.com","threadId":"38423","inReplyTo":"CAGZ79kZ5fdDiT=zXhQkt17kxLBbkJnh1F06nyZJN7ta9WZ2dmQ@mail.gmail.com","subject":"Re: [PATCHv1 0/6] Fix bug in large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T17:58:58Z","receivedAt":"2015-01-22T17:58:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> v2 applies to sb/atomic-push instead of next and will result in a one\n> line merge conflict with next.\n\nI acctually tried to apply on 'next' and also on 'sb/atomic-push'\nand both failed.  I had to wiggle the patches to make them apply on\nthe latter, and that is what is queued on 'pu' now, but I would not\nbe surprised if I made silly mistakes while doing so, so please\ndouble check the result and catch them if I did.\n\nThanks.\n"},{"id":"255115","messageId":"CAGZ79kYH9qO8Dghk1WU8CF04L+6oH2FEX09z1inALv8LDbwP_g@mail.gmail.com","threadId":"38423","inReplyTo":"xmqqk30eu271.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv1 0/6] Fix bug in large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-22T18:29:05Z","receivedAt":"2015-01-22T18:29:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 22, 2015 at 9:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> v2 applies to sb/atomic-push instead of next and will result in a one\n>> line merge conflict with next.\n>\n> I acctually tried to apply on 'next' and also on 'sb/atomic-push'\n> and both failed.\n\nThat's strange indeed. I just checked out origin/sb/atomic-push (which\npoints at 04b39f195baf, 2015-01-12 16:24:02, Document receive.advertiseatomic)\nand applied the patches v2 from my outgoing mailbox without problems.\n\nI do not remember to have changed my development process a lot there.\n(I was not using notes, but that should not matter at all)\ngit format-patch --subject-prefix=PATCHv2 --cover-letter\norigin/sb/atomic-push..HEAD\n$EDITOR 0000-cover-letter.patch\ngit send-email 00* --to=...\n\n>I had to wiggle the patches to make them apply on\n> the latter, and that is what is queued on 'pu' now, but I would not\n> be surprised if I made silly mistakes while doing so, so please\n> double check the result and catch them if I did.\n\nI just fetched origin/sb/atomic-push-fix and the only difference I can spot\ncompared to the local branch is in d4ad3f1cdcb (refs.c: remove lock_fd\nfrom struct ref_lock) in refs.c in the hunk:\n\n@@ -2335,8 +2333,8 @@ static struct ref_lock\n*lock_ref_sha1_basic(const char *refname,\n  goto error_return;\n  }\n\n- lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n- if (lock->lock_fd < 0) {\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  * Maybe somebody just deleted one of the\n\n(whitespaces broken here)\nThat hunk has the additional line \"+ last_errno = errno;\", which I do\nnot have locally, but that's the exact problem I spotted when trying to\nmerge my local branch to origin/next which then results in a merge\nconflict with the patch from origin/jk/lock-ref-sha1-basic-return-errors\nas that introduces the line \"+ last_errno = errno;\".\n\nSorry for the confusion,\nStefan\n"}]}