On 12-08-02 05:20 PM, Junio C Hamano wrote:
> Paul Gortmaker <paul.gortma...@windriver.com> writes:
>
>> The '-D' or '--irreversible-delete' option of format-patch is
>> great for sending out patches to mailing lists, where there
>> is little value in seeing thousands of lines of deleted code.
>> Attention can then be focused on the changes relating to
>> the binding of the deleted code (Makefiles, etc).
>>
>> However the original intent of commit 467ddc14f ("git diff -D: omit
>> the preimage of deletes") was as follows:
>>
>> To prevent such a patch from being applied by mistake, the
>> output is designed not to be usable by "git apply" (or GNU "patch");
>> it is strictly for human consumption.
>>
>> The downside of this, is that patches to mailing lists which are
>> then either managed with patchworks, or dealt with directly by
>> maintainers, will need manual intervention if they are to be used.
>>
>> But with the index lines, there is no reason why we can't act
>> intelligently and automatically on these with "git apply".
>> If we can unambiguously map what was recorded as the deleted
>> SHA prefix to the SHA of the matching blob filename in our tree,
>> then we set the image len to zero which facilitates the delete.
>>
>> Signed-off-by: Paul Gortmaker <paul.gortma...@windriver.com>
>> ---
>>
>> For a recent use case example, see:
>> http://www.spinics.net/lists/netdev/msg206519.html
>>
>> Could be wrapped in an "am.applyirreversible" if for some reason
>> global deployment was considered unwise?
>>
>> Documentation/diff-options.txt | 5 +++--
>> builtin/apply.c | 30 ++++++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index cf4b216..efaaf1c 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -328,8 +328,9 @@ endif::git-log[]
>> --irreversible-delete::
>> Omit the preimage for deletes, i.e. print only the header but not
>> the diff between the preimage and `/dev/null`. The resulting patch
>> - is not meant to be applied with `patch` nor `git apply`; this is
>> - solely for people who want to just concentrate on reviewing the
>> + is not meant to be applied with `patch` (but can be with `git apply`).
>
> ... but only when you are applying to the exact version the patch
> was created from, no?
True, I can add that extra detail/limitation to the docs.
>
>> + This is for people who want to avoid seeing/mailing all the deleted
>> + file content, and instead just concentrate on reviewing the
>> text after the change. In addition, the output obviously lack
>> enough information to apply such a patch in reverse, even manually,
>> hence the name of the option.
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index d453c83..363da63 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -2933,6 +2933,36 @@ static int apply_fragments(struct image *img, struct
>> patch *patch)
>> if (patch->is_binary)
>> return apply_binary(img, patch);
>>
>> + /* output from --irreversible-delete (looks like empty file delete) */
>> + if (patch->is_delete > 0 && !frag && img->len > 0) {
>
> What is (img->len > 0) part trying to ensure?
>
> If somebody gives you an irreversible deletion of an empty file,
> shouldn't this codepath handle it the same way?
The format-patch output of the deletion of an empty file is
identical with or without the switch, so I didn't want to
accidentally limit people from normal empty file deletions
by invoking special checks on them that did not exist before.
>
>> + unsigned char file_sha1[20], patch_sha1[20];
>> + struct object_context oc;
>> +
>> + if (apply_in_reverse) {
>> + error(_("can not reverse an irreversible-delete patch"
>> + "on file '%s'."), name);
>> + return -1;
>> + }
>
> The return value of error() is already -1, so you can just return
> it without { stmt; return -1; }.
OK, will update. I'd inadvertently got the separate statements
by copying the code below it, which did a conditional return based
on what apply_with_reject was set to, but I'm not sure any special
reject behaviour for an irreversible delete fail makes sense.
>
>> +
>> + strcpy(oc.path, name);
>> + if (get_sha1_with_context(patch->old_sha1_prefix,
>> + GET_SHA1_BLOB | GET_SHA1_QUIETLY, patch_sha1, &oc)) {
>> + error(_("the deleted SHA prefix of file '%s' (%s), does"
>> + " not seem to exist in this repository."), name,
>> + patch->old_sha1_prefix);
>> + return -1;
>> + }
>
> This is not sufficient to make sure patch_sha1 exists in your
> repository and is indeed a blob object. GET_SHA1_BLOB is a hint to
> say "if there are more than one that shares this prefix, ignore ones
> that are not blob---if there is only one remains, then even though
> the prefix is ambiguous in this repository, we will take it". If
> you only have a commit or a tree that has the prefix but not the
> blob object the patch wants to touch, you will get the object name
> of that commit or tree you have in your repository.
>
> Also oc is an output parameter; it is not about "I want to make sure
> the found object is at this pathname"---that is an impossible request
Thanks for the clarification. I didn't realize that.
> to begin with. I think something like this, without oc, would be
> what you want:
>
> if (get_sha1_with_context(patch->old_sha1_prefix,
> GET_SHA1_BLOB | GET_SHA1_QUIETLY,
> patch_sha1, NULL) ||
> sha1_object_info(patch_sha1, NULL) != OBJ_BLOB)
> return error(...);
>
> But I think the test itself (not the way you tested, but what you
> are trying to test---the uniqueness of abbrevited object name) is
> pointless. The submitter of the patch may have far fewer objects
> than you do, and it is perfectly normal if the old_sha1_prefix that
> was sufficiently long to identify the blob for the submitter is not
> unambiguous enough to identify the blob uniquely for you when you
> try to apply the patch. You may have other unrelated blobs that
> happen to share the prefix in your repository.
>
> Hashing img->buf and making sure it matches old_sha1_prefix is the
> best you can do. If the extra ambiguity coming from that approach
> bothers you, then the entire "force apply an --irreversible-delete
> patch" idea also should.
That makes sense to me. So then it would look something like:
hash_sha1_file(img->buf, img->len, blob_type, sha1);
if (strncmp(sha1_to_hex(sha1), patch->old_sha1_prefix,
strlen(patch->old_sha1_prefix))
return error(...)
if I understand you correctly?
Thanks for the prompt review.
Paul.
--
>
>> + hash_sha1_file(img->buf, img->len, blob_type, file_sha1);
>> + if (memcmp(file_sha1, patch_sha1, 20)) {
>> + error(_("the delete requested of '%s' (%s), does not"
>> + " match the current file contents."), name,
>> + sha1_to_hex(patch_sha1));
>> + return -1;
>
> return error(...);
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
