From: Omar Polo Subject: Re: got patch: resolve paths from $PWD To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Fri, 22 Apr 2022 22:58:54 +0200 Stefan Sperling wrote: > On Fri, Apr 22, 2022 at 04:47:59PM +0200, Omar Polo wrote: > > 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. > > Oh, indeed! That is probably a mistake. I don't see a reason why paths > returned from got_worktree_resolve_path() must be relative. I can look > at fixing this. No reason for you to walk futher into that rabbit hole :) > > > 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. > > This kind of easy confusion is why standardizing on absolute paths > for internal use is a good idea. Subversion made a switch from mixed > relative/absolute to absolute-only in the 1.6 -> 1.7 timeframe, and > this caught a lot of issues. For what it's worth, I'm all in for using only absolute paths. Maybe also using only paths relative to the worktree root and resolving them from a file descriptor associated with the worktree root is an option? I'm missing the bigger picture here thought. > > > 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. > > Yes, this seems nicer indeed. > The work of figuring out which path the patch wants to add, delete, > or modify is now done by got_worktree_resolve_path(), correct? just as before, yes. (I need to simplify a bit the call chain...) The paths are computed in worktree.c:got_worktree_patch_check_path via patch_check_path, and they're build off got_worktree_resolve_path. I'm still keeping the paths relative to the worktree root in the pathlists, otherwise I'd broke everything. I'm then building (again) the absolute paths in apply_patch before running the patch machinery. I'm only feeding absolute paths to lstat/fopen/rename & co. > > (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. > > > 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;; > > What is the reason for switching these variable names to 'file'? > I like having "path" as a part of names for variables which contain > paths, and "file" as a part of names for variables which represent > open files. This is just a matter of taste, of course. I like *path more than *file too, the rename was to have another name (old/newpath are already the parameters.) In the updated diff I'm calling the argument of apply_patch "old" and "new", and the absolute paths "oldpath" and "newpath". less churn too. > > 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; > > + } > > This looks like a step which would become unnecessary later, if work tree > paths returned from lib/worktree.c were already absolute. Right? Yes. To be fair it could be somehow avoided right now, I just need to propagate the path I build in worktree.c:patch_check_path up until apply_patch. (it's the "ondisk_path" from patch_check_path). but that can be done in a follow-up commit, diff belows already has three different fixes, it's probably enough ;) P.S.: sorry for the previous non-applying diff, i left an extra character by mistake while reading the diff in my editor. diff refs/heads/main refs/heads/ppath2 blob - 9b49b4b1349c2d2994dfa6e667c9265975aea642 blob + adf9459c88ef262d9070733fe62620364926f5db --- lib/patch.c +++ lib/patch.c @@ -565,13 +565,14 @@ patch_add(void *arg, unsigned char status, const char static const struct got_error * apply_patch(struct got_worktree *worktree, struct got_repository *repo, - const char *oldpath, const char *newpath, struct got_patch *p, - int nop, struct patch_args *pa, got_cancel_cb cancel_cb, void *cancel_arg) + const char *old, const char *new, struct got_patch *p, int nop, + struct patch_args *pa, got_cancel_cb cancel_cb, void *cancel_arg) { const struct got_error *err = NULL; struct got_pathlist_head oldpaths, newpaths; struct got_pathlist_entry *pe; int file_renamed = 0; + char *oldpath = NULL, *newpath = NULL; char *tmppath = NULL, *template = NULL, *parent = NULL;; FILE *tmp = NULL; mode_t mode = GOT_DEFAULT_FILE_MODE; @@ -579,13 +580,25 @@ apply_patch(struct got_worktree *worktree, struct got_ TAILQ_INIT(&oldpaths); TAILQ_INIT(&newpaths); - err = got_pathlist_insert(&pe, &oldpaths, oldpath, NULL); + err = got_pathlist_insert(&pe, &oldpaths, old, NULL); if (err) goto done; - err = got_pathlist_insert(&pe, &newpaths, newpath, NULL); + err = got_pathlist_insert(&pe, &newpaths, new, NULL); if (err) goto done; + if (asprintf(&oldpath, "%s/%s", got_worktree_get_root_path(worktree), + old) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } + + if (asprintf(&newpath, "%s/%s", got_worktree_get_root_path(worktree), + new) == -1) { + err = got_error_from_errno("asprintf"); + goto done; + } + file_renamed = strcmp(oldpath, newpath); if (asprintf(&template, "%s/got-patch", @@ -612,7 +625,7 @@ 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", tmppath); goto done; } @@ -650,8 +663,7 @@ apply_patch(struct got_worktree *worktree, struct got_ if (err) unlink(newpath); } else - err = report_progress(pa, oldpath, newpath, GOT_STATUS_MODIFY, - NULL); + err = report_progress(pa, old, new, GOT_STATUS_MODIFY, NULL); done: got_pathlist_free(&oldpaths); @@ -661,6 +673,8 @@ done: if (tmppath != NULL) unlink(tmppath); free(tmppath); + free(oldpath); + free(newpath); return err; } blob - bfebb46526cc0f3f0deda90d0868edad7c6946c9 blob + bbe95ae655a519c5a9d4dc6eac25f7ace38c7452 --- 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 blob + af1fd72e33001d46b8f822e0b0a7f00a5b1ad7b7 --- regress/cmdline/patch.sh +++ regress/cmdline/patch.sh @@ -1273,6 +1273,46 @@ EOF test_done $testroot 0 } +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