| Message ID | 15c4cce5a027d56c7ddbe5523cf0f3beabd06ed7.1575637705.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add -i: close some regression test gaps | expand |
"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
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 >
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.
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
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