Re: Bug in git archive + .gitattributes + relative path

["René Scharfe" <l.s.r@web.de>]

Am 06.03.23 um 23:34 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Neither of them resolves "../$PWD/" parts to "" like git ls-tree does,
>> but I can accept that difference.  And then we'd need to keep leading
>> "../", I suppose.  Still unsure.
>
> I offhand do not know how well it would mix with --strip-components
> if we leave the leading "../".

Not too well when entries from $PWD are shortened and mixed with ones
from elsewhere that aren't.  But that seems like a strange thing to do.
If two sub-trees are needed then git archive should be started in a
shared parent directory higher up.

> But it certainly would be nice if we somehow:
>
>  * can keep the current behaviour where "git -C sub archive" records
>    paths relative to "sub" for backward compatibility.

Right.  That's what relative_path() provides in the patch.

>  * fail loudly when "git -C sub archive <pathspec>" makes us use
>    "../" prefix because <pathspec> goes above the $PWD for backward
>    compatibility and sanity.

Without the patch this fails, but are there really people that depend on
it failing?  We could certainly forbid it, but do we need to?  It costs
a few lines of code (see patch below).

>  * with --some-option, make "git -C sub archive --some-option :/"
>    act exactly like "git archive :/".

Perhaps I'm reading this too literally, but it would be easier to remove
"-C sub" from that command. Or to add "-C $(git rev-parse --show-cdup)".
We could add a shortcut for that (see patch below).

>> And I don't know why PATHSPEC_PREFER_CWD is necessary.

PATHSPEC_PREFER_FULL makes the empty pathspec match everything, while
PATHSPEC_PREFER_CWD makes it match the current working directory, only.
In git archive without these patches both mean the same because we
make the tree of the current working directory the root of our pathspec
worldview.  When we stop doing that it actually makes a difference and
we need the more limited variant.

René


 Documentation/git.txt |  4 ++++
 archive.c             | 36 ++++++++++++++++++++++--------------
 git.c                 | 18 ++++++++++++++++++
 t/t0056-git-C.sh      |  6 ++++++
 4 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 74973d3cc4..e254572fec 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -73,6 +73,10 @@ example the following invocations are equivalent:
     git --git-dir=a.git --work-tree=b -C c status
     git --git-dir=c/a.git --work-tree=c/b status

+--cdup::
+	Run as if git was started at the root of the repository or
+	worktree.  Same as `-C $(git rev-parse --show-cdup)`.
+
 -c <name>=<value>::
 	Pass a configuration parameter to the command. The value
 	given will override values from configuration files.
diff --git a/archive.c b/archive.c
index c7e9f58b02..6f587876e5 100644
--- a/archive.c
+++ b/archive.c
@@ -396,6 +396,7 @@ static const struct archiver *lookup_archiver(const char *name)
 struct path_exists_context {
 	struct pathspec pathspec;
 	struct archiver_args *args;
+	int dotdot;
 };

 static int reject_entry(const struct object_id *oid UNUSED,
@@ -405,35 +406,43 @@ static int reject_entry(const struct object_id *oid UNUSED,
 {
 	int ret = -1;
 	struct path_exists_context *ctx = context;
+	const char *prefix = ctx->args->prefix;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf scratch = STRBUF_INIT;

+	strbuf_addbuf(&sb, base);
+	strbuf_addstr(&sb, filename);
 	if (S_ISDIR(mode)) {
-		struct strbuf sb = STRBUF_INIT;
-		strbuf_addbuf(&sb, base);
-		strbuf_addstr(&sb, filename);
 		if (!match_pathspec(ctx->args->repo->index,
 				    &ctx->pathspec,
 				    sb.buf, sb.len, 0, NULL, 1))
 			ret = READ_TREE_RECURSIVE;
-		strbuf_release(&sb);
+	} else {
+		if (starts_with(relative_path(sb.buf, prefix, &scratch), "../"))
+			ctx->dotdot = 1;
 	}
+	strbuf_release(&sb);
+	strbuf_release(&scratch);
 	return ret;
 }

-static int path_exists(struct archiver_args *args, const char *prefix,
+static void check_path(struct archiver_args *args, const char *prefix,
 		       const char *path)
 {
 	const char *paths[] = { path, NULL };
 	struct path_exists_context ctx;
-	int ret;

 	ctx.args = args;
+	ctx.dotdot = 0;
 	parse_pathspec(&ctx.pathspec, 0, 0, prefix, paths);
 	ctx.pathspec.recursive = 1;
-	ret = read_tree(args->repo, args->tree,
-			&ctx.pathspec,
-			reject_entry, &ctx);
+	if (!read_tree(args->repo, args->tree, &ctx.pathspec,
+		       reject_entry, &ctx))
+		die(_("pathspec '%s' did not match any files"), path);
+	if (ctx.dotdot)
+		die(_("pathspec '%s' matches files above current directory"),
+		    path);
 	clear_pathspec(&ctx.pathspec);
-	return ret != 0;
 }

 static void parse_pathspec_arg(const char **pathspec, const char *prefix,
@@ -445,7 +454,7 @@ static void parse_pathspec_arg(const char **pathspec, const char *prefix,
 		pathspec = match_all;

 	/*
-	 * must be consistent with parse_pathspec in path_exists()
+	 * must be consistent with parse_pathspec in check_path()
 	 * Also if pathspec patterns are dependent, we're in big
 	 * trouble as we test each one separately
 	 */
@@ -455,9 +464,8 @@ static void parse_pathspec_arg(const char **pathspec, const char *prefix,
 	ar_args->pathspec.recursive = 1;
 	if (pathspec) {
 		while (*pathspec) {
-			if (**pathspec &&
-			    !path_exists(ar_args, prefix, *pathspec))
-				die(_("pathspec '%s' did not match any files"), *pathspec);
+			if (**pathspec)
+				check_path(ar_args, prefix, *pathspec);
 			pathspec++;
 		}
 	}
diff --git a/git.c b/git.c
index 6171fd6769..22993991a9 100644
--- a/git.c
+++ b/git.c
@@ -293,6 +293,24 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			}
 			(*argv)++;
 			(*argc)--;
+		} else if (!strcmp(cmd, "--cdup")) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+			struct strbuf err = STRBUF_INIT;
+
+			cp.git_cmd = 1;
+			strvec_pushl(&cp.args, "rev-parse", "--show-cdup", NULL);
+			if (pipe_command(&cp, NULL, 0, &out, 0, &err, 0)) {
+				strbuf_trim_trailing_newline(&err);
+				if (err.len)
+					die("%s", err.buf);
+				die(_("unable to get repo root or worktree"));
+			}
+			strbuf_trim_trailing_newline(&out);
+			if (out.len && chdir(out.buf))
+				die_errno(_("cannot change to '%s'"), out.buf);
+			strbuf_release(&out);
+			strbuf_release(&err);
 		} else if (skip_prefix(cmd, "--list-cmds=", &cmd)) {
 			trace2_cmd_name("_query_");
 			if (!strcmp(cmd, "parseopt")) {
diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh
index 752aa8c945..95107d724e 100755
--- a/t/t0056-git-C.sh
+++ b/t/t0056-git-C.sh
@@ -92,4 +92,10 @@ test_expect_success 'Relative followed by fullpath: "-C ./here -C /there" is equ
 	test_cmp expected actual
 '

+test_expect_success '"--cdup" goes back' '
+	git status >expected &&
+	git -C c --cdup status >actual &&
+	test_cmp expected actual
+'
+
 test_done