[7/7] apply --allow-overlap: fix a corner case
diff mbox series

Message ID 15c4cce5a027d56c7ddbe5523cf0f3beabd06ed7.1575637705.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • add -i: close some regression test gaps
Related show

Commit Message

Heba Waly via GitGitGadget Dec. 6, 2019, 1:08 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Yes, yes, this is supposed to be only a band-aid option for `git add -p`
not Doing The Right Thing. But as long as we carry the `--allow-overlap`
option, we might just as well get it right.

This fixes the case where one hunk inserts a line before the first line,
and is followed by a hunk whose context overlaps with the first one's
and which appends a line at the end.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 apply.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Junio C Hamano Dec. 6, 2019, 1:45 p.m. UTC | #1
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Yes, yes, this is supposed to be only a band-aid option for `git add -p`
> not Doing The Right Thing. But as long as we carry the `--allow-overlap`
> option, we might just as well get it right.

It probably depends on the definition of "right", where it may not
even exist (otherwise it wouldn't be a band-aid but be a real
feature) ;-)

> This fixes the case where one hunk inserts a line before the first line,
> and is followed by a hunk whose context overlaps with the first one's
> and which appends a line at the end.

The in-code comment makes me wonder if we need to further loosen the
check in the other direction, though.  What makes begin more special
than end?  Can a hunk be seen that pretends to extend to the end but
no longer does because there was an overlapping hunk that has been
wiggled in?

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  apply.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index f8a046a6a5..720a631eaa 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,
>  	unsigned long backwards, forwards, current;
>  	int backwards_lno, forwards_lno, current_lno;
>  
> +	/*
> +	 * When running with --allow-overlap, it is possible that a hunk is
> +	 * seen that pretends to start at the beginning (but no longer does),
> +	 * and that *still* needs to match the end. So trust `match_end` more
> +	 * than `match_beginning`.
> +	 */
> +	if (state->allow_overlap && match_beginning && match_end &&
> +	    img->nr - preimage->nr != 0)
> +		match_beginning = 0;
> +
>  	/*
>  	 * If match_beginning or match_end is specified, there is no
>  	 * point starting from a wrong line that will never match and
Johannes Schindelin Dec. 6, 2019, 2:11 p.m. UTC | #2
Hi Junio,

On Fri, 6 Dec 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Yes, yes, this is supposed to be only a band-aid option for `git add -p`
> > not Doing The Right Thing. But as long as we carry the `--allow-overlap`
> > option, we might just as well get it right.
>
> It probably depends on the definition of "right", where it may not
> even exist (otherwise it wouldn't be a band-aid but be a real
> feature) ;-)

Indeed. My hope is that we can get rid of it once the scripted
`git-add--interactive.perl` is removed in favor of the built-in add -i/-p.
This is a long way off, of course.

> > This fixes the case where one hunk inserts a line before the first line,
> > and is followed by a hunk whose context overlaps with the first one's
> > and which appends a line at the end.
>
> The in-code comment makes me wonder if we need to further loosen the
> check in the other direction, though.  What makes begin more special
> than end?  Can a hunk be seen that pretends to extend to the end but
> no longer does because there was an overlapping hunk that has been
> wiggled in?

The beginning is more special than the end because it is associated with
the line number 1. The end line number is flexible already.

There is another difference: after splitting hunks, the first hunk is
applied first, and may render the line numbers of succeeding hunks
incorrect. The same is not true for the last hunk: it cannot render the
preceding hunks' line numbers incorrect, as it has not been applied yet.

I think that's why `--allow-overlap` works with this patch when adding
lines both to the beginning and to the end after splitting a single hunk.

Ciao,
Dscho

> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  apply.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/apply.c b/apply.c
> > index f8a046a6a5..720a631eaa 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -2661,6 +2661,16 @@ static int find_pos(struct apply_state *state,
> >  	unsigned long backwards, forwards, current;
> >  	int backwards_lno, forwards_lno, current_lno;
> >
> > +	/*
> > +	 * When running with --allow-overlap, it is possible that a hunk is
> > +	 * seen that pretends to start at the beginning (but no longer does),
> > +	 * and that *still* needs to match the end. So trust `match_end` more
> > +	 * than `match_beginning`.
> > +	 */
> > +	if (state->allow_overlap && match_beginning && match_end &&
> > +	    img->nr - preimage->nr != 0)
> > +		match_beginning = 0;
> > +
> >  	/*
> >  	 * If match_beginning or match_end is specified, there is no
> >  	 * point starting from a wrong line that will never match and
>
Junio C Hamano Dec. 6, 2019, 4:40 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The beginning is more special than the end because it is associated with
> the line number 1. The end line number is flexible already.

Yeah, we do not insist "the lineno must be X" at the end like we do
at the beginning, but we still insist "there cannot be no post
context if we are adding at the end" just like there cannot be any
pre context for a patch that adds at the beginning, no?

> There is another difference: after splitting hunks, the first hunk is
> applied first, and may render the line numbers of succeeding hunks
> incorrect. The same is not true for the last hunk: it cannot render the
> preceding hunks' line numbers incorrect, as it has not been applied yet.

This truly may make quite a difference, especially because the hunks
are applied in order.

Thanks.
Johannes Schindelin Dec. 6, 2019, 5:56 p.m. UTC | #4
Hi Junio,

On Fri, 6 Dec 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The beginning is more special than the end because it is associated with
> > the line number 1. The end line number is flexible already.
>
> Yeah, we do not insist "the lineno must be X" at the end like we do
> at the beginning, but we still insist "there cannot be no post
> context if we are adding at the end" just like there cannot be any
> pre context for a patch that adds at the beginning, no?

True.

> > There is another difference: after splitting hunks, the first hunk is
> > applied first, and may render the line numbers of succeeding hunks
> > incorrect. The same is not true for the last hunk: it cannot render the
> > preceding hunks' line numbers incorrect, as it has not been applied yet.
>
> This truly may make quite a difference, especially because the hunks
> are applied in order.

I think you're right, I fooled myself into believing that the line number
1 is special, but the real culprit is that the second hunk is applied
_after_ the first one may have changed the (overlapping) context. The same
is not true for the second-to-last hunk: it will always be applied before
the end of the file can possibly become no longer the end of the file.

I'll try to think up a concise paragraph about this, and stick it into the
commit message.

Ciao,
Dscho

Patch
diff mbox series

diff --git a/apply.c b/apply.c
index f8a046a6a5..720a631eaa 100644
--- a/apply.c
+++ b/apply.c
@@ -2661,6 +2661,16 @@  static int find_pos(struct apply_state *state,
 	unsigned long backwards, forwards, current;
 	int backwards_lno, forwards_lno, current_lno;
 
+	/*
+	 * When running with --allow-overlap, it is possible that a hunk is
+	 * seen that pretends to start at the beginning (but no longer does),
+	 * and that *still* needs to match the end. So trust `match_end` more
+	 * than `match_beginning`.
+	 */
+	if (state->allow_overlap && match_beginning && match_end &&
+	    img->nr - preimage->nr != 0)
+		match_beginning = 0;
+
 	/*
 	 * If match_beginning or match_end is specified, there is no
 	 * point starting from a wrong line that will never match and