Bug in `git bundle create <file> <not-a-branch>`

[Philip Oakley]

I'm doing a bit of hacking on `git bundle` (to unconfuse which branch was
HEAD after transfer) and I tripped over this inability to unlink a locked
file if a bad ref is passed to `git bundle create`.

In this case I hadn't created a next or pu branch to track the upstream,
hence the unkown rev.

However we then get an inability to unlink the partially created locked
bundle file which looks like a long standing bug:

Philip@PhilipOakley MINGW32 ~
$ cd /c/git-sdk-32/usr/src/git/

Philip@PhilipOakley MINGW32 /c/git-sdk-32/usr/src/git (bundle1)
$ git bundle create test.bndl HEAD bundle1 master next
pu --since=2.weeks.ago
fatal: ambiguous argument 'next': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error: rev-list died
Unlink of file 'C:/git-sdk-32/usr/src/git/test.bndl.lock' failed. Should I
try again? (y/n) n
warning: unable to unlink C:/git-sdk-32/usr/src/git/test.bndl.lock: Invalid
argument

Philip@PhilipOakley MINGW32 /c/git-sdk-32/usr/src/git (bundle1)
$ git version
git version 2.7.1.windows.1

Philip@PhilipOakley MINGW32 /c/git-sdk-32/usr/src/git (bundle1)
$

Is someone able to confirm the behaviour on Linux?

The test above is using my regular C:/Program Files/ version, rather than
the SDK, but my latest SDK version (with hacks) also has the same issues.

It looks like compute_and_write_prerequisites in git/bundle.c is trying too
hard, - determining the prequisites via running rev-list should be separated
from the writing of them to the bundle so that the header can be created at
that point, but I may not be understanding how the various dying parts
interact.
--
Philip

[Philip Oakley]

I've confirmed this bug on another Win 7 (x64) PC. Previous fail was on my Win XP netbook.

The bug is the failure to unlink the .lock file of the start of the bundle. The passing of the bad ref is the trigger.


$ git bundle create test.bndl --all crap --since=10.weeks.ago
fatal: ambiguous argument 'crap': unknown revision or path not in the working tree.

Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error: rev-list died

Unlink of file 'C:/Users/Philip/Documents/xxx/test.bndl.lock' failed. Should I try again? (y/n) n
warning: unable to unlink C:/Users/Philip/Documents/xxx/test.bndl.lock: Invalid argument

$ git --version
git version 2.7.1.windows.1

Does it fail for others on their Win PC, and/or Linux?

Philip

> --
> You received this message because you are subscribed to the Google Groups
> "git-for-windows" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to git-for-windo...@googlegroups.com.
> To post to this group, send email to git-for...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/git-for-
> windows/2683039F1D034E518279328E7AD285E0%40PhilipOakley.
> For more options, visit https://groups.google.com/d/optout.
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2016.0.7497 / Virus Database: 4545/11898 - Release Date: 03/27/16

[Duy Nguyen]

On Mon, Mar 28, 2016 at 7:29 PM, Philip Oakley <philip...@iee.org> wrote:
> I've confirmed this bug on another Win 7 (x64) PC. Previous fail was on my Win XP netbook.
>
> The bug is the failure to unlink the .lock file of the start of the bundle. The passing of the bad ref is the trigger.
>
>
> $ git bundle create test.bndl --all crap --since=10.weeks.ago
> fatal: ambiguous argument 'crap': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> error: rev-list died
> Unlink of file 'C:/Users/Philip/Documents/xxx/test.bndl.lock' failed. Should I try again? (y/n) n
> warning: unable to unlink C:/Users/Philip/Documents/xxx/test.bndl.lock: Invalid argument
>
> $ git --version
> git version 2.7.1.windows.1
>
> Does it fail for others on their Win PC, and/or Linux?

If it helps, 2.8.0-rc0 (+ some stuff) on linux

$ git bundle create test.bndl --all crap --since=10.weeks.ago
fatal: ambiguous argument 'crap': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error: rev-list died

strace shows it unlinks ok

open("/home/pclouds/w/git/temp/test.bndl.lock", O_RDWR|O_CREAT|O_EXCL, 0666) = 3
...
unlink("/home/pclouds/w/git/temp/test.bndl.lock") = 0
--
Duy

[Johannes Schindelin]

Hi Philip,

On Sat, 26 Mar 2016, Philip Oakley wrote:

