Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Apr 04, 2022 at 09:53:17PM +0200, Omar Polo wrote:
> > in the updated diff i've moved the strip function to path.c and made it
> > errors when trying to strip more components than what are present.
> > 
> > diff refs/heads/main refs/heads/pstrip
> > blob - a9f60f53d707e188b849ee505d8cedffab3a89b9
> > blob + 3f3f4e07bca1f7023b974c2d2a95a5bf759784c4
> > --- got/got.1
> > +++ got/got.1
> > @@ -1285,7 +1285,7 @@ option)
> >  .El
> >  .El
> >  .Tg pa
> > -.It Cm patch Oo Fl n Oc Op Ar patchfile
> > +.It Cm patch Oo Fl n Oc Oo Fl p Ar strip Oc Op Ar patchfile
> 
> s/strip/strip-count/

ack.  I thought that `strip' alone sounded better, but `strip-count'
is also used in patch(1) so let's stick with that.

> >  .Dl Pq alias: Cm pa
> >  Apply changes from
> >  .Ar patchfile
> > @@ -1354,6 +1354,20 @@ If the
> >  .Ar patchfile
> >  contains diffs that affect the same file multiple times, the results
> >  displayed may be incorrect.
> > +.It Fl p Ar strip
> 
> s/strip/strip-count/
> 
> > +Specify the number of leading path components to strip from paths
> > +parsed from
> > +.Ar patchfile .
> 
> > +If not given,
> > +recognize
> > +.Xr git 1
> > +style diffs and automatically drops the
> > +.Sq a/
> > +and
> > +.Sq b/
> > +prefixes.
> 
> Rewording suggestion for the above:
> 
> If the
> .Fl p
> option is not used,
> .Sq a/
> and
> .Sq b/
> path prefixes generated by
> .Xr git-diff 1
> will be recognized and stripped automatically.

it reads better, thanks!

> > +It's an error to attempt to strip more component than present in
> > +.Ar patchfile .
> 
> I don't think we need to document this error condition here.
> When this error occurs the reason should be obvious to the user.

i wasn't sure either.  dropped in the updated diff

> > blob - f2d6ba2e472496846c8519d36002ed09491009ed
> > blob + 2453c214e0eca525ba6f32c7d6c09c944f279d31
> > --- include/got_path.h
> > +++ include/got_path.h
> > @@ -41,6 +41,12 @@ const struct got_error *got_canonpath(const char *, ch
> >  const struct got_error *got_path_skip_common_ancestor(char **, const char *,
> >      const char *);
> >  
> > +/*
> > + * Remove leading components from path.  It's an error to strip more
> > + * component than present.  The result is allocated dynamically.
> > + */
> > +const struct got_error *got_path_strip(const char *, int, char **);
> > +
> 
> Please put the output argument (char **) first.
> This is not done 100% consistently but I usually try to put output
> arguments before any inputs.

thanks for pointing this out, I thought the opposite was preferred
actually (output argument last).  fixed and i'll keep in mind for the
future :)

> > blob - 0e05a1100c321abafef637ae199c587f70a6e018
> > blob + 51eb3d8d6a3c1c9e440695f0421c3e16571d509e
> > --- lib/path.c
> > +++ lib/path.c
> > @@ -117,6 +117,28 @@ got_path_skip_common_ancestor(char **child, const char
> >  	return NULL;
> >  }
> >  
> > +const struct got_error *
> > +got_path_strip(const char *path, int n, char **out)
> > +{
> > +	const char *p, *c;
> > +
> > +	p = path;
> > +	*out = NULL;
> > +
> > +	while (n > 0 && (c = strchr(path, '/')) != NULL) {
> > +		path = c + 1;
> > +		n--;
> > +	}
> > +
> > +	if (n > 0)
> > +		return got_error_fmt(GOT_ERR_BAD_PATH,
> > +		    "can't strip %d components from %s", n, p);
> 
> s/%d components/%d path-components/
>
> Please add test-coverage for this error case.
> As far as I can see the test below is only checking for success.

Yeah, right, i haven't thought about that when adding the error.
Updated diff has a test for it too.

diff refs/heads/main refs/heads/pstrip
blob - a9f60f53d707e188b849ee505d8cedffab3a89b9
blob + bd6e06c564ce9499b313b1d667097c58c2d16873
--- got/got.1
+++ got/got.1
@@ -1285,7 +1285,7 @@ option)
 .El
 .El
 .Tg pa
