Re: bug: git-bundle create foo --stdin -> segfault

Hi,

On Mon, 18 Jan 2010, Joey Hess wrote:

Current 'next' fails, too.

Some previous Git versions failed with a message like this:

error: unrecognized argument: --stdin'

Other previous Git versions failed at least with a message such as this:

fatal: exec rev-list failed. error: rev-list died

Something similar happens to me when I run the initial official revision of git-bundle. The reason is that "git rev-list --boundary --pretty=oneline --stdin" refuses to run.

The bad versions either segfault, or "refuse to create empty bundles".

And while 8b3dce5 purports to clean things up (even acknowledging that support code for --stdin was removed from bundle.c!), at that time git-bundle was obviously not tested/fixed.

Now, I invested a lot of time into the new Git wiki, and into trying to bisect this (it took many, many more steps than the suggested 13, and somewhere in between, the number of commits to be tested even increased!).

If you want to fix it, I suggest requiring --stdin to be the only parameter after the bundle file name, and adding a function using strbuf_getline() to parse the stdin into a string_list. Once that is done, you can substitute the argv for the rev-list call with that list (for that, you need to prepopulate with "rev-list", "--boundary" and "--pretty=oneline"). You can reuse that list for the call to setup_revisions().

Alternatively, you can try to implement the rev-list --boundary by hand (the --pretty=oneline is only needed to get a boundary marker IIRC), taking care to reset the commit flags that were set in the process. (We need to know the boundary commits before actually starting to write the pack, because the bundle file format dictates that the boundary commits are listed as prerequsites in the bundle header.)

If you want to go that route (which is arguably more elegant anyway), I suggest having a look at the merge_bases() and get_merge_bases() functions in commit.c, which do something similar (i.e. a revwalk without using revision.c's functions -- because you cannot tell what flags they will use in the future, and they have to be reset after the walk).

Ciao, Dscho