[PATCHv1 0/6] Fix bug in large transactions

[PATCHv1 0/6] Fix bug in large transactions

[Stefan Beller]

(reported as: git update-ref --stdin : too many open files, 2014-12-20)

First a test case is introduced to demonstrate the failure,
the patches 2-6 are little refactoring and the last patch 
fixes the bug and also marks the bugs as resolved in the
test suite.

Unfortunately this applies on top of origin/next.

Any feedback would be welcome!

Thanks,
Stefan

Stefan Beller (6):
  update-ref: Test handling large transactions properly
  refs.c: remove lock_fd from struct ref_lock
  refs.c: replace write_str_in_full by write_in_full
  refs.c: Have a write_in_full_to_lock_file wrapping write_in_full
  refs.c: write to a lock file only once
  refs.c: enable large transactions

 refs.c                | 41 +++++++++++++++++++++++++++++------------
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 12 deletions(-)

-- 
2.2.1.62.g3f15098

[PATCH 1/6] update-ref: Test handling large transactions properly

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6805b9e..ea98b9b 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1065,4 +1065,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
+run_with_limited_open_files () {
+	(ulimit -n 32 && "$@")
+}
+
+test_lazy_prereq ULIMIT 'run_with_limited_open_files true'
+
+test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '
+(
+	for i in $(seq 33)
+	do
+		echo "create refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	git rev-parse --verify -q refs/heads/33
+)
+'
+
+test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '
+(
+	for i in $(seq 33)
+	do
+		echo "delete refs/heads/$i HEAD"
+	done >large_input &&
+	run_with_limited_open_files git update-ref --stdin <large_input &&
+	test_must_fail git rev-parse --verify -q refs/heads/33
+)
+'
+
 test_done
-- 
2.2.1.62.g3f15098

Re: [PATCH 1/6] update-ref: Test handling large transactions properly

[Jeff King]

On Wed, Jan 21, 2015 at 03:23:40PM -0800, Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 6805b9e..ea98b9b 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1065,4 +1065,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
>  	test_must_fail git rev-parse --verify -q $c
>  '
>  
> +run_with_limited_open_files () {
> +	(ulimit -n 32 && "$@")
> +}
> +
> +test_lazy_prereq ULIMIT 'run_with_limited_open_files true'

We already have a ULIMIT prereq in t7004 that does something similar but
different.  The two do not conflict as long as they are in separate
scripts, but they would if one gets moved into test-lib.sh. Should we
maybe give these more descriptive names? It is not just about "ulimit",
but the individual limit option. I can imagine a platform where "ulimit
-s" works, but "ulimit -n" does not (or the other way around).

I almost also suggested that the two "ulimit -s" instances share the
same function and lazy prereq, but I think that's probably not a good
idea. One cares about limiting the stack, and the other care about
limiting the cmdline size. The latter _happens_ to be done using "ulimit
-s". That works on Linux, but no clue about elsewhere. I could easily
imagine a platform where there is some other way, and we add a run-time
switch.

