[PATCH 0/1] bundle: fix issue when bundles would be empty

[PATCH 0/1] bundle: fix issue when bundles would be empty

[Johannes Schindelin via GitGitGadget]

And yet another patch coming through Git for Windows...

Gaël Lhez (1):
  bundle: refuse to create empty bundle

 bundle.c                | 7 ++++---
 t/t5607-clone-bundle.sh | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/79
-- 
gitgitgadget

[PATCH 1/1] bundle: refuse to create empty bundle

[Gaël Lhez via GitGitGadget]

From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>

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 <gael.lhez@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 bundle.c                | 7 ++++---
 t/t5607-clone-bundle.sh | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/bundle.c b/bundle.c
index 1ef584b93b..4e349feff9 100644
--- a/bundle.c
+++ b/bundle.c
@@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path,
 	object_array_remove_duplicates(&revs.pending);
 
 	ref_count = write_bundle_refs(bundle_fd, &revs);
-	if (!ref_count)
-		die(_("Refusing to create empty bundle."));
-	else if (ref_count < 0)
+	if (ref_count <= 0)  {
+		if (!ref_count)
+			error(_("Refusing to create empty bundle."));
 		goto err;
+	}
 
 	/* write pack */
 	if (write_pack_data(bundle_fd, &revs)) {
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 348d9b3bc7..f84b875950 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' '
 	git bundle verify bundle
 '
 
+test_expect_success 'try to create a bundle with empty ref count' '
+	test_expect_code 1 git bundle create foobar.bundle master..master
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 1/1] bundle: refuse to create empty bundle

[Stefan Beller]

On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>
>
> 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 <gael.lhez@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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"

Re: [PATCH 1/1] bundle: refuse to create empty bundle

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

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 ?

(Sent back in text mode because HTML is considered SPAM or Virus by
git@vger.kernel.org).

Regards,
Gaël

Le mar. 13 nov. 2018 à 20:28, Stefan Beller <sbeller@google.com> a écrit :
>
> On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>
> >
> > 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 <gael.lhez@gmail.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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"

Re: [PATCH 1/1] bundle: refuse to create empty bundle

[Stefan Beller]

On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote:
>
> 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!

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

> 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

Re: [PATCH 1/1] bundle: refuse to create empty bundle

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

Hi Stefan,

On Tue, 13 Nov 2018, Stefan Beller wrote:

> On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote:
> >
> > 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!
> 
> >
> > 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.
> 
> > 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.

He will.

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

As I merged the patch into Git for Windows' `master`, I consider it my
responsibility to push this upstream (unless the contributor wants to take
matters into their own hands).

In the future, my hope is that GitGitGadget will make contributing patches
to the Git mailing list a more convenient experience, to the point that it
will hopefully be pretty much as easy as iterating a PR in
https://github.com/git-for-windows/git. At that point, I will ask
contributors to do exactly that in order to shepherd their patches into
git.git.

Ciao,
Dscho

> 
> Cheers,
> Stefan
> 

[PATCH v2 0/1] bundle: fix issue when bundles would be empty

[Johannes Schindelin via GitGitGadget]

And yet another patch coming through Git for Windows...

Change since v1:

 * Using a better oneline now.

Gaël Lhez (1):
  bundle: cleanup lock files on error

 bundle.c                | 7 ++++---
 t/t5607-clone-bundle.sh | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)


base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/79

