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/

>  .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'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.

> 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.

> 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.

> blob - 654d85e31ae466ed47907c34cc051be70a8351e4
> blob + b8c7e1512b611a7edb1e03ca3c25730eb9a323ee
> --- regress/cmdline/patch.sh
> +++ regress/cmdline/patch.sh
> @@ -1211,6 +1211,40 @@ 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
> +	fi
> +	test_done $testroot $ret
> +}
> +
>  test_parseargs "$@"
>  run_test test_patch_simple_add_file
>  run_test test_patch_simple_rm_file
> @@ -1231,3 +1265,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
> 
>