Re: Regression in patch add?

[Phillip Wood]

On 11/05/18 03:47, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> Yes, I think it probably makes sense to do that. Originally I didn't
>> count empty lines as context lines in case the user accidentally added
>> some empty lines at the end of the hunk but if 'git apply' does then I
>> think 'git add -p' should as well
> 
> I am not sure if "adding to the tail" should be tolerated, but in
> any case, newer GNU diff can show an empty unaffected line as an
> empty line (unlike traditional unified context format in which such
> a line is expressed as a line with a lone SP on it), which is
> allowed as "implementation defined" by POSIX.1 [*1*]. Modern "git
> apply" knows about this.

Thanks for the reference, I hadn't realized the space was optional.

> If "add -p" parses a patch, it should learn to do so, too.

I'm about to go off line for a while, I'll send a fix when I'm back up
and running at next month (unfortunately the reroll of pw/add-p-select
will have to wait until then as well)

Best Wishes

Phillip


> 
> [Reference]
> 
> *1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
> 
>>
>>> Meanwhile, I can easily configure my editor not to do this for `*.diff` 
>>> files.
>>>
>>> Thanks for your help, Phillip and Martin!
>>
>> Thanks for posting an example so we could test it, it makes it much
>> easier to track the problem down
>>
>> Best Wishes
>>
>> Phillip
>>
>>> Mahmoud, does this also explain your problem as per your original post?
>>>

Re: Regression in patch add?

[Junio C Hamano]

Phillip Wood  writes:

> Yes, I think it probably makes sense to do that. Originally I didn't
> count empty lines as context lines in case the user accidentally added
> some empty lines at the end of the hunk but if 'git apply' does then I
> think 'git add -p' should as well

I am not sure if "adding to the tail" should be tolerated, but in
any case, newer GNU diff can show an empty unaffected line as an
empty line (unlike traditional unified context format in which such
a line is expressed as a line with a lone SP on it), which is
allowed as "implementation defined" by POSIX.1 [*1*]. Modern "git
apply" knows about this.

If "add -p" parses a patch, it should learn to do so, too.


[Reference]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html

>
>> Meanwhile, I can easily configure my editor not to do this for `*.diff` 
>> files.
>> 
>> Thanks for your help, Phillip and Martin!
>
> Thanks for posting an example so we could test it, it makes it much
> easier to track the problem down
>
> Best Wishes
>
> Phillip
>
>> Mahmoud, does this also explain your problem as per your original post?
>> 

Re: Regression in patch add?

[Phillip Wood]

On 10/05/18 15:11, Oliver Joseph Ash wrote:
> You found the problem Phillip! My editor was trimming trailing white space, 
> which breaks the context line.

I'm glad we found the source of the problem (and that it wasn't some
obscure bug)

> I had tried to use an alternative editor to account for any editor specific 
> behaviour, but it turns out both the editors I tested in were doing this!
> 
> I suspect this change in behaviour will effect a lot of users? If so, it 
> would be good if `git add -p` allowed for this behaviour, in the same way 
> `git apply` does.

Yes, I think it probably makes sense to do that. Originally I didn't
count empty lines as context lines in case the user accidentally added
some empty lines at the end of the hunk but if 'git apply' does then I
think 'git add -p' should as well

> Meanwhile, I can easily configure my editor not to do this for `*.diff` files.
> 
> Thanks for your help, Phillip and Martin!

Thanks for posting an example so we could test it, it makes it much
easier to track the problem down

Best Wishes

Phillip

> Mahmoud, does this also explain your problem as per your original post?
> 

Re: Regression in patch add?

[Oliver Joseph Ash]

You found the problem Phillip! My editor was trimming trailing white space, 
which breaks the context line.

I had tried to use an alternative editor to account for any editor specific 
behaviour, but it turns out both the editors I tested in were doing this!

I suspect this change in behaviour will effect a lot of users? If so, it would 
be good if `git add -p` allowed for this behaviour, in the same way `git apply` 
does.

Meanwhile, I can easily configure my editor not to do this for `*.diff` files.

Thanks for your help, Phillip and Martin!