Range-diff vs v1:

 1:  6276372ad3 ! 1:  c88887f05a bundle: refuse to create empty bundle
     @@ -1,6 +1,6 @@
      Author: Gaël Lhez <gael.lhez@gmail.com>
      
     -    bundle: refuse to create empty bundle
     +    bundle: cleanup lock files on error
      
          When an user tries to create an empty bundle via `git bundle create
          <bundle> <revlist>` where `<revlist>` resolves to an empty list (for

-- 
gitgitgadget

[PATCH v2 1/1] bundle: cleanup lock files on error

[Gaël Lhez via GitGitGadget]

From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>

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 <gael.lhez@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 bundle.c                | 7 ++++---
 t/t5607-clone-bundle.sh | 4 ++++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/bundle.c b/bundle.c
index 1ef584b93b..4e349feff9 100644
--- a/bundle.c
+++ b/bundle.c
@@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path,
 	object_array_remove_duplicates(&revs.pending);
 
 	ref_count = write_bundle_refs(bundle_fd, &revs);
-	if (!ref_count)
-		die(_("Refusing to create empty bundle."));
-	else if (ref_count < 0)
+	if (ref_count <= 0)  {
+		if (!ref_count)
+			error(_("Refusing to create empty bundle."));
 		goto err;
+	}
 
 	/* write pack */
 	if (write_pack_data(bundle_fd, &revs)) {
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 348d9b3bc7..f84b875950 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' '
 	git bundle verify bundle
 '
 
+test_expect_success 'try to create a bundle with empty ref count' '
+	test_expect_code 1 git bundle create foobar.bundle master..master
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Martin Ågren]

On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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?

>         ref_count = write_bundle_refs(bundle_fd, &revs);
> -       if (!ref_count)
> -               die(_("Refusing to create empty bundle."));
> -       else if (ref_count < 0)
> +       if (ref_count <= 0)  {
> +               if (!ref_count)
> +                       error(_("Refusing to create empty bundle."));
>                 goto err;
> +       }

One less `die()` in libgit -- nice.

> +test_expect_success 'try to create a bundle with empty ref count' '
> +       test_expect_code 1 git bundle create foobar.bundle master..master
> +'

This tries to make sure that we don't just die, but that we exit in a
"controlled" way with exit code 1. After reading the log message, I had
perhaps expected something like

  test_must_fail git bundle [...] &&
  test_path_is_missing foobar.bundle.lock

That relies on magically knowing the ".lock" suffix, but my point is
that for me (on Linux), this alternative test passes both before and
after the patch. Before, because tempfile.c cleans up; after, because
bundle.c does so. Doesn't this happen on Windows too? What am I missing?

Martin

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Stefan Beller]

On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:
>
> On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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?

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?

There was a refactoring of tempfiles merged in 89563ec379
(Merge branch 'jk/incore-lockfile-removal', 2017-09-19), which sounded
promising at first, as it is dated after the original patch[1] date
(June 2016), but it has no references for Windows being different,
so we might still have the original issue; most of the lockfile
infrastructure was done in 2015 via db86e61cbb (Merge branch
'mh/tempfile', 2015-08-25)

[1] https://github.com/git-for-windows/git/pull/797

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Jeff King]

On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:

> On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:
> >
> > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > 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?
> 
> 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.

-Peff

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]

Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:
> 
> > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:
> > >
> > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > > <gitgitgadget@gmail.com> wrote:
> > > > 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?
> > 
> > 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.

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
++ exit_code=128
++ test 128 = 1
++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master'
test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master
++ return 1
error: last command exited with $?=1
not ok 9 - try to create a bundle with empty ref count
#
#               test_expect_code 1 git bundle create foobar.bundle master..master
#
-- snap --

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. If we tried to just close whatever file descriptor is still open to
that file before deleting it, we would possibly cause problems in code that is
still to be executed and assumes that it has a perfectly valid file descriptor.
Besides, trying to do this kind of "automatically" won't work, like, at all,
when it is one child process that holds an open file descriptor while another
process wants to delete the file.

Ciao,
Dscho

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Jeff King]

On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:

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

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?

diff --git a/bundle.c b/bundle.c
index 1ef584b93b..dc26551b83 100644
--- a/bundle.c
+++ b/bundle.c
@@ -244,7 +244,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. */
-static int write_pack_data(int bundle_fd, struct rev_info *revs)
+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;
 	int i;
@@ -256,6 +256,14 @@ 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;
+
+	/*
+	 * At this point we know that start_command is going to close our
+	 * bundle_fd, whether successful or not. Tell the lock code that
+	 * it is no longer in charge of it, so we don't try to double-close.
+	 */
+	lock_file_release_descriptor(lock);
+
 	if (start_command(&pack_objects))
 		return error(_("Could not spawn pack-objects"));
 
@@ -421,21 +429,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 +460,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, &lock, &revs))
 		goto err;
-	}
 
 	if (!bundle_to_stdout) {
 		if (commit_lock_file(&lock))
@@ -474,11 +469,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;
 }
 