> Philip@PhilipOakley MINGW32 /c/git-sdk-32/usr/src/git (bundle1)
> $ git bundle create test.bndl HEAD bundle1 master next pu --since=2.weeks.ago
> fatal: ambiguous argument 'next': unknown revision or path not in the working
> tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> error: rev-list died
> Unlink of file 'C:/git-sdk-32/usr/src/git/test.bndl.lock' failed. Should I try
> again? (y/n) n
> warning: unable to unlink C:/git-sdk-32/usr/src/git/test.bndl.lock: Invalid
> argument

The reason is that we have technical debt in create_bundle() where it
duplicates a file handle to allow closing it twice (instead of teaching
the first code path *not* to close it).

In my tests, this hacky work-around (which you are welcome to use as a
starting point for a contribution) fixes the symptom:

-- snipsnap --
diff --git a/bundle.c b/bundle.c
index 506ac49..cc2db63 100644
--- a/bundle.c
+++ b/bundle.c
@@ -399,6 +399,16 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
return ref_count;
}

+static int bundle_fd_dup = -1;
+
+static void close_bundle_fd_dup(void)
+{
+ if (bundle_fd_dup >= 0) {
+ close(bundle_fd_dup);
+ bundle_fd_dup = -1;
+ }
+}
+
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
@@ -421,9 +431,10 @@ int create_bundle(struct bundle_header *header, const char *path,
* lockfile's fd. So make a copy of the file
* descriptor to avoid trying to close it twice.
*/
- bundle_fd = dup(bundle_fd);
+ bundle_fd_dup = bundle_fd = dup(bundle_fd);
if (bundle_fd < 0)
die_errno("unable to dup file descriptor");
+ atexit(close_bundle_fd_dup);
}

/* write signature */
@@ -455,6 +466,7 @@ int create_bundle(struct bundle_header *header, const
char *path,
return -1;

if (!bundle_to_stdout) {
+ bundle_fd_dup = -1;
if (commit_lock_file(&lock))
die_errno(_("cannot create '%s'"), path);
}

[Philip Oakley]

From: "Duy Nguyen" <pcl...@gmail.com>

Thanks;
That confirmed my expectation that this was an issue about the differences
between Windows and Linux.

There is a comment in the code about duplicating the file descriptor (fd)
because the writing of the pack into the bundle will auto-close it, and
other closings are needed. I suspect that its the way windows counts the
open/closing/fd's.

In addition the bundle code is clunky [1]. The <rev-list-arg> options should
be parsed for errors before the bundle file is opened. At the moment the
errors are detected deep in the code.

Junio has also been working on using bundles for the resumable clone (though
is going away from that idea).

I hope to have something "soon" about marking the ref that is HEAD withing
the pre-requisites so that the clone transport doen't have to guess. It may
be transferrable to the regular transport as well.
--
Philip

[1]
http://thread.gmane.org/gmane.comp.version-control.git/288080/focus=288111

[Philip Oakley]

From: "Johannes Schindelin" <Johannes....@gmx.de>

> Hi Philip,
>
> On Sat, 26 Mar 2016, Philip Oakley wrote:
>
>> Philip@PhilipOakley MINGW32 /c/git-sdk-32/usr/src/git (bundle1)
>> $ git bundle create test.bndl HEAD bundle1 master next
>> pu --since=2.weeks.ago
>> fatal: ambiguous argument 'next': unknown revision or path not in the
>> working
>> tree.
>> Use '--' to separate paths from revisions, like this:
>> 'git <command> [<revision>...] -- [<file>...]'
>> error: rev-list died
>> Unlink of file 'C:/git-sdk-32/usr/src/git/test.bndl.lock' failed. Should
>> I try
>> again? (y/n) n
>> warning: unable to unlink C:/git-sdk-32/usr/src/git/test.bndl.lock:
>> Invalid
>> argument
>
> The reason is that we have technical debt in create_bundle() where it
> duplicates a file handle to allow closing it twice (instead of teaching
> the first code path *not* to close it).

I had suspected as much. Hence the request for a quick Linux check.

I just replied to Duy's confirmation with a bit more detail.

I'll probably avoid the hack approach ;-)
My likely priority order is :
* Ignore it;
* Split/refactor the rev-list-args checking so the bundle isn't started in
this case;
* Learn the rev machinary and then bring bundle's usage up to date (I
need/want HEAD first).

latest hack (without tidy-up/rebase):
https://github.com/PhilipOakley/git/tree/bundle1

Thanks.
--

Philip