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

> > 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."

> >> +	/*
> >> +	 * 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.