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

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

Am 06.03.23 um 19:59 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
> It somehow feels that the use of pathspec in "git archive" is
> somewhat iffy, e.g.
>
>    $ cd sha1dc && git archive HEAD :/ | tar tf -
>
> does not compare very well with
>
>    $ cd sha1dc && git ls-tree -r HEAD :/
>
> For that matter, replacing ":/" (full tree) with ".." (we know one
> level above is the root of the working tree) has the same "why don't
> they work the same way???" confusion.

Right,
https://lore.kernel.org/git/CA+X7ob8DWGmoVTxSVvrFN68V=pcaZripfP=s+LpWvXN-6L7W7Q@mail.gmail.com/
reports the second one as well.

It's a consequence of effectively cd'ing into the prefix tree in
archive.c::parse_treeish_arg() and using the empty string as prefix from
then on.  If we stop doing that then we're getting much closer (path at
the end):

   $ git -C xdiff ls-files '../sha1dc/*.c' '../xdiff/*.c'
   ../sha1dc/sha1.c
   ../sha1dc/ubc_check.c
   xdiffi.c
   xemit.c
   xhistogram.c
   xmerge.c
   xpatience.c
   xprepare.c
   xutils.c


   $ git -C xdiff archive HEAD '../sha1dc/*.c' '../xdiff/*.c' | tar tf -
   ../sha1dc/
   ../sha1dc/sha1.c
   ../sha1dc/ubc_check.c
   xdiffi.c
   xemit.c
   xhistogram.c
   xmerge.c
   xpatience.c
   xprepare.c
   xutils.c

Not sure if we want those leading double dots.  bsdtar has them:

   $ (cd xdiff && tar cf - ../sha1dc/*.c ../xdiff/*.c) | tar tf -
   ../sha1dc/sha1.c
   ../sha1dc/ubc_check.c
   ../xdiff/xdiffi.c
   ../xdiff/xemit.c
   ../xdiff/xhistogram.c
   ../xdiff/xmerge.c
   ../xdiff/xpatience.c
   ../xdiff/xprepare.c
   ../xdiff/xutils.c

... but GNU tar strips them off and warns about them:

   $ (cd xdiff && gtar cf - ../sha1dc/*.c ../xdiff/*.c) | tar tf -
   gtar: Removing leading `../' from member names
   gtar: Removing leading `../' from hard link targets
   sha1dc/sha1.c
   sha1dc/ubc_check.c
   xdiff/xdiffi.c
   xdiff/xemit.c
   xdiff/xhistogram.c
   xdiff/xmerge.c
   xdiff/xpatience.c
   xdiff/xprepare.c
   xdiff/xutils.c

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.

And I don't know why PATHSPEC_PREFER_CWD is necessary.

So no sign-off, yet.


 archive.c | 60 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/archive.c b/archive.c
index 9aeaf2bd87..c7e9f58b02 100644
--- a/archive.c
+++ b/archive.c
@@ -139,6 +139,7 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
 		void *context)
 {
 	static struct strbuf path = STRBUF_INIT;
+	static struct strbuf scratch = STRBUF_INIT;
 	struct archiver_context *c = context;
 	struct archiver_args *args = c->args;
 	write_archive_entry_fn_t write_entry = c->write_entry;
@@ -148,6 +149,14 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
 	void *buffer;
 	enum object_type type;

+	/*
+	 * NEEDSWORK: variable names could be clearer:
+	 * - args->prefix is the current working directory,
+	 * - args->base with args->baselen is the --prefix value,
+	 * - base with baselen is the path of the current tree,
+	 * - args->base + base + filename is the path in the archive,
+	 * - path_without_prefix is base + filename.
+	 */
 	args->convert = 0;
 	strbuf_reset(&path);
 	strbuf_grow(&path, PATH_MAX);
@@ -166,6 +175,15 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
 		args->convert = check_attr_export_subst(check);
 	}

+	if (args->prefix) {
+		const char *rel = relative_path(path_without_prefix,
+						args->prefix, &scratch);
+		if (!strcmp(rel, "./"))
+			return S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;
+		strbuf_setlen(&path, args->baselen);
+		strbuf_addstr(&path, rel);
+	}
+
 	if (args->verbose)
 		fprintf(stderr, "%.*s\n", (int)path.len, path.buf);

@@ -401,14 +419,15 @@ static int reject_entry(const struct object_id *oid UNUSED,
 	return ret;
 }

-static int path_exists(struct archiver_args *args, const char *path)
+static int path_exists(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;
-	parse_pathspec(&ctx.pathspec, 0, 0, "", paths);
+	parse_pathspec(&ctx.pathspec, 0, 0, prefix, paths);
 	ctx.pathspec.recursive = 1;
 	ret = read_tree(args->repo, args->tree,
 			&ctx.pathspec,
@@ -417,30 +436,35 @@ static int path_exists(struct archiver_args *args, const char *path)
 	return ret != 0;
 }

-static void parse_pathspec_arg(const char **pathspec,
+static void parse_pathspec_arg(const char **pathspec, const char *prefix,
 		struct archiver_args *ar_args)
 {
+	const char *match_all[] = { ".", NULL };
+
+	if (prefix && !*pathspec)
+		pathspec = match_all;
+
 	/*
 	 * must be consistent with parse_pathspec in path_exists()
 	 * Also if pathspec patterns are dependent, we're in big
 	 * trouble as we test each one separately
 	 */
 	parse_pathspec(&ar_args->pathspec, 0,
-		       PATHSPEC_PREFER_FULL,
-		       "", pathspec);
+		       PATHSPEC_PREFER_CWD,
+		       prefix, pathspec);
 	ar_args->pathspec.recursive = 1;
 	if (pathspec) {
 		while (*pathspec) {
-			if (**pathspec && !path_exists(ar_args, *pathspec))
+			if (**pathspec &&
+			    !path_exists(ar_args, prefix, *pathspec))
 				die(_("pathspec '%s' did not match any files"), *pathspec);
 			pathspec++;
 		}
 	}
 }

-static void parse_treeish_arg(const char **argv,
-		struct archiver_args *ar_args, const char *prefix,
-		int remote)
+static void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,
+			      int remote)
 {
 	const char *name = argv[0];
 	const struct object_id *commit_oid;
@@ -479,20 +503,6 @@ static void parse_treeish_arg(const char **argv,
 	if (!tree)
 		die(_("not a tree object: %s"), oid_to_hex(&oid));

-	if (prefix) {
-		struct object_id tree_oid;
-		unsigned short mode;
-		int err;
-
-		err = get_tree_entry(ar_args->repo,
-				     &tree->object.oid,
-				     prefix, &tree_oid,
-				     &mode);
-		if (err || !S_ISDIR(mode))
-			die(_("current working directory is untracked"));
-
-		tree = parse_tree_indirect(&tree_oid);
-	}
 	ar_args->refname = ref;
 	ar_args->tree = tree;
 	ar_args->commit_oid = commit_oid;
@@ -710,8 +720,8 @@ int write_archive(int argc, const char **argv, const char *prefix,
 		setup_git_directory();
 	}

-	parse_treeish_arg(argv, &args, prefix, remote);
-	parse_pathspec_arg(argv + 1, &args);
+	parse_treeish_arg(argv, &args, remote);
+	parse_pathspec_arg(argv + 1, prefix, &args);

 	rc = ar->write_archive(ar, &args);