{"thread":{"id":"31172","subject":"[RFC/PATCH] apply: parse and act on --irreversible-delete output","startedAt":"2012-08-02T20:35:48Z","lastAt":"2012-08-02T22:23:57Z","messageCount":3,"participants":["Paul Gortmaker","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196368","messageId":"1343939748-12256-1-git-send-email-paul.gortmaker@windriver.com","threadId":"31172","inReplyTo":null,"subject":"[RFC/PATCH] apply: parse and act on --irreversible-delete output","fromName":"Paul Gortmaker","fromEmail":"paul.gortmaker@windriver.com","sentAt":"2012-08-02T20:35:48Z","receivedAt":"2012-08-02T20:35:48Z","isPatch":true,"sender":{"key":"paul.gortmaker@windriver.com","avatar":null},"body":"The '-D' or '--irreversible-delete' option of format-patch is\ngreat for sending out patches to mailing lists, where there\nis little value in seeing thousands of lines of deleted code.\nAttention can then be focused on the changes relating to\nthe binding of the deleted code (Makefiles, etc).\n\nHowever the original intent of commit 467ddc14f (\"git diff -D: omit\nthe preimage of deletes\") was as follows:\n\n    To prevent such a patch from being applied by mistake, the\n    output is designed not to be usable by \"git apply\" (or GNU \"patch\");\n    it is strictly for human consumption.\n\nThe downside of this, is that patches to mailing lists which are\nthen either managed with patchworks, or dealt with directly by\nmaintainers, will need manual intervention if they are to be used.\n\nBut with the index lines, there is no reason why we can't act\nintelligently and automatically on these with \"git apply\".\nIf we can unambiguously map what was recorded as the deleted\nSHA prefix to the SHA of the matching blob filename in our tree,\nthen we set the image len to zero which facilitates the delete.\n\nSigned-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>\n---\n\nFor a recent use case example, see:\n\thttp://www.spinics.net/lists/netdev/msg206519.html\n\nCould be wrapped in an \"am.applyirreversible\" if for some reason\nglobal deployment was considered unwise?\n\n Documentation/diff-options.txt |  5 +++--\n builtin/apply.c                | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex cf4b216..efaaf1c 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -328,8 +328,9 @@ endif::git-log[]\n --irreversible-delete::\n \tOmit the preimage for deletes, i.e. print only the header but not\n \tthe diff between the preimage and `/dev/null`. The resulting patch\n-\tis not meant to be applied with `patch` nor `git apply`; this is\n-\tsolely for people who want to just concentrate on reviewing the\n+\tis not meant to be applied with `patch` (but can be with `git apply`).\n+\tThis is for people who want to avoid seeing/mailing all the deleted\n+\tfile content, and instead just concentrate on reviewing the\n \ttext after the change. In addition, the output obviously lack\n \tenough information to apply such a patch in reverse, even manually,\n \thence the name of the option.\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex d453c83..363da63 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2933,6 +2933,36 @@ static int apply_fragments(struct image *img, struct patch *patch)\n \tif (patch->is_binary)\n \t\treturn apply_binary(img, patch);\n \n+\t/* output from --irreversible-delete (looks like empty file delete) */\n+\tif (patch->is_delete > 0 && !frag && img->len > 0) {\n+\t\tunsigned char file_sha1[20], patch_sha1[20];\n+\t\tstruct object_context oc;\n+\n+\t\tif (apply_in_reverse) {\n+\t\t\terror(_(\"can not reverse an irreversible-delete patch\"\n+\t\t\t      \"on file '%s'.\"), name);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tstrcpy(oc.path, name);\n+\t\tif (get_sha1_with_context(patch->old_sha1_prefix,\n+\t\t    GET_SHA1_BLOB | GET_SHA1_QUIETLY, patch_sha1, &oc)) {\n+\t\t\terror(_(\"the deleted SHA prefix of file '%s' (%s), does\"\n+\t\t\t      \" not seem to exist in this repository.\"), name,\n+\t\t\t      patch->old_sha1_prefix);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\thash_sha1_file(img->buf, img->len, blob_type, file_sha1);\n+\t\tif (memcmp(file_sha1, patch_sha1, 20)) {\n+\t\t\terror(_(\"the delete requested of '%s' (%s), does not\"\n+\t\t\t      \" match the current file contents.\"), name,\n+\t\t\t      sha1_to_hex(patch_sha1));\n+\t\t\treturn -1;\n+\t\t}\n+\t\timg->len = 0;\n+\t}\n+\n \twhile (frag) {\n \t\tnth++;\n \t\tif (apply_one_fragment(img, frag, inaccurate_eof, ws_rule, nth)) {\n-- \n1.7.12.rc1.dirty\n"},{"id":"196373","messageId":"7vr4rpc7nz.fsf@alter.siamese.dyndns.org","threadId":"31172","inReplyTo":"1343939748-12256-1-git-send-email-paul.gortmaker@windriver.com","subject":"Re: [RFC/PATCH] apply: parse and act on --irreversible-delete output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-02T21:20:48Z","receivedAt":"2012-08-02T21:20:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Gortmaker <paul.gortmaker@windriver.com> writes:\n\n> The '-D' or '--irreversible-delete' option of format-patch is\n> great for sending out patches to mailing lists, where there\n> is little value in seeing thousands of lines of deleted code.\n> Attention can then be focused on the changes relating to\n> the binding of the deleted code (Makefiles, etc).\n>\n> However the original intent of commit 467ddc14f (\"git diff -D: omit\n> the preimage of deletes\") was as follows:\n>\n>     To prevent such a patch from being applied by mistake, the\n>     output is designed not to be usable by \"git apply\" (or GNU \"patch\");\n>     it is strictly for human consumption.\n>\n> The downside of this, is that patches to mailing lists which are\n> then either managed with patchworks, or dealt with directly by\n> maintainers, will need manual intervention if they are to be used.\n>\n> But with the index lines, there is no reason why we can't act\n> intelligently and automatically on these with \"git apply\".\n> If we can unambiguously map what was recorded as the deleted\n> SHA prefix to the SHA of the matching blob filename in our tree,\n> then we set the image len to zero which facilitates the delete.\n>\n> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>\n> ---\n>\n> For a recent use case example, see:\n> \thttp://www.spinics.net/lists/netdev/msg206519.html\n>\n> Could be wrapped in an \"am.applyirreversible\" if for some reason\n> global deployment was considered unwise?\n>\n>  Documentation/diff-options.txt |  5 +++--\n>  builtin/apply.c                | 30 ++++++++++++++++++++++++++++++\n>  2 files changed, 33 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index cf4b216..efaaf1c 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -328,8 +328,9 @@ endif::git-log[]\n>  --irreversible-delete::\n>  \tOmit the preimage for deletes, i.e. print only the header but not\n>  \tthe diff between the preimage and `/dev/null`. The resulting patch\n> -\tis not meant to be applied with `patch` nor `git apply`; this is\n> -\tsolely for people who want to just concentrate on reviewing the\n> +\tis not meant to be applied with `patch` (but can be with `git apply`).\n\n... but only when you are applying to the exact version the patch\nwas created from, no?\n\n> +\tThis is for people who want to avoid seeing/mailing all the deleted\n> +\tfile content, and instead just concentrate on reviewing the\n>  \ttext after the change. In addition, the output obviously lack\n>  \tenough information to apply such a patch in reverse, even manually,\n>  \thence the name of the option.\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index d453c83..363da63 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -2933,6 +2933,36 @@ static int apply_fragments(struct image *img, struct patch *patch)\n>  \tif (patch->is_binary)\n>  \t\treturn apply_binary(img, patch);\n>  \n> +\t/* output from --irreversible-delete (looks like empty file delete) */\n> +\tif (patch->is_delete > 0 && !frag && img->len > 0) {\n\nWhat is (img->len > 0) part trying to ensure?\n\nIf somebody gives you an irreversible deletion of an empty file,\nshouldn't this codepath handle it the same way?\n\n> +\t\tunsigned char file_sha1[20], patch_sha1[20];\n> +\t\tstruct object_context oc;\n> +\n> +\t\tif (apply_in_reverse) {\n> +\t\t\terror(_(\"can not reverse an irreversible-delete patch\"\n> +\t\t\t      \"on file '%s'.\"), name);\n> +\t\t\treturn -1;\n> +\t\t}\n\nThe return value of error() is already -1, so you can just return\nit without { stmt; return -1; }.\n\n> +\n> +\t\tstrcpy(oc.path, name);\n> +\t\tif (get_sha1_with_context(patch->old_sha1_prefix,\n> +\t\t    GET_SHA1_BLOB | GET_SHA1_QUIETLY, patch_sha1, &oc)) {\n> +\t\t\terror(_(\"the deleted SHA prefix of file '%s' (%s), does\"\n> +\t\t\t      \" not seem to exist in this repository.\"), name,\n> +\t\t\t      patch->old_sha1_prefix);\n> +\t\t\treturn -1;\n> +\t\t}\n\nThis is not sufficient to make sure patch_sha1 exists in your\nrepository and is indeed a blob object.  GET_SHA1_BLOB is a hint to\nsay \"if there are more than one that shares this prefix, ignore ones\nthat are not blob---if there is only one remains, then even though\nthe prefix is ambiguous in this repository, we will take it\".  If\nyou only have a commit or a tree that has the prefix but not the\nblob object the patch wants to touch, you will get the object name\nof that commit or tree you have in your repository.\n\nAlso oc is an output parameter; it is not about \"I want to make sure\nthe found object is at this pathname\"---that is an impossible request\nto begin with.  I think something like this, without oc, would be\nwhat you want:\n\n\tif (get_sha1_with_context(patch->old_sha1_prefix,\n        \t\tGET_SHA1_BLOB | GET_SHA1_QUIETLY,\n        \t\tpatch_sha1, NULL) ||\n\t    sha1_object_info(patch_sha1, NULL) != OBJ_BLOB)\n\t\treturn error(...);\n\nBut I think the test itself (not the way you tested, but what you\nare trying to test---the uniqueness of abbrevited object name) is\npointless.  The submitter of the patch may have far fewer objects\nthan you do, and it is perfectly normal if the old_sha1_prefix that\nwas sufficiently long to identify the blob for the submitter is not\nunambiguous enough to identify the blob uniquely for you when you\ntry to apply the patch.  You may have other unrelated blobs that\nhappen to share the prefix in your repository.\n\nHashing img->buf and making sure it matches old_sha1_prefix is the\nbest you can do.  If the extra ambiguity coming from that approach\nbothers you, then the entire \"force apply an --irreversible-delete\npatch\" idea also should.\n\n> +\t\thash_sha1_file(img->buf, img->len, blob_type, file_sha1);\n> +\t\tif (memcmp(file_sha1, patch_sha1, 20)) {\n> +\t\t\terror(_(\"the delete requested of '%s' (%s), does not\"\n> +\t\t\t      \" match the current file contents.\"), name,\n> +\t\t\t      sha1_to_hex(patch_sha1));\n> +\t\t\treturn -1;\n\nreturn error(...);\n"},{"id":"196385","messageId":"501AFDFD.3010900@windriver.com","threadId":"31172","inReplyTo":"7vr4rpc7nz.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] apply: parse and act on --irreversible-delete output","fromName":"Paul Gortmaker","fromEmail":"paul.gortmaker@windriver.com","sentAt":"2012-08-02T22:23:57Z","receivedAt":"2012-08-02T22:23:57Z","isPatch":true,"sender":{"key":"paul.gortmaker@windriver.com","avatar":null},"body":"On 12-08-02 05:20 PM, Junio C Hamano wrote:\n> Paul Gortmaker <paul.gortmaker@windriver.com> writes:\n> \n>> The '-D' or '--irreversible-delete' option of format-patch is\n>> great for sending out patches to mailing lists, where there\n>> is little value in seeing thousands of lines of deleted code.\n>> Attention can then be focused on the changes relating to\n>> the binding of the deleted code (Makefiles, etc).\n>>\n>> However the original intent of commit 467ddc14f (\"git diff -D: omit\n>> the preimage of deletes\") was as follows:\n>>\n>>     To prevent such a patch from being applied by mistake, the\n>>     output is designed not to be usable by \"git apply\" (or GNU \"patch\");\n>>     it is strictly for human consumption.\n>>\n>> The downside of this, is that patches to mailing lists which are\n>> then either managed with patchworks, or dealt with directly by\n>> maintainers, will need manual intervention if they are to be used.\n>>\n>> But with the index lines, there is no reason why we can't act\n>> intelligently and automatically on these with \"git apply\".\n>> If we can unambiguously map what was recorded as the deleted\n>> SHA prefix to the SHA of the matching blob filename in our tree,\n>> then we set the image len to zero which facilitates the delete.\n>>\n>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>\n>> ---\n>>\n>> For a recent use case example, see:\n>> \thttp://www.spinics.net/lists/netdev/msg206519.html\n>>\n>> Could be wrapped in an \"am.applyirreversible\" if for some reason\n>> global deployment was considered unwise?\n>>\n>>  Documentation/diff-options.txt |  5 +++--\n>>  builtin/apply.c                | 30 ++++++++++++++++++++++++++++++\n>>  2 files changed, 33 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index cf4b216..efaaf1c 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -328,8 +328,9 @@ endif::git-log[]\n>>  --irreversible-delete::\n>>  \tOmit the preimage for deletes, i.e. print only the header but not\n>>  \tthe diff between the preimage and `/dev/null`. The resulting patch\n>> -\tis not meant to be applied with `patch` nor `git apply`; this is\n>> -\tsolely for people who want to just concentrate on reviewing the\n>> +\tis not meant to be applied with `patch` (but can be with `git apply`).\n> \n> ... but only when you are applying to the exact version the patch\n> was created from, no?\n\nTrue, I can add that extra detail/limitation to the docs.\n\n> \n>> +\tThis is for people who want to avoid seeing/mailing all the deleted\n>> +\tfile content, and instead just concentrate on reviewing the\n>>  \ttext after the change. In addition, the output obviously lack\n>>  \tenough information to apply such a patch in reverse, even manually,\n>>  \thence the name of the option.\n>> diff --git a/builtin/apply.c b/builtin/apply.c\n>> index d453c83..363da63 100644\n>> --- a/builtin/apply.c\n>> +++ b/builtin/apply.c\n>> @@ -2933,6 +2933,36 @@ static int apply_fragments(struct image *img, struct patch *patch)\n>>  \tif (patch->is_binary)\n>>  \t\treturn apply_binary(img, patch);\n>>  \n>> +\t/* output from --irreversible-delete (looks like empty file delete) */\n>> +\tif (patch->is_delete > 0 && !frag && img->len > 0) {\n> \n> What is (img->len > 0) part trying to ensure?\n> \n> If somebody gives you an irreversible deletion of an empty file,\n> shouldn't this codepath handle it the same way?\n\nThe format-patch output of the deletion of an empty file is\nidentical with or without the switch, so I didn't want to\naccidentally limit people from normal empty file deletions\nby invoking special checks on them that did not exist before.\n\n> \n>> +\t\tunsigned char file_sha1[20], patch_sha1[20];\n>> +\t\tstruct object_context oc;\n>> +\n>> +\t\tif (apply_in_reverse) {\n>> +\t\t\terror(_(\"can not reverse an irreversible-delete patch\"\n>> +\t\t\t      \"on file '%s'.\"), name);\n>> +\t\t\treturn -1;\n>> +\t\t}\n> \n> The return value of error() is already -1, so you can just return\n> it without { stmt; return -1; }.\n\nOK, will update.  I'd inadvertently got the separate statements\nby copying the code below it, which did a conditional return based\non what apply_with_reject was set to, but I'm not sure any special\nreject behaviour for an irreversible delete fail makes sense.\n\n> \n>> +\n>> +\t\tstrcpy(oc.path, name);\n>> +\t\tif (get_sha1_with_context(patch->old_sha1_prefix,\n>> +\t\t    GET_SHA1_BLOB | GET_SHA1_QUIETLY, patch_sha1, &oc)) {\n>> +\t\t\terror(_(\"the deleted SHA prefix of file '%s' (%s), does\"\n>> +\t\t\t      \" not seem to exist in this repository.\"), name,\n>> +\t\t\t      patch->old_sha1_prefix);\n>> +\t\t\treturn -1;\n>> +\t\t}\n> \n> This is not sufficient to make sure patch_sha1 exists in your\n> repository and is indeed a blob object.  GET_SHA1_BLOB is a hint to\n> say \"if there are more than one that shares this prefix, ignore ones\n> that are not blob---if there is only one remains, then even though\n> the prefix is ambiguous in this repository, we will take it\".  If\n> you only have a commit or a tree that has the prefix but not the\n> blob object the patch wants to touch, you will get the object name\n> of that commit or tree you have in your repository.\n> \n> Also oc is an output parameter; it is not about \"I want to make sure\n> the found object is at this pathname\"---that is an impossible request\n\nThanks for the clarification.  I didn't realize that.\n\n> to begin with.  I think something like this, without oc, would be\n> what you want:\n> \n> \tif (get_sha1_with_context(patch->old_sha1_prefix,\n>         \t\tGET_SHA1_BLOB | GET_SHA1_QUIETLY,\n>         \t\tpatch_sha1, NULL) ||\n> \t    sha1_object_info(patch_sha1, NULL) != OBJ_BLOB)\n> \t\treturn error(...);\n> \n> But I think the test itself (not the way you tested, but what you\n> are trying to test---the uniqueness of abbrevited object name) is\n> pointless.  The submitter of the patch may have far fewer objects\n> than you do, and it is perfectly normal if the old_sha1_prefix that\n> was sufficiently long to identify the blob for the submitter is not\n> unambiguous enough to identify the blob uniquely for you when you\n> try to apply the patch.  You may have other unrelated blobs that\n> happen to share the prefix in your repository.\n> \n> Hashing img->buf and making sure it matches old_sha1_prefix is the\n> best you can do.  If the extra ambiguity coming from that approach\n> bothers you, then the entire \"force apply an --irreversible-delete\n> patch\" idea also should.\n\nThat makes sense to me.  So then it would look something like:\n\n   hash_sha1_file(img->buf, img->len, blob_type, sha1);\n   if (strncmp(sha1_to_hex(sha1), patch->old_sha1_prefix, strlen(patch->old_sha1_prefix))\n\t return error(...)\n\nif I understand you correctly?\n\nThanks for the prompt review.\n\nPaul.\n--\n\n> \n>> +\t\thash_sha1_file(img->buf, img->len, blob_type, file_sha1);\n>> +\t\tif (memcmp(file_sha1, patch_sha1, 20)) {\n>> +\t\t\terror(_(\"the delete requested of '%s' (%s), does not\"\n>> +\t\t\t      \" match the current file contents.\"), name,\n>> +\t\t\t      sha1_to_hex(patch_sha1));\n>> +\t\t\treturn -1;\n> \n> return error(...);\n> \n"}]}