Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2015, 01:56 UTC
Message-ID: <xmqq61bjsqoo.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150203011139.GC31946@peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:
>
>> +static struct string_list symlink_changes;
>
> I notice we don't duplicate strings for this list. Are the paths we
> register here always stable? I think they should be, as they are part of
> the "struct patch".

Yeah, and I also forgot to free this string-list itself. Needs fixing.

Show 10 quoted lines

> I think this means we'll be
> overly cautious with a patch that does:
>
>   1. add foo as a symlink
>
>   2. remove foo
>
>   3. add foo/bar
>
> which is perfectly OK

No, such a patchset is broken.

A valid "git apply" input must *not* depend on the order of patches in it. The consequence is that "an input to 'git apply' must not mention the fate of each path at most once."

"create B by copying A" and "modify A in-place" can appear in the same patchset in any order, and the new file B will have the contents from the original A, not the result of modifying A in-place, which is what will be in the resulting A. That is how "git diff" expresses renames and copies, and that is why rearranging the patchset using "git diff -Oorderfile" is safe.

> but we'll reject. I suspect this is making things
> much simpler for you, because now we don't have to worry about order of
> application that we were discussing the other day.

It is not "now this new decision made things simpler". "git diff" output and "git apply" application have been designed to work that way from day one. At least from day one of rename/copy feature.

We probably should start thinking about ripping out the fn_table[] crud. It fundamentally cannot correctly work on an input that concatenates more than one "git diff" outputs that have renames and/or copies of the same file, and it never will, and that is not due to a bug in the implementation.

The reason why the incremental application is a fundamentally flawed concept in the context of "git apply" is because you cannot tell the boundary between the original "git diff" outputs.

Imagine that you have three versions, A, B and C, and the original two "git diff -M A B" and "git diff -M B C" output said this:

    (A -> B)
    edit X in place and add two lines at the beginning
    create Z by copying X

    (B -> C)
    create Y by renaming X and add a line at the end

If you take "git diff -M A C", it should say:

    (A -> C)
    edit X in place and add two lines at the beginning
    create Y by copying X and add two lines at the beginning
        and a line at the end
    create Z by copying X

Now, if you concatenate two "git diff" outputs and feed it to "git apply", you would want it to express a patchset that goes from A to C, but think if you can really get such a semantics.

    edit X in place and add two lines at the beginning
    create Z by copying X
    create Y by renaming X and add a line at the end

You fundamentally cannot tell that Z needs to be a copy of X before the change to X (which is what the usual "git apply" does), but Y needs to start from a copy of X after the change to X. There is no clue to tell "git apply" that there is a boundary between the first two operations and the third one. It is impossible for the concatenated patch to express the same thing as "(A -> C)" patch does, without inventng some "I am now switching to a new basis" marker in the "git apply" input stream.

Show 12 quoted lines

>> +	/*
>> +	 * An attempt to read from or delete a path that is beyond
>> +	 * a symbolic link will be prevented by load_patch_target()
>> +	 * that is called at the beginning of apply_data().  We need
>> +	 * to make sure that the patch result is not deposited to
>> +	 * a path that is beyond a symbolic link ourselves.
>> +	 */
>> +	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
>> +		return error(_("affected file '%s' is beyond a symbolic link"),
>> +			     patch->new_name);
>
> Do we need to check the patch->is_delete case here (with patch->old_name)?

> I had a suspicion that the new patch 3/4 to check the reading-side might
> help with that, but the comment here sounds like we do need to check
> here, too

Hmm, the comment above was meant to tell you that we do not have to worry about the deletion case (because load_patch_target() will try to read the original to verify we are deleting what we expect to delete at the beginning of apply_data(), and it will notice that old_name is beyond a symbolic link), but we still need to check the non-deletion case. Strictly speaking, modify-in-place case does not have to depend on this code (the same load_patch_target() check will catch it because it wants to check the preimage).

May need rephrasing to clarify but I thought it was clear enough.