[PATCH 0/3] Large transactions in git

[PATCH 0/3] Large transactions in git

[Stefan Beller]

On Git Merge Wilhelm Bierbaum from Twitter made clear that we'd have problems
with large transactions in git. As I have been working on that series a few
months ago and it still bugs me, I thought about reviving the series.

However the series got stale a few months ago because we were not sure how
to do it right. The first few patches were deemed uncontroversial though,
which I am resending now. 

The first patch contains tests telling us what needs fixing.
The second patch renames the precondition for other tests, as ULIMIT is
a bit generic. The third patch removes the `lock_fd` field from struct
ref_lock which was duplicate information.

That said, this series is not fixing the actual issue, but I'd guess
having these patches is step forward actually.

Thanks,
Stefan

Stefan Beller (3):
  update-ref: test handling large transactions properly
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
  refs.c: remove lock_fd from struct ref_lock

 refs.c                | 16 ++++++----------
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 t/t7004-tag.sh        |  4 ++--
 3 files changed, 36 insertions(+), 12 deletions(-)

-- 
2.3.0.81.gc37f363

[PATCH 1/3] update-ref: test handling large transactions properly

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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 7b4707b..47d2fe9 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -973,4 +973,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_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+(
+	for i in $(test_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_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+(
+	for i in $(test_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.3.0.81.gc37f363

[PATCH 2/3] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Stefan Beller]

During creation of the patch series our discussion we could have a
more descriptive name for the prerequisite for the test so it stays
unique when other limits of ulimit are introduced.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7004-tag.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 796e9f7..06b8e0d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1463,10 +1463,10 @@ run_with_limited_stack () {
 	(ulimit -s 128 && "$@")
 }
 
-test_lazy_prereq ULIMIT 'run_with_limited_stack true'
+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
 
 # we require ulimit, this excludes Windows
-test_expect_success ULIMIT '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
 	>expect &&
 	i=1 &&
 	while test $i -lt 8000
-- 
2.3.0.81.gc37f363

[PATCH 3/3] 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 14e52ca..4066752 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;
 };
 
@@ -2259,7 +2258,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;
@@ -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
@@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
 {
 	if (close_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
 {
 	if (commit_lock_file(lock->lk))
 		return -1;
-	lock->lock_fd = -1;
 	return 0;
 }
 
@@ -3090,8 +3086,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);
@@ -4047,9 +4043,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.3.0.81.gc37f363

Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.

It is somewhat strange to hear "in a later patch" in [PATCH 3/3] of
a 3-patch series ;-), but I think this makes sense.  Whenever
we take a ref-lock, and we are going to actually write something
into the filesystem, we would go thru the lock_file API, so we can
depend on lk to have its own file descriptor field.




>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  refs.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 14e52ca..4066752 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;
>  };
>  
> @@ -2259,7 +2258,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;
> @@ -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
> @@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
>  {
>  	if (close_lock_file(lock->lk))
>  		return -1;
> -	lock->lock_fd = -1;
>  	return 0;
>  }
>  
> @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
>  {
>  	if (commit_lock_file(lock->lk))
>  		return -1;
> -	lock->lock_fd = -1;
>  	return 0;
>  }
>  
> @@ -3090,8 +3086,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);
> @@ -4047,9 +4043,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);

Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

[Michael Haggerty]

On 04/15/2015 12:25 AM, Stefan Beller wrote:
> 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.

This whole series LGTM; however, I suggest that this patch be split up.
See below.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  refs.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 14e52ca..4066752 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -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
> [...]

Here you add the line "last_errno = errno". It is a good change, but it
is not part of removing ref_lock::lock_fd.

I suggest that you move this change to a separate commit. You might also
consider moving the new line to the "else" clause, because it's really
about preserving errno around the call to error() and preparing for
"goto error_return".

With or without this split, this patch is

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This whole series LGTM; however, I suggest that this patch be split up.
> See below.
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  refs.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> 
>> diff --git a/refs.c b/refs.c
>> index 14e52ca..4066752 100644
>> --- a/refs.c
>> +++ b/refs.c
>> [...]
>> @@ -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
>> [...]
>
> Here you add the line "last_errno = errno". It is a good change, but it
> is not part of removing ref_lock::lock_fd.

I think this patch came from an ancient codebase before 06839515
(lock_ref_sha1_basic: do not die on locking errors, 2014-11-19),
which added the "last_errno = errno", and was not rebased to match
more recent codebase.

I am planning to apply these on top of v2.4.0-rc, so there will be
no new "save to last_errno" in the end.

Thanks.