From: Omar Polo Subject: Re: got patch: resolve paths from $PWD To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Fri, 22 Apr 2022 16:47:59 +0200 Stefan Sperling wrote: > On Tue, Apr 19, 2022 at 10:40:11PM +0200, Omar Polo wrote: > > hello, > > > > as reminded by naddy@, 'got patch' resolve paths from the worktree root > > and not from the current working directory. > > > > let's fix it. > > > > The attached diff changes `patch_check_path' to build the paths from the > > current directory. I'm also moving the pathlist handling from got_patch > > to patch_check_path too, it makes more sense to fix the pathlist where > > we're also doing the other path operations. > > > > The only bit I don't really like is the changes at the end of > > apply_patch. I want to call report_progress with paths that are > > absolute from the worktree root, and that's what happens when > > You mean "relative to the work tree root"? yep, exactly. > > report_progress is called via patch_add/delete (the callbacks for > > got_worktree_schedule_add/delete.) But for the "edited file" case I > > have to peek into the tailq. The `newpath' and `oldpath' paths are > > relative to $PWD, so I can't use them. It's not incredibly bad, I know > > for sure that the pathlists have one and exactly one element in them, > > but it doesn't read too well IMHO. (but neither too bad maybe?) > > Would it help to convert paths to relative form only just before passing > them on for display purposes? You can always obtain the worktree's root > path if you can get at a pointer to your struct got_worktree somehow, > to make any absolute path relative the work tree root. > > I don't understand why you would need to store relative paths on a queue. > In general, any paths which get passed around should be absolute. > Someone might adjust this code later, not realizing which paths are > relative and which are absolute, and easily create a bug. i agree with using absolute paths as much as possible, but we're already storing "relative" paths in pathlists. the current code use got_worktree_resolve_path to resolve a path from the patch which returns a path that's relative to the work tree root. I was confused by which functions wants an absolute path and which ones are fine with relative ones. For example (fixed in the diff below) got_fileindex_entry_get wants (for what i can see) a path relative to the work tree root, but get_file_status wants an absolute path (even if it doesn't enforce it for the case dir=-1) while I was passing to it a relative (to the worktree) path. > You are already converting relative paths found in the patch file to > absolute ones based on $PWD and the work tree root. That code looks > correct to me. Why not keep using this absolute path as much as possible? I've come up with a different approach, that's probably simpler. The issue with 'got patch' and $PWD arise from the fact that I'm using paths relative to the work tree as they were relative to $PWD: this can work only when $PWD is the root of a worktree. In patch below I'm keeping the paths relative to the work tree root as they're now, and computing absolute paths from those when needed. (i'm also sneakingly fixing a typo in an error reporting: if fchmod fails it failed on tmppath -- now tmpfile -- not on 'newfile' which has yet to be created) If you're happy with the direction I can then avoid computing the "ondisk_path" twice (once in patch_check_path and then again in apply_patch) and just propagate it from patch_check_path. > > *staged_status = GOT_STATUS_NO_CHANGE; > > *status = GOT_STATUS_UNVERSIONED; > > - if (lstat(ondisk_path, &sb) == -1) { > > + if (lstat(*resolved_path, &sb) == -1) { > > At this point *resolved_path is a relative path, correct? > I would refrain from passing a relative path to a function that will > use it for anything other than displaying the path to the user. diff 3313bcd840cd85a1b349abf664992608a42bbbde /home/op/w/got blob - 9b49b4b1349c2d2994dfa6e667c9265975aea642 file + lib/patch.c --- lib/patch.c +++ lib/patch.c @@ -572,7 +572,8 @@ apply_patch(struct got_worktree *worktree, struct got_ struct got_pathlist_head oldpaths, newpaths; struct got_pathlist_entry *pe; int file_renamed = 0; - char *tmppath = NULL, *template = NULL, *parent = NULL;; + char *oldfile = NULL, *newfile = NULL; + char *tmpfile = NULL, *template = NULL, *parent = NULL;; FILE *tmp = NULL; mode_t mode = GOT_DEFAULT_FILE_MODE; @@ -588,6 +589,18 @@ apply_patch(struct got_worktree *worktree, struct got_ file_renamed = strcmp(oldpath, newpath); + if (asprintf(&oldfile, "%s/%s", got_worktree_get_root_path(worktree), + oldpath) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } + + if (asprintf(&newfile, "%s/%s", got_worktree_get_root_path(worktree), + newpath) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } + if (asprintf(&template, "%s/got-patch", got_worktree_get_root_path(worktree)) == -1) { err = got_error_from_errno(template); @@ -595,10 +608,10 @@ apply_patch(struct got_worktree *worktree, struct got_ } if (!nop) - err = got_opentemp_named(&tmppath, &tmp, template); + err = got_opentemp_named(&tmpfile, &tmp, template); if (err) goto done; - err = patch_file(p, oldpath, tmp, nop, &mode); + err = patch_file(p, oldfile, tmp, nop, &mode); if (err) goto done; @@ -612,26 +625,26 @@ apply_patch(struct got_worktree *worktree, struct got_ } if (fchmod(fileno(tmp), mode) == -1) { - err = got_error_from_errno2("chmod", newpath); + err = got_error_from_errno2("chmod", tmpfile); goto done; } - if (rename(tmppath, newpath) == -1) { + if (rename(tmpfile, newfile) == -1) { if (errno != ENOENT) { - err = got_error_from_errno3("rename", tmppath, - newpath); + err = got_error_from_errno3("rename", tmpfile, + newfile); goto done; } - err = got_path_dirname(&parent, newpath); + err = got_path_dirname(&parent, newfile); if (err != NULL) goto done; err = got_path_mkdir(parent); if (err != NULL) goto done; - if (rename(tmppath, newpath) == -1) { - err = got_error_from_errno3("rename", tmppath, - newpath); + if (rename(tmpfile, newfile) == -1) { + err = got_error_from_errno3("rename", tmpfile, + newfile); goto done; } } @@ -643,12 +656,12 @@ apply_patch(struct got_worktree *worktree, struct got_ err = got_worktree_schedule_add(worktree, &newpaths, patch_add, pa, repo, 1); if (err) - unlink(newpath); + unlink(newfile); } else if (p->old == NULL) { err = got_worktree_schedule_add(worktree, &newpaths, patch_add, pa, repo, 1); if (err) - unlink(newpath); + unlink(newfile); } else err = report_progress(pa, oldpath, newpath, GOT_STATUS_MODIFY, NULL); @@ -658,9 +671,11 @@ done: got_pathlist_free(&newpaths); free(parent); free(template); - if (tmppath != NULL) - unlink(tmppath); - free(tmppath); + if (tmpfile != NULL) + unlink(tmpfile); + free(tmpfile); + free(oldfile); + free(newfile); return err; } blob - bfebb46526cc0f3f0deda90d0868edad7c6946c9 file + lib/worktree.c --- lib/worktree.c +++ lib/worktree.c @@ -8795,7 +8795,8 @@ patch_check_path(const char *p, char **path, unsigned ie = got_fileindex_entry_get(fileindex, *path, strlen(*path)); if (ie) { *staged_status = get_staged_status(ie); - err = get_file_status(status, &sb, ie, *path, -1, NULL, repo); + err = get_file_status(status, &sb, ie, ondisk_path, -1, NULL, + repo); if (err) goto done; } else { blob - b3f10d2dc6e282f6548c695befab125a7f8f680e file + regress/cmdline/patch.sh --- regress/cmdline/patch.sh +++ regress/cmdline/patch.sh @@ -1273,6 +1273,46 @@ EOF test_done $testroot 0 } v+test_patch_relative_paths() { + local testroot=`test_init patch_orig` + + got checkout $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + cat < $testroot/wt/gamma/patch +--- delta ++++ delta +@@ -1 +1 @@ +-delta ++DELTA +--- /dev/null ++++ eta +@@ -0,0 +1 @@ ++eta +EOF + + (cd $testroot/wt/gamma && got patch patch) > $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + test_done $testroot $ret + return 1 + fi + + echo 'M gamma/delta' > $testroot/stdout.expected + echo 'A gamma/eta' >> $testroot/stdout.expected + + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + fi + test_done $testroot $ret +} + test_parseargs "$@" run_test test_patch_simple_add_file run_test test_patch_simple_rm_file @@ -1294,3 +1334,4 @@ run_test test_patch_with_offset run_test test_patch_prefer_new_path run_test test_patch_no_newline run_test test_patch_strip +run_test test_patch_relative_paths