{"thread":{"id":"39485","subject":"[RFC] solving a bug with hunks starting at line 1 in git apply","startedAt":"2015-06-01T17:07:55Z","lastAt":"2015-06-01T21:37:39Z","messageCount":9,"participants":["Remi LESPINET","Junio C Hamano","Matthieu Moy","Remi Lespinet"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"262620","messageId":"87k2vnqrv8.fsf@ensimag.grenoble-inp.fr","threadId":"39485","inReplyTo":null,"subject":"[RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Remi LESPINET","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-06-01T17:07:55Z","receivedAt":"2015-06-01T17:07:55Z","isPatch":false,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"\n======================================================================\n= 1. The bug\n======================================================================\n\nhunks of the form:\n\n@@ -1,k +n,m @@\n\nhave a special behavior because of the variable match_begining.  For\nthese hunks, offset is not allowed which means that the fragment has to\nmatch at the first line of the image. This is done for the sake of\nsafety (This behavior does not exist in the basic patch command). Here's\nan example\n\nfirst original file:\n\n10\n20\n30\n40\n\nfor the following diff file:\n\n@@ -1,2 +1,2 @@\n 20\n-30\n+35\n 40\n\nThe patch will not be applied with a git apply command, but it will\nwith a basic patch command. However, there's no problem with the\nfollowing diff for both:\n\n@@ -2,2 +2,2 @@\n 10\n-20\n+25\n 30\n\nThe problem comes when we have a diff file like that (which can be\nobtained by splitting a hunk with git add -p for example (unsolved bug\nreported in 1bf01040f0c1101f410f9caa5e715460cdd0cbe0))\n\n@@ -1,1 +1,2 @@\n+5\n 10\n@@ -1,3 +2,3 @@\n 10\n+15\n-20\n 30\n\nIn this case the first hunk will be treated, the image will then become\n\n5\n10\n20\n30\n\nThe second will try to match the lines\n\n10\n20\n30\n\nwith the first lines of the image, will not succeed and will return that\nit is not possible without trying to match the image with offsets,\nbecause the hunk begin with 1.\n\nMoreover, there are cases where both the git apply and the\npatch command will work, giving different outputs.\n\nsecond original file:\n\n10\n10\n20\n\ndiff file associated:\n\n@@ -1,1 +1,2 @@\n+10\n 10\n@@ -1,2 +2,3 @@\n 10\n+cc\n 10\n\nWith git apply, the problem will silently appear, and the command will\ngive the following output:\n\n10\ncc\n10\n10\n20\n\nwhereas the output given by the simple patch command will be:\n\n10\n10\ncc\n10\n20\n\n======================================================================\n= 2. Correction\n======================================================================\n\nI see mainly two ways to fix the bug:\n\n********************************************************************\n* 2.1 first method (the most basic)\n********************************************************************\n\nThe most basic is to change the code so that the special behavior\nonly affects the hunks of the form\n\n@@ -1,k +1,m @@\n\nWhich makes the previous diff file possible to use. And fixes the\nbug reported in 1bf01040f0c1101f410f9caa5e715460cdd0cbe0\n\nNote that this modification, which changes positively behavior of hunks\n\n@@ -1,k +l,m @@\n\nwhere l is not 1, when there's a hunk\n\n@@ -1,a +1,b @@\n\nabove, also changes the behavior when there's no such bloc above:\n\nFor example: the diff file\n\n@@ -1,2 +2,2 @@\n 20\n-30\n+35\n 40\n\nwould have given an error before this patch (with the first original file)\nwhereas it works with the modification introduced by the patch.\n\n**********************************************************************\n* 2.2 second method\n**********************************************************************\n\nThe other method, is to pass a base_line and an offset to the match_fragment\nfunction instead of passing directly the line to test.\n\nThis does not involve any change in the behavior of apply, and therefore\nhas not the problem of the most basic method.\n\n======================================================================\n= 3. Question\n======================================================================\n\nI personnaly think that the second method is better, but this implies more\nmodifications, and it will be useful only to keep the failing behavior of \nsome hand-modified patches... Do you think that the second method is\nworth implementing ?\n"},{"id":"262621","messageId":"xmqqiob773iy.fsf@gitster.dls.corp.google.com","threadId":"39485","inReplyTo":"87k2vnqrv8.fsf@ensimag.grenoble-inp.fr","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T17:16:21Z","receivedAt":"2015-06-01T17:16:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n\n> first original file:\n>\n> 10\n> 20\n> 30\n> 40\n>\n> for the following diff file:\n>\n> @@ -1,2 +1,2 @@\n>  20\n> -30\n> +35\n>  40\n>\n> The patch will not be applied with a git apply command, but it will\n> with a basic patch command.\n\nDoesn't that merely indicate a bug in \"patch\", though?\n\nThe patch text says that the preimage must begin with \"20\" but the\ntarget begins with \"10\".\n\nI think we have the same \"the boundary must match\" logic not just\nat the beginning of the file but also at the end of the file.  No\nmatter what the answer to the above question is, we would need to\ndo the same for both the beginning and the end, I would think.\n\nThanks.\n"},{"id":"262623","messageId":"xmqqeglv72vd.fsf@gitster.dls.corp.google.com","threadId":"39485","inReplyTo":"xmqqiob773iy.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T17:30:30Z","receivedAt":"2015-06-01T17:30:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n>\n>> first original file:\n>>\n>> 10\n>> 20\n>> 30\n>> 40\n>>\n>> for the following diff file:\n>>\n>> @@ -1,2 +1,2 @@\n>>  20\n>> -30\n>> +35\n>>  40\n>>\n>> The patch will not be applied with a git apply command, but it will\n>> with a basic patch command.\n>\n> Doesn't that merely indicate a bug in \"patch\", though?\n\nThis needs a bit gentler explanation.  Imagine you are doing a usual\n\"git diff\" with two lines of context, with a longer original.  A\nchange to do s/30/35/ would look like this:\n\n@@ -1,5 +1,5 @@\n 10\n 20\n-30\n+35\n 40\n 50\n\nAnd an attempt to apply this patch to an original that does not have\n10, 20, 30, 40 and 50 in this order must fail.\n\nOn the other hand, if the change were instead s/20/35/, then the\ndiff output would look like this instead:\n\n@@ -1,4 +1,4 @@\n 10\n-20\n+25\n 30\n 40\n\nNotice that the pre-context lines in this second example is only one\nline.  Is it giving the same degree of safety if we rejected an\nattempt to apply this patch only when the original does not have 10,\n20, 30 and 40 in this order?\n\nNo.  Because we are doing two-line context patch, the patch is not\njust saying \"this change applies to a place where the first line is\n10\".  It also is saying \"there is no line before that line\".\n\nTo answer my own question in the previous message, we cannot tell\nonly from that example which of \"git apply\" and \"patch\" is wrong.\nIf you generated the patch with \"git diff -U1\", then \"git apply\"\nthat insists that there must be no line before that \"20\" is wrong.\nIf you generated the patch with \"git diff\" with the default two-line\ncontext, then \"patch\" is wrong if it does not make sure \"20\" is the\nfirst line.\n"},{"id":"262631","messageId":"vpqa8wj48c1.fsf@anie.imag.fr","threadId":"39485","inReplyTo":"xmqqiob773iy.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-01T18:00:46Z","receivedAt":"2015-06-01T18:00:46Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n>\n>> first original file:\n>>\n>> 10\n>> 20\n>> 30\n>> 40\n>>\n>> for the following diff file:\n>>\n>> @@ -1,2 +1,2 @@\n>>  20\n>> -30\n>> +35\n>>  40\n>>\n>> The patch will not be applied with a git apply command, but it will\n>> with a basic patch command.\n>\n> Doesn't that merely indicate a bug in \"patch\", though?\n\nNo, it's just that patch does a fuzzy match in this case:\n\n$ patch < patch.diff \npatching file pre.txt\nHunk #1 succeeded at 2 with fuzz 1 (offset 1 line).\n\nIt's different from the other case:\n\nRemi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n\n> @@ -1,1 +1,2 @@\n> +5\n>  10\n> @@ -1,3 +2,3 @@\n>  10\n> +15\n> -20\n>  30\n\nWith this one, I get:\n\n$ git apply < p2.diff   \nerror: patch failed: pre.txt:1\nerror: pre.txt: patch does not apply\n$ patch < p2.diff \npatching file pre.txt\n\n=> no fuzzy matching for patch, git apply should actually work.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262632","messageId":"vpq382b47fq.fsf@anie.imag.fr","threadId":"39485","inReplyTo":"87k2vnqrv8.fsf@ensimag.grenoble-inp.fr","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-01T18:20:09Z","receivedAt":"2015-06-01T18:20:09Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr> writes:\n\n> ======================================================================\n> = 2. Correction\n> ======================================================================\n>\n> I see mainly two ways to fix the bug:\n>\n> ********************************************************************\n> * 2.1 first method (the most basic)\n> ********************************************************************\n>\n> The most basic is to change the code so that the special behavior\n> only affects the hunks of the form\n>\n> @@ -1,k +1,m @@\n\nI do not think that this would work in all cases. It seems git apply has\nissues with overlapping contexts, not just with hunks starting at line\n1. See:\n\n$ cat pre.txt\n-1\n0\n10\n20\n30\n40\n$ cat p2.diff \n--- pre.txt\n+++ pre.txt\n@@ -2,2 +2,3 @@\n 0\n+5\n 10\n@@ -3,3 +4,3 @@\n 10\n+15\n-20\n 30\n$ git apply p2.diff\nerror: patch failed: pre.txt:3\nerror: pre.txt: patch does not apply\n$ patch < p2.diff \npatching file pre.txt\n$ cat pre.txt\n-1\n0\n5\n10\n15\n30\n40\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262636","messageId":"xmqqpp5f5l7v.fsf@gitster.dls.corp.google.com","threadId":"39485","inReplyTo":"vpqa8wj48c1.fsf@anie.imag.fr","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T18:37:08Z","receivedAt":"2015-06-01T18:37:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> @@ -1,1 +1,2 @@\n>> +5\n>>  10\n>> @@ -1,3 +2,3 @@\n>>  10\n>> +15\n>> -20\n>>  30\n>\n> With this one, I get:\n>\n> $ git apply < p2.diff   \n> error: patch failed: pre.txt:1\n> error: pre.txt: patch does not apply\n> $ patch < p2.diff \n> patching file pre.txt\n>\n> => no fuzzy matching for patch, git apply should actually work.\n\nI am not sure what you are trying to do with that patch that tries\nto touch the same line twice.  Is this the same old laziness coming\nback to bite us, the one that we attempted to work around with\n933e44d3 (\"add -p\": work-around an old laziness that does not\ncoalesce hunks, 2011-04-06)?\n\nIn other words, isn't the right fix to coalesce that input, so that\nthe second hunk does *not* require fuzzy application in the first\nplace?\n"},{"id":"262639","messageId":"vpqiob7z2n7.fsf@anie.imag.fr","threadId":"39485","inReplyTo":"xmqqpp5f5l7v.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-01T18:47:56Z","receivedAt":"2015-06-01T18:47:56Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>>> @@ -1,1 +1,2 @@\n>>> +5\n>>>  10\n>>> @@ -1,3 +2,3 @@\n>>>  10\n>>> +15\n>>> -20\n>>>  30\n>>\n>> With this one, I get:\n>>\n>> $ git apply < p2.diff   \n>> error: patch failed: pre.txt:1\n>> error: pre.txt: patch does not apply\n>> $ patch < p2.diff \n>> patching file pre.txt\n>>\n>> => no fuzzy matching for patch, git apply should actually work.\n>\n> I am not sure what you are trying to do with that patch that tries\n> to touch the same line twice.  Is this the same old laziness coming\n> back to bite us, the one that we attempted to work around with\n> 933e44d3 (\"add -p\": work-around an old laziness that does not\n> coalesce hunks, 2011-04-06)?\n\nIndeed, \"git apply\" works with --allow-overlap in this case. But this is\nnot sufficient to fix \"git add -p\" which already uses it. So, there's\nsomething else.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262640","messageId":"xmqqeglv5kg1.fsf@gitster.dls.corp.google.com","threadId":"39485","inReplyTo":"vpqiob7z2n7.fsf@anie.imag.fr","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T18:53:50Z","receivedAt":"2015-06-01T18:53:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> I am not sure what you are trying to do with that patch that tries\n>> to touch the same line twice.  Is this the same old laziness coming\n>> back to bite us, the one that we attempted to work around with\n>> 933e44d3 (\"add -p\": work-around an old laziness that does not\n>> coalesce hunks, 2011-04-06)?\n>\n> Indeed, \"git apply\" works with --allow-overlap in this case. But this is\n> not sufficient to fix \"git add -p\" which already uses it. So, there's\n> something else.\n\nI do not have time to go back to the list archive myself at this\nmoment, but I suspect that in the discussion around the time back\nwhen 0beee4c6 (git-add--interactive: remove hunk coalescing,\n2008-07-02) was done, we'd find some material for me to say \"I told\nyou so\" X-< to those who added that laziness to \"add--interactive\".\n"},{"id":"262654","messageId":"864699446.258590.1433194659960.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39485","inReplyTo":"xmqqeglv5kg1.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] solving a bug with hunks starting at line 1 in git apply","fromName":"Remi Lespinet","fromEmail":"remi.lespinet@ensimag.grenoble-inp.fr","sentAt":"2015-06-01T21:37:39Z","receivedAt":"2015-06-01T21:37:39Z","isPatch":false,"sender":{"key":"remi.lespinet@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/11941160?v=4"},"body":"Ok, Thanks for all the informations\n\n> Notice that the pre-context lines in this second example is only one\n> line.  Is it giving the same degree of safety if we rejected an\n> attempt to apply this patch only when the original does not have 10,\n> 20, 30 and 40 in this order?\n> \n> No.  Because we are doing two-line context patch, the patch is not\n> just saying \"this change applies to a place where the first line is\n> 10\".  It also is saying \"there is no line before that line\".\n\nYes, It's obvious that \"patch\" has not the same degree of safety as the\ngit apply command. But I thought that any patch working with\ngit apply should work with the \"patch\" command and give the same output,\nIt seems that this is not true, regarding\n\nfile:\n\n10\n10\n10\n10\n20\n\ndiff:\n\n@@ -1,2 +1,3 @@\n+10\n 10\n 10\n@@ -1,4 +2,5 @@\n 10\n 10\n+cc\n 10\n 10\n\n(I changed it to have 2 line context).  Of course these are\nhand-written patches, which can't be obtained with a diff (except with\nthe non coalescing git add -p as you said yourself). Wouldn't it \nbe a problem?\n\n> In other words, isn't the right fix to coalesce that input, so that\n> the second hunk does *not* require fuzzy application in the first\n> place?\n\nYes, you're right, that will be fixed if we restore coalescing, I\ndidn't thought of this possibility. This will cause fewer split but\nwe have the edit option anyway.\n\n\nThanks\n"}]}