{"thread":{"id":"46608","subject":"git add -p breaks after split on change at the top of the file","startedAt":"2017-08-16T20:25:10Z","lastAt":"2017-08-20T16:25:49Z","messageCount":8,"participants":["Simon Ruderich","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"326556","messageId":"20170816202442.aumzwa443spqgyul@ruderich.org","threadId":"46608","inReplyTo":null,"subject":"git add -p breaks after split on change at the top of the file","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-08-16T20:25:04Z","receivedAt":"2017-08-16T20:25:10Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"Hello,\n\nThe following session reproduces the issue with Git 2.14.1:\n\n    $ git init test\n    $ cd test\n    $ echo x >file\n    $ git add file\n    $ git commit -m commit\n    $ printf 'a\\nb\\nx\\nc\\n' >file\n\n    $ git add -p\n    diff --git a/file b/file\n    index 587be6b..74a69a0 100644\n    --- a/file\n    +++ b/file\n    @@ -1 +1,4 @@\n    +a\n    +b\n     x\n    +c\n    Stage this hunk [y,n,q,a,d,/,s,e,?]? <-- press s\n    Split into 2 hunks.\n    @@ -1 +1,3 @@\n    +a\n    +b\n     x\n    Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? <-- press e\n\n    Now delete the line \"+a\" in your editor and save.\n\n    error: patch failed: file:1\n    error: file: patch does not apply\n\nI expected git add -p to stage this change without error. It\nworks fine without splitting the hunk (by deleting the first and\nlast + line in the diff).\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"326589","messageId":"20170817084109.ba7g2hnymtwqeclw@sigill.intra.peff.net","threadId":"46608","inReplyTo":"20170816202442.aumzwa443spqgyul@ruderich.org","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T08:41:09Z","receivedAt":"2017-08-17T08:41:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Junio, as this gets deep into git-apply innards]\n\nOn Wed, Aug 16, 2017 at 10:25:04PM +0200, Simon Ruderich wrote:\n\n>     $ git add -p\n>     diff --git a/file b/file\n>     index 587be6b..74a69a0 100644\n>     --- a/file\n>     +++ b/file\n>     @@ -1 +1,4 @@\n>     +a\n>     +b\n>      x\n>     +c\n>     Stage this hunk [y,n,q,a,d,/,s,e,?]? <-- press s\n>     Split into 2 hunks.\n>     @@ -1 +1,3 @@\n>     +a\n>     +b\n>      x\n>     Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? <-- press e\n> \n>     Now delete the line \"+a\" in your editor and save.\n> \n>     error: patch failed: file:1\n>     error: file: patch does not apply\n> \n> I expected git add -p to stage this change without error. It\n> works fine without splitting the hunk (by deleting the first and\n> last + line in the diff).\n\nInteresting case. The problem isn't in add--interactive itself (I don't\nthink), but in git-apply. This is the diff we end up feeding to \"git\napply --cached --check --recount --allow-overlap\" to see if it applies:\n\n  diff --git a/file b/file\n  index 587be6b..74a69a0 100644\n  --- a/file\n  +++ b/file\n  @@ -1 +1,3 @@\n  +b\n   x\n  @@ -1 +3,2 @@\n   x\n  +c\n\nThe first hunk (that we edited) applies fine. But the second one does\nnot. Its hunk header says that it starts at line \"1\", so we expect to\nfind it at the beginning of the file. But of course it _isn't_ at the\nbeginning of the file anymore, because the first hunk added a line\nbefore there.\n\nSo this diff is somewhat bogus; it has two hunks which start at the same\nspot. But I think that's exactly the sort of thing that\n\"--allow-overlap\" should handle. Doing this makes your case work for me:\n\ndiff --git a/apply.c b/apply.c\nindex 41ee63e1db..606db58218 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2966,8 +2966,9 @@ static int apply_one_fragment(struct apply_state *state,\n \t * In other words, a hunk that is (frag->oldpos <= 1) with or\n \t * without leading context must match at the beginning.\n \t */\n-\tmatch_beginning = (!frag->oldpos ||\n-\t\t\t   (frag->oldpos == 1 && !state->unidiff_zero));\n+\tmatch_beginning = (nth_fragment == 1 &&\n+\t\t\t   (!frag->oldpos ||\n+\t\t\t    (frag->oldpos == 1 && !state->unidiff_zero)));\n \n \t/*\n \t * A hunk without trailing lines must match at the end.\n\n\nBut I'm not quite sure if that's right. The rest of the overlap code\nseems to mark patched lines with a flag. Meaning that instead of giving\nup and saying \"well, this is the second line so we can't ever try\nmatching the beginning\", we should be redefining \"beginning\" in that\ncase to allow 0 or more PATCHED lines starting from the beginning of the\nfile.\n\n-Peff\n"},{"id":"326591","messageId":"20170817090308.yh3jnztgfmkwtqpr@sigill.intra.peff.net","threadId":"46608","inReplyTo":"20170817084109.ba7g2hnymtwqeclw@sigill.intra.peff.net","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T09:03:08Z","receivedAt":"2017-08-17T09:03:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2017 at 04:41:09AM -0400, Jeff King wrote:\n\n> diff --git a/apply.c b/apply.c\n> index 41ee63e1db..606db58218 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2966,8 +2966,9 @@ static int apply_one_fragment(struct apply_state *state,\n>  \t * In other words, a hunk that is (frag->oldpos <= 1) with or\n>  \t * without leading context must match at the beginning.\n>  \t */\n> -\tmatch_beginning = (!frag->oldpos ||\n> -\t\t\t   (frag->oldpos == 1 && !state->unidiff_zero));\n> +\tmatch_beginning = (nth_fragment == 1 &&\n> +\t\t\t   (!frag->oldpos ||\n> +\t\t\t    (frag->oldpos == 1 && !state->unidiff_zero)));\n>  \n>  \t/*\n>  \t * A hunk without trailing lines must match at the end.\n> \n> \n> But I'm not quite sure if that's right. The rest of the overlap code\n> seems to mark patched lines with a flag. Meaning that instead of giving\n> up and saying \"well, this is the second line so we can't ever try\n> matching the beginning\", we should be redefining \"beginning\" in that\n> case to allow 0 or more PATCHED lines starting from the beginning of the\n> file.\n\nHmm, actually I was reading that part of the code backwards. We record\nthe PATCHED flag _only_ when allow_overlap isn't set, not the other way\naround. So I had been imagining we'd want something like this:\n\ndiff --git a/apply.c b/apply.c\nindex 41ee63e1db..4048751807 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2489,8 +2489,11 @@ static int match_fragment(struct apply_state *state,\n \t\treturn 0;\n \t}\n \n-\tif (match_beginning && try_lno)\n-\t\treturn 0;\n+\tif (match_beginning) {\n+\t\tfor (i = 0; i < try_lno; i++)\n+\t\t\tif (!(img->line[i].flag & LINE_PATCHED))\n+\t\t\t\treturn 0;\n+\t}\n \n \t/* Quick hash check */\n \tfor (i = 0; i < preimage_limit; i++)\n\nBut that does the opposite of what we want: it makes this case work when\n--allow-overlap isn't specified. I think my first attempt is probably\ncloser to the right direction (but we'd want to have it kick in only\nwhen --allow-overlap is specified; well formed patches shouldn't overlap\nbut we'd want to barf loudly if they do).\n\nI'll stop here for now before digging any further. I'm not all that\nfamiliar with the apply code, so I have a feeling Junio's comments may\nstop me from going down an unproductive dead end. :)\n\n-Peff\n"},{"id":"326592","messageId":"20170817091503.vsd4lncogphy5gxx@sigill.intra.peff.net","threadId":"46608","inReplyTo":"20170817090308.yh3jnztgfmkwtqpr@sigill.intra.peff.net","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T09:15:04Z","receivedAt":"2017-08-17T09:15:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2017 at 05:03:08AM -0400, Jeff King wrote:\n\n> But that does the opposite of what we want: it makes this case work when\n> --allow-overlap isn't specified. I think my first attempt is probably\n> closer to the right direction (but we'd want to have it kick in only\n> when --allow-overlap is specified; well formed patches shouldn't overlap\n> but we'd want to barf loudly if they do).\n> \n> I'll stop here for now before digging any further. I'm not all that\n> familiar with the apply code, so I have a feeling Junio's comments may\n> stop me from going down an unproductive dead end. :)\n\nOK, last email, I promise. :)\n\nMy first attempt which turns off match_beginning for the overlapping\nhunk really does feel wrong. It would blindly match a similar hunk later\nin the file. So if you did something like:\n\n  1. The first hunk deletes context line \"x\".\n\n  2. The second overlapping hunk claims to start at the beginning of the\n     file (line 1), and then adds a line after \"x\".\n\n  3. Later in the file, we have another line \"x\".\n\nThen the second hunk should be rejected, and not find the unrelated \"x\"\nfrom (3). But with my patch, I suspect it would be found (because we\ndropped the requirement to find it at the beginning).\n\nOf course this is a pretty ridiculous input in the first place. In\ntheory it _could_ be figured out, but overlapping hunks like this are\nalways going to cause problems (in this case, context from the second\nhunk became a removal, and the second hunk no longer applies).\n\nSo maybe it's not worth caring about.  But I think the most robust\nsolution would involve checking the lines between try_lno and the start\nto make sure they were all added. I just don't know that we have an easy\nway of checking that. Perhaps if we recorded the cumulative number of\nlines added in previous hunks, and used that as our starting point\n(instead of \"line 0\").\n\nI also suspect that match_end has a similar problem, but I couldn't\ntrigger it in practice (perhaps it's impossible, or perhaps my brain is\njust not up to the challenge today).\n\n-Peff\n"},{"id":"326626","messageId":"xmqqa82ygggp.fsf@gitster.mtv.corp.google.com","threadId":"46608","inReplyTo":"20170817084109.ba7g2hnymtwqeclw@sigill.intra.peff.net","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T19:08:22Z","receivedAt":"2017-08-17T19:08:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [+cc Junio, as this gets deep into git-apply innards]\n\nI've written off --recount and --allow-overlap as ugly workaround\nthat happens to work some of the time but cannot be trusted long\ntime ago.\n\nIIRC, before the \"(e)dit\" thing was added to \"add -p\", we counted\nthe line numbers correctly and merged the adjacent hunks before\napplying and neither of these two kluge was necessary.\n\nThese threads may give us a bit more background:\n\n  https://public-inbox.org/git/7viqk1ndlk.fsf@alter.siamese.dyndns.org/\n  https://public-inbox.org/git/1304117373-592-1-git-send-email-gitster@pobox.com/\n\nThe original that introduced the \"(e)dit\" thing was in this thread:\n\n  https://public-inbox.org/git/200805232221.45406.trast@student.ethz.ch/\n\nAs you can see, I was very much against it, as it cannot\nfundamentally sanely implemented (which I think is the same\nconclusion you reached at the end of the current thread).\n\nI think there should be a better failure mode, though.\n"},{"id":"326627","messageId":"xmqq60dmgftg.fsf@gitster.mtv.corp.google.com","threadId":"46608","inReplyTo":"20170817091503.vsd4lncogphy5gxx@sigill.intra.peff.net","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T19:22:19Z","receivedAt":"2017-08-17T19:22:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Of course this is a pretty ridiculous input in the first place. In\n> theory it _could_ be figured out, but overlapping hunks like this are\n> always going to cause problems (in this case, context from the second\n> hunk became a removal, and the second hunk no longer applies).\n\nTo be honest, I do not think it could be figured out by \"git apply\",\nbut \"git add -p\" _should_ be able to.  I've said already this\nearlier in the discussion:\n\n  https://public-inbox.org/git/7vab5cn7wr.fsf@alter.siamese.dyndns.org/\n\n\"add -p\" knows how the hunk _before_ it gave the user to edit looked\nlike, and it knows how the hunk after the user edited looks like, so\nit may have enough information to figure it out.  But the \"(e)dit\"\nfeature was done in a way to discard that information and forced an\nimpossible \"guess what the correct shape of the patch would be, out\nof this broken patch input\" with --recount and --allow-overlap.  \n\nIf we want to make \"add -p/(e)dit\" work in the corner cases and make\nit trustworthy, \"apply\" is a wrong tree to back at.  A major part of\nthe effort to fix would go to \"add -p\", not to \"apply\".\n\n"},{"id":"326799","messageId":"20170820081936.i2e2rdvxl5wmpepd@sigill.intra.peff.net","threadId":"46608","inReplyTo":"xmqq60dmgftg.fsf@gitster.mtv.corp.google.com","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-20T08:19:37Z","receivedAt":"2017-08-20T08:19:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2017 at 12:22:19PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Of course this is a pretty ridiculous input in the first place. In\n> > theory it _could_ be figured out, but overlapping hunks like this are\n> > always going to cause problems (in this case, context from the second\n> > hunk became a removal, and the second hunk no longer applies).\n> \n> To be honest, I do not think it could be figured out by \"git apply\",\n> but \"git add -p\" _should_ be able to.  I've said already this\n> earlier in the discussion:\n> \n>   https://public-inbox.org/git/7vab5cn7wr.fsf@alter.siamese.dyndns.org/\n> \n> \"add -p\" knows how the hunk _before_ it gave the user to edit looked\n> like, and it knows how the hunk after the user edited looks like, so\n> it may have enough information to figure it out.  But the \"(e)dit\"\n> feature was done in a way to discard that information and forced an\n> impossible \"guess what the correct shape of the patch would be, out\n> of this broken patch input\" with --recount and --allow-overlap.\n> \n> If we want to make \"add -p/(e)dit\" work in the corner cases and make\n> it trustworthy, \"apply\" is a wrong tree to back at.  A major part of\n> the effort to fix would go to \"add -p\", not to \"apply\".\n\nIn theory \"add -p\" could give all the information it has to git-apply in\nsome form, and it could figure it out. That may sound crazy, but I just\nwonder if it makes things easier because git-apply already knows quite a\nlot about how to parse and interpret diffs. Whereas \"add -p\" is largely\nstupid and just parroting back lines.\n\nBut I certainly wouldn't make such a decision until figuring out the\nactual algorithm, which should then make it obvious where is the best\nplace to implement it. :)\n\nSo just thinking about this particular situation. We have two hunks\nafter the split:\n\n  @@ -1, +1,3 @@\n  +a\n  +b\n   x\n\nand\n\n  @@ -1, +3,2 @@\n   x\n  +c\n\nThe root of the issue, I think, is that the hunk header for the second\none is bogus. It claims to start at the beginning, but of course if the\nother hunk is applied first, it doesn't. The right header in that case\nwould be (assuming we do not copy the extra context):\n\n  @@ -3, +3,2 @@\n   x\n  +c\n\nOne point of interest is that without an (e)dit, we get this case right,\ndespite the bogus hunk header. I'm not quite sure why that is. After the\nfailing edit we would end up with:\n\n  @@ -1, +1,3 @@\n  +b\n   x\n\n  @@ -1, +3,2 @@\n   x\n  +c\n\nClearly the count in the first one is now wrong, but we fix that with\n--recount. But it seems to me that the second hunk is equally wrong as\nit would be in the non-edit case. I guess in addition to the \"-1\" being\na \"-2\", the \"+3\" should now also be a \"+2\". But I didn't think that\nwould impact us finding the preimage side.\n\nSo that's one question I'm puzzled by: why does it work without an edit,\nbut fail with one.\n\nMy second question is how \"add -p\" could generate the corrected hunk\nheaders.\n\nIn the non-edit case, we know that our second hunk's leading context\nmust always match the first hunk's trailing context (since that's how we\ndo a split). So we'd know that the preimage \"-pos\" field of the second\nhunk header should be offset by the net sum of the added/changed lines\nfor any previous split hunks.\n\nIn the original case we had net two added lines in the first hunk. So if\nit's selected, that's how our \"-1\" becomes \"-3\". And after the edit, we\nshould be able to apply the same, I think? We have a single added line,\nso it becomes \"-2\". That doesn't require knowing what was in the hunk\nbefore the user edited at all. We just need to know which other hunks\nit's split from, so we know which overlapping hunks we need to take into\naccount when recomputing.\n\n\"add -p\" certainly has that information. I _think_ git-apply also has\nthat information if it bothers to find out. Right now --allow-overlap is\njust \"do not complain of overlap\". But it could actively recount the\noffsets when it detects overlap.\n\nI'm assuming here that the edit process isn't changing the hunk headers,\nintroducing new hunks, etc. I feel like all bets are off, then.\n\n-Peff\n"},{"id":"326821","messageId":"xmqq4lt28aux.fsf@gitster.mtv.corp.google.com","threadId":"46608","inReplyTo":"20170820081936.i2e2rdvxl5wmpepd@sigill.intra.peff.net","subject":"Re: git add -p breaks after split on change at the top of the file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-20T16:25:42Z","receivedAt":"2017-08-20T16:25:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So that's one question I'm puzzled by: why does it work without an edit,\n> but fail with one.\n\nAs this is the part of the system I long time ago declared a \"works\nonly some of the time\" hack, I do not recall the exact details, but\na key phrase \"DIRTY\" in the old discussion thread made me look.\n\nDoesn't coalesce_overlapping_hunks in add-interactive.perl play a\nrole in the difference?  I _think_ that one tries to do the right\nthing by enabling the logic before the \"(e)dit\" hack was introduced;\nI haven't actually checked out and looked the code before the\naddition of the option, but \"add-p\" should back then have actually\nunderstood the offsets and hunk length and merged the surviving\nhunks as needed to avoid feeding overlaps to \"git apply\"---it may\neven have fixed the offset before doing so.  Edited hunks whose\nmechanical integrity has become dubious are marked with DIRTY, as\nthe hunk touched by \"(e)dit\" codepath, by the time it reaches that\nhelper function, no longer has trustable information in the correct\noffset information there.\n\n> My second question is how \"add -p\" could generate the corrected hunk\n> headers.\n>\n> In the non-edit case, we know that our second hunk's leading context\n> must always match the first hunk's trailing context (since that's how we\n> do a split). So we'd know that the preimage \"-pos\" field of the second\n> hunk header should be offset by the net sum of the added/changed lines\n> for any previous split hunks.\n>\n> In the original case we had net two added lines in the first hunk. So if\n> it's selected, that's how our \"-1\" becomes \"-3\". And after the edit, we\n> should be able to apply the same, I think? We have a single added line,\n> so it becomes \"-2\". That doesn't require knowing what was in the hunk\n> before the user edited at all. We just need to know which other hunks\n> it's split from, so we know which overlapping hunks we need to take into\n> account when recomputing.\n>\n> \"add -p\" certainly has that information. I _think_ git-apply also has\n> that information if it bothers to find out.\n\nI am not sure about the latter.  If we declare that we assume the\nuser never touches the hunk header line (i.e. \"@@ ... @@\") when\nediting the patch text (i.e. \" /-/+\" lines), the receiving end can\nprobably guess by counting how many preimage and postimage lines are\ndescribed in the hunk and then seeing the change from the numbers on\nthe hunk header.  \n\nI do not think the \"--recount/--overlap\" code trusts its input that\nmuch in the current code, and there may be a room to tweak that band\naid with such an additional assumption.\n\nBut I dunno.  Once we go that route, and then if we want to assume\nless about random things the end-user may do to make it more robust,\n\"add -p\" would need to keep the hunk header before giving the hunk\nto the user, and then replace the (possibly modified) hunk header in\nthe edited patch with the original to tell the receiving end what\nthe original numbers were to help --recount logic.  At that point, I\ndo not think it is too big a stretch for the \"(e)dit\" to actually\ncorrect the hunk headers to not just express the correct offset but\nalso show the right number of lines, so that it does not have to be\nmarked with the \"DIRTY\" bit.  That way, coalesce_overlapping_hunks\nlogic can finish it off just like other \"picked but unedited\" hunks.\n\nAnd when that happens, we may not even need to run \"apply\" with the\n\"--recount\" and \"--allow-overlap\" options from this codepath to go\nback to the state before 8cbd4310 (\"git-add--interactive: replace\nhunk recounting with apply --recount\", 2008-07-02), where \"add -p\"\ndid not fly blind and always knew (or at least \"supposed to know\")\neverything that is happening, which IMO is a preferrable endgame.\nIf the language spoken between \"add -p\" and \"apply\" is \"patch\", we\nshould try to stick to that language.  Butchering the listener to\nallow this speaker to speak in loose and ambiguous terms, especially\nwhen we can teach the speaker to do so, was a big mistake.\n\n"}]}