bundle: fix issue when bundles would be empty

From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <[email protected]>

When an user tries to create an empty bundle via git bundle create <bundle> <revlist> where <revlist> resolves to an empty list (for example, like master..master), the command fails and warns the user about how it does not want to create empty bundle.

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

This closes https://github.com/git-for-windows/git/issues/790

Signed-off-by Gaël Lhez <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

Quoted 8 lines · Gaël Lhez via GitGitGadget

From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <[email protected]>

When an user tries to create an empty bundle via git bundle create <bundle> <revlist> where <revlist> resolves to an empty list (for example, like master..master), the command fails and warns the user about how it does not want to create empty bundle.

Quoted 7 lines

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

This closes https://github.com/git-for-windows/git/issues/790

Signed-off-by Gaël Lhez <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

The code and the commit message make sense, but by reading the subject line

I would have expected a different commit. Maybe
"bundle: cleanup lock files on error"

Hello,

I don't know why I receive these message (and especially now given the time at which I pushed this) but I suppose someone (Johannes Schindelin ?) probably pushed back my original commit from git for windows github to GIT git repository.

If you think "bundle: cleanup lock files on error" is better, then no problem with me. I'm not a native english speaker and I simply expressed the reason for my problem but - after reading back my commit - neither this mail' subject and my original commit subject (see https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) express the core problem.

As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ?

Quoted 4 lines · Gaël Lhez

Hello,

I don't know why I receive these message (and especially now given the time at which I pushed this) but I suppose someone (Johannes Schindelin ?) probably pushed back my original commit from git for windows github to GIT git repository.

Yes that is pretty much what is happening. Johannes (GfW maintainer) tries to push a lot of patches upstream to git and cc's people who originally authored the patch.
Thanks for taking a look, again, even after this long time!

Quoted 2 lines

If you think "bundle: cleanup lock files on error" is better, then no problem with me. I'm not a native english speaker and I simply expressed the reason for my problem but - after reading back my commit - neither this mail' subject and my original commit subject (see https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) express the core problem.

I am not a native speaker either, which makes it extra hard to understand some commits. ;-) So I propose a wording which would have helped me.

Quoted 1 line

As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ?

I don't know how Johannes handles pushing changes upstream, maybe he will take on the work of resending a reworded patch.

Let's hear his thoughts on it. I would guess you're more than welcome to take your patches from GitForWindows into Git itself.

Cheers,
Stefan

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

Quoted 2 lines · Gaël Lhez via GitGitGadget

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

Hmmm, doesn't the tempfile machinery remove the lock file when we die?

Quoted 6 lines · Martin Ågren

Quoted 2 lines · Gaël Lhez via GitGitGadget

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

Quoted 1 line

Hmmm, doesn't the tempfile machinery remove the lock file when we die?

On Windows this seems not to be the case. (Open files cannot be deleted as the open file is not kept by inode or similar but by the file path there?)

Rewording your concern: Could the tempfile machinery be taught to work properly on Windows, e.g. by first closing all files and then deleting them afterwards?

Quoted 7 lines · Stefan Beller

Quoted 6 lines · Martin Ågren

Quoted 2 lines · Gaël Lhez via GitGitGadget

However, the .lock file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue.

Quoted 8 lines

Quoted 1 line

Hmmm, doesn't the tempfile machinery remove the lock file when we die?

On Windows this seems not to be the case. (Open files cannot be deleted as the open file is not kept by inode or similar but by the file path there?)

Rewording your concern: Could the tempfile machinery be taught to work properly on Windows, e.g. by first closing all files and then deleting them afterwards?

It already tries to do so. See delete_tempfile(), or more likely in the die() case, the remove_tempfiles() handler which is called at exit.

Are we sure this is still a problem?

I looked at the test to see if it would pass, but it is not even checking anything about lockfiles! It just checks that we exit 1 by returning up the callstack instead of calling die(). And of course it would not have a problem under Linux either way. But if I run something similar under strace, I see:

  $ strace ./git bundle create foobar.bundle HEAD..HEAD
  [...]
  openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
  [...]
  close(3)                                = 0
  unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
  exit_group(128)                         = ?

which seems right.

Quoted 14 lines

I looked at the test to see if it would pass, but it is not even checking anything about lockfiles! It just checks that we exit 1 by returning up the callstack instead of calling die(). And of course it would not have a problem under Linux either way. But if I run something similar under strace, I see:

  $ strace ./git bundle create foobar.bundle HEAD..HEAD
  [...]
  openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
  [...]
  close(3)                                = 0
  unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
  exit_group(128)                         = ?

Quoted 1 line

which seems right.

Without the fix, the added regression test fails thusly:

-- snip --
[...]
++ test_expect_code 1 git bundle create foobar.bundle master..master
++ want_code=1
++ shift
++ git bundle create foobar.bundle master..master
fatal: Refusing to create empty bundle.
warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied

Quoted 18 lines · Johannes Schindelin

