On Mon, Feb 02, 2015 at 05:56:55PM -0800, Junio C Hamano wrote:
> > 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."
Ah, right, I forgot we covered this already in the earlier discussion
(but thanks for elaborating; I think the reason I forgot is that I did
not really understand all of the implications). If we do not have to
worry about that, then it's not a problem.
> >> + /*
> >> + * 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.
Ah, OK. I totally misread it, thinking that load_patch_target was what
set up the symlink-table, and that was what you were referring to. It
might be more clear after "...that is called at the beginning of
apply_data()" to add "...so we do not have to worry about that case
here".
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html