Mahmoud, does this also explain your problem as per your original post?

Re: Regression in patch add?

[Martin Ågren]

On 10 May 2018 at 15:16, Oliver Joseph Ash  wrote:
> (Apologies, I accidentally sent this as a reply to the original post, instead 
> of your email. I'm new to this!)

(No worries.) ;-)

>> does your test involve unusual file systems, funny characters in filenames, 
>> ..? You are on some sort of Linux, right?
>
> I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
> as I'm aware. The filename in my test case is simply `foo`.

I'm not too familiar with Mac, unfortunately, but let's see..

> I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
> and applying it, and it succeeded.
>
>> should now show bar2 in the first hunk and bar1 in the second hunk, just 
>> like your edited test.patch.
>
> That was the case, although I had to remove the `--check` flag from `git 
> apply`.

Hmm, you mean that `git apply --check test.patch` failed? With error
messages? Or, you had to remove the --check flag in order for the patch
to actually be applied on disk? I would guess it's the latter, but just
to be clear.

>> How comfortable are you with building Git from the sources?
>
> I've never done it before, but I assume it's well documented, so I'm willing 
> to give it a shot!
>
> Happy to try any steps to debug this! Although I'm a bit surprised no-one 
> else can reproduce it with the same version of Git, which makes it seem less 
> likely this could be a bug, and more likely it's something in my setup.

Where do the git 2.17.0 and 2.16.2 come from that you have been testing?
Homebrew? Apple? (Ple

So you should be able to do `git clone https://github.com/git/git.git`
and read INSTALL. It might be useful to start with `git checkout
v2.17.0` to make sure you're testing roughly the same thing as before.

As for obtaining the dependencies, since I'm not familiar with Mac, I
cannot give any good hints.

I see now that Phillip has replied with a good guess. Let's hope he
has managed to circle in on what's causing your problem.

Martin

Re: Regression in patch add?

[Phillip Wood]

On 10/05/18 13:17, Martin Ågren wrote:
> 
> On 10 May 2018 at 12:41, Oliver Joseph Ash  wrote:
>> I just ran into a similar problem: 
>> https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks
>>
>> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.
>>
>> Is this a bug?
> 
> I would think so. Thanks for finding this thread. To keep history
> around, it would be nice to have your reproduction recipe on the list,
> not just on stackoverflow. That said, I cannot reproduce on v2.17.0
> using your recipe. I suspect there is something quite interesting going
> on here, considering how trivial your edit is.

Thanks Oliver for posting an example that we can test, that said I can't
reproduce it on Linux if the hunk is edited correctly. However if I
remove the leading space from the empty line between 'baz' and 'foo'
then I get the same error as you. Perhaps your editor is stripping
trailing white space? If so that will lead to problems when editing
diffs as the leading space is needed for apply to know that it's an
empty context line.

For the mailing list the hunk in question looks like
@@ -1,5 +1,5 @@
 foo
-bar
+bar1
 baz

 foo

I've tried using 'git apply --recount --cached' directly and was
surprised to see that it accepts the patch with the broken context line.
In 2.17.0 'add -p' no longer uses the --recount option, instead it
counts the patch it's self but stops counting when it runs out of lines
starting with [- +], this explains the difference from earlier versions.
It seems it's not uncommon for editors to strip the space from empty
context lines so maybe 'add -p' should take that into account when
recounting patches. I'm about to go off line for a couple of weeks so it
will probably be next month before I'm able to put a patch together
(assuming Junio agrees we should support broken hunks)

Best Wishes

Phillip


> As a shot in the dark, does your test involve unusual file systems,
> funny characters in filenames, ..? You are on some sort of Linux, right?
> 
> The first thing to try out might be something like
> 
> $ # create the initial file as before, with "bar"
> $ # git add, git commit ...
> $ # do the "change bar to bar1" everywhere
> $ git diff >test-patch
> $ git reset --hard
> $ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)
> $ git apply --check test.patch && echo "ok..."
> $ git apply test.patch
> 
> Does that succeed at all?
> 
> $ git diff
> 
> should now show bar2 in the first hunk and bar1 in the second hunk,
> just like your edited test.patch.
> 
> If that works, it would seem that the problem is with `git add -p`, and
> how it is generating the patches for `git apply`. I have some ideas
> about how to debug from there, but ... How comfortable are you with
> building Git from the sources? Or with temporarily fiddling around with
> your Git installation? (git-add--interactive is a Perl script, so it
> would be possible to edit it in place to emit various debug
> information. That has potential for messing up royally, though.)
> 
> Martin
> 

