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

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

[Carlos Martín Nieto]

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Junio C Hamano]

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

> Whilst trying to do some work related to fetch, I came across a
> regression in the 'next' branch....
>
> $ ./git-fetch git://repo.or.cz/git

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

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

[Jeff King]

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

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

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

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

[Carlos Martín Nieto]

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

On Wed, 2011-09-28 at 14:53 -0400, Jeff King wrote:
> On Wed, Sep 28, 2011 at 06:04:27PM +0200, Carlos Martín Nieto wrote:
> 
> > 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... ]
> 
> 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?

Yes, this seems indeed to be the case.

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

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Jakub Narebski]

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

> 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

Have you tried

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

?

-- 
Jakub Narębski

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

[Carlos Martín Nieto]

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

On Wed, 2011-09-28 at 12:12 -0700, Jakub Narebski wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > 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
> 
> Have you tried
> 
>   $ ./git fetch git://repo.or.cz/git

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

[Jeff King]

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

> > Have you tried
> > 
> >   $ ./git fetch git://repo.or.cz/git
> 
> 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.

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.

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

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

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

[Philip Oakley]

From: "Jeff King" <peff@peff.net>
> On Sat, Oct 01, 2011 at 01:54:08AM +0200, Carlos Martín Nieto wrote:
>
>> > Have you tried
>> >
>> >   $ ./git fetch git://repo.or.cz/git
>>
>> 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.
>
> 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.
>
>> I guess I'll have to either properly install git from 'next' or base my
>> changed on 'maint'
>
> 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

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 

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

[Jeff King]

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

> 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

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