Quoted 14 lines

I looked at the test to see if it would pass, but it is not even checking anything about lockfiles! It just checks that we exit 1 by returning up the callstack instead of calling die(). And of course it would not have a problem under Linux either way. But if I run something similar under strace, I see:

  $ strace ./git bundle create foobar.bundle HEAD..HEAD
  [...]
  openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
  [...]
  close(3)                                = 0
  unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
  exit_group(128)                         = ?

Quoted 1 line

which seems right.

Without the fix, the added regression test fails thusly:

Quoted 8 lines

-- snip --
[...]
++ test_expect_code 1 git bundle create foobar.bundle master..master
++ want_code=1
++ shift
++ git bundle create foobar.bundle master..master
fatal: Refusing to create empty bundle.
warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied

Hmph. So who has it open, and why isn't the tempfile code working as designed?

Aha, I see the problem. We dup() the descriptor in create_bundle(). So the patch _is_ necessary and (fairly) correct. But the explanation probably ought to be something like:

In create_bundle(), we duplicate the lockfile descriptor via dup(). This means that even though the lockfile code carefully calls close() before unlinking the lockfile, we may still have the file open. Unix systems don't care, but under Windows, this prevents the unlink (causing an annoying warning and a stale lockfile).

But that also means that all of the other places we could die (e.g., in write_or_die) are going to have the same problem. We've fixed only one. Is there a way we can avoid doing the dup() in the first place?

The comment there explains that we duplicate because write_pack_data() will close the descriptor. Unfortunately, that's hard to change because it comes from run-command. But we don't actually need the descriptor ourselves after it's closed; we're just trying to appease the lockfile code; see e54c347c1c (create_bundle(): duplicate file descriptor to avoid closing it twice, 2015-08-10).

We just need some reasonable way of telling the lock code what's happening. Something like the patch below, which is a moral revert of e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API.

Does this make your warning go away?