-.It Cm patch Oo Fl n Oc Op Ar patchfile
+.It Cm patch Oo Fl n Oc Oo Fl p Ar strip-count Oc Op Ar patchfile
 .Dl Pq alias: Cm pa
 Apply changes from
 .Ar patchfile
@@ -1354,6 +1354,19 @@ If the
 .Ar patchfile
 contains diffs that affect the same file multiple times, the results
 displayed may be incorrect.
+.It Fl p Ar strip-count
+Specify the number of leading path components to strip from paths
+parsed from
+.Ar patchfile .
+If the
+.Fl p
+option is not used,
+.Sq a/
+and
+.Sq b/
+path prefixes generated by
+.Xr git-diff 1
+will be recognized and stripped automatically.
 .El
 .Tg rv
 .It Cm revert Oo Fl p Oc Oo Fl F Ar response-script Oc Oo Fl R Oc Ar path ...
blob - f3608d970c5ae77fee89a4e9ca8fdb89b015c5f8
blob + 36bd7fe005dea2816d23161fab0d7aed3b56e159
--- got/got.c
+++ got/got.c
@@ -7222,15 +7222,22 @@ cmd_patch(int argc, char *argv[])
 	const struct got_error *error = NULL, *close_error = NULL;
 	struct got_worktree *worktree = NULL;
 	struct got_repository *repo = NULL;
+	const char *errstr;
 	char *cwd = NULL;
-	int ch, nop = 0;
+	int ch, nop = 0, strip = -1;
 	int patchfd;
 
