Re: [PATCH] apply: refuse touching a file beyond symlink
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 30, 2015, 21:10 UTC
- Message-ID
- <xmqqmw50asan.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20150130204805.GA10616@peff.net>
Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Ah, OK. Yeah, doing it progressively can only be accurate if our > name-checks follow the same order as applying, because we are checking > against a particular state. > > But could we instead pull this check to just before the write-out time? > That is, to let any horrible thing happen in-core, as long as what we > write out to the index and the filesystem is sane?
That would make me feel dirty.
I noticed one thing. The PATH_TO_BE_DELETED/PATH_WAS_DELETED crud kicks in -only- during the actual application phase, and all patches that remove paths from the end result should have been appropriately marked in fn_table[] by the call to prepare_fn_table() at the beginning of check_patch_list() as PATH_TO_BE_DELETED.
But it was wrong to call previous_patch() in my fix. The function is the cause of evil I see in the "let's support concatenated patch, making the later patch depend on the result of earlier ones" and deliberately ignores PATH_TO_BE_DELETED patches. We would need to do the early part of previous_patch() without the filtering.
This is a preparatory step to clean-up the mess I have in mind. It does not mean to change the semantics (applied to the codebase with or without the changes we have been discussing); it only makes it always return the "previous" patch to the callers and makes them responsible to see if the previous was to-be-deleted or was-deleted.
With that change, I think my symlink fix plus the "check the deleted one with old_name, too" change has a better chance to do the moral equivalent of your two-phase thing. Essentially, "First see what will be deleted in the input as a whole" has already been done by the prepare_fn_table() thing.
builtin/apply.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c index 41b7236..a064017 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3097,25 +3097,12 @@ static int checkout_target(struct cache_entry *ce, struct stat *st) return 0; } -static struct patch *previous_patch(struct patch *patch, int *gone) +static struct patch *previous_patch(struct patch *patch) { - struct patch *previous; - - *gone = 0; if (patch->is_copy || patch->is_rename) return NULL; /* "git" patches do not depend on the order */ - previous = in_fn_table(patch->old_name); - if (!previous) - return NULL; - - if (to_be_deleted(previous)) - return NULL; /* the deletion hasn't happened yet */ - - if (was_deleted(previous)) - *gone = 1; - - return previous; + return in_fn_table(patch->old_name); } static int verify_index_match(const struct cache_entry *ce, struct stat *st) @@ -3170,11 +3157,11 @@ static int load_preimage(struct image *image, struct patch *previous; int status; - previous = previous_patch(patch, &status); - if (status) + previous = previous_patch(patch); + if (was_deleted(previous)) return error(_("path %s has been renamed/deleted"), patch->old_name); - if (previous) { + if (previous && !to_be_deleted(previous)) { /* We have a patched copy in memory; use that. */ strbuf_add(&buf, previous->result, previous->resultsize); } else { @@ -3384,18 +3371,18 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s { const char *old_name = patch->old_name; struct patch *previous = NULL; - int stat_ret = 0, status; + int stat_ret = 0; unsigned st_mode = 0; if (!old_name) return 0; assert(patch->is_new <= 0); - previous = previous_patch(patch, &status); + previous = previous_patch(patch); - if (status) + if (was_deleted(previous)) return error(_("path %s has been renamed/deleted"), old_name); - if (previous) { + if (previous && !to_be_deleted(previous)) { st_mode = previous->new_mode; } else if (!cached) { stat_ret = lstat(old_name, st); @@ -3403,6 +3390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s return error(_("%s: %s"), old_name, strerror(errno)); } + if (to_be_deleted(previous)) + previous = NULL; + if (check_index && !previous) { int pos = cache_name_pos(old_name, strlen(old_name)); if (pos < 0) {