I cannot claim that I wrapped my head around your explanation or your diff (I am busy trying to prepare Git for Windows' master for rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so much!

The line test_expect_code 1 ... needs to be adjusted to test_expect_code 128, of course, and to discern from the fixed problem (which also exits with code 128), the error output should be verified, like so:

-- snip --
test_expect_success 'try to create a bundle with empty ref count' '

	test_must_fail git bundle create foobar.bundle master..master 2>err &&
	test_i18ngrep "Refusing to create empty bundle" err

'
-- snap --

Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch?

So yes, we are trying to unlink the .lock file, and as far as I can tell that unlink() call comes from the tempfile cleanup asked for by Martin. However, as we still have a handle open to that file, that call fails.

I do not think that there is any better way to fix this than to close the file explicitly.

Quoted 10 lines · Johannes Schindelin

I cannot claim that I wrapped my head around your explanation or your diff (I am busy trying to prepare Git for Windows' master for rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so much!

The line test_expect_code 1 ... needs to be adjusted to test_expect_code 128, of course, and to discern from the fixed problem (which also exits with code 128), the error output should be verified, like so:

Quoted 6 lines

-- snip --
test_expect_success 'try to create a bundle with empty ref count' '

	test_must_fail git bundle create foobar.bundle master..master 2>err &&
	test_i18ngrep "Refusing to create empty bundle" err

'
-- snap --

It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like:

test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock

That would already pass on non-Windows platforms, but that's OK. It will never give a false failure.

If you don't mind, can you confirm that the test above fails without either of the two patches under discussion?

Quoted 2 lines

Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch?

I'll wrap it up with a commit message and a test.

Quoted 7 lines

It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like:

test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock

Quoted 5 lines

That would already pass on non-Windows platforms, but that's OK. It will never give a false failure.

If you don't mind, can you confirm that the test above fails without either of the two patches under discussion?

This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test.

Quoted 4 lines

Quoted 2 lines

Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch?

I'll wrap it up with a commit message and a test.

Quoted 7 lines · Johannes Schindelin

Quoted 7 lines

It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like:

test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock

Quoted 8 lines

Quoted 5 lines

That would already pass on non-Windows platforms, but that's OK. It will never give a false failure.

If you don't mind, can you confirm that the test above fails without either of the two patches under discussion?

This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test.

Thanks for checking!

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch?

I'll wrap it up with a commit message and a test.

Actually, I realized there's an even simpler way to do this. Here it is.

-- >8 --
Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use

When writing a bundle to a file, the bundle code actually creates "your.bundle.lock" using our lockfile interface. We feed that output descriptor to a child git-pack-objects via run-command, which has the quirk that it closes the output descriptor in the parent.

To avoid confusing the lockfile code (which still thinks the descriptor is valid), we dup() it, and operate on the duplicate.

However, this has a confusing side effect: after the dup() but before we call pack-objects, we have _two_ descriptors open to the lockfile. If we call die() during that time, the lockfile code will try to clean up the partially-written file. It knows to close() the file before unlinking, since on some platforms (i.e., Windows) the open file would block the deletion. But it doesn't know about the duplicate descriptor. On Windows, triggering an error at the right part of the code will result in the cleanup failing and the lockfile being left in the filesystem.

We can solve this by moving the dup() much closer to start_command(), shrinking the window in which we have the second descriptor open. It's easy to place this in such a way that no die() is possible. We could still die due to a signal in the exact wrong moment, but we already tolerate races there (e.g., a signal could come before we manage to put the file on the cleanup list in the first place).

As a bonus, this shields create_bundle() itself from the duplicate-fd trick, and we can simplify its error handling (note that the lock rollback now happens unconditionally, but that's OK; it's a noop if we didn't open the lock in the first place).

The included test uses an empty bundle to cause a failure at the right spot in the code, because that's easy to trigger (the other likely errors are write() problems like ENOSPC). Note that it would already pass on non-Windows systems (because they are happy to unlink an already-open file).

Based-on-a-patch-by Gaël Lhez <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 bundle.c                | 39 ++++++++++++++++++---------------------
 t/t5607-clone-bundle.sh |  6 ++++++
 2 files changed, 24 insertions(+), 21 deletions(-)

bundle.c +18 -21

index 1ef584b93b..6b0f6d8f10 100644
@@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 }
 
 
-/* Write the pack data to bundle_fd, then close it if it is > 1. */
+/* Write the pack data to bundle_fd */
 static int write_pack_data(int bundle_fd, struct rev_info *revs)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;

@@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs)
 	pack_objects.in = -1;
 	pack_objects.out = bundle_fd;
 	pack_objects.git_cmd = 1;
+
+	/*
+	 * start_command() will close our descriptor if it's >1. Duplicate it
+	 * to avoid surprising the caller.
+	 */
+	if (pack_objects.out > 1) {
+		pack_objects.out = dup(pack_objects.out);
+		if (pack_objects.out < 0) {
+			error_errno(_("unable to dup bundle descriptor"));
+			child_process_clear(&pack_objects);
+			return -1;
+		}
+	}
+
 	if (start_command(&pack_objects))
 		return error(_("Could not spawn pack-objects"));
 

@@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path,
 	bundle_to_stdout = !strcmp(path, "-");
 	if (bundle_to_stdout)
 		bundle_fd = 1;
-	else {
+	else
 		bundle_fd = hold_lock_file_for_update(&lock, path,
 						      LOCK_DIE_ON_ERROR);
 
-		/*
-		 * write_pack_data() will close the fd passed to it,
-		 * but commit_lock_file() will also try to close the
-		 * lockfile's fd. So make a copy of the file
-		 * descriptor to avoid trying to close it twice.
-		 */
-		bundle_fd = dup(bundle_fd);
-		if (bundle_fd < 0)
-			die_errno("unable to dup file descriptor");
-	}
-
 	/* write signature */
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
 

@@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 		goto err;
 
 	/* write pack */
-	if (write_pack_data(bundle_fd, &revs)) {
-		bundle_fd = -1; /* already closed by the above call */
+	if (write_pack_data(bundle_fd, &revs))
 		goto err;
-	}
 
 	if (!bundle_to_stdout) {
 		if (commit_lock_file(&lock))

@@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	}
 	return 0;
 err:
-	if (!bundle_to_stdout) {
-		if (0 <= bundle_fd)
-			close(bundle_fd);
-		rollback_lock_file(&lock);
-	}
+	rollback_lock_file(&lock);
 	return -1;
 }
 

t/t5607-clone-bundle.sh +6 -0

index 348d9b3bc7..cf39e9e243 100755
@@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' '
 	git bundle verify bundle
 '
 
+test_expect_success 'failed bundle creation does not leave cruft' '
+	# This fails because the bundle would be empty.
+	test_must_fail git bundle create fail.bundle master..master &&
+	test_path_is_missing fail.bundle.lock
+'
+
 test_done

Signature · 1 line

2.19.1.1636.gc7a073d580

Actually, I realized there's an even simpler way to do this. Here it is.

-- >8 --
Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use

When writing a bundle to a file, the bundle code actually creates "your.bundle.lock" using our lockfile interface. We feed that output descriptor to a child git-pack-objects via run-command, which has the quirk that it closes the output descriptor in the parent.

To avoid confusing the lockfile code (which still thinks the descriptor is valid), we dup() it, and operate on the duplicate.

We can solve this by moving the dup() much closer to start_command(), shrinking the window in which we have the second descriptor open. It's easy to place this in such a way that no die() is possible. We could still die due to a signal in the exact wrong moment, but we already tolerate races there (e.g., a signal could come before we manage to put the file on the cleanup list in the first place).

As a bonus, this shields create_bundle() itself from the duplicate-fd trick, and we can simplify its error handling (note that the lock rollback now happens unconditionally, but that's OK; it's a noop if we didn't open the lock in the first place).