diff --git a/lockfile.h b/lockfile.h
index 35403ccc0d..968cd0e4f6 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -304,4 +304,14 @@ static inline void rollback_lock_file(struct lock_file *lk)
 	delete_tempfile(&lk->tempfile);
 }
 
+/*
+ * Release the file descriptor and/or file pointer for an open lockfile,
+ * passing ownership to the caller (who is responsible for closing it). It's a
+ * NOOP to call this on an inactive or already-closed lockfile.
+ */
+static inline void lock_file_release_descriptor(struct lock_file *lk)
+{
+	tempfile_release_descriptor(lk->tempfile);
+}
+
 #endif /* LOCKFILE_H */
diff --git a/tempfile.c b/tempfile.c
index d43ad8c191..59bbdcf4ba 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -319,3 +319,9 @@ void delete_tempfile(struct tempfile **tempfile_p)
 	deactivate_tempfile(tempfile);
 	*tempfile_p = NULL;
 }
+
+void tempfile_release_descriptor(struct tempfile *tempfile)
+{
+	tempfile->fd = -1;
+	tempfile->fp = NULL;
+}
diff --git a/tempfile.h b/tempfile.h
index 61d8dc4d1b..7489c5e4e9 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -262,4 +262,11 @@ extern void delete_tempfile(struct tempfile **tempfile_p);
  */
 extern int rename_tempfile(struct tempfile **tempfile_p, const char *path);
 
+/*
+ * Release the file descriptor and/or file pointer for an open tempfile,
+ * passing ownership to the caller (who is responsible for closing it). It's a
+ * NOOP to call this on an inactive or already-closed tempfile.
+ */
+void tempfile_release_descriptor(struct tempfile *tempfile);
+
 #endif /* TEMPFILE_H */

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Johannes Schindelin]

Hi Peff,

On Thu, 15 Nov 2018, Jeff King wrote:

> On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:
> 
> > > 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.
> > 
> > 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
> 
> 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?
> 
> diff --git a/bundle.c b/bundle.c
> [...]

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?

Ciao,
Dscho

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Jeff King]

On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:

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

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?

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

-Peff

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Duy Nguyen]

On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.

I may be talking nonsense here but I remember someone mentioned
something about deleting a file while it's still open on Windows and
after a quick internet search, it may be FILE_SHARE_DELETE. Since _we_
open these files, can we just open them with this to be able to delete
them?
-- 
Duy

Re: [PATCH v2 1/1] bundle: cleanup lock files on error

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

Hi Peff,

On Thu, 15 Nov 2018, Jeff King wrote:

> On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:
> 
> > 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 --
> 
> 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?

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.

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

Thank you so much!
Dscho

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

[Jeff King]

On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote:

> > 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?
> 
> 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!

> > > 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 <gael.lhez@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 bundle.c                | 39 ++++++++++++++++++---------------------
 t/t5607-clone-bundle.sh |  6 ++++++
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/bundle.c b/bundle.c
index 1ef584b93b..6b0f6d8f10 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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;
 }
 
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index 348d9b3bc7..cf39e9e243 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -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
-- 
2.19.1.1636.gc7a073d580

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6859 bytes --]

Hi Peff,

On Fri, 16 Nov 2018, Jeff King wrote:

> On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote:
> 
> > > 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?
> > 
> > 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!
> 
> > > > 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).

Thanks, this is a very nice explanation (and now that I do not feel so
stressed as I did yesterday, I can easily wrap my head around it).

I can confirm that the test fails without the changes to bundle.c, and
succeeds with the changes.

Thank you so much!
Dscho

> Based-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  bundle.c                | 39 ++++++++++++++++++---------------------
>  t/t5607-clone-bundle.sh |  6 ++++++
>  2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 1ef584b93b..6b0f6d8f10 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -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;
>  }
>  
> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> index 348d9b3bc7..cf39e9e243 100755
> --- a/t/t5607-clone-bundle.sh
> +++ b/t/t5607-clone-bundle.sh
> @@ -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
> -- 
> 2.19.1.1636.gc7a073d580
> 
> 

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yes...

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

... I found this way too clever for me, but by following the
codeflow, it was easy to convince myself that this does the right
thing.  Almost magical ;-)

Will queue, with a Tested-by: with Dscho's name on it.

TAhanks, both.