-	while ((ch = getopt(argc, argv, "n")) != -1) {
+	while ((ch = getopt(argc, argv, "np:")) != -1) {
 		switch (ch) {
 		case 'n':
 			nop = 1;
 			break;
+		case 'p':
+			strip = strtonum(optarg, 0, INT_MAX, &errstr);
+			if (errstr != NULL)
+				errx(1, "pathname strip count is %s: %s",
+				     errstr, optarg);
+			break;
 		default:
 			usage_patch();
 			/* NOTREACHED */
@@ -7278,8 +7285,8 @@ cmd_patch(int argc, char *argv[])
 		err(1, "pledge");
 #endif
 
-	error = got_patch(patchfd, worktree, repo, nop, &patch_progress,
-	    NULL, check_cancelled, NULL);
+	error = got_patch(patchfd, worktree, repo, nop, strip,
+	    &patch_progress, NULL, check_cancelled, NULL);
 
 done:
 	if (repo) {
blob - 959a2129f32e5f561fc9ccfdc186d10507d45e00
blob + 62c76b0eb2926a03364ef10e416183cdfa18f398
--- include/got_patch.h
+++ include/got_patch.h
@@ -31,5 +31,5 @@ typedef const struct got_error *(*got_patch_progress_c
  * The patch file descriptor *must* be seekable.
  */
 const struct got_error *
-got_patch(int, struct got_worktree *, struct got_repository *, int,
+got_patch(int, struct got_worktree *, struct got_repository *, int, int,
     got_patch_progress_cb, void *, got_cancel_cb, void *);
blob - f2d6ba2e472496846c8519d36002ed09491009ed
blob + a223de14baf9eab1c747904cbc10cbaf30003d8c
--- include/got_path.h
+++ include/got_path.h
@@ -41,6 +41,12 @@ const struct got_error *got_canonpath(const char *, ch
 const struct got_error *got_path_skip_common_ancestor(char **, const char *,
     const char *);
 
+/*
+ * Remove leading components from path.  It's an error to strip more
+ * component than present.  The result is allocated dynamically.
+ */
+const struct got_error *got_path_strip(char **, const char *, int);
+
 /* Determine whether a path points to the root directory "/" . */
 int got_path_is_root_dir(const char *);
 
blob - fef20e3a85c35f0faa4743d896a34ac04f0a4397
blob + 110fe049d86c1a33fb3b33e4fe74ffa8a3dbbfa8
--- lib/got_lib_privsep.h
+++ lib/got_lib_privsep.h
@@ -525,6 +525,7 @@ struct got_imsg_remotes {
  * Structure for GOT_IMSG_PATCH data.
  */
 struct got_imsg_patch {
+	int	git;
 	char	old[PATH_MAX];
 	char	new[PATH_MAX];
 };
blob - 4bea692e5b2c362795981a69b86a1446aa52ec7c
blob + 9b49b4b1349c2d2994dfa6e667c9265975aea642
--- lib/patch.c
+++ lib/patch.c
@@ -144,7 +144,7 @@ pushline(struct got_patch_hunk *h, const char *line)
 }
 
 static const struct got_error *
-recv_patch(struct imsgbuf *ibuf, int *done, struct got_patch *p)
+recv_patch(struct imsgbuf *ibuf, int *done, struct got_patch *p, int strip)
 {
 	const struct got_error *err = NULL;
 	struct imsg imsg;
@@ -173,14 +173,30 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
 		goto done;
 	}
 	memcpy(&patch, imsg.data, sizeof(patch));
-	if (*patch.old != '\0' && (p->old = strdup(patch.old)) == NULL) {
-		err = got_error_from_errno("strdup");
-		goto done;
+
+	/* automatically set strip=1 for git-style diffs */
+	if (strip == -1 && patch.git &&
+	    (*patch.old == '\0' || !strncmp(patch.old, "a/", 2)) &&
+	    (*patch.new == '\0' || !strncmp(patch.new, "b/", 2)))
+		strip = 1;
+
+	/* prefer the new name if not /dev/null for not git-style diffs */
+	if (!patch.git && *patch.new != '\0' && *patch.old != '\0') {
+		err = got_path_strip(&p->old, patch.new, strip);
+		if (err)
+			goto done;
+	} else if (*patch.old != '\0') {
+		err = got_path_strip(&p->old, patch.old, strip);
+		if (err)
+			goto done;
 	}
-	if (*patch.new != '\0' && (p->new = strdup(patch.new)) == NULL) {
-		err = got_error_from_errno("strdup");
-		goto done;
+
+	if (*patch.new != '\0') {
+		err = got_path_strip(&p->new, patch.new, strip);
+		if (err)
+			goto done;
 	}
+
 	if (p->old == NULL && p->new == NULL) {
 		err = got_error(GOT_ERR_PATCH_MALFORMED);
 		goto done;
@@ -650,7 +666,7 @@ done:
 
 const struct got_error *
 got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo,
-    int nop, got_patch_progress_cb progress_cb, void *progress_arg,
+    int nop, int strip, got_patch_progress_cb progress_cb, void *progress_arg,
     got_cancel_cb cancel_cb, void *cancel_arg)
 {
 	const struct got_error *err = NULL;
@@ -706,7 +722,7 @@ got_patch(int fd, struct got_worktree *worktree, struc
 		pa.progress_arg = progress_arg;
 		pa.head = &p.head;
 
-		err = recv_patch(ibuf, &done, &p);
+		err = recv_patch(ibuf, &done, &p, strip);
 		if (err || done)
 			break;
 
blob - 0e05a1100c321abafef637ae199c587f70a6e018
blob + 9a0217a5e7d82ee2539af6b0e563a2c537f26621
--- lib/path.c
+++ lib/path.c
@@ -117,6 +117,28 @@ got_path_skip_common_ancestor(char **child, const char
 	return NULL;
 }
 
+const struct got_error *
+got_path_strip(char **out, const char *path, int n)
+{
+	const char *p, *c;
+
+	p = path;
+	*out = NULL;
+
+	while (n > 0 && (c = strchr(path, '/')) != NULL) {
+		path = c + 1;
+		n--;
+	}
+
+	if (n > 0)
+		return got_error_fmt(GOT_ERR_BAD_PATH,
+		    "can't strip %d path-components from %s", n, p);
+
+	if ((*out = strdup(path)) == NULL)
+		return got_error_from_errno("strdup");
+	return NULL;
+}
+
 int
 got_path_is_root_dir(const char *path)
 {
blob - 718736881402bf79f5c0307528c91464deea3105
blob + b72f7f88829c3931ef203cba5d632edc4dc06195
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -66,18 +66,13 @@ send_patch(const char *oldname, const char *newname, i
 
 	memset(&p, 0, sizeof(p));
 
-	/*
-	 * Prefer the new name if it's not /dev/null and it's not
-	 * a git-style diff.
-	 */
-	if (!git && newname != NULL && oldname != NULL)
-		strlcpy(p.old, newname, sizeof(p.old));
-	else if (oldname != NULL)
+	if (oldname != NULL)
 		strlcpy(p.old, oldname, sizeof(p.old));
 
 	if (newname != NULL)
 		strlcpy(p.new, newname, sizeof(p.new));
 
+	p.git = git;
 	if (imsg_compose(&ibuf, GOT_IMSG_PATCH, 0, 0, -1,
 	    &p, sizeof(p)) == -1)
 		return got_error_from_errno("imsg_compose GOT_IMSG_PATCH");
@@ -97,10 +92,9 @@ send_patch_done(void)
 
 /* based on fetchname from usr.bin/patch/util.c */
 static const struct got_error *
-filename(const char *at, char **name, int strip)
+filename(const char *at, char **name)
 {
-	char	*fullname, *t;
-	int	 l, tab;
+	char	*tmp, *t;
 
 	*name = NULL;
 	if (*at == '\0')
@@ -113,24 +107,16 @@ filename(const char *at, char **name, int strip)
 	if (!strncmp(at, _PATH_DEVNULL, sizeof(_PATH_DEVNULL) - 1))
 		return NULL;
 
-	t = strdup(at);
-	if (t == NULL)
+	tmp = strdup(at);
+	if (tmp == NULL)
 		return got_error_from_errno("strdup");
-	*name = fullname = t;
-	tab = strchr(t, '\t') != NULL;
+	if ((t = strchr(tmp, '\t')) != NULL)
+		*t = '\0';
+	if ((t = strchr(tmp, '\n')) != NULL)
+		*t = '\0';
 
-	/* strip off path components and NUL-terminate */
-	for (l = strip;
-	    *t != '\0' && ((tab && *t != '\t') || !isspace((unsigned char)*t));
-	    ++t) {
-		if (t[0] == '/' && t[1] != '/' && t[1] != '\0')
-			if (--l >= 0)
-				*name = t + 1;
-	}
-	*t = '\0';
-
-	*name = strdup(*name);
-	free(fullname);
+	*name = strdup(tmp);
+	free(tmp);
 	if (*name == NULL)
 		return got_error_from_errno("strdup");
 	return NULL;
@@ -152,18 +138,12 @@ find_patch(FILE *fp)
 		 * we don't have to follow POSIX.
 		 */
 
-		if (git && !strncmp(line, "--- a/", 6)) {
+		if (!strncmp(line, "--- ", 4)) {
 			free(old);
-			err = filename(line+6, &old, 0);
-		} else if (!strncmp(line, "--- ", 4)) {
-			free(old);
-			err = filename(line+4, &old, 0);
-		} else if (git && !strncmp(line, "+++ b/", 6)) {
-			free(new);
-			err = filename(line+6, &new, 0);
+			err = filename(line+4, &old);
 		} else if (!strncmp(line, "+++ ", 4)) {
 			free(new);
-			err = filename(line+4, &new, 0);
+			err = filename(line+4, &new);
 		} else if (!strncmp(line, "diff --git a/", 13))
 			git = 1;
 
blob - 654d85e31ae466ed47907c34cc051be70a8351e4
blob + 25c544e951905631db897f09c792aa04d7f11a0d
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1211,6 +1211,59 @@ EOF
 	test_done $testroot $ret
 }
 
+test_patch_strip() {
+	local testroot=`test_init patch_strip`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cat <<EOF > $testroot/wt/patch
+--- foo/bar/alpha.orig
++++ foo/bar/alpha
+@@ -1 +1 @@
+-alpha
++ALPHA
+EOF
+
+	(cd $testroot/wt && got patch -p2 patch) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo "M  alpha" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got revert alpha) > /dev/null 2>&1
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	(cd $testroot/wt && got patch -p3 patch) \
+		> $testroot/stdout 2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "stripped more components than available!"
+		test_done $testroot 1
+		return 1
+	fi
+
+	test_done $testroot 0
+}
+
 test_parseargs "$@"
 run_test test_patch_simple_add_file
 run_test test_patch_simple_rm_file
@@ -1231,3 +1284,4 @@ run_test test_patch_create_dirs
 run_test test_patch_with_offset
 run_test test_patch_prefer_new_path
 run_test test_patch_no_newline
+run_test test_patch_strip