Download raw body.
check file status before applying patches (second try)
Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Mar 12, 2022 at 11:22:37PM +0100, Omar Polo wrote:
> > Hello,
> >
> > currently 'got patch' is happy to do all sorts of things to all sorts of
> > files. Well, it's not really ideal I think. The following patch adds
> > some checks to the targeted files before patching 'em.
> >
> > I think it's fine to allow patching modified files, but 'got patch'
> > should disallow patching non-existant or unknown files. Or add files
> > that are already there, or delete files there are not known.
> >
> > There's a bit of churn because I just had to re-introduce in apply_patch
> > the pathlists i segregated to schedule_* in previous commit. Also, at
> > this points those two helpers are useless.
> >
> > This actually made the rename test to fail because in the last part
> > `eta' (the target of the rename) exists (untracked) and so it won't
> > proceed with the rename.
> >
> > note that it needs the latest commit by stsp "fix 'got status' with an
> > obstructed file".
> >
> > thoughts?
>
> This is a great start, yes.
>
> Some comments below.
>
> > +can_rm(void *arg, unsigned char status, unsigned char staged_status,
>
> > +can_add(void *arg, unsigned char status, unsigned char staged_status,
>
> > +can_edit(void *arg, unsigned char status, unsigned char staged_status,
>
> Please document the status checks done by these functions in
> the man page. These are essentially pre-conditions which are
> specific to 'got patch' and should be documented.
> Otherwise errors raised here will be difficult to interpret.
yeah absolutely. i forgot about it, sorry. Updated diff belows has
also some manpage improvements too. (but mind that i suck at writing,
so any suggestion is well accepted :-)
> If a patch file contains a series of patches which will be applied
> in sequence, then patches which pass pre-conditions will affect
> the work tree, and any later patch which does not pass pre-conditions
> will make 'got patch' error out, leaving any modifications made by
> previous patches behind.
> I think this behaviour is correct. The application of patch N to
> the work tree can affect pre-condition checks for patch N+1, so
> there is no way to check pre-conditions for patch N+1 before
> applying patch N to the work tree.
> Still, this might be worth documenting because users might otherwise
> expect that 'got patch' will not modify the work tree in any way
> unless *all* patches in the input pass pre-condition checks.
Yep, i tried to point this out in the diff below.
In theory it should be possible to apply a patch in a way that if
everything succeeds then the changes are persisted but otherwise the
worktree is unmodified, but it seems a bit difficult to do correctly.
If we strengthen the preconditions so that only unmodified files are
allowed to be changed, than we could run an implicit 'got revert' but
i don't really like the idea. Producing .rej files (which atm we still
don't do) or conflict markers on failures but still trying to continue
seems the most useful behavior to me, at least by default.
(I have a follow-up diff to add a nop mode to test a diff without
affecting the worktree and another one after to recover from some
classes of errors.)
> > - err = got_worktree_resolve_path(&oldpath, worktree,
> > - p->old != NULL ? p->old : p->new);
> > + TAILQ_INIT(&oldpaths);
> > + TAILQ_INIT(&newpaths);
> > +
> > + err = build_pathlist(p->old != NULL ? p->old : p->new, &oldpath,
> > + &oldpaths, worktree);
> > if (err)
> > goto done;
>
> Maybe worktree.c should provide a build_pathlist style funcion?
> This function returns a pathlist that corresponds to a single path
> in the work tree, and could be useful elsewhere, e.g. in got.c?
> In any case, we can refactor this later.
I'm not really sure which other parts of got works with only one file at
time...
Furthermore, I'm expecting to handle some other paths operation there in
the future. Atm we fail to work on `got checkout -p ...' worktrees and
with patches that have "non-worktree-absolute" paths. I still haven't
solved these problems, but I'm guessing that I'll be able to solve them
in `build_pathlist'.
> > if (p->old != NULL && p->new == NULL) {
> > /*
> > * special case: delete a file. don't try to match
> > * the lines but just schedule the removal.
> > */
>
> In the long term, this code should try to match the lines.
> If lines don't match then we are deleting "with fuzz", which
> might not be what the user wants. It should at least result in a
> warning or fuzz factor or similar appearing in progress output.
absolutely. This "special case" is really just a workaround for the
missing fuzzying in locate_hunk and test_hunk.
> > +test_patch_illegal_status() {
> > + local testroot=`test_init patch_illegal_status`
> > +
> > + got checkout $testroot/repo $testroot/wt > /dev/null
> > + ret=$?
> > + if [ $ret -ne 0 ]; then
> > + test_done $testroot $ret
> > + return 1
> > + fi
> > +
> > + # edit an non-existant and unknown file
>
> typo: should say "non-existent"
fixed
> > + cat <<EOF > $testroot/wt/patch
> > +--- iota
> > ++++ iota
> > +@@ -1 +1 @@
> > +- iota
> > ++ IOTA
> > +EOF
> > +
> > + (cd $testroot/wt && got patch patch) > /dev/null \
> > + 2> $testroot/stderr
> > + ret=$?
> > + if [ $ret -eq 0 ]; then
> > + echo "edited a missing file" >&2
> > + test_done $testroot $ret
> > + return 1
> > + fi
> > +
> > + echo "got: iota: file has unexpected status" \
> > + > $testroot/stderr.expected
>
> Maybe refine this error to say "iota: No such file or directory"?
> You could use got_error_set_errno(ENOENT, path) somewhere in this
> specific case.
agreed, it's more intuitive this way. I've used got_error_set_errno in
can_rm and can_edit if the file is NONEXISTENT.
I've also added two other checks to test_patch_illegal_status.
-----------------------------------------------
commit 5044ab17d83bcf78d9cca7d38815f9e9e6427e2d
from: Omar Polo <op@omarpolo.com>
date: Sun Mar 13 10:06:20 2022 UTC
check file status before applying the patch
Don't allow `got patch' to delete file that are not known, or add files
that are already known (but as an exception allow to re-add missing
files) and only change file that are known and without conflicts or
obstructions.
diff c0c2b3d1e2c80fe9ee36670cff033f948cca9fd6 5b12f37b51cbef2c1d56cba3d6811d8172c60a5d
blob - 09c52c1e9bd95ceb4f5fa1f135b3fb570b7285b0
blob + 4bcea52af617043cda2fa50d87f21effce198199
--- got/got.c
+++ got/got.c
@@ -7245,7 +7245,7 @@ cmd_patch(int argc, char *argv[])
#endif
error = got_patch(patchfd, worktree, repo, &print_remove_status,
- NULL, &add_progress, NULL);
+ NULL, &add_progress, NULL, check_cancelled, NULL);
done:
if (repo) {
blob - 04a23fc7f7da83078d23e8ec24820ce98aa43702
blob + 448bb17e65d75d8d5ba94bd577f5cde4fef566bf
--- include/got_patch.h
+++ include/got_patch.h
@@ -22,4 +22,5 @@
*/
const struct got_error *
got_patch(int, struct got_worktree *, struct got_repository *,
- got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *);
+ got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *,
+ got_cancel_cb, void *);
blob - 8dc632ef6f52371e65674a21a73f8c9d2496afd5
blob + a8c35b3dbc60b36d31157aa8c3a0e6fb6d789bb3
--- lib/patch.c
+++ lib/patch.c
@@ -28,6 +28,7 @@
#include <sys/socket.h>
#include <sys/uio.h>
+#include <errno.h>
#include <limits.h>
#include <sha1.h>
#include <stdint.h>
@@ -381,44 +382,6 @@ apply_hunk(FILE *tmp, struct got_patch_hunk *h, long *
}
static const struct got_error *
-schedule_add(const char *path, struct got_worktree *worktree,
- struct got_repository *repo, got_worktree_checkout_cb add_cb,
- void *add_arg)
-{
- static const struct got_error *err = NULL;
- struct got_pathlist_head paths;
- struct got_pathlist_entry *pe;
-
- TAILQ_INIT(&paths);
-
- err = got_pathlist_insert(&pe, &paths, path, NULL);
- if (err == NULL)
- err = got_worktree_schedule_add(worktree, &paths,
- add_cb, add_arg, repo, 1);
- got_pathlist_free(&paths);
- return err;
-}
-
-static const struct got_error *
-schedule_del(const char *path, struct got_worktree *worktree,
- struct got_repository *repo, got_worktree_delete_cb delete_cb,
- void *delete_arg)
-{
- static const struct got_error *err = NULL;
- struct got_pathlist_head paths;
- struct got_pathlist_entry *pe;
-
- TAILQ_INIT(&paths);
-
- err = got_pathlist_insert(&pe, &paths, path, NULL);
- if (err == NULL)
- err = got_worktree_schedule_delete(worktree, &paths,
- 0, NULL, delete_cb, delete_arg, repo, 0, 0);
- got_pathlist_free(&paths);
- return err;
-}
-
-static const struct got_error *
patch_file(struct got_patch *p, const char *path, FILE *tmp)
{
const struct got_error *err = NULL;
@@ -503,33 +466,132 @@ done:
}
static const struct got_error *
+build_pathlist(const char *p, char **path, struct got_pathlist_head *head,
+ struct got_worktree *worktree)
+{
+ const struct got_error *err;
+ struct got_pathlist_entry *pe;
+
+ err = got_worktree_resolve_path(path, worktree, p);
+ if (err == NULL)
+ err = got_pathlist_insert(&pe, head, *path, NULL);
+ return err;
+}
+
+static const struct got_error *
+can_rm(void *arg, unsigned char status, unsigned char staged_status,
+ const char *path, struct got_object_id *blob_id,
+ struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+ int dirfd, const char *de_name)
+{
+ if (status == GOT_STATUS_NONEXISTENT)
+ return got_error_set_errno(ENOENT, path);
+ if (status != GOT_STATUS_NO_CHANGE &&
+ status != GOT_STATUS_ADD &&
+ status != GOT_STATUS_MODIFY &&
+ status != GOT_STATUS_MODE_CHANGE)
+ return got_error_path(path, GOT_ERR_FILE_STATUS);
+ if (staged_status == GOT_STATUS_DELETE)
+ return got_error_path(path, GOT_ERR_FILE_STATUS);
+ return NULL;
+}
+
+static const struct got_error *
+can_add(void *arg, unsigned char status, unsigned char staged_status,
+ const char *path, struct got_object_id *blob_id,
+ struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+ int dirfd, const char *de_name)
+{
+ if (status != GOT_STATUS_NONEXISTENT &&
+ status != GOT_STATUS_MISSING)
+ return got_error_path(path, GOT_ERR_FILE_STATUS);
+ return NULL;
+}
+
+static const struct got_error *
+can_edit(void *arg, unsigned char status, unsigned char staged_status,
+ const char *path, struct got_object_id *blob_id,
+ struct got_object_id *staged_blob_id, struct got_object_id *commit_id,
+ int dirfd, const char *de_name)
+{
+ if (status == GOT_STATUS_NONEXISTENT)
+ return got_error_set_errno(ENOENT, path);
+ if (status != GOT_STATUS_NO_CHANGE &&
+ status != GOT_STATUS_ADD &&
+ status != GOT_STATUS_MODIFY)
+ return got_error_path(path, GOT_ERR_FILE_STATUS);
+ if (staged_status == GOT_STATUS_DELETE)
+ return got_error_path(path, GOT_ERR_FILE_STATUS);
+ return NULL;
+}
+
+static const struct got_error *
+check_file_status(struct got_patch *p, int file_renamed,
+ struct got_worktree *worktree, struct got_repository *repo,
+ struct got_pathlist_head *old, struct got_pathlist_head *new,
+ got_cancel_cb cancel_cb, void *cancel_arg)
+{
+ static const struct got_error *err;
+
+ if (p->old != NULL && p->new == NULL)
+ return got_worktree_status(worktree, old, repo, 0,
+ can_rm, NULL, cancel_cb, cancel_arg);
+ else if (file_renamed) {
+ err = got_worktree_status(worktree, old, repo, 0,
+ can_rm, NULL, cancel_cb, cancel_arg);
+ if (err)
+ return err;
+ return got_worktree_status(worktree, new, repo, 0,
+ can_add, NULL, cancel_cb, cancel_arg);
+ } else if (p->old == NULL)
+ return got_worktree_status(worktree, new, repo, 0,
+ can_add, NULL, cancel_cb, cancel_arg);
+ else
+ return got_worktree_status(worktree, new, repo, 0,
+ can_edit, NULL, cancel_cb, cancel_arg);
+}
+
+static const struct got_error *
apply_patch(struct got_worktree *worktree, struct got_repository *repo,
struct got_patch *p, got_worktree_delete_cb delete_cb, void *delete_arg,
- got_worktree_checkout_cb add_cb, void *add_arg)
+ got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
+ void *cancel_arg)
{
const struct got_error *err = NULL;
+ struct got_pathlist_head oldpaths, newpaths;
int file_renamed = 0;
char *oldpath = NULL, *newpath = NULL;
char *tmppath = NULL, *template = NULL;
FILE *tmp = NULL;
- err = got_worktree_resolve_path(&oldpath, worktree,
- p->old != NULL ? p->old : p->new);
+ TAILQ_INIT(&oldpaths);
+ TAILQ_INIT(&newpaths);
+
+ err = build_pathlist(p->old != NULL ? p->old : p->new, &oldpath,
+ &oldpaths, worktree);
if (err)
goto done;
- err = got_worktree_resolve_path(&newpath, worktree,
- p->new != NULL ? p->new : p->old);
+ err = build_pathlist(p->new != NULL ? p->new : p->old, &newpath,
+ &newpaths, worktree);
if (err)
goto done;
+ if (p->old != NULL && p->new != NULL && strcmp(p->old, p->new))
+ file_renamed = 1;
+
+ err = check_file_status(p, file_renamed, worktree, repo, &oldpaths,
+ &newpaths, cancel_cb, cancel_arg);
+ if (err)
+ goto done;
+
if (p->old != NULL && p->new == NULL) {
/*
* special case: delete a file. don't try to match
* the lines but just schedule the removal.
*/
- err = schedule_del(p->old, worktree, repo, delete_cb,
- delete_arg);
+ err = got_worktree_schedule_delete(worktree, &oldpaths,
+ 0, NULL, delete_cb, delete_arg, repo, 0, 0);
goto done;
}
@@ -551,26 +613,27 @@ apply_patch(struct got_worktree *worktree, struct got_
goto done;
}
- file_renamed = p->old != NULL && strcmp(p->old, p->new);
if (file_renamed) {
- err = schedule_del(oldpath, worktree, repo, delete_cb,
- delete_arg);
+ err = got_worktree_schedule_delete(worktree, &oldpaths,
+ 0, NULL, delete_cb, delete_arg, repo, 0, 0);
if (err == NULL)
- err = schedule_add(newpath, worktree, repo,
- add_cb, add_arg);
+ err = got_worktree_schedule_add(worktree, &newpaths,
+ add_cb, add_arg, repo, 1);
} else if (p->old == NULL)
- err = schedule_add(newpath, worktree, repo, add_cb,
- add_arg);
+ err = got_worktree_schedule_add(worktree, &newpaths,
+ add_cb, add_arg, repo, 1);
else
printf("M %s\n", oldpath); /* XXX */
done:
- if (err != NULL && (file_renamed || p->old == NULL))
+ if (err != NULL && newpath != NULL && (file_renamed || p->old == NULL))
unlink(newpath);
free(template);
if (tmppath != NULL)
unlink(tmppath);
free(tmppath);
+ got_pathlist_free(&oldpaths);
+ got_pathlist_free(&newpaths);
free(oldpath);
free(newpath);
return err;
@@ -579,7 +642,8 @@ done:
const struct got_error *
got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo,
got_worktree_delete_cb delete_cb, void *delete_arg,
- got_worktree_checkout_cb add_cb, void *add_arg)
+ got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
+ void *cancel_arg)
{
const struct got_error *err = NULL;
struct imsgbuf *ibuf;
@@ -628,7 +692,7 @@ got_patch(int fd, struct got_worktree *worktree, struc
break;
err = apply_patch(worktree, repo, &p, delete_cb, delete_arg,
- add_cb, add_arg);
+ add_cb, add_arg, cancel_cb, cancel_arg);
patch_free(&p);
if (err)
break;
blob - 5916921f260f5a0eaefdbf6036b813fbf3570dd5
blob + 36c9ef55d45f97cd15c3f787c35d5a7623def72a
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -685,6 +685,7 @@ EOF
test_done $testroot $ret
return 1
fi
+ rm $testroot/wt/eta
cat <<EOF > $testroot/wt/patch
--- alpha
@@ -730,6 +731,151 @@ EOF
test_done $testroot $ret
}
+test_patch_illegal_status() {
+ local testroot=`test_init patch_illegal_status`
+
+ got checkout $testroot/repo $testroot/wt > /dev/null
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ test_done $testroot $ret
+ return 1
+ fi
+
+ # edit an non-existent and unknown file
+ cat <<EOF > $testroot/wt/patch
+--- iota
++++ iota
+@@ -1 +1 @@
+- iota
++ IOTA
+EOF
+
+ (cd $testroot/wt && got patch patch) > /dev/null \
+ 2> $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "edited a missing file" >&2
+ test_done $testroot $ret
+ return 1
+ fi
+
+ echo "got: iota: No such file or directory" \
+ > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done $testroot $ret
+ return 1
+ fi
+
+ # create iota and re-try
+ echo iota > $testroot/wt/iota
+
+ (cd $testroot/wt && got patch patch) > /dev/null \
+ 2> $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "patched an unknown file" >&2
+ test_done $testroot $ret
+ return 1
+ fi
+
+ echo "got: iota: file has unexpected status" \
+ > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done $testroot $ret
+ return 1
+ fi
+
+ rm $testroot/wt/iota
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ test_done $testroot $ret
+ return 1
+ fi
+
+ # edit obstructed file
+ rm $testroot/wt/alpha
+ mkdir $testroot/wt/alpha
+ cat <<EOF > $testroot/wt/patch
+--- alpha
++++ alpha
+@@ -1 +1,2 @@
+ alpha
++was edited
+EOF
+
+ (cd $testroot/wt && got patch patch) > /dev/null \
+ 2> $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "edited a missing file" >&2
+ test_done $testroot $ret
+ return 1
+ fi
+
+ echo "got: alpha: file has unexpected status" \
+ > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -ne 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done $testroot $ret
+ return 1
+ fi
+
+ # delete an unknown file
+ cat <<EOF > $testroot/wt/patch
+--- iota
++++ /dev/null
+@@ -1 +0,0 @@
+-iota
+EOF
+
+ (cd $testroot/wt && got patch patch) > /dev/null \
+ 2> $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "deleted a missing file?" >&2
+ test_done $testroot $ret
+ return 1
+ fi
+
+ echo "got: iota: No such file or directory" \
+ > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ test_done $testroot $ret
+ return 1
+ fi
+
+ # try again with iota in place but still not registered
+ echo iota > $testroot/wt/iota
+ (cd $testroot/wt && got patch patch) > /dev/null \
+ 2> $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ echo "deleted an unversioned file?" >&2
+ test_done $testroot $ret
+ return 1
+ fi
+
+ echo "got: iota: file has unexpected status" \
+ > $testroot/stderr.expected
+ cmp -s $testroot/stderr.expected $testroot/stderr
+ ret=$?
+ if [ $ret -eq 0 ]; then
+ diff -u $testroot/stderr.expected $testroot/stderr
+ fi
+ test_done $testroot $ret
+}
+
test_parseargs "$@"
run_test test_patch_simple_add_file
run_test test_patch_simple_rm_file
@@ -743,3 +889,4 @@ run_test test_patch_malformed
run_test test_patch_no_patch
run_test test_patch_equals_for_context
run_test test_patch_rename
+run_test test_patch_illegal_status
-----------------------------------------------
commit 82f60a9a23894c1fa5c3098d8d9d9559c2ad2329 (patchcfs)
from: Omar Polo <op@omarpolo.com>
date: Sun Mar 13 10:06:20 2022 UTC
improve `got patch' section of the manpage
Simplify some pharsings, explain what preconditions `got patch' has and
what happens to the worktree after an error occurs.
diff 5b12f37b51cbef2c1d56cba3d6811d8172c60a5d 9eec00bfb3e7e3a68db41095f894e1f33a0d4727
blob - 61596d1cfc534598b2516ffa737754cfb667abb1
blob + d7bdf56abfff3111d815aab14e11d14fb3c31c58
--- got/got.1
+++ got/got.1
@@ -889,7 +889,6 @@ arguments are provided, only show differences for the
Cannot be used together with the
.Fl P
option.
-.Pp
.It Fl C Ar number
Set the number of context lines shown in the diff.
By default, 3 lines of context are shown.
@@ -1291,18 +1290,19 @@ option)
Apply changes from
.Ar patchfile
.Pq or standard input
-and record the state of the affected files afterwards.
-The content of
-.Ar patchfile
-must be an unified diff.
+that must be an unified diff.
If
.Ar patchfile
contains more than one patch,
.Nm
.Cm patch
-will try to apply them all.
+will try to apply each of them in sequence.
+Files added or removed from the repository as consequence are automatically
+scheduled as such.
.Pp
-Show the status of each affected file, using the following status codes:
+While applying the
+.Ar patchfile ,
+show the status of each affected file, using the following status codes:
.Bl -column XYZ description
.It M Ta modified file
.It D Ta deleted file
@@ -1314,6 +1314,23 @@ If a change does not match at its exact line number,
.Cm patch
applies it somewhere else in the file if it can find a good spot before
giving up.
+.Pp
+.Nm
+.Cm patch
+will refuse to apply a patch if certain preconditions are not met.
+Files to be deleted must be present in the repository and not have been
+scheduled for deletion already.
+Files to be added must be unknown and not present in the worktree, but
+are also allowed to be missing.
+Files to be modified must be already present in the repository and in
+the worktree, and not have conflicts or being obstructed.
+.Pp
+If a patch fails to apply for any reason
+.Nm
+stops and the affected file is unchanged.
+Modifications done to the worktree due to previous patches in the same
+.Ar patchfile
+are still visible.
.Tg rv
.It Cm revert Oo Fl p Oc Oo Fl F Ar response-script Oc Oo Fl R Oc Ar path ...
.Dl Pq alias: Cm rv
check file status before applying patches (second try)