Re: git apply vs patch -p1

Folks,

Lately I've been running into a lot of reports of false conflicts
reported by "git apply". The most recent one was the "points" patch,
which git apply rejected for completely ficticious reasons (it claimed
that the patch was trying to create a new file where a file already
existed, which it wasn't).

I think we should modify the patch review and developer instructions to
recommend always using patch -p1 (or -p0, depending), even if the patch
was produced with "git diff".

Thoughts?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 09/14/2013 02:37 PM, Josh Berkus wrote:
> Folks,
>
> Lately I've been running into a lot of reports of false conflicts
> reported by "git apply". The most recent one was the "points" patch,
> which git apply rejected for completely ficticious reasons (it claimed
> that the patch was trying to create a new file where a file already
> existed, which it wasn't).
>
> I think we should modify the patch review and developer instructions to
> recommend always using patch -p1 (or -p0, depending), even if the patch
> was produced with "git diff".
>
> Thoughts?
>

FWIW that's what I invariably use.

You do have to be careful to git-add/git-rm any added/deleted files,
which git-apply does for you (as well as renames) - I've been caught by
that a couple of times.

cheers

andrew

On 2013-09-14 15:03:52 -0400, Andrew Dunstan wrote:
>
> On 09/14/2013 02:37 PM, Josh Berkus wrote:
> >Folks,
> >
> >Lately I've been running into a lot of reports of false conflicts
> >reported by "git apply". The most recent one was the "points" patch,
> >which git apply rejected for completely ficticious reasons (it claimed
> >that the patch was trying to create a new file where a file already
> >existed, which it wasn't).
> >
> >I think we should modify the patch review and developer instructions to
> >recommend always using patch -p1 (or -p0, depending), even if the patch
> >was produced with "git diff".
> >
> >Thoughts?
> >
>
>
> FWIW that's what I invariably use.
>
> You do have to be careful to git-add/git-rm any added/deleted files, which
> git-apply does for you (as well as renames) - I've been caught by that a
> couple of times.

git reset?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/14/2013 03:08 PM, Andres Freund wrote:
> On 2013-09-14 15:03:52 -0400, Andrew Dunstan wrote:
>> On 09/14/2013 02:37 PM, Josh Berkus wrote:
>>> Folks,
>>>
>>> Lately I've been running into a lot of reports of false conflicts
>>> reported by "git apply". The most recent one was the "points" patch,
>>> which git apply rejected for completely ficticious reasons (it claimed
>>> that the patch was trying to create a new file where a file already
>>> existed, which it wasn't).
>>>
>>> I think we should modify the patch review and developer instructions to
>>> recommend always using patch -p1 (or -p0, depending), even if the patch
>>> was produced with "git diff".
>>>
>>> Thoughts?
>>>
>>
>> FWIW that's what I invariably use.
>>
>> You do have to be careful to git-add/git-rm any added/deleted files, which
>> git-apply does for you (as well as renames) - I've been caught by that a
>> couple of times.
> git reset?
>
>

Yes, of course you can roll back as long as you haven't published your
commits. But it's a nuisance.

cheers

andrew

On Sat, 2013-09-14 at 11:37 -0700, Josh Berkus wrote:
> Lately I've been running into a lot of reports of false conflicts
> reported by "git apply". The most recent one was the "points" patch,
> which git apply rejected for completely ficticious reasons (it claimed
> that the patch was trying to create a new file where a file already
> existed, which it wasn't)

Every file in that patch contains

new file mode 100644

which means it is creating a new file. I would review how that patch
was created.

On Sun, Sep 15, 2013 at 12:38 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-09-14 15:03:52 -0400, Andrew Dunstan wrote:
> >
> > On 09/14/2013 02:37 PM, Josh Berkus wrote:
> > >Folks,
> > >
> > >Lately I've been running into a lot of reports of false conflicts
> > >reported by "git apply". The most recent one was the "points" patch,
> > >which git apply rejected for completely ficticious reasons (it claimed
> > >that the patch was trying to create a new file where a file already
> > >existed, which it wasn't).
> > >
> > >I think we should modify the patch review and developer instructions to
> > >recommend always using patch -p1 (or -p0, depending), even if the patch
> > >was produced with "git diff".
> > >
> > >Thoughts?
> > >
> >
> >
> > FWIW that's what I invariably use.
> >
> > You do have to be careful to git-add/git-rm any added/deleted files,
> which
> > git-apply does for you (as well as renames) - I've been caught by that a
> > couple of times.
>
> git reset?
>
>
git reset wouldn't remove the files that were newly added by the patch,
would it?

> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2013-09-16 10:16:37 +0530, Ashutosh Bapat wrote:
> On Sun, Sep 15, 2013 at 12:38 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2013-09-14 15:03:52 -0400, Andrew Dunstan wrote:
> > >
> > > On 09/14/2013 02:37 PM, Josh Berkus wrote:
> > > >Folks,
> > > >
> > > >Lately I've been running into a lot of reports of false conflicts
> > > >reported by "git apply". The most recent one was the "points" patch,
> > > >which git apply rejected for completely ficticious reasons (it claimed
> > > >that the patch was trying to create a new file where a file already
> > > >existed, which it wasn't).
> > > >
> > > >I think we should modify the patch review and developer instructions to
> > > >recommend always using patch -p1 (or -p0, depending), even if the patch
> > > >was produced with "git diff".
> > > >
> > > >Thoughts?
> > > >
> > >
> > >
> > > FWIW that's what I invariably use.
> > >
> > > You do have to be careful to git-add/git-rm any added/deleted files,
> > which
> > > git-apply does for you (as well as renames) - I've been caught by that a
> > > couple of times.
> >
> > git reset?
> >
> >
> git reset wouldn't remove the files that were newly added by the patch,
> would it?

Depends on how you do it. I simply commit patches I look at - then they
can easily be removed using git reset --hard HEAD^. And it allows to
make further changes/annotations during review that are clearly
separated from the patch.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/15/2013 11:46 PM, Ashutosh Bapat wrote:
> On Sun, Sep 15, 2013 at 12:38 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>> git reset?
>>
>>
> git reset wouldn't remove the files that were newly added by the patch,
> would it?

The issue isn't that, it's that git apply is just buggy and can't tell
the difference between a new file and a modified one.

The "points" patch contained no new files, just modifications. But for
some reason, git apply read it as being all new files, which failed.

"patch -p1" worked fine.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

>>>>> "Josh" == Josh Berkus <josh(at)agliodbs(dot)com> writes:

Josh> The issue isn't that, it's that git apply is just buggy and
Josh> can't tell the difference between a new file and a modified
Josh> one.

It's not the fault of git apply; the patch contained explicit
annotations on all the files claiming that they were new. Both the
patches I've looked at so far (picksplit NaNs and enable_material)
had the same defect.

The question is, how are these submitters preparing their patches?

--
Andrew (irc:RhodiumToad)

On Mon, Sep 16, 2013 at 12:11 PM, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk
> wrote:

> >>>>> "Josh" == Josh Berkus <josh(at)agliodbs(dot)com> writes:
>
> Josh> The issue isn't that, it's that git apply is just buggy and
> Josh> can't tell the difference between a new file and a modified
> Josh> one.
>
> It's not the fault of git apply; the patch contained explicit
> annotations on all the files claiming that they were new. Both the
> patches I've looked at so far (picksplit NaNs and enable_material)
> had the same defect.
>
> The question is, how are these submitters preparing their patches?
>

I used "git diff" configured to use src/tools/git-external-diff, as
described here:

http://wiki.postgresql.org/wiki/Working_with_Git

The resulting patch applies fine with patch, but not with git apply.

If I instead generate a patch with git diff --no-ext-diff, then it applies
with git apply.

Cheers,

Jeff

>>>>> "Jeff" == Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:

Jeff> I used "git diff" configured to use
Jeff> src/tools/git-external-diff, as described here:

hmm...

so that git-external-diff script is trying to fake git diff output,
including using 'diff -git' and index lines, but the context-diff
output wouldn't work with git apply even if the header lines were
correct (as far as I can see git apply accepts only git's unified-diff
format - the git people clearly have no truck with context diffs).

--
Andrew (irc:RhodiumToad)