> +test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '
> +(
> +	for i in $(seq 33)

Use test_seq here, for portability.

> +test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '
> +(
> +	for i in $(seq 33)

Ditto here.

The rest of the tests looked good to me.

-Peff

[PATCH 2/6] refs.c: remove lock_fd from struct ref_lock

[Stefan Beller]

The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove
it. You may argue this introduces more coupling as we need to know more
about the internals of the lock file mechanism, but this will be solved in
a later patch.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ab2f2a9..e905f51 100644
--- a/refs.c
+++ b/refs.c
@@ -11,7 +11,6 @@ struct ref_lock {
 	char *orig_ref_name;
 	struct lock_file *lk;
 	unsigned char old_sha1[20];
-	int lock_fd;
 	int force_write;
 };
 
@@ -2262,7 +2261,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int attempts_remaining = 3;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
-	lock->lock_fd = -1;
 
 	if (mustexist)
 		resolve_flags |= RESOLVE_REF_READING;
@@ -2338,8 +2336,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		goto error_return;
 	}
 
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
-	if (lock->lock_fd < 0) {
+	if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
 		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
@@ -2916,7 +2913,6 @@ static int close_ref(struct ref_lock *lock)
 {
 	if (close_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -2924,7 +2920,6 @@ static int commit_ref(struct ref_lock *lock)
 {
 	if (commit_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -3102,8 +3097,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full(lock->lock_fd, &term, 1) != 1 ||
+	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
+	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -4083,9 +4078,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full(lock->lock_fd,
+			(write_in_full(lock->lk->fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
+			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

[PATCH 3/6] refs.c: replace write_str_in_full by write_in_full

[Stefan Beller]

There is another occurrence where we could have used write_str_in_full
(line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state
is inconsistent. This replaces the only occurrence of write_str_in_full
by write_in_full, so we only need to wrap write_in_full in the next patch.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index e905f51..8281bed 100644
--- a/refs.c
+++ b/refs.c
@@ -4080,7 +4080,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
 			(write_in_full(lock->lk->fd,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_str_in_full(lock->lk->fd, "\n") != 1 ||
+			 write_in_full(lock->lk->fd, "\n", 1) != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full

[Jeff King]

On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:

> There is another occurrence where we could have used write_str_in_full
> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state
> is inconsistent. This replaces the only occurrence of write_str_in_full
> by write_in_full, so we only need to wrap write_in_full in the next patch.

I had to read the first sentence a few times to figure out what you
meant. But I am not sure it is even relevant. We do not care about the
inconsistency. It is just "we are about to change how callers of
write_in_full in this file behave, the wrapper gets in the way, and it
does not add enough value by itself to merit making our future changes
in two places".

-Peff

Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full

[Stefan Beller]

On Wed, Jan 21, 2015 at 3:38 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:
>
>> There is another occurrence where we could have used write_str_in_full
>> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state
>> is inconsistent. This replaces the only occurrence of write_str_in_full
>> by write_in_full, so we only need to wrap write_in_full in the next patch.
>
> I had to read the first sentence a few times to figure out what you
> meant. But I am not sure it is even relevant. We do not care about the
> inconsistency.

You're not the first who needs to reread my stuff :/
I have the impression my English worsened since coming into the USA.

We do not care about the inconsistency, but we may care about the change itself:
"write_str_in_full is way better than write_in_full, so why the step backwards?"
And  I am trying to explain that this is not a huge step backwards but
rather improves
consistency.

> It is just "we are about to change how callers of
> write_in_full in this file behave, the wrapper gets in the way, and it
> does not add enough value by itself to merit making our future changes
> in two places".

That's actually true. Though that sounds as if we'd be lazy ("we only
want to make
one change, so let's bend over here")

I'll rethink the commit message.
Thanks,
Stefan

Re: [PATCH 3/6] refs.c: replace write_str_in_full by write_in_full

[Jeff King]

On Wed, Jan 21, 2015 at 03:44:36PM -0800, Stefan Beller wrote:

> On Wed, Jan 21, 2015 at 3:38 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Jan 21, 2015 at 03:23:42PM -0800, Stefan Beller wrote:
> >
> >> There is another occurrence where we could have used write_str_in_full
> >> (line 3107: write_in_full(lock->lk->fd, &term, 1)), so the current state
> >> is inconsistent. This replaces the only occurrence of write_str_in_full
> >> by write_in_full, so we only need to wrap write_in_full in the next patch.
> >
> > I had to read the first sentence a few times to figure out what you
> > meant. But I am not sure it is even relevant. We do not care about the
> > inconsistency.
> 
> You're not the first who needs to reread my stuff :/
> I have the impression my English worsened since coming into the USA.

Actually, it was my fault in this case. I read it as "_this_ is another
occurrence", and then I scratched my head wondering what the first
occurrence was (was there a previous change that you should have been
referencing?). I finally got it on the third try. :)

> We do not care about the inconsistency, but we may care about the
> change itself: "write_str_in_full is way better than write_in_full, so
> why the step backwards?" And  I am trying to explain that this is not
> a huge step backwards but rather improves consistency.

But you could improve consistency by going the other way, too. :) I
think the point is that you should lead in with the _real_ reason for
the change, not justifications. You can put in the justifications, too,
for the people who say "wait, but couldn't you do this other thing...".

> > It is just "we are about to change how callers of
> > write_in_full in this file behave, the wrapper gets in the way, and it
> > does not add enough value by itself to merit making our future changes
> > in two places".
> 
> That's actually true. Though that sounds as if we'd be lazy ("we only
> want to make
> one change, so let's bend over here")

It's not laziness. It's avoiding duplicating logic, which would end up
costing more lines and providing worse maintainability than just
dropping the wrapper, which is after all only saving us a few characters
(and not anything conceptually hard).

> I'll rethink the commit message.

Everything I said above is rather subjective, of course. I do appreciate
you breaking your commits apart and explaining each one in the first
place. IOW, while I have thoughts on improving them (obviously), the
current iteration is not so bad that I would be upset to see it go into
git. Don't waste too much time on it.

-Peff

[PATCH 4/6] refs.c: Have a write_in_full_to_lock_file wrapping write_in_full

[Stefan Beller]

Now we only have one place where we need to touch the internals of
the lock_file struct.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 8281bed..311599b 100644
--- a/refs.c
+++ b/refs.c
@@ -3064,6 +3064,13 @@ int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
+static ssize_t write_in_full_to_lockfile(struct lock_file *lock,
+					 const void *buf,
+					 size_t count)
+{
+	return write_in_full(lock->fd, buf, count);
+}
+
 /*
  * Write sha1 into the ref specified by the lock. Make sure that errno
  * is sane on error.
@@ -3097,8 +3104,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
+	if (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||
+	    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -4078,9 +4085,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full(lock->lk->fd,
+			(write_in_full_to_lockfile(lock->lk,
 				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_in_full(lock->lk->fd, "\n", 1) != 1 ||
+			 write_in_full_to_lockfile(lock->lk, "\n", 1) != 1 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
-- 
2.2.1.62.g3f15098

Re: [PATCH 4/6] refs.c: Have a write_in_full_to_lock_file wrapping write_in_full

[Jeff King]

On Wed, Jan 21, 2015 at 03:23:43PM -0800, Stefan Beller wrote:

> Now we only have one place where we need to touch the internals of
> the lock_file struct.

I wonder if the point of the previous patch would be more obvious if it
were combined with this one. I am OK leaving them separate, though.

-Peff

[PATCH 5/6] refs.c: write to a lock file only once

[Stefan Beller]

Instead of having to call write_in_full_to_lock_file twice get a formatted
string such that we only need to invoke writing to the lock file once.

This is helpful for the next patch when we only open the file descriptors
as needed. The lock file API has a reopen_lock_file which currently
doesn't open for appending.

No functional changes intended.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 311599b..0161667 100644
--- a/refs.c
+++ b/refs.c
@@ -3078,8 +3078,8 @@ static ssize_t write_in_full_to_lockfile(struct lock_file *lock,
 static int write_ref_sha1(struct ref_lock *lock,
 	const unsigned char *sha1, const char *logmsg)
 {
-	static char term = '\n';
 	struct object *o;
+	const char *sha1_lf;
 
 	if (!lock) {
 		errno = EINVAL;
@@ -3104,8 +3104,9 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
-	if (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||
-	    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||
+
+	sha1_lf = xstrfmt("%s\n",  sha1_to_hex(sha1));
+	if (write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||
 	    close_ref(lock) < 0) {
 		int save_errno = errno;
 		error("Couldn't write %s", lock->lk->filename.buf);
@@ -3113,6 +3114,7 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = save_errno;
 		return -1;
 	}
+	free((void*)sha1_lf);
 	clear_loose_ref_cache(&ref_cache);
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
@@ -4081,13 +4083,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	(*cleanup_fn)(cb.policy_cb);
 
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
+		const char *sha1_lf = xstrfmt("%s\n",
+					      sha1_to_hex(cb.last_kept_sha1));
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
 					strerror(errno));
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			(write_in_full_to_lockfile(lock->lk,
-				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-			 write_in_full_to_lockfile(lock->lk, "\n", 1) != 1 ||
+			(write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||
 			 close_ref(lock) < 0)) {
 			status |= error("couldn't write %s",
 					lock->lk->filename.buf);
@@ -4098,6 +4100,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
 			status |= error("couldn't set %s", lock->ref_name);
 		}
+		free((void*)sha1_lf);
 	}
 	free(log_file);
 	unlock_ref(lock);
-- 
2.2.1.62.g3f15098

Re: [PATCH 5/6] refs.c: write to a lock file only once

[Jeff King]

On Wed, Jan 21, 2015 at 03:23:44PM -0800, Stefan Beller wrote:

> +	const char *sha1_lf;
>  
>  	if (!lock) {
>  		errno = EINVAL;
> @@ -3104,8 +3104,9 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = EINVAL;
>  		return -1;
>  	}
> -	if (write_in_full_to_lockfile(lock->lk, sha1_to_hex(sha1), 40) != 40 ||
> -	    write_in_full_to_lockfile(lock->lk, &term, 1) != 1 ||
> +
> +	sha1_lf = xstrfmt("%s\n",  sha1_to_hex(sha1));
> +	if (write_in_full_to_lockfile(lock->lk, sha1_lf, 41) != 41 ||
>  	    close_ref(lock) < 0) {
>  		int save_errno = errno;
>  		error("Couldn't write %s", lock->lk->filename.buf);
> @@ -3113,6 +3114,7 @@ static int write_ref_sha1(struct ref_lock *lock,
>  		errno = save_errno;
>  		return -1;
>  	}
> +	free((void*)sha1_lf);

It looks like you leak sha1_lf in the error code path here.

It's a shame that we must allocate at all, when we are really just
passing through to a buffer. Lockfiles have a "FILE *" pointer these
days. Could we just allow:

  lockfile_printf(lock->lk, "%s\n", sha1_to_hex(sha1));

or similar?

-Peff

[PATCH 6/6] refs.c: enable large transactions

[Stefan Beller]

By closing the file descriptors after creating the lock file we are not
limiting the size of the transaction by the number of available file
descriptors.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c                | 14 +++++++++++++-
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 0161667..014ef59 100644
--- a/refs.c
+++ b/refs.c
@@ -3068,7 +3068,17 @@ static ssize_t write_in_full_to_lockfile(struct lock_file *lock,
 					 const void *buf,
 					 size_t count)
 {
-	return write_in_full(lock->fd, buf, count);
+	if (lock->fd == -1) {
+		int ret;
+		if (reopen_lock_file(lock) < 0)
+			return -1;
+		ret = write_in_full(lock->fd, buf, count);
+		if (close_lock_file(lock) < 0)
+			return -1;
+		return ret;
+	} else {
+		return write_in_full(lock->fd, buf, count);
+	}
 }
 
 /*
@@ -3797,6 +3807,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		/* Do not keep all lock files open at the same time. */
+		close_lock_file(update->lock->lk);
 	}
 
 	/* Perform updates first so live commits remain referenced */
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index ea98b9b..751b6dc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1071,7 +1071,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(seq 33)
 	do
@@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT 'large transaction creating branches does not burst o
 )
 '
 
-test_expect_failure ULIMIT 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(seq 33)
 	do
-- 
2.2.1.62.g3f15098

Re: [PATCHv1 0/6] Fix bug in large transactions

[Jeff King]

On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:

> (reported as: git update-ref --stdin : too many open files, 2014-12-20)
> 
> First a test case is introduced to demonstrate the failure,
> the patches 2-6 are little refactoring and the last patch 
> fixes the bug and also marks the bugs as resolved in the
> test suite.
> 
> Unfortunately this applies on top of origin/next.

Saying "applies on next" is not very useful to Junio. He is not going to
branch a topic straight from "next", as merging it to master would pull
in all of the topics cooking in "next" (not to mention a bunch of merge
commits which are generally never part of "master").

Instead, figure out which topic in next you actually _need_ to build on,
and then it can be branched from there. And if there is no such topic,
then you should not be building on next, of course. :) But I think you
know that part already.

-Peff

Re: [PATCHv1 0/6] Fix bug in large transactions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:
>
>> (reported as: git update-ref --stdin : too many open files, 2014-12-20)
>> 
>> First a test case is introduced to demonstrate the failure,
>> the patches 2-6 are little refactoring and the last patch 
>> fixes the bug and also marks the bugs as resolved in the
>> test suite.
>> 
>> Unfortunately this applies on top of origin/next.
>
> Saying "applies on next" is not very useful to Junio. He is not going to
> branch a topic straight from "next", as merging it to master would pull
> in all of the topics cooking in "next" (not to mention a bunch of merge
> commits which are generally never part of "master").
>
> Instead, figure out which topic in next you actually _need_ to build on,
> and then it can be branched from there. And if there is no such topic,
> then you should not be building on next, of course. :) But I think you
> know that part already.

All very true.

I consider anything new that appears late in the cycle, especially
during deep in the pre-release freeze, less for me to apply but more
for others to eyeball the preview of a series the submitter plans to
work on once the next cycle starts, so basing on 'next' does not
hurt too much.  For interested others,

	git checkout origin/next^0

would be shorter to type than

	git checkout "origin/next^{/^Merge branch 'sb/atomic-push'}^2"

so... ;-)

But what is more troublesome is that neither this or updated v2
applies to 'next'.

Let me try to wiggle it in first.

Thanks.

Re: [PATCHv1 0/6] Fix bug in large transactions

[Stefan Beller]

On Thu, Jan 22, 2015 at 12:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 21, 2015 at 03:23:39PM -0800, Stefan Beller wrote:
>>
>>> (reported as: git update-ref --stdin : too many open files, 2014-12-20)
>>>
>>> First a test case is introduced to demonstrate the failure,
>>> the patches 2-6 are little refactoring and the last patch
>>> fixes the bug and also marks the bugs as resolved in the
>>> test suite.
>>>
>>> Unfortunately this applies on top of origin/next.
>>
>> Saying "applies on next" is not very useful to Junio. He is not going to
>> branch a topic straight from "next", as merging it to master would pull
>> in all of the topics cooking in "next" (not to mention a bunch of merge
>> commits which are generally never part of "master").
>>
>> Instead, figure out which topic in next you actually _need_ to build on,
>> and then it can be branched from there. And if there is no such topic,
>> then you should not be building on next, of course. :) But I think you
>> know that part already.
>
> All very true.
>
> I consider anything new that appears late in the cycle, especially
> during deep in the pre-release freeze, less for me to apply but more
> for others to eyeball the preview of a series the submitter plans to
> work on once the next cycle starts, so basing on 'next' does not
> hurt too much.  For interested others,
>
>         git checkout origin/next^0
>
> would be shorter to type than
>
>         git checkout "origin/next^{/^Merge branch 'sb/atomic-push'}^2"
>
> so... ;-)
>
> But what is more troublesome is that neither this or updated v2
> applies to 'next'.

v2 applies to sb/atomic-push instead of next and will result in a one
line merge conflict with next.

I know we're late in the cycle, hence I was just sending out for reviews.
Though as it's not a feature, but a bug fix it may be worth picking it
up now if it's easy to apply.

Thanks,
Stefan

>
> Let me try to wiggle it in first.
>
> Thanks.

Re: [PATCHv1 0/6] Fix bug in large transactions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> v2 applies to sb/atomic-push instead of next and will result in a one
> line merge conflict with next.

I acctually tried to apply on 'next' and also on 'sb/atomic-push'
and both failed.  I had to wiggle the patches to make them apply on
the latter, and that is what is queued on 'pu' now, but I would not
be surprised if I made silly mistakes while doing so, so please
double check the result and catch them if I did.

Thanks.

Re: [PATCHv1 0/6] Fix bug in large transactions

[Stefan Beller]

On Thu, Jan 22, 2015 at 9:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> v2 applies to sb/atomic-push instead of next and will result in a one
>> line merge conflict with next.
>
> I acctually tried to apply on 'next' and also on 'sb/atomic-push'
> and both failed.

That's strange indeed. I just checked out origin/sb/atomic-push (which
points at 04b39f195baf, 2015-01-12 16:24:02, Document receive.advertiseatomic)
and applied the patches v2 from my outgoing mailbox without problems.

I do not remember to have changed my development process a lot there.
(I was not using notes, but that should not matter at all)
git format-patch --subject-prefix=PATCHv2 --cover-letter
origin/sb/atomic-push..HEAD
$EDITOR 0000-cover-letter.patch
git send-email 00* --to=...

>I had to wiggle the patches to make them apply on
> the latter, and that is what is queued on 'pu' now, but I would not
> be surprised if I made silly mistakes while doing so, so please
> double check the result and catch them if I did.

I just fetched origin/sb/atomic-push-fix and the only difference I can spot
compared to the local branch is in d4ad3f1cdcb (refs.c: remove lock_fd
from struct ref_lock) in refs.c in the hunk:

@@ -2335,8 +2333,8 @@ static struct ref_lock
*lock_ref_sha1_basic(const char *refname,
  goto error_return;
  }

- lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
- if (lock->lock_fd < 0) {
+ if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
+ last_errno = errno;
  if (errno == ENOENT && --attempts_remaining > 0)
  /*
  * Maybe somebody just deleted one of the

(whitespaces broken here)
That hunk has the additional line "+ last_errno = errno;", which I do
not have locally, but that's the exact problem I spotted when trying to
merge my local branch to origin/next which then results in a merge
conflict with the patch from origin/jk/lock-ref-sha1-basic-return-errors
as that introduces the line "+ last_errno = errno;".

Sorry for the confusion,
Stefan