Re: Regression in patch add?

[Oliver Joseph Ash]

(Apologies, I accidentally sent this as a reply to the original post, instead 
of your email. I'm new to this!)

> does your test involve unusual file systems, funny characters in filenames, 
> ..? You are on some sort of Linux, right?

I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
as I'm aware. The filename in my test case is simply `foo`.

I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
and applying it, and it succeeded.

> should now show bar2 in the first hunk and bar1 in the second hunk, just like 
> your edited test.patch.

That was the case, although I had to remove the `--check` flag from `git apply`.

> How comfortable are you with building Git from the sources?

I've never done it before, but I assume it's well documented, so I'm willing to 
give it a shot!

Happy to try any steps to debug this! Although I'm a bit surprised no-one else 
can reproduce it with the same version of Git, which makes it seem less likely 
this could be a bug, and more likely it's something in my setup.

Re: Regression in patch add?

[Oliver Joseph Ash]

> does your test involve unusual file systems, funny characters in filenames, 
> ..? You are on some sort of Linux, right?

I'm running macOS 10.13.4. I don't have any unusual file system setup, as far 
as I'm aware. The filename in my test case is simply `foo`.

I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, 
and applying it, and it succeeded.

> should now show bar2 in the first hunk and bar1 in the second hunk, just like 
> your edited test.patch.

That was the case, although I had to remove the `--check` flag from `git apply`.

> How comfortable are you with building Git from the sources?

I've never done it before, but I assume it's well documented, so I'm willing to 
give it a shot!

Happy to try any steps to debug this! Although I'm a bit surprised no-one else 
can reproduce it with the same version of Git, which makes it seem less likely 
this could be a bug, and more likely it's something in my setup.

Re: Regression in patch add?

[Martin Ågren]

On 10 May 2018 at 12:41, Oliver Joseph Ash  wrote:
> I just ran into a similar problem: 
> https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks
>
> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.
>
> Is this a bug?

I would think so. Thanks for finding this thread. To keep history
around, it would be nice to have your reproduction recipe on the list,
not just on stackoverflow. That said, I cannot reproduce on v2.17.0
using your recipe. I suspect there is something quite interesting going
on here, considering how trivial your edit is.

As a shot in the dark, does your test involve unusual file systems,
funny characters in filenames, ..? You are on some sort of Linux, right?

The first thing to try out might be something like

$ # create the initial file as before, with "bar"
$ # git add, git commit ...
$ # do the "change bar to bar1" everywhere
$ git diff >test-patch
$ git reset --hard
$ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)
$ git apply --check test.patch && echo "ok..."
$ git apply test.patch

Does that succeed at all?

$ git diff

should now show bar2 in the first hunk and bar1 in the second hunk,
just like your edited test.patch.

If that works, it would seem that the problem is with `git add -p`, and
how it is generating the patches for `git apply`. I have some ideas
about how to debug from there, but ... How comfortable are you with
building Git from the sources? Or with temporarily fiddling around with
your Git installation? (git-add--interactive is a Perl script, so it
would be possible to edit it in place to emit various debug
information. That has potential for messing up royally, though.)

Martin

Re: Regression in patch add?

[Oliver Joseph Ash]

I just ran into a similar problem: 
https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks

I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.

Is this a bug?

Re: Regression in patch add?

[Phillip Wood]

On 15/04/18 14:59, Martin Ågren wrote:
> Hi Mahmoud
> 
> On 15 April 2018 at 14:21,   wrote:
>> I first run `git add -p`, then manually edit a chunk (after hitting `s`
>> once, if it matters). The chunk originally contains the following:
> 
> [...]
> 
>> Under git 2.7.4, I can edit it to the following, which is accepted
>> without a problem:
>>
>> ```diff
>> # Manual hunk edit mode -- see bottom for a quick guide
>> @@ -20,7 +20,7 @@
>> "call dein#add('Shougo/dein.vim', {'rev': 'master'})
>>
>> " Add or remove your plugins here:
>> -   " call dein#add('flazz/vim-colorschemes')
>> -   call dein#add('Haron-Prime/evening_vim')
>> +   call dein#add('flazz/vim-colorschemes')
>> +   call dein#add('Haron-Prime/evening_vim')
>>
>> "core plugins that change the behavior of vim and how we use it 
>> globally
>> ```
>>
>> All I did here was remove one `+` line and manually add another (which
>> is a variant of the second `-` line).
> 
> So the line is identical (sans s/^-/+/). Interesting.
> 
>> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for
>> editing (and if left unmodified applies OK), but when modified in the
>> to the same exact value, after exiting the editor I receive the
>> following error from git:
>>
>> error: patch fragment without header at line 15: @@ -25,7 +25,8 @@
> 
> I can't seem to reproduce this with some very simple testing. Are you
> able to share your files? Or even better, derive a minimal reproduction
> recipe?
> 
> What happens if you do not do a "remove this line, then add it again",
> but instead turn that unchanged line into context? That is, you edit the
> hunk into something like this (but without white-space damage):
> 
> ...
> -   " call dein#add('flazz/vim-colorschemes')
> +   call dein#add('flazz/vim-colorschemes')
> call dein#add('Haron-Prime/evening_vim')
> ...

That's a good idea to try

> Adding Phillip to cc, since he was recently working in this area

Thanks for cc-ing me

> and might have an idea.

I wish I did!

Best Wishes

Phillip
> 
> Martin
> 

Re: Regression in patch add?

[Phillip Wood]

On 15/04/18 13:21, mqudsi wrote:
> 
> Hello all,
> 
> I'm currently running the latest version of git built from `master`, and
> I'm running into what appears to be a regression in the behavior of the
> piecewise `git add -p` when applying a manually edited chunk.
> 
> I first run `git add -p`, then manually edit a chunk (after hitting `s`
> once, if it matters).

Thanks for mentioning that, it can matter as the code that stitches
split hunks back together can't cope with edited hunks properly (though
the code that checks the hunk immediately after it's been edited doesn't
bother to try and stitch things back together).

> The chunk originally contains the following:
> 
> ```diff
> # Manual hunk edit mode -- see bottom for a quick guide
> @@ -20,7 +20,7 @@
>   "call dein#add('Shougo/dein.vim', {'rev': 'master'})
> 
>   " Add or remove your plugins here:
> - " call dein#add('flazz/vim-colorschemes')
> - call dein#add('Haron-Prime/evening_vim')
> + call dein#add('flazz/vim-colorschemes')
> + call dein#add('danilo-augusto/vim-afterglow')
> 
>   "core plugins that change the behavior of vim and how we use it globally
> ```
> 
> Under git 2.7.4, I can edit it to the following, which is accepted
> without a problem:
> 
> ```diff
> # Manual hunk edit mode -- see bottom for a quick guide
> @@ -20,7 +20,7 @@
>   "call dein#add('Shougo/dein.vim', {'rev': 'master'})
> 
>   " Add or remove your plugins here:
> - " call dein#add('flazz/vim-colorschemes')
> - call dein#add('Haron-Prime/evening_vim')
> + call dein#add('flazz/vim-colorschemes')
> + call dein#add('Haron-Prime/evening_vim')
> 
>   "core plugins that change the behavior of vim and how we use it globally
> ```
> 
> All I did here was remove one `+` line and manually add another (which
> is a variant of the second `-` line).
> 
> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for
> editing (and if left unmodified applies OK), but when modified in the
> to the same exact value, after exiting the editor I receive the
> following error from git:
> 
> error: patch fragment without header at line 15: @@ -25,7 +25,8 @@

I'm not quite sure what that error message is telling us, I need to
spend some time understanding the code in apply.c that creates this
error message.

I assume that the header is coming from the next hunk which was created
when you split the original hunk. If you could post the original hunk
before it was split and the hunk starting at line 25 after it was split
that might help.

As Martin said if you could share the files or come up with a
reproducible example that would really help in figuring out what is
going wrong.

Thanks for reporting this

Phillip


> I'm not sure what to make of this.
> 
> Thank you,
> 
> Mahmoud Al-Qudsi
> NeoSmart Technologies
> 
> 

Re: Regression in patch add?

[Martin Ågren]

Hi Mahmoud

On 15 April 2018 at 14:21,   wrote:
> I first run `git add -p`, then manually edit a chunk (after hitting `s`
> once, if it matters). The chunk originally contains the following:

[...]

> Under git 2.7.4, I can edit it to the following, which is accepted
> without a problem:
>
> ```diff
> # Manual hunk edit mode -- see bottom for a quick guide
> @@ -20,7 +20,7 @@
> "call dein#add('Shougo/dein.vim', {'rev': 'master'})
>
> " Add or remove your plugins here:
> -   " call dein#add('flazz/vim-colorschemes')
> -   call dein#add('Haron-Prime/evening_vim')
> +   call dein#add('flazz/vim-colorschemes')
> +   call dein#add('Haron-Prime/evening_vim')
>
> "core plugins that change the behavior of vim and how we use it 
> globally
> ```
>
> All I did here was remove one `+` line and manually add another (which
> is a variant of the second `-` line).

So the line is identical (sans s/^-/+/). Interesting.

> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for
> editing (and if left unmodified applies OK), but when modified in the
> to the same exact value, after exiting the editor I receive the
> following error from git:
>
> error: patch fragment without header at line 15: @@ -25,7 +25,8 @@

I can't seem to reproduce this with some very simple testing. Are you
able to share your files? Or even better, derive a minimal reproduction
recipe?

What happens if you do not do a "remove this line, then add it again",
but instead turn that unchanged line into context? That is, you edit the
hunk into something like this (but without white-space damage):

...
-   " call dein#add('flazz/vim-colorschemes')
+   call dein#add('flazz/vim-colorschemes')
call dein#add('Haron-Prime/evening_vim')
...

Adding Phillip to cc, since he was recently working in this area and
might have an idea.

Martin

Regression in patch add?

[mqudsi]

Hello all,

I'm currently running the latest version of git built from `master`, and
I'm running into what appears to be a regression in the behavior of the
piecewise `git add -p` when applying a manually edited chunk.

I first run `git add -p`, then manually edit a chunk (after hitting `s`
once, if it matters). The chunk originally contains the following:

```diff
# Manual hunk edit mode -- see bottom for a quick guide
@@ -20,7 +20,7 @@
"call dein#add('Shougo/dein.vim', {'rev': 'master'})

" Add or remove your plugins here:
-   " call dein#add('flazz/vim-colorschemes')
-   call dein#add('Haron-Prime/evening_vim')
+   call dein#add('flazz/vim-colorschemes')
+   call dein#add('danilo-augusto/vim-afterglow')

"core plugins that change the behavior of vim and how we use it globally
```

Under git 2.7.4, I can edit it to the following, which is accepted
without a problem:

```diff
# Manual hunk edit mode -- see bottom for a quick guide
@@ -20,7 +20,7 @@
"call dein#add('Shougo/dein.vim', {'rev': 'master'})

" Add or remove your plugins here:
-   " call dein#add('flazz/vim-colorschemes')
-   call dein#add('Haron-Prime/evening_vim')
+   call dein#add('flazz/vim-colorschemes')
+   call dein#add('Haron-Prime/evening_vim')

"core plugins that change the behavior of vim and how we use it globally
```

All I did here was remove one `+` line and manually add another (which
is a variant of the second `-` line).

Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for
editing (and if left unmodified applies OK), but when modified in the
to the same exact value, after exiting the editor I receive the
following error from git:

error: patch fragment without header at line 15: @@ -25,7 +25,8 @@

I'm not sure what to make of this.

Thank you,

Mahmoud Al-Qudsi
NeoSmart Technologies