diff mbox series

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

Message ID 6276372ad3dc6fa4b9b352abb2b0192a6d010528.1542121775.git.gitgitgadget@gmail.com
State New, archived
Headers show
Series bundle: fix issue when bundles would be empty | expand

Commit Message

Elijah Newren via GitGitGadget Nov. 13, 2018, 3:09 p.m. UTC
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(-)

Comments

Stefan Beller Nov. 13, 2018, 7:28 p.m. UTC | #1
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"
Gaël Lhez Nov. 13, 2018, 8:37 p.m. UTC | #2
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"
Stefan Beller Nov. 13, 2018, 9:11 p.m. UTC | #3
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
Johannes Schindelin Nov. 14, 2018, 3:23 p.m. UTC | #4
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
>
diff mbox series

Patch

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