Re: [PATCH] apply: refuse touching a file beyond symlink
- From
Jeff King <peff@peff.net>
- Date
- Jan 30, 2015, 20:16 UTC
- Message-ID
- <20150130201620.GA4133@peff.net>
- In-Reply-To
- <xmqq1tmcc9l9.fsf@gitster.dls.corp.google.com>
On Fri, Jan 30, 2015 at 12:11:30PM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> I am not sure how to fix this, without completely ripping out the > misguided "We should be able to concatenate outputs from multiple > invocations of 'git diff' into a single file and apply the result > with a single invocation of 'git apply'" change I grudgingly > accepted long time ago (7a07841c (git-apply: handle a patch that > touches the same path more than once better, 2008-06-27). > > "git diff" output is designed each patch to apply independently to > the preimage to produce the postimage, and that allows patches to > two files can be swapped via -Oorderfile mechanism, and also "X was > created by copying from Y and Y is modified in place" will result in > X with the contents of Y in the preimage (i.e. before the in-place > modification of Y in the same patch) regardless of the order of X > and Y in the "git diff" output. The above input used by t4114.11 > expects to remove 'foo/baz' (leaving an empty directory foo as an > result but we do not track directories so it can be nuked to make > room if other patch in the same input wants to put something else, > either a regular file or a symbolic link, there) and create a blob > at 'foo', and such an input should apply regardless of the order of > patches in it. > > The in_fn_table[] stuff broke that design completely.
I had the impression that we did not apply in any arbitrary order that could work, but rather that we did deletions first followed by additions. But I am fairly ignorant of the apply code.
If that assumption is correct, then I think we could just follow the same phases that the actual application does. Here's a hacky version below. Probably the check of phase versus is_delete needs to be better (and ideally the logic would be factored out of write_one_result so they always match).
diff --git a/builtin/apply.c b/builtin/apply.c index f5491cd..85364b8 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3593,7 +3593,7 @@ symlink_found: * Check and apply the patch in-core; leave the result in patch->result * for the caller to write it out to the final destination. */ -static int check_patch(struct patch *patch) +static int check_patch(struct patch *patch, int phase) { struct stat st; const char *old_name = patch->old_name; @@ -3604,6 +3604,9 @@ static int check_patch(struct patch *patch) int ok_if_exists; int status; + if (!phase != patch->is_delete) + return 0; + patch->rejected = 1; /* we will drop this after we succeed */ status = check_preimage(patch, &ce, &st); @@ -3679,6 +3682,9 @@ static int check_patch(struct patch *patch) if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) return error(_("affected file '%s' is beyond a symbolic link"), patch->new_name); + if (patch->is_delete && path_is_beyond_symlink(patch->old_name)) + return error(_("affected file '%s' is beyond a symbolic link"), + patch->old_name); if (apply_data(patch, &st, ce) < 0) return error(_("%s: patch does not apply"), name); @@ -3686,7 +3692,7 @@ static int check_patch(struct patch *patch) return 0; } -static int check_patch_list(struct patch *patch) +static int check_patch_list_1(struct patch *patch, int phase) { int err = 0; @@ -3695,12 +3701,22 @@ static int check_patch_list(struct patch *patch) if (apply_verbosely) say_patch_name(stderr, _("Checking patch %s..."), patch); - err |= check_patch(patch); + err |= check_patch(patch, phase); patch = patch->next; } return err; } +static int check_patch_list(struct patch *patch) +{ + int err = 0; + int phase; + + for (phase = 0; phase < 2; phase++) + err |= check_patch_list_1(patch, phase); + return err; +} + /* This function tries to read the sha1 from the current index */ static int get_current_sha1(const char *path, unsigned char *sha1) {