6d4bb3833c3d2114d (fetch: verify we have everything we need before updating our ref) breaks fetch

Hello,

Whilst trying to do some work related to fetch, I came across a regression in the 'next' branch. Bisecting gave me this commit as breaking point (and I tried with the parent and there it worked). When doing 'git fetch', rev-list will complain about usage, and fetch will say that we didn't receive enough, even though earlier versions of git have no problems. This fails both on github and on git.or.cz and for git and http transports:

$ ./git-fetch git://repo.or.cz/git
usage: git rev-list [OPTION] <commit-id>... [ -- paths... ]
  limiting output:
    --max-count=<n>
    --max-age=<epoch>
    --min-age=<epoch>
    --sparse
    --no-merges
    --min-parents=<n>
    --no-min-parents
    --max-parents=<n>
    --no-max-parents
    --remove-empty
    --all
    --branches
    --tags
    --remotes
    --stdin
    --quiet
  ordering output:
    --topo-order
    --date-order
    --reverse
  formatting output:
    --parents
    --children
    --objects | --objects-edge
    --unpacked
    --header | --pretty
    --abbrev=<n> | --no-abbrev
    --abbrev-commit
    --left-right
  special purpose:
    --bisect
    --bisect-vars
    --bisect-all
error: git://repo.or.cz/git did not send all necessary objects

Carlos Martín Nieto <cmn@elego.de> writes:

That invocation of ./git-fetch looks suspicious.

Are you sure that it internally invokes ./git-rev-list from the same version that knows --verify-objects option (you just built in your current directory), or is it invoking an old git-rev-list that is installed and is reachable from your usual $PATH, which does not know that option yet?

When I try a new version that was just built in my current directory, here is an incantation I use:

    GIT_EXEC_PATH=`pwd`
    PATH=`pwd`:/usr/bin:/bin
    GITPERLLIB=`pwd`/perl/blib/lib

    export GIT_EXEC_PATH PATH GITPERLLIB

On Wed, Sep 28, 2011 at 06:04:27PM +0200, Carlos Martín Nieto wrote:

Hmm. I notice you're running a not-installed version of fetch. Might this be a problem with a new git fetch running an older, installed version of rev-list?

The commit you mention calls rev-list with --verify-objects, but that feature is only added in the parent commit. So I can reproduce your issue with:

  $ git checkout 6d4bb38~2 ;# or anything before --verify-objects
  $ make install
  $ git checkout 6d4bb38
  $ make
  $ ./git-fetch git://repo.or.cz/git

but this works (because it sets the exec path properly):

  $ ./bin-wrappers/git fetch git://repo.or.cz/git

as does this:

  $ make install
  $ ./git-fetch git://repo.or.cz/git

So I don't think there's a bug. It's just that running compiled programs straight out of the build directory isn't supported. It works _most_ of the time, but as you can see, you may end up calling older, installed versions of git. The bin-wrappers scripts set up the exec path properly to let you test.

-Peff

On Wed, 2011-09-28 at 14:53 -0400, Jeff King wrote:

Yes, this seems indeed to be the case.

Indeed, as both you and Junio pointed out (within three minutes of each other :) I was running git from the build directory and expected it to work, as I was testing a few changed I had made to the fetch code.

Mea culpa, I tend to forget that git tends to behave like a bunch of shell scripts that happen to be written in C. Thanks to both.

   cmn

Carlos Martín Nieto <cmn@elego.de> writes:

Have you tried

  $ ./git fetch git://repo.or.cz/git

?

On Wed, 2011-09-28 at 12:12 -0700, Jakub Narebski wrote:

But this would execute /usr/local/libexec/git-fetch, wouldn't it? That is precisely what I don't want to execute, because I changed some code in builtin/fetch.c that I want to test.

But yes, the problem was that the local git-fetch was trying to pass an option to rev-list that my older installed binary didn't understand. In this particular case I don't want to run the older git-fetch, but otherwise, that would work.

I guess I'll have to either properly install git from 'next' or base my changed on 'maint'

   cmn

On Sat, Oct 01, 2011 at 01:54:08AM +0200, Carlos Martín Nieto wrote:

No, but only because fetch is a builtin. However, it still doesn't set up exec_path correctly, so your rev-list problem would not go away.

Just use bin-wrappers/git. That's exactly what it's there for (and it's what the test scripts use to make sure we are testing what is compiled).

Your change isn't the problem; only your testing method.

-Peff

From: "Jeff King" <peff@peff.net>

Peff,

Is there a write up of the the git testing method and how to use bin-wrappers etc. I didn't see anything in the Documentation, but I may not have looked carefully enough

Philip 

On Sat, Oct 01, 2011 at 11:38:08AM +0100, Philip Oakley wrote:

Bin-wrappers (and the alternative, which is setting up the exec-path yourself) are mentioned briefly in INSTALL. Running tests is described in t/README.

Other than that, you're left on your own to read the code and the commit messages. :)

-Peff