{"thread":{"id":"59334","subject":"Bug in git archive + .gitattributes + relative path","startedAt":"2023-03-03T11:07:26Z","lastAt":"2023-03-27T16:09:52Z","messageCount":22,"participants":["Cristian Le","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"472952","messageId":"42f13cda-9de6-bfc6-7e81-64c94f5640db@mpsd.mpg.de","threadId":"59334","inReplyTo":null,"subject":"Bug in git archive + .gitattributes + relative path","fromName":"Cristian Le","fromEmail":"cristian.le@mpsd.mpg.de","sentAt":"2023-03-03T10:25:01Z","receivedAt":"2023-03-03T11:07:26Z","isPatch":false,"sender":{"key":"cristian.le@mpsd.mpg.de","avatar":null},"body":"Using `git archive` with or without `--worktree-attributes` does not \nproperly read `.gitattributes` files if using a relative path in \n``.\nRelated github comment: \nhttps://github.com/rpm-software-management/tito/pull/445#issuecomment-1450298871\nRelated stackoverflow discussion: \nhttps://stackoverflow.com/questions/52804334/how-to-ignore-files-directories-in-git-archive-and-only-create-an-archive-of-a\n\nGit version: `2.39.2`\nMwe git repo: Two files:\n```\n# .gitattributes:\n.git_archival.txt export-subst\n```\n```\n# .git_archival.txt:\nnode: $Format:%H$\n```\n\nCommands to reproduce and expected behaviour:\n```console\n$ git archive HEAD:./ --output=test.tar.gz\n$ tar -axf test.tar.gz .git_archival.txt -O\nnode: 745ce26169fb44e04d91d40ee581cccd591c941e\n```\nImportant: Notice the path `./` given after `HEAD`.\n\nActual output:\n```console\n$ tar -axf test.tar.gz .git_archival.txt -O\nnode: $Format:%H$\n```\n\nIt doesn't matter if `.gitattributes` is in a subfolder, or if I change \nthe relative path `./` to a subfolder, the files are still not properly \ngenerated.\n\nUsing `--worktree-attributes` did not have any effect either.\nAccording to the documentation, I understand that the expected behaviour \nwith regards to `--worktree-attributes`:\n- Read the `.gitattributes` of the relative path, e.g. `./sub_dir` \nregardless of `--worktree-attributes`. (similar behaviour as not passing \na relative path)\n- Include the `.gitattributes` of the top-level path if \n`--worktree-attributes` is passed\n\nMaybe the intended behaviour is to completely ignore all \n`.gitattributes` unless `--worktree-attributes` is provided, in which \ncase, it does not have the intended behaviour and please include a flag \nto achieve the above behaviour.\n\n"},{"id":"472960","messageId":"c7b21faa-68dd-8bd9-4670-2cf609741094@web.de","threadId":"59334","inReplyTo":"42f13cda-9de6-bfc6-7e81-64c94f5640db@mpsd.mpg.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-03T15:19:31Z","receivedAt":"2023-03-03T15:19:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.03.23 um 11:25 schrieb Cristian Le:\n> Using `git archive` with or without `--worktree-attributes` does not properly read `.gitattributes` files if using a relative path in ``.\n> Related github comment: https://github.com/rpm-software-management/tito/pull/445#issuecomment-1450298871\n> Related stackoverflow discussion: https://stackoverflow.com/questions/52804334/how-to-ignore-files-directories-in-git-archive-and-only-create-an-archive-of-a\n>\n> Git version: `2.39.2`\n> Mwe git repo: Two files:\n> ```\n> # .gitattributes:\n> .git_archival.txt export-subst\n> ```\n> ```\n> # .git_archival.txt:\n> node: $Format:%H$\n> ```\n>\n> Commands to reproduce and expected behaviour:\n> ```console\n> $ git archive HEAD:./ --output=test.tar.gz\n> $ tar -axf test.tar.gz .git_archival.txt -O\n> node: 745ce26169fb44e04d91d40ee581cccd591c941e\n> ```\n> Important: Notice the path `./` given after `HEAD`.\n>\n> Actual output:\n> ```console\n> $ tar -axf test.tar.gz .git_archival.txt -O\n> node: $Format:%H$\n> ``>\n> It doesn't matter if `.gitattributes` is in a subfolder, or if I change the relative path `./` to a subfolder, the files are still not properly generated.\n>\n> Using `--worktree-attributes` did not have any effect either.\n\nThat's expected behavior. You specify a tree (HEAD:./), while\nexport-subst requires a commit. git-archive(1) doesn't spell that out,\nbut advises to \"See gitattributes(5) for details.\", which states: \"The\nexpansion depends on the availability of a commit ID, i.e., if\ngit-archive(1) has been given a tree instead of a commit or a tag then\nno replacement will be done.\".\n\nThe other attribute supported by git archive, export-ignore, does not\nrequire a commit.\n\nThe Stack Overflow discussion seems to be about a different issue:\nAttributes are always based on the repository root directory, which\ngives unexpected results when archiving a subtree.\n\n> According to the documentation, I understand that the expected behaviour with regards to `--worktree-attributes`:\n> - Read the `.gitattributes` of the relative path, e.g. `./sub_dir` regardless of `--worktree-attributes`. (similar behaviour as not passing a relative path)\n> - Include the `.gitattributes` of the top-level path if `--worktree-attributes` is passed\n>\n> Maybe the intended behaviour is to completely ignore all `.gitattributes` unless `--worktree-attributes` is provided, in which case, it does not have the intended behaviour and please include a flag to achieve the above behaviour.\n>\n\nThe option --worktree-attributes allows uncommitted .gitattribute files\nto be read, but has no effect on whether export-subst can actually be\napplied.\n\nSo on the Git side we could improve the documentation and improve\nreading attributes when archiving subtrees (no idea how, though,\nadmittedly). But that wouldn't help you, I suppose. In your issue\n#444 you write that \"git archive HEAD\" works, but \"git archive HEAD:./\"\ndoesn't. Why do you need to use the latter?\n\nRené\n\n"},{"id":"472961","messageId":"8d04019d-511f-0f99-42cc-d0b25720cd71@mpsd.mpg.de","threadId":"59334","inReplyTo":"c7b21faa-68dd-8bd9-4670-2cf609741094@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Cristian Le","fromEmail":"cristian.le@mpsd.mpg.de","sentAt":"2023-03-03T15:38:59Z","receivedAt":"2023-03-03T15:39:07Z","isPatch":false,"sender":{"key":"cristian.le@mpsd.mpg.de","avatar":null},"body":"> In your issue #444 you write that \"git archive HEAD\" works, but \"git archive HEAD:./\" doesn't. Why do you need to use the latter?\n\nSpecifically we want to allow for `HEAD:./sub_dir` where `./sub_dir` \ncontains `.gitattributes` and `.git_archive.txt`.\n\nAlternatively, it would be helpful if we can pass `--transform` commands \nof `tar` directly so that we can change the paths.\n\nOverall what we are doing in tito is that the source would be in `./src` \nand outside is metadata like `./my_package.spec`. We are using `git \narchive HEAD:./src --prefix=my_package-1.0.0` to pass the appropriate \nform that the rpm spec file can locate. In a tar command we can use \n`--transform=s|^src/|my_package-1.0.0/|` to achieve the equivalent. \nHowever we cannot use the `tar` directly because that would affect the \ntimestamps and permissions of the file that are set by `git archive`.\n\nSo allowing for something like `git archive HEAD \n--transform=s|^src/|my_package-1.0.0/|`, where the transform is done \nafter `.gitattributes` is performed would solve this issue.\n\nOn 2023/03/03 16:19, René Scharfe wrote:\n> External Email\n> ________________________________\n>\n> Am 03.03.23 um 11:25 schrieb Cristian Le:\n>> Using `git archive` with or without `--worktree-attributes` does not properly read `.gitattributes` files if using a relative path in ``.\n>> Related github comment: https://github.com/rpm-software-management/tito/pull/445#issuecomment-1450298871\n>> Related stackoverflow discussion: https://stackoverflow.com/questions/52804334/how-to-ignore-files-directories-in-git-archive-and-only-create-an-archive-of-a\n>>\n>> Git version: `2.39.2`\n>> Mwe git repo: Two files:\n>> ```\n>> # .gitattributes:\n>> .git_archival.txt export-subst\n>> ```\n>> ```\n>> # .git_archival.txt:\n>> node: $Format:%H$\n>> ```\n>>\n>> Commands to reproduce and expected behaviour:\n>> ```console\n>> $ git archive HEAD:./ --output=test.tar.gz\n>> $ tar -axf test.tar.gz .git_archival.txt -O\n>> node: 745ce26169fb44e04d91d40ee581cccd591c941e\n>> ```\n>> Important: Notice the path `./` given after `HEAD`.\n>>\n>> Actual output:\n>> ```console\n>> $ tar -axf test.tar.gz .git_archival.txt -O\n>> node: $Format:%H$\n>> ``>\n>> It doesn't matter if `.gitattributes` is in a subfolder, or if I change the relative path `./` to a subfolder, the files are still not properly generated.\n>>\n>> Using `--worktree-attributes` did not have any effect either.\n> That's expected behavior. You specify a tree (HEAD:./), while\n> export-subst requires a commit. git-archive(1) doesn't spell that out,\n> but advises to \"See gitattributes(5) for details.\", which states: \"The\n> expansion depends on the availability of a commit ID, i.e., if\n> git-archive(1) has been given a tree instead of a commit or a tag then\n> no replacement will be done.\".\n>\n> The other attribute supported by git archive, export-ignore, does not\n> require a commit.\n>\n> The Stack Overflow discussion seems to be about a different issue:\n> Attributes are always based on the repository root directory, which\n> gives unexpected results when archiving a subtree.\n>\n>> According to the documentation, I understand that the expected behaviour with regards to `--worktree-attributes`:\n>> - Read the `.gitattributes` of the relative path, e.g. `./sub_dir` regardless of `--worktree-attributes`. (similar behaviour as not passing a relative path)\n>> - Include the `.gitattributes` of the top-level path if `--worktree-attributes` is passed\n>>\n>> Maybe the intended behaviour is to completely ignore all `.gitattributes` unless `--worktree-attributes` is provided, in which case, it does not have the intended behaviour and please include a flag to achieve the above behaviour.\n>>\n> The option --worktree-attributes allows uncommitted .gitattribute files\n> to be read, but has no effect on whether export-subst can actually be\n> applied.\n>\n> So on the Git side we could improve the documentation and improve\n> reading attributes when archiving subtrees (no idea how, though,\n> admittedly). But that wouldn't help you, I suppose. In your issue\n> #444 you write that \"git archive HEAD\" works, but \"git archive HEAD:./\"\n> doesn't. Why do you need to use the latter?\n>\n> René\n>\n"},{"id":"473004","messageId":"70f10864-2cc7-cb9e-f868-2ac0011cad58@web.de","threadId":"59334","inReplyTo":"8d04019d-511f-0f99-42cc-d0b25720cd71@mpsd.mpg.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-04T13:58:40Z","receivedAt":"2023-03-04T13:58:50Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.03.23 um 16:38 schrieb Cristian Le:\n>> In your issue #444 you write that \"git archive HEAD\" works, but\n>> \"git archive HEAD:./\" doesn't. Why do you need to use the latter?\n>\n> Specifically we want to allow for `HEAD:./sub_dir` where `./sub_dir`\n> contains `.gitattributes` and `.git_archive.txt`.\n>\n> Alternatively, it would be helpful if we can pass `--transform`\n> commands of `tar` directly so that we can change the paths.\n>\n> Overall what we are doing in tito is that the source would be in\n> `./src` and outside is metadata like `./my_package.spec`. We are\n> using `git archive HEAD:./src --prefix=my_package-1.0.0` to pass the\n> appropriate form that the rpm spec file can locate. In a tar command\n> we can use `--transform=s|^src/|my_package-1.0.0/|` to achieve the\n> equivalent.\n\nWhat is Tito? https://github.com/rpm-software-management/tito says:\n\"Tito is a tool for managing RPM based projects using git for their\nsource code repository.\" It supports Git repositories containing\nmultiple projects. I suppose that means e.g. for Git's own repo that\nTito would allow creating a separate RPM file for e.g. git-gui.\n\nSide note: Tito features include: \"Create reliable tar.gz files with\nconsistent checksums from any tag.\" That's achieved by compressing\nusing \"gzip -n -c\". Avoiding the native tgz support of git archive --\nprobably only because the code predates it -- shields Tito from the\nchange to use our internal gzip implementation discussed recently in\nhttps://lore.kernel.org/git/a812a664-67ea-c0ba-599f-cb79e2d96694@gmail.com/\n\nNote, however, that the tar output of git archive is not guaranteed to\nbe stable between Git versions, either. Recently adding such a stable\nformat was proposed in\nhttps://lore.kernel.org/git/20230205221728.4179674-1-sandals@crustytoothpaste.net/\n\nThe code for calling git archive with a tree was present in Tito's\ninitial commit, which says that it was taken from Spacewalk:\nhttps://github.com/rpm-software-management/tito/commit/e87345d7b7.\nThere it was introduced along with a script that changes the mtime\nof archive entries from the current time to the commit timestamp by\nhttps://github.com/spacewalkproject/spacewalk/commit/34267e39d472.\n\nI don't fully understand the explanation in its commit message\n(\"make it possible to call make srpm even if the directory of the\npackage has changed\"); perhaps it requires more domain knowledge.\nBut I can understand the need for archiving sub-directories in the\ncontext of supporting multi-project repositories.\n\n> However we cannot use the `tar` directly because that would affect\n> the timestamps and permissions of the file that are set by `git\n> archive`.\n\nGNU tar has the options --mode and --mtime to chose permissions and\nmodifications of files added to an archive.\n\ngit archive is going to get an --mtime option as well in the next\nrelease, by the way.\n\n> So allowing for something like `git archive HEAD\n> --transform=s|^src/|my_package-1.0.0/|`, where the transform is done\n> after `.gitattributes` is performed would solve this issue.\n\nGNU tar has this --transform option, bsdtar similarly has -s. Both\nalso have --strip-components (GNU tar only for extraction, though),\nwhich is a bit simpler and should suffice for your use case.\n\n--- >8 ---\nSubject: [PATCH] archive: add --strip-components\n\nAllow removing leading elements from paths of archive entries. That's\nuseful when archiving sub-directories and not wanting to keep the\ncommon path prefix, e.g.:\n\n $ git archive --strip-components=1 HEAD sha1dc | tar tf -\n .gitattributes\n LICENSE.txt\n sha1.c\n sha1.h\n ubc_check.c\n ubc_check.h\n\nThe same can be achieved by specifying a tree instead of a commit and\na pathspec:\n\n $ git archive HEAD:sha1dc | tar tf -\n .gitattributes\n LICENSE.txt\n sha1.c\n sha1.h\n ubc_check.c\n ubc_check.h\n\nHowever, this doesn't support the export-subst attribute, doesn't\ninclude the commit hash as an archive comment and uses the current time\ninstead of the commit date as mtime for archive entries.\n\nThe new option is adapted from bsdtar. GNU tar provides it as well, but\nonly for extraction.\n\nThe new option does not affect the paths of entries added by --add-file\nand --add-virtual-file because they are handcrafted to their desired\nvalues already. Similarly, the value of --prefix is not subject to\ncomponent stripping.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-archive.txt | 6 ++++++\n archive.c | 16 ++++++++++++++++\n archive.h | 1 +\n t/t5000-tar-tree.sh | 13 +++++++++++++\n 4 files changed, 36 insertions(+)\n\ndiff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt\nindex 6bab201d37..5dad917e7b 100644\n--- a/Documentation/git-archive.txt\n+++ b/Documentation/git-archive.txt\n@@ -55,6 +55,12 @@ OPTIONS\n \trightmost value is used for all tracked files. See below which\n \tvalue gets used by `--add-file` and `--add-virtual-file`.\n\n+--strip-components=::\n+\tRemove the specified number of leading path elements. Pathnames\n+\twith fewer elements will be silently skipped. Does not affect\n+\tthe prefix added by `--prefix`, nor entries added with\n+\t`--add-file` or `--add-virtual-file`.\n+\n -o ::\n --output=::\n \tWrite the archive to instead of stdout.\ndiff --git a/archive.c b/archive.c\nindex 9aeaf2bd87..8308d4d9c4 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -166,6 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\targs->convert = check_attr_export_subst(check);\n \t}\n\n+\tif (args->strip_components > 0) {\n+\t\tsize_t orig_baselen = baselen;\n+\t\tfor (int i = 0; i < args->strip_components; i++) {\n+\t\t\tconst char *slash = memchr(base, '/', baselen);\n+\t\t\tif (!slash)\n+\t\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n+\t\t\tbaselen -= slash - base + 1;\n+\t\t\tbase = slash + 1;\n+\t\t}\n+\t\tstrbuf_remove(&path, args->baselen, orig_baselen - baselen);\n+\t}\n+\n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\n@@ -593,12 +605,15 @@ static int parse_archive_args(int argc, const char **argv,\n \tint verbose = 0;\n \tint i;\n \tint list = 0;\n+\tint strip_components = 0;\n \tint worktree_attributes = 0;\n \tstruct option opts[] = {\n \t\tOPT_GROUP(\"\"),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"fmt\"), N_(\"archive format\")),\n \t\tOPT_STRING(0, \"prefix\", &base, N_(\"prefix\"),\n \t\t\tN_(\"prepend prefix to each pathname in the archive\")),\n+\t\tOPT_INTEGER(0, \"strip-components\", &strip_components,\n+\t\t\tN_(\"remove leading path elements\")),\n \t\t{ OPTION_CALLBACK, 0, \"add-file\", args, N_(\"file\"),\n \t\t N_(\"add untracked file to archive\"), 0, add_file_cb,\n \t\t (intptr_t)&base },\n@@ -675,6 +690,7 @@ static int parse_archive_args(int argc, const char **argv,\n \targs->baselen = strlen(base);\n \targs->worktree_attributes = worktree_attributes;\n \targs->mtime_option = mtime_option;\n+\targs->strip_components = strip_components;\n\n \treturn argc;\n }\ndiff --git a/archive.h b/archive.h\nindex 7178e2a9a2..e9becbd57d 100644\n--- a/archive.h\n+++ b/archive.h\n@@ -23,6 +23,7 @@ struct archiver_args {\n \tunsigned int worktree_attributes : 1;\n \tunsigned int convert : 1;\n \tint compression_level;\n+\tint strip_components;\n \tstruct string_list extra_files;\n \tstruct pretty_print_context *pretty_ctx;\n };\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 918a2fc7c6..629d2e78d7 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -271,6 +271,19 @@ test_expect_success 'git get-tar-commit-id' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'git archive --strip-components' '\n+\tgit archive --strip-components=3 HEAD >strip3.tar &&\n+\t(\n+\t\tmkdir strip3 &&\n+\t\tcd strip3 &&\n+\t\t\"$TAR\" xf ../strip3.tar &&\n+\t\tfind . | grep -v \"^\\.\\$\" | sort >../strip3.lst\n+\t) &&\n+\tsed -ne \"s-\\([^/]*/\\)\\{3\\}-./-p\" a.lst >expect &&\n+\ttest_cmp expect strip3.lst &&\n+\tdiff -r a/long_path_to_a_file/long_path_to_a_file strip3\n+'\n+\n test_expect_success 'git archive with --output, override inferred format' '\n \tgit archive --format=tar --output=d4.zip HEAD &&\n \ttest_cmp_bin b.tar d4.zip\n--\n2.39.2\n"},{"id":"473005","messageId":"566c2d09-5e18-49c9-fc7c-7b92d1d7c198@mpsd.mpg.de","threadId":"59334","inReplyTo":"70f10864-2cc7-cb9e-f868-2ac0011cad58@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Cristian Le","fromEmail":"cristian.le@mpsd.mpg.de","sentAt":"2023-03-04T15:11:15Z","receivedAt":"2023-03-04T15:11:27Z","isPatch":false,"sender":{"key":"cristian.le@mpsd.mpg.de","avatar":null},"body":"> I suppose that means e.g. for Git's own repo that\n> Tito would allow creating a separate RPM file for e.g. git-gui.\nIndeed, that pretty much sums the main idea of why they need sub \ndirectories there. Ideally we don't want different special cases, and \ninstead use as much native behaviour as possible. The main issue here is \njust making it work with `export-subst`, and if it can be achieved with \nboth `strip-components` and `prefix`, that might work, even though \n`--transform` would be more flexible.\n\n> However we cannot use the `tar` directly because that would affect\n> the timestamps and permissions of the file that are set by `git\n> archive`.\nI should have explained more thoroughly, I have tried to work around by \ndoing a `git archive` and then extracting and re-compressing after \nfixing the paths, but this does not preserve the owner and timestamp of \nthe original `git archive`, nor can I use the current implementation of \ntheir tar fixer to correct these since the headers are different. I do \nnot have enough expertise to know what headers need to be set, how to \nset the timestamps and so on. I don't know if the `gzip -n -c` could do \na better job at that, but we would still not be able to use it as is \nbecause we would want the correctly generate `export-subst` files, for \nexample for a project built with `setuptools_scm` that injects the \nversion of the python package from the last tag:\n\nhttps://github.com/pypa/setuptools_scm#git-archives\n\n> There it was introduced along with a script that changes the mtime\n> of archive entries from the current time to the commit timestamp by\n> https://github.com/spacewalkproject/spacewalk/commit/34267e39d472.\nThanks for pointing me to this. From your understanding, if we only use \nthe git commit directly, we would in principle not need the whole tar \nfixer \n(https://github.com/rpm-software-management/tito/blob/91ef962220ec5154722760dbbd982bed032ee484/src/tito/tar.py#L28-L60 \nand \nhttps://github.com/rpm-software-management/tito/blob/91ef962220ec5154722760dbbd982bed032ee484/src/tito/common.py#L871-L890)? \nIf there is no crucial information in the header that is different \nbetween the `git archive` and the `tar`/`gzip` with appropriate `mtime`, \n`mode` and maybe others, maybe there is some hope in fixing this locally.\n\nI'll be looking forward to `--strip-components`, but just to confirm my \nreading of the email, the intent is to have it working with \n`export-subst` right?\n\nCheers and thanks for your intuitive answers.\n\n"},{"id":"473024","messageId":"c91db499-fa26-b902-3ab8-14c52959482f@web.de","threadId":"59334","inReplyTo":"566c2d09-5e18-49c9-fc7c-7b92d1d7c198@mpsd.mpg.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-05T09:32:02Z","receivedAt":"2023-03-05T09:32:14Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.03.23 um 16:11 schrieb Cristian Le:\n> I should have explained more thoroughly, I have tried to work around\n> by doing a `git archive` and then extracting and re-compressing after\n> fixing the paths, but this does not preserve the owner and timestamp\n> of the original `git archive`, nor can I use the current\n> implementation of their tar fixer to correct these since the headers\n> are different. I do not have enough expertise to know what headers\n> need to be set, how to set the timestamps and so on.\n\nGNU tar has the options --owner, --group and --mtime that allow you to\nspecify arbitrary values for these fields when archiving, no header\ntinkering needed. That said, tarring with git archive, untarring and\nretarring with GNU tar is overly complicated and horrible when git\narchive can do the whole job itself.\n\n> I don't know if the `gzip -n -c` could do a better job at that,\n\nThat just compresses tar files to tgz (or tar.gz) format and does not\naffect any tar headers.\n\n> but we would still\n> not be able to use it as is because we would want the correctly\n> generate `export-subst` files, for example for a project built with\n> `setuptools_scm` that injects the version of the python package from\n> the last tag:\n>\n> https://github.com/pypa/setuptools_scm#git-archives\n\nNote that git archive allows injecting extra files with the options\n --add-file and --add-virtual-file. Could that feature perhaps be used\ninstead of export-subst?\n\n>> There it was introduced along with a script that changes the mtime\n>> of archive entries from the current time to the commit timestamp by\n>> https://github.com/spacewalkproject/spacewalk/commit/34267e39d472.\n> Thanks for pointing me to this. From your understanding, if we only use the git commit directly, we would in principle not need the whole tar fixer (https://github.com/rpm-software-management/tito/blob/91ef962220ec5154722760dbbd982bed032ee484/src/tito/tar.py#L28-L60 and https://github.com/rpm-software-management/tito/blob/91ef962220ec5154722760dbbd982bed032ee484/src/tito/common.py#L871-L890)? If there is no crucial information in the header that is different between the `git archive` and the `tar`/`gzip` with appropriate `mtime`, `mode` and maybe others, maybe there is some hope in fixing this locally.\n\nThe tar fixer was rewritten in Python in the meantime and I didn't look\nit it closely, but if it only sets the mtime of all archive entries to\nthe commit date then it's not needed when giving git archive a commit\nor tag instead of a tree.\n\n> I'll be looking forward to `--strip-components`, but just to confirm\n> my reading of the email, the intent is to have it working with\n> `export-subst` right?\nThat option would work the same regardless whether git archive is given\na tree, commit or tag, and it doesn't care about attributes or file\ncontents. It would be especially helpful when giving a commit or tag\nand a pathspec, and export-subst needs a commit or tag. So in short:\nYes. :)\n\nRené\n"},{"id":"473060","messageId":"xmqqy1o9byye.fsf@gitster.g","threadId":"59334","inReplyTo":"70f10864-2cc7-cb9e-f868-2ac0011cad58@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T16:56:41Z","receivedAt":"2023-03-06T16:57:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> $ git archive --strip-components=1 HEAD sha1dc | tar tf -\n> .gitattributes\n> LICENSE.txt\n> sha1.c\n> sha1.h\n> ubc_check.c\n> ubc_check.h\n\nWhat should happen to paths that match the given pathspec that do\nnot have enough number of components? E.g. \"cache.h\" when the\ncommand is \"git archive --strip-components=1 HEAD \\*.h\"? Should it\nbe documented?\n\n> The new option does not affect the paths of entries added by --add-file\n> and --add-virtual-file because they are handcrafted to their desired\n> values already. Similarly, the value of --prefix is not subject to\n> component stripping.\n\nVery sensible.\n\n> diff --git a/archive.c b/archive.c\n> index 9aeaf2bd87..8308d4d9c4 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -166,6 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n> \t\targs->convert = check_attr_export_subst(check);\n> \t}\n\nWe probably could save attribute lookup overhead by moving the new\nlogic a bit higher in the function?\n\nNo, that would invalidate the path_without_prefix variable by using\nstrbuf_remove() on &path, and will break the attribute look-up. The\nvariable is used only once before this point and never used later,\nbut as an independent future-proofing, we may want to remove the\nvariable or narrow the scope. It's totally out of scope of the\npatch, though.\n\n> +\tif (args->strip_components > 0) {\n> +\t\tsize_t orig_baselen = baselen;\n> +\t\tfor (int i = 0; i < args->strip_components; i++) {\n> +\t\t\tconst char *slash = memchr(base, '/', baselen);\n> +\t\t\tif (!slash)\n> +\t\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n> +\t\t\tbaselen -= slash - base + 1;\n> +\t\t\tbase = slash + 1;\n> +\t\t}\n> +\t\tstrbuf_remove(&path, args->baselen, orig_baselen - baselen);\n> +\t}\n\nNice to see that the core logic of the new feature is surprisingly\nsmall.\n\n> \tif (args->verbose)\n> \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\nBy having the verbose output after the path stripping, we won't show\nthe leading components we stripped, making it similar to what we\nwould see when we piped the resulting archive to \"| tar tf -\". I\nguess this makes sense than showing the original path.\n"},{"id":"473063","messageId":"xmqqcz5lbxiq.fsf@gitster.g","threadId":"59334","inReplyTo":"70f10864-2cc7-cb9e-f868-2ac0011cad58@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T17:27:41Z","receivedAt":"2023-03-06T17:28:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Subject: [PATCH] archive: add --strip-components\n>\n> Allow removing leading elements from paths of archive entries. That's\n> useful when archiving sub-directories and not wanting to keep the\n> common path prefix, e.g.:\n>\n> $ git archive --strip-components=1 HEAD sha1dc | tar tf -\n> .gitattributes\n> LICENSE.txt\n> sha1.c\n> sha1.h\n> ubc_check.c\n> ubc_check.h\n>\n> The same can be achieved by specifying a tree instead of a commit and\n> a pathspec:\n>\n> $ git archive HEAD:sha1dc | tar tf -\n> .gitattributes\n> LICENSE.txt\n> sha1.c\n> sha1.h\n> ubc_check.c\n> ubc_check.h\n\nAnother way I am not sure is working as designed is\n\n $ cd sha1dc && git archive HEAD . | tar tf -\n .gitattributes\n LICENSE.txt\n sha1.c\n sha1.h\n ubc_check.c\n ubc_check.h\n\nI didn't check if the attribute look-up is done on the correct path\nor export-subst kicks in in such a use, though.\n"},{"id":"473069","messageId":"a86f05cb-407e-f5c6-ed15-a1fbf1be0584@web.de","threadId":"59334","inReplyTo":"xmqqy1o9byye.fsf@gitster.g","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-06T17:51:18Z","receivedAt":"2023-03-06T18:05:52Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.23 um 17:56 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> $ git archive --strip-components=1 HEAD sha1dc | tar tf -\n>> .gitattributes\n>> LICENSE.txt\n>> sha1.c\n>> sha1.h\n>> ubc_check.c\n>> ubc_check.h\n>\n> What should happen to paths that match the given pathspec that do\n> not have enough number of components? E.g. \"cache.h\" when the\n> command is \"git archive --strip-components=1 HEAD \\*.h\"? Should it\n> be documented?\n\nEntries whose full path is stripped away don't make it into the archive.\nThat behavior is copied from bsdtar along with the option name and most\nof its description in git-archive.txt.\n\nAlternatively we could warn or die. The latter would be a bit awkward\nbecause we'd either have to check all paths first or risk reporting them\nafter writing at least some headers.\n\nNo strong preference, but following the precedence set by bsdtar makes\nthe most sense to me.\n\n>> The new option does not affect the paths of entries added by --add-file\n>> and --add-virtual-file because they are handcrafted to their desired\n>> values already. Similarly, the value of --prefix is not subject to\n>> component stripping.\n>\n> Very sensible.\n>\n>> diff --git a/archive.c b/archive.c\n>> index 9aeaf2bd87..8308d4d9c4 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -166,6 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n>> \t\targs->convert = check_attr_export_subst(check);\n>> \t}\n>\n> We probably could save attribute lookup overhead by moving the new\n> logic a bit higher in the function?\n>\n> No, that would invalidate the path_without_prefix variable by using\n> strbuf_remove() on &path, and will break the attribute look-up. The\n> variable is used only once before this point and never used later,\n> but as an independent future-proofing, we may want to remove the\n> variable or narrow the scope. It's totally out of scope of the\n> patch, though.\n\nWould you have noticed that attribute lookup breakage without the\npresence of that variable? :)\n\nThe sad thing is that we concatenate base and filename here and\nthen attr.c::collect_some_attrs() goes and splits them again. It\nalso uses the concatenated path, but perhaps that can be avoided?\n\n>> +\tif (args->strip_components > 0) {\n>> +\t\tsize_t orig_baselen = baselen;\n>> +\t\tfor (int i = 0; i < args->strip_components; i++) {\n>> +\t\t\tconst char *slash = memchr(base, '/', baselen);\n>> +\t\t\tif (!slash)\n>> +\t\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n>> +\t\t\tbaselen -= slash - base + 1;\n>> +\t\t\tbase = slash + 1;\n>> +\t\t}\n>> +\t\tstrbuf_remove(&path, args->baselen, orig_baselen - baselen);\n>> +\t}\n>\n> Nice to see that the core logic of the new feature is surprisingly\n> small.\n>\n>> \tif (args->verbose)\n>> \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>\n> By having the verbose output after the path stripping, we won't show\n> the leading components we stripped, making it similar to what we\n> would see when we piped the resulting archive to \"| tar tf -\". I\n> guess this makes sense than showing the original path.\n\nRight, printing the path as it appears in the archive makes sense.\nbsdtar does the same..\n\nRené\n"},{"id":"473072","messageId":"d16c6a22-05d8-182c-97b4-53263d22eaa6@web.de","threadId":"59334","inReplyTo":"xmqqcz5lbxiq.fsf@gitster.g","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-06T18:28:02Z","receivedAt":"2023-03-06T18:28:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.23 um 18:27 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Subject: [PATCH] archive: add --strip-components\n>>\n>> Allow removing leading elements from paths of archive entries. That's\n>> useful when archiving sub-directories and not wanting to keep the\n>> common path prefix, e.g.:\n>>\n>> $ git archive --strip-components=1 HEAD sha1dc | tar tf -\n>> .gitattributes\n>> LICENSE.txt\n>> sha1.c\n>> sha1.h\n>> ubc_check.c\n>> ubc_check.h\n>>\n>> The same can be achieved by specifying a tree instead of a commit and\n>> a pathspec:\n>>\n>> $ git archive HEAD:sha1dc | tar tf -\n>> .gitattributes\n>> LICENSE.txt\n>> sha1.c\n>> sha1.h\n>> ubc_check.c\n>> ubc_check.h\n>\n> Another way I am not sure is working as designed is\n>\n> $ cd sha1dc && git archive HEAD . | tar tf -\n> .gitattributes\n> LICENSE.txt\n> sha1.c\n> sha1.h\n> ubc_check.c\n> ubc_check.hq\n>\n> I didn't check if the attribute look-up is done on the correct path\n> or export-subst kicks in in such a use, though.\n\nexport-subst is supported in that invocation because git archive has a\ncommit to work with.\n\nI can kinda see others preferring the directory prefix \"sha1dc/\" added\nto those entries. Perhaps it depends on what git archive is supposed to\narchive: A commit or the files of a commit? I'm in the latter camp, and\nexpect to see the same paths as given by git ls-files or git ls-tree.\n\nBut that invocation in a sub-directory probably has the same problem\nwith attributes as the one with a sub-tree above it, i.e. that\nattributes are always looked up relative to the repository root. I\nwonder if 47cfc9bd7d (attr: add flag `--source` to work with tree-ish,\n2023-01-14) provided the means to fix this when it added a tree_oid\nparameter to git_check_attr().\n\nRené\n"},{"id":"473076","messageId":"xmqqo7p59049.fsf@gitster.g","threadId":"59334","inReplyTo":"d16c6a22-05d8-182c-97b4-53263d22eaa6@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T18:59:50Z","receivedAt":"2023-03-06T19:00:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> Another way I am not sure is working as designed is\n>>\n>> $ cd sha1dc && git archive HEAD . | tar tf -\n>> .gitattributes\n>> LICENSE.txt\n>> sha1.c\n>> sha1.h\n>> ubc_check.c\n>> ubc_check.hq\n>>\n>> I didn't check if the attribute look-up is done on the correct path\n>> or export-subst kicks in in such a use, though.\n>\n> export-subst is supported in that invocation because git archive has a\n> commit to work with.\n>\n> I can kinda see others preferring the directory prefix \"sha1dc/\" added\n> to those entries. Perhaps it depends on what git archive is supposed to\n> archive: A commit or the files of a commit? I'm in the latter camp, and\n> expect to see the same paths as given by git ls-files or git ls-tree.\n>\n> But that invocation in a sub-directory probably has the same problem\n> with attributes as the one with a sub-tree above it, i.e. that\n> attributes are always looked up relative to the repository root. I\n> wonder if 47cfc9bd7d (attr: add flag `--source` to work with tree-ish,\n> 2023-01-14) provided the means to fix this when it added a tree_oid\n> parameter to git_check_attr().\n\nIt somehow feels that the use of pathspec in \"git archive\" is\nsomewhat iffy, e.g.\n\n $ cd sha1dc && git archive HEAD :/ | tar tf -\n\ndoes not compare very well with\n\n $ cd sha1dc && git ls-tree -r HEAD :/\n\nFor that matter, replacing \":/\" (full tree) with \"..\" (we know one\nlevel above is the root of the working tree) has the same \"why don't\nthey work the same way???\" confusion.\n\n"},{"id":"473086","messageId":"3da35216-ca42-9759-d4f9-20451a44c231@web.de","threadId":"59334","inReplyTo":"xmqqo7p59049.fsf@gitster.g","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-06T21:32:06Z","receivedAt":"2023-03-06T21:32:36Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.23 um 19:59 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n> It somehow feels that the use of pathspec in \"git archive\" is\n> somewhat iffy, e.g.\n>\n> $ cd sha1dc && git archive HEAD :/ | tar tf -\n>\n> does not compare very well with\n>\n> $ cd sha1dc && git ls-tree -r HEAD :/\n>\n> For that matter, replacing \":/\" (full tree) with \"..\" (we know one\n> level above is the root of the working tree) has the same \"why don't\n> they work the same way???\" confusion.\n\nRight,\nhttps://lore.kernel.org/git/CA+X7ob8DWGmoVTxSVvrFN68V=pcaZripfP=s+LpWvXN-6L7W7Q@mail.gmail.com/\nreports the second one as well.\n\nIt's a consequence of effectively cd'ing into the prefix tree in\narchive.c::parse_treeish_arg() and using the empty string as prefix from\nthen on. If we stop doing that then we're getting much closer (path at\nthe end):\n\n $ git -C xdiff ls-files '../sha1dc/*.c' '../xdiff/*.c'\n ../sha1dc/sha1.c\n ../sha1dc/ubc_check.c\n xdiffi.c\n xemit.c\n xhistogram.c\n xmerge.c\n xpatience.c\n xprepare.c\n xutils.c\n\n\n $ git -C xdiff archive HEAD '../sha1dc/*.c' '../xdiff/*.c' | tar tf -\n ../sha1dc/\n ../sha1dc/sha1.c\n ../sha1dc/ubc_check.c\n xdiffi.c\n xemit.c\n xhistogram.c\n xmerge.c\n xpatience.c\n xprepare.c\n xutils.c\n\nNot sure if we want those leading double dots. bsdtar has them:\n\n $ (cd xdiff && tar cf - ../sha1dc/*.c ../xdiff/*.c) | tar tf -\n ../sha1dc/sha1.c\n ../sha1dc/ubc_check.c\n ../xdiff/xdiffi.c\n ../xdiff/xemit.c\n ../xdiff/xhistogram.c\n ../xdiff/xmerge.c\n ../xdiff/xpatience.c\n ../xdiff/xprepare.c\n ../xdiff/xutils.c\n\n... but GNU tar strips them off and warns about them:\n\n $ (cd xdiff && gtar cf - ../sha1dc/*.c ../xdiff/*.c) | tar tf -\n gtar: Removing leading `../' from member names\n gtar: Removing leading `../' from hard link targets\n sha1dc/sha1.c\n sha1dc/ubc_check.c\n xdiff/xdiffi.c\n xdiff/xemit.c\n xdiff/xhistogram.c\n xdiff/xmerge.c\n xdiff/xpatience.c\n xdiff/xprepare.c\n xdiff/xutils.c\n\nNeither of them resolves \"../$PWD/\" parts to \"\" like git ls-tree does,\nbut I can accept that difference. And then we'd need to keep leading\n\"../\", I suppose. Still unsure.\n\nAnd I don't know why PATHSPEC_PREFER_CWD is necessary.\n\nSo no sign-off, yet.\n\n\n archive.c | 60 +++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 35 insertions(+), 25 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 9aeaf2bd87..c7e9f58b02 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -139,6 +139,7 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\tvoid *context)\n {\n \tstatic struct strbuf path = STRBUF_INIT;\n+\tstatic struct strbuf scratch = STRBUF_INIT;\n \tstruct archiver_context *c = context;\n \tstruct archiver_args *args = c->args;\n \twrite_archive_entry_fn_t write_entry = c->write_entry;\n@@ -148,6 +149,14 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \tvoid *buffer;\n \tenum object_type type;\n\n+\t/*\n+\t * NEEDSWORK: variable names could be clearer:\n+\t * - args->prefix is the current working directory,\n+\t * - args->base with args->baselen is the --prefix value,\n+\t * - base with baselen is the path of the current tree,\n+\t * - args->base + base + filename is the path in the archive,\n+\t * - path_without_prefix is base + filename.\n+\t */\n \targs->convert = 0;\n \tstrbuf_reset(&path);\n \tstrbuf_grow(&path, PATH_MAX);\n@@ -166,6 +175,15 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\targs->convert = check_attr_export_subst(check);\n \t}\n\n+\tif (args->prefix) {\n+\t\tconst char *rel = relative_path(path_without_prefix,\n+\t\t\t\t\t\targs->prefix, &scratch);\n+\t\tif (!strcmp(rel, \"./\"))\n+\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n+\t\tstrbuf_setlen(&path, args->baselen);\n+\t\tstrbuf_addstr(&path, rel);\n+\t}\n+\n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\n@@ -401,14 +419,15 @@ static int reject_entry(const struct object_id *oid UNUSED,\n \treturn ret;\n }\n\n-static int path_exists(struct archiver_args *args, const char *path)\n+static int path_exists(struct archiver_args *args, const char *prefix,\n+\t\t const char *path)\n {\n \tconst char *paths[] = { path, NULL };\n \tstruct path_exists_context ctx;\n \tint ret;\n\n \tctx.args = args;\n-\tparse_pathspec(&ctx.pathspec, 0, 0, \"\", paths);\n+\tparse_pathspec(&ctx.pathspec, 0, 0, prefix, paths);\n \tctx.pathspec.recursive = 1;\n \tret = read_tree(args->repo, args->tree,\n \t\t\t&ctx.pathspec,\n@@ -417,30 +436,35 @@ static int path_exists(struct archiver_args *args, const char *path)\n \treturn ret != 0;\n }\n\n-static void parse_pathspec_arg(const char **pathspec,\n+static void parse_pathspec_arg(const char **pathspec, const char *prefix,\n \t\tstruct archiver_args *ar_args)\n {\n+\tconst char *match_all[] = { \".\", NULL };\n+\n+\tif (prefix && !*pathspec)\n+\t\tpathspec = match_all;\n+\n \t/*\n \t * must be consistent with parse_pathspec in path_exists()\n \t * Also if pathspec patterns are dependent, we're in big\n \t * trouble as we test each one separately\n \t */\n \tparse_pathspec(&ar_args->pathspec, 0,\n-\t\t PATHSPEC_PREFER_FULL,\n-\t\t \"\", pathspec);\n+\t\t PATHSPEC_PREFER_CWD,\n+\t\t prefix, pathspec);\n \tar_args->pathspec.recursive = 1;\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n-\t\t\tif (**pathspec && !path_exists(ar_args, *pathspec))\n+\t\t\tif (**pathspec &&\n+\t\t\t !path_exists(ar_args, prefix, *pathspec))\n \t\t\t\tdie(_(\"pathspec '%s' did not match any files\"), *pathspec);\n \t\t\tpathspec++;\n \t\t}\n \t}\n }\n\n-static void parse_treeish_arg(const char **argv,\n-\t\tstruct archiver_args *ar_args, const char *prefix,\n-\t\tint remote)\n+static void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,\n+\t\t\t int remote)\n {\n \tconst char *name = argv[0];\n \tconst struct object_id *commit_oid;\n@@ -479,20 +503,6 @@ static void parse_treeish_arg(const char **argv,\n \tif (!tree)\n \t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n\n-\tif (prefix) {\n-\t\tstruct object_id tree_oid;\n-\t\tunsigned short mode;\n-\t\tint err;\n-\n-\t\terr = get_tree_entry(ar_args->repo,\n-\t\t\t\t &tree->object.oid,\n-\t\t\t\t prefix, &tree_oid,\n-\t\t\t\t &mode);\n-\t\tif (err || !S_ISDIR(mode))\n-\t\t\tdie(_(\"current working directory is untracked\"));\n-\n-\t\ttree = parse_tree_indirect(&tree_oid);\n-\t}\n \tar_args->refname = ref;\n \tar_args->tree = tree;\n \tar_args->commit_oid = commit_oid;\n@@ -710,8 +720,8 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \t\tsetup_git_directory();\n \t}\n\n-\tparse_treeish_arg(argv, &args, prefix, remote);\n-\tparse_pathspec_arg(argv + 1, &args);\n+\tparse_treeish_arg(argv, &args, remote);\n+\tparse_pathspec_arg(argv + 1, prefix, &args);\n\n \trc = ar->write_archive(ar, &args);\n\n\n"},{"id":"473091","messageId":"xmqq4jqx8q6q.fsf@gitster.g","threadId":"59334","inReplyTo":"3da35216-ca42-9759-d4f9-20451a44c231@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T22:34:21Z","receivedAt":"2023-03-06T22:34:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Neither of them resolves \"../$PWD/\" parts to \"\" like git ls-tree does,\n> but I can accept that difference. And then we'd need to keep leading\n> \"../\", I suppose. Still unsure.\n\nI offhand do not know how well it would mix with --strip-components\nif we leave the leading \"../\".\n\nBut it certainly would be nice if we somehow:\n\n * can keep the current behaviour where \"git -C sub archive\" records\n paths relative to \"sub\" for backward compatibility.\n\n * fail loudly when \"git -C sub archive \" makes us use\n \"../\" prefix because goes above the $PWD for backward\n compatibility and sanity.\n\n * with --some-option, make \"git -C sub archive --some-option :/\"\n act exactly like \"git archive :/\".\n\n> And I don't know why PATHSPEC_PREFER_CWD is necessary.\n>\n> So no sign-off, yet.\n>\n>\n> archive.c | 60 +++++++++++++++++++++++++++++++++++-------------------------\n> 1 file changed, 35 insertions(+), 25 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 9aeaf2bd87..c7e9f58b02 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -139,6 +139,7 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n> \t\tvoid *context)\n> {\n> \tstatic struct strbuf path = STRBUF_INIT;\n> +\tstatic struct strbuf scratch = STRBUF_INIT;\n> \tstruct archiver_context *c = context;\n> \tstruct archiver_args *args = c->args;\n> \twrite_archive_entry_fn_t write_entry = c->write_entry;\n> @@ -148,6 +149,14 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n> \tvoid *buffer;\n> \tenum object_type type;\n>\n> +\t/*\n> +\t * NEEDSWORK: variable names could be clearer:\n> +\t * - args->prefix is the current working directory,\n> +\t * - args->base with args->baselen is the --prefix value,\n> +\t * - base with baselen is the path of the current tree,\n> +\t * - args->base + base + filename is the path in the archive,\n> +\t * - path_without_prefix is base + filename.\n> +\t */\n> \targs->convert = 0;\n> \tstrbuf_reset(&path);\n> \tstrbuf_grow(&path, PATH_MAX);\n> @@ -166,6 +175,15 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n> \t\targs->convert = check_attr_export_subst(check);\n> \t}\n>\n> +\tif (args->prefix) {\n> +\t\tconst char *rel = relative_path(path_without_prefix,\n> +\t\t\t\t\t\targs->prefix, &scratch);\n> +\t\tif (!strcmp(rel, \"./\"))\n> +\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n> +\t\tstrbuf_setlen(&path, args->baselen);\n> +\t\tstrbuf_addstr(&path, rel);\n> +\t}\n> +\n> \tif (args->verbose)\n> \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>\n> @@ -401,14 +419,15 @@ static int reject_entry(const struct object_id *oid UNUSED,\n> \treturn ret;\n> }\n>\n> -static int path_exists(struct archiver_args *args, const char *path)\n> +static int path_exists(struct archiver_args *args, const char *prefix,\n> +\t\t const char *path)\n> {\n> \tconst char *paths[] = { path, NULL };\n> \tstruct path_exists_context ctx;\n> \tint ret;\n>\n> \tctx.args = args;\n> -\tparse_pathspec(&ctx.pathspec, 0, 0, \"\", paths);\n> +\tparse_pathspec(&ctx.pathspec, 0, 0, prefix, paths);\n> \tctx.pathspec.recursive = 1;\n> \tret = read_tree(args->repo, args->tree,\n> \t\t\t&ctx.pathspec,\n> @@ -417,30 +436,35 @@ static int path_exists(struct archiver_args *args, const char *path)\n> \treturn ret != 0;\n> }\n>\n> -static void parse_pathspec_arg(const char **pathspec,\n> +static void parse_pathspec_arg(const char **pathspec, const char *prefix,\n> \t\tstruct archiver_args *ar_args)\n> {\n> +\tconst char *match_all[] = { \".\", NULL };\n> +\n> +\tif (prefix && !*pathspec)\n> +\t\tpathspec = match_all;\n> +\n> \t/*\n> \t * must be consistent with parse_pathspec in path_exists()\n> \t * Also if pathspec patterns are dependent, we're in big\n> \t * trouble as we test each one separately\n> \t */\n> \tparse_pathspec(&ar_args->pathspec, 0,\n> -\t\t PATHSPEC_PREFER_FULL,\n> -\t\t \"\", pathspec);\n> +\t\t PATHSPEC_PREFER_CWD,\n> +\t\t prefix, pathspec);\n> \tar_args->pathspec.recursive = 1;\n> \tif (pathspec) {\n> \t\twhile (*pathspec) {\n> -\t\t\tif (**pathspec && !path_exists(ar_args, *pathspec))\n> +\t\t\tif (**pathspec &&\n> +\t\t\t !path_exists(ar_args, prefix, *pathspec))\n> \t\t\t\tdie(_(\"pathspec '%s' did not match any files\"), *pathspec);\n> \t\t\tpathspec++;\n> \t\t}\n> \t}\n> }\n>\n> -static void parse_treeish_arg(const char **argv,\n> -\t\tstruct archiver_args *ar_args, const char *prefix,\n> -\t\tint remote)\n> +static void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,\n> +\t\t\t int remote)\n> {\n> \tconst char *name = argv[0];\n> \tconst struct object_id *commit_oid;\n> @@ -479,20 +503,6 @@ static void parse_treeish_arg(const char **argv,\n> \tif (!tree)\n> \t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n>\n> -\tif (prefix) {\n> -\t\tstruct object_id tree_oid;\n> -\t\tunsigned short mode;\n> -\t\tint err;\n> -\n> -\t\terr = get_tree_entry(ar_args->repo,\n> -\t\t\t\t &tree->object.oid,\n> -\t\t\t\t prefix, &tree_oid,\n> -\t\t\t\t &mode);\n> -\t\tif (err || !S_ISDIR(mode))\n> -\t\t\tdie(_(\"current working directory is untracked\"));\n> -\n> -\t\ttree = parse_tree_indirect(&tree_oid);\n> -\t}\n> \tar_args->refname = ref;\n> \tar_args->tree = tree;\n> \tar_args->commit_oid = commit_oid;\n> @@ -710,8 +720,8 @@ int write_archive(int argc, const char **argv, const char *prefix,\n> \t\tsetup_git_directory();\n> \t}\n>\n> -\tparse_treeish_arg(argv, &args, prefix, remote);\n> -\tparse_pathspec_arg(argv + 1, &args);\n> +\tparse_treeish_arg(argv, &args, remote);\n> +\tparse_pathspec_arg(argv + 1, prefix, &args);\n>\n> \trc = ar->write_archive(ar, &args);\n"},{"id":"473409","messageId":"f7949f1b-4bad-e1bf-4754-f8b79e3ce279@web.de","threadId":"59334","inReplyTo":"xmqq4jqx8q6q.fsf@gitster.g","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-11T20:47:14Z","receivedAt":"2023-03-11T20:47:27Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.23 um 23:34 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Neither of them resolves \"../$PWD/\" parts to \"\" like git ls-tree does,\n>> but I can accept that difference. And then we'd need to keep leading\n>> \"../\", I suppose. Still unsure.\n>\n> I offhand do not know how well it would mix with --strip-components\n> if we leave the leading \"../\".\n\nNot too well when entries from $PWD are shortened and mixed with ones\nfrom elsewhere that aren't. But that seems like a strange thing to do.\nIf two sub-trees are needed then git archive should be started in a\nshared parent directory higher up.\n\n> But it certainly would be nice if we somehow:\n>\n> * can keep the current behaviour where \"git -C sub archive\" records\n> paths relative to \"sub\" for backward compatibility.\n\nRight. That's what relative_path() provides in the patch.\n\n> * fail loudly when \"git -C sub archive \" makes us use\n> \"../\" prefix because goes above the $PWD for backward\n> compatibility and sanity.\n\nWithout the patch this fails, but are there really people that depend on\nit failing? We could certainly forbid it, but do we need to? It costs\na few lines of code (see patch below).\n\n> * with --some-option, make \"git -C sub archive --some-option :/\"\n> act exactly like \"git archive :/\".\n\nPerhaps I'm reading this too literally, but it would be easier to remove\n\"-C sub\" from that command. Or to add \"-C $(git rev-parse --show-cdup)\".\nWe could add a shortcut for that (see patch below).\n\n>> And I don't know why PATHSPEC_PREFER_CWD is necessary.\n\nPATHSPEC_PREFER_FULL makes the empty pathspec match everything, while\nPATHSPEC_PREFER_CWD makes it match the current working directory, only.\nIn git archive without these patches both mean the same because we\nmake the tree of the current working directory the root of our pathspec\nworldview. When we stop doing that it actually makes a difference and\nwe need the more limited variant.\n\nRené\n\n\n Documentation/git.txt | 4 ++++\n archive.c | 36 ++++++++++++++++++++++--------------\n git.c | 18 ++++++++++++++++++\n t/t0056-git-C.sh | 6 ++++++\n 4 files changed, 50 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 74973d3cc4..e254572fec 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -73,6 +73,10 @@ example the following invocations are equivalent:\n git --git-dir=a.git --work-tree=b -C c status\n git --git-dir=c/a.git --work-tree=c/b status\n\n+--cdup::\n+\tRun as if git was started at the root of the repository or\n+\tworktree. Same as `-C $(git rev-parse --show-cdup)`.\n+\n -c =::\n \tPass a configuration parameter to the command. The value\n \tgiven will override values from configuration files.\ndiff --git a/archive.c b/archive.c\nindex c7e9f58b02..6f587876e5 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -396,6 +396,7 @@ static const struct archiver *lookup_archiver(const char *name)\n struct path_exists_context {\n \tstruct pathspec pathspec;\n \tstruct archiver_args *args;\n+\tint dotdot;\n };\n\n static int reject_entry(const struct object_id *oid UNUSED,\n@@ -405,35 +406,43 @@ static int reject_entry(const struct object_id *oid UNUSED,\n {\n \tint ret = -1;\n \tstruct path_exists_context *ctx = context;\n+\tconst char *prefix = ctx->args->prefix;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf scratch = STRBUF_INIT;\n\n+\tstrbuf_addbuf(&sb, base);\n+\tstrbuf_addstr(&sb, filename);\n \tif (S_ISDIR(mode)) {\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tstrbuf_addbuf(&sb, base);\n-\t\tstrbuf_addstr(&sb, filename);\n \t\tif (!match_pathspec(ctx->args->repo->index,\n \t\t\t\t &ctx->pathspec,\n \t\t\t\t sb.buf, sb.len, 0, NULL, 1))\n \t\t\tret = READ_TREE_RECURSIVE;\n-\t\tstrbuf_release(&sb);\n+\t} else {\n+\t\tif (starts_with(relative_path(sb.buf, prefix, &scratch), \"../\"))\n+\t\t\tctx->dotdot = 1;\n \t}\n+\tstrbuf_release(&sb);\n+\tstrbuf_release(&scratch);\n \treturn ret;\n }\n\n-static int path_exists(struct archiver_args *args, const char *prefix,\n+static void check_path(struct archiver_args *args, const char *prefix,\n \t\t const char *path)\n {\n \tconst char *paths[] = { path, NULL };\n \tstruct path_exists_context ctx;\n-\tint ret;\n\n \tctx.args = args;\n+\tctx.dotdot = 0;\n \tparse_pathspec(&ctx.pathspec, 0, 0, prefix, paths);\n \tctx.pathspec.recursive = 1;\n-\tret = read_tree(args->repo, args->tree,\n-\t\t\t&ctx.pathspec,\n-\t\t\treject_entry, &ctx);\n+\tif (!read_tree(args->repo, args->tree, &ctx.pathspec,\n+\t\t reject_entry, &ctx))\n+\t\tdie(_(\"pathspec '%s' did not match any files\"), path);\n+\tif (ctx.dotdot)\n+\t\tdie(_(\"pathspec '%s' matches files above current directory\"),\n+\t\t path);\n \tclear_pathspec(&ctx.pathspec);\n-\treturn ret != 0;\n }\n\n static void parse_pathspec_arg(const char **pathspec, const char *prefix,\n@@ -445,7 +454,7 @@ static void parse_pathspec_arg(const char **pathspec, const char *prefix,\n \t\tpathspec = match_all;\n\n \t/*\n-\t * must be consistent with parse_pathspec in path_exists()\n+\t * must be consistent with parse_pathspec in check_path()\n \t * Also if pathspec patterns are dependent, we're in big\n \t * trouble as we test each one separately\n \t */\n@@ -455,9 +464,8 @@ static void parse_pathspec_arg(const char **pathspec, const char *prefix,\n \tar_args->pathspec.recursive = 1;\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n-\t\t\tif (**pathspec &&\n-\t\t\t !path_exists(ar_args, prefix, *pathspec))\n-\t\t\t\tdie(_(\"pathspec '%s' did not match any files\"), *pathspec);\n+\t\t\tif (**pathspec)\n+\t\t\t\tcheck_path(ar_args, prefix, *pathspec);\n \t\t\tpathspec++;\n \t\t}\n \t}\ndiff --git a/git.c b/git.c\nindex 6171fd6769..22993991a9 100644\n--- a/git.c\n+++ b/git.c\n@@ -293,6 +293,24 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t}\n \t\t\t(*argv)++;\n \t\t\t(*argc)--;\n+\t\t} else if (!strcmp(cmd, \"--cdup\")) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tstrvec_pushl(&cp.args, \"rev-parse\", \"--show-cdup\", NULL);\n+\t\t\tif (pipe_command(&cp, NULL, 0, &out, 0, &err, 0)) {\n+\t\t\t\tstrbuf_trim_trailing_newline(&err);\n+\t\t\t\tif (err.len)\n+\t\t\t\t\tdie(\"%s\", err.buf);\n+\t\t\t\tdie(_(\"unable to get repo root or worktree\"));\n+\t\t\t}\n+\t\t\tstrbuf_trim_trailing_newline(&out);\n+\t\t\tif (out.len && chdir(out.buf))\n+\t\t\t\tdie_errno(_(\"cannot change to '%s'\"), out.buf);\n+\t\t\tstrbuf_release(&out);\n+\t\t\tstrbuf_release(&err);\n \t\t} else if (skip_prefix(cmd, \"--list-cmds=\", &cmd)) {\n \t\t\ttrace2_cmd_name(\"_query_\");\n \t\t\tif (!strcmp(cmd, \"parseopt\")) {\ndiff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh\nindex 752aa8c945..95107d724e 100755\n--- a/t/t0056-git-C.sh\n+++ b/t/t0056-git-C.sh\n@@ -92,4 +92,10 @@ test_expect_success 'Relative followed by fullpath: \"-C ./here -C /there\" is equ\n \ttest_cmp expected actual\n '\n\n+test_expect_success '\"--cdup\" goes back' '\n+\tgit status >expected &&\n+\tgit -C c --cdup status >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n"},{"id":"473442","messageId":"xmqqjzzly84q.fsf@gitster.g","threadId":"59334","inReplyTo":"f7949f1b-4bad-e1bf-4754-f8b79e3ce279@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-12T21:25:41Z","receivedAt":"2023-03-12T21:25:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I offhand do not know how well it would mix with --strip-components\n>> if we leave the leading \"../\".\n>\n> Not too well when entries from $PWD are shortened and mixed with ones\n> from elsewhere that aren't. But that seems like a strange thing to do.\n\nYes, it is a strange thing to do.\n\n> If two sub-trees are needed then git archive should be started in a\n> shared parent directory higher up.\n>\n>> But it certainly would be nice if we somehow:\n>>\n>> * can keep the current behaviour where \"git -C sub archive\" records\n>> paths relative to \"sub\" for backward compatibility.\n>\n> Right. That's what relative_path() provides in the patch.\n>\n>> * fail loudly when \"git -C sub archive \" makes us use\n>> \"../\" prefix because goes above the $PWD for backward\n>> compatibility and sanity.\n>\n> Without the patch this fails, but are there really people that depend on\n> it failing? We could certainly forbid it, but do we need to?\n\nI dunno. It was an obvious way to avoid having to think about\ninteraction with --strip-components and \"../\", but there certainly\nmay be other solutions for it people can think of. \n\nAlso on the receiving end, don't people get upset to see that their\n\"tar xf\" escapes the directory they just created only to extract the\ntarball?\n\n>> * with --some-option, make \"git -C sub archive --some-option :/\"\n>> act exactly like \"git archive :/\".\n>\n> Perhaps I'm reading this too literally, but it would be easier to remove\n> \"-C sub\" from that command. Or to add \"-C $(git rev-parse --show-cdup)\".\n> We could add a shortcut for that (see patch below).\n\nMore like\n\n\t$ cd some/deep/place\n\t... work work work\n\t$ git archive --full-tree :/other :/hier :/archy\n\nis what I had in mind. Without --full-tree, due to the first bullet\npoint above, paths in our archive are relative to some/deep/place.\n\nThanks.\n"},{"id":"473695","messageId":"9e215e5c-0b67-0362-fd53-8c22b8d348ff@web.de","threadId":"59334","inReplyTo":"xmqqjzzly84q.fsf@gitster.g","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-18T21:30:24Z","receivedAt":"2023-03-18T21:30:43Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.03.23 um 22:25 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>> * fail loudly when \"git -C sub archive \" makes us use\n>>> \"../\" prefix because goes above the $PWD for backward\n>>> compatibility and sanity.\n>>\n>> Without the patch this fails, but are there really people that depend on\n>> it failing? We could certainly forbid it, but do we need to?\n>\n> I dunno. It was an obvious way to avoid having to think about\n> interaction with --strip-components and \"../\", but there certainly\n> may be other solutions for it people can think of.\n>\n> Also on the receiving end, don't people get upset to see that their\n> \"tar xf\" escapes the directory they just created only to extract the\n> tarball?\n\nbsdtar creates entries starting with \"../\" without complaint, but\nrefuses to extract them as-is -- you need to e.g. use --strip-components\nto get rid of those dots. Awkward.\n\nNot allowing those entries to be created is more consistent, especially\nsince we're already restrictive like that. OK.\n\n>>> * with --some-option, make \"git -C sub archive --some-option :/\"\n>>> act exactly like \"git archive :/\".\n>>\n>> Perhaps I'm reading this too literally, but it would be easier to remove\n>> \"-C sub\" from that command. Or to add \"-C $(git rev-parse --show-cdup)\".\n>> We could add a shortcut for that (see patch below).\n>\n> More like\n>\n> \t$ cd some/deep/place\n> \t... work work work\n> \t$ git archive --full-tree :/other :/hier :/archy\n>\n> is what I had in mind. Without --full-tree, due to the first bullet\n> point above, paths in our archive are relative to some/deep/place.\n\nI don't see the difference. Here \":/other\" is the youngest commit with\n\"other\" in its message, \":/hier\" and \":/archy\" are pathspecs selecting\nsubdirectories. If we stay in a different subdirectory then this is\nsimply currently forbidden and we'd keep it like that:\n\n $ cd xdiff\n $ git archive :/needle :/t/t0019 :/t/t4020 | tar tf -\n fatal: pathspec ':/t/t0019' did not match any files\n\nWe'd just improve the message a bit. Going up still helps:\n\n $ git -C \"$(git rev-parse --show-cdup)\" archive :/needle :/t/t0019 :/t/t4020 | tar tf -\n t/\n t/t0019/\n t/t0019/parse_json.perl\n t/t4020/\n t/t4020/diff.NUL\n\n # We could shorten that.\n $ ../git --cdup archive :/needle :/t/t0019 :/t/t4020 | tar tf -\n t/\n t/t0019/\n t/t0019/parse_json.perl\n t/t4020/\n t/t4020/diff.NUL\n\nOr am I missing something here?\n\nRené\n"},{"id":"473780","messageId":"xmqqlejro0t6.fsf@gitster.g","threadId":"59334","inReplyTo":"9e215e5c-0b67-0362-fd53-8c22b8d348ff@web.de","subject":"Re: Bug in git archive + .gitattributes + relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-20T16:16:37Z","receivedAt":"2023-03-20T16:23:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> \t$ cd some/deep/place\n>> \t... work work work\n>> \t$ git archive --full-tree :/other :/hier :/archy\n>>\n>> is what I had in mind. Without --full-tree, due to the first bullet\n>> point above, paths in our archive are relative to some/deep/place.\n>\n> I don't see the difference.\n\nOK. I do not work in Java anyway ;-)\n"},{"id":"473809","messageId":"e923e844-6891-76dc-e7db-4771b2d91792@web.de","threadId":"59334","inReplyTo":"9e215e5c-0b67-0362-fd53-8c22b8d348ff@web.de","subject":"[PATCH] archive: improve support for running in a subdirectory","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-20T20:02:53Z","receivedAt":"2023-03-20T20:03:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When git archive is started in a subdirectory, it archives its\ncorresponding tree and its child objects, only. That is intended. It\ndoes that by effectively cd'ing into that tree and setting \"prefix\" to\nthe empty string.\n\nThis has unfortunate consequences, though: Attributes are anchored at\nthe root of the repository and git archive still applies them to\nsubtrees, causing mismatches. And when checking pathspecs it cannot\ntell the difference between one that doesn't match anything and one that\nmatches something outside of the subdirectory, leading to a confusing\nerror message.\n\nFix that by keeping the \"prefix\" value and passing it to functions\nrelated to pathspecs and attributes, and shortening the paths written to\nthe archive and (if --verbose is given) to stdout using relative_path().\n\nStill reject attempts to archive files outside the current directory,\nbut print a more specific error in that case. Recognizing it requires a\nfull traversal of the subtree for each pathspec, however. Allowing them\nwould be easier, but archive entry paths starting with \"../\" can be\nproblematic to extract -- e.g. bsdtar skips them by default.\n\nReported-by: Cristian Le \nReported-by: Matthias Görgens \nSigned-off-by: René Scharfe \n---\n archive.c | 71 +++++++++++++++++++++++++++++------------\n t/t5000-tar-tree.sh | 13 ++++++++\n t/t5001-archive-attr.sh | 16 ++++++++++\n 3 files changed, 79 insertions(+), 21 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 1c2ca78e52..c8d66169d1 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -168,6 +168,25 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\targs->convert = check_attr_export_subst(check);\n \t}\n\n+\tif (args->prefix) {\n+\t\tstatic struct strbuf buf = STRBUF_INIT;\n+\t\tconst char *rel;\n+\n+\t\trel = relative_path(path_without_prefix, args->prefix, &buf);\n+\n+\t\t/*\n+\t\t * We don't add an entry for the current working\n+\t\t * directory when we are at the root; skip it also when\n+\t\t * we're in a subdirectory or submodule. Skip entries\n+\t\t * higher up as well.\n+\t\t */\n+\t\tif (!strcmp(rel, \"./\") || starts_with(rel, \"../\"))\n+\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n+\n+\t\tstrbuf_setlen(&path, args->baselen);\n+\t\tstrbuf_addstr(&path, rel);\n+\t}\n+\n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\n@@ -403,6 +422,27 @@ static int reject_entry(const struct object_id *oid UNUSED,\n \treturn ret;\n }\n\n+static int reject_outside(const struct object_id *oid UNUSED,\n+\t\t\t struct strbuf *base, const char *filename,\n+\t\t\t unsigned mode, void *context)\n+{\n+\tstruct archiver_args *args = context;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tif (S_ISDIR(mode))\n+\t\treturn READ_TREE_RECURSIVE;\n+\n+\tstrbuf_addbuf(&path, base);\n+\tstrbuf_addstr(&path, filename);\n+\tif (starts_with(relative_path(path.buf, args->prefix, &buf), \"../\"))\n+\t\tret = -1;\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&path);\n+\treturn ret;\n+}\n+\n static int path_exists(struct archiver_args *args, const char *path)\n {\n \tconst char *paths[] = { path, NULL };\n@@ -410,8 +450,13 @@ static int path_exists(struct archiver_args *args, const char *path)\n \tint ret;\n\n \tctx.args = args;\n-\tparse_pathspec(&ctx.pathspec, 0, 0, \"\", paths);\n+\tparse_pathspec(&ctx.pathspec, 0, PATHSPEC_PREFER_CWD,\n+\t\t args->prefix, paths);\n \tctx.pathspec.recursive = 1;\n+\tif (args->prefix && read_tree(args->repo, args->tree, &ctx.pathspec,\n+\t\t\t\t reject_outside, args))\n+\t\tdie(_(\"pathspec '%s' matches files outside the \"\n+\t\t \"current directory\"), path);\n \tret = read_tree(args->repo, args->tree,\n \t\t\t&ctx.pathspec,\n \t\t\treject_entry, &ctx);\n@@ -427,9 +472,8 @@ static void parse_pathspec_arg(const char **pathspec,\n \t * Also if pathspec patterns are dependent, we're in big\n \t * trouble as we test each one separately\n \t */\n-\tparse_pathspec(&ar_args->pathspec, 0,\n-\t\t PATHSPEC_PREFER_FULL,\n-\t\t \"\", pathspec);\n+\tparse_pathspec(&ar_args->pathspec, 0, PATHSPEC_PREFER_CWD,\n+\t\t ar_args->prefix, pathspec);\n \tar_args->pathspec.recursive = 1;\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n@@ -441,8 +485,7 @@ static void parse_pathspec_arg(const char **pathspec,\n }\n\n static void parse_treeish_arg(const char **argv,\n-\t\tstruct archiver_args *ar_args, const char *prefix,\n-\t\tint remote)\n+\t\t\t struct archiver_args *ar_args, int remote)\n {\n \tconst char *name = argv[0];\n \tconst struct object_id *commit_oid;\n@@ -481,20 +524,6 @@ static void parse_treeish_arg(const char **argv,\n \tif (!tree)\n \t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n\n-\tif (prefix) {\n-\t\tstruct object_id tree_oid;\n-\t\tunsigned short mode;\n-\t\tint err;\n-\n-\t\terr = get_tree_entry(ar_args->repo,\n-\t\t\t\t &tree->object.oid,\n-\t\t\t\t prefix, &tree_oid,\n-\t\t\t\t &mode);\n-\t\tif (err || !S_ISDIR(mode))\n-\t\t\tdie(_(\"current working directory is untracked\"));\n-\n-\t\ttree = parse_tree_indirect(&tree_oid);\n-\t}\n \tar_args->refname = ref;\n \tar_args->tree = tree;\n \tar_args->commit_oid = commit_oid;\n@@ -712,7 +741,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \t\tsetup_git_directory();\n \t}\n\n-\tparse_treeish_arg(argv, &args, prefix, remote);\n+\tparse_treeish_arg(argv, &args, remote);\n \tparse_pathspec_arg(argv + 1, &args);\n\n \trc = ar->write_archive(ar, &args);\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 918a2fc7c6..a60ae6145e 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -433,6 +433,19 @@ test_expect_success 'catch non-matching pathspec' '\n \ttest_must_fail git archive -v HEAD -- \"*.abc\" >/dev/null\n '\n\n+test_expect_success 'reject paths outside the current directory' '\n+\ttest_must_fail git -C a/bin archive HEAD .. >/dev/null 2>err &&\n+\tgrep \"outside the current directory\" err\n+'\n+\n+test_expect_success 'allow pathspecs that resolve to the current directory' '\n+\tgit -C a/bin archive -v HEAD ../bin >/dev/null 2>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tsh\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Pull the size and date of each entry in a tarfile using the system tar.\n #\n # We'll pull out only the year from the date; that avoids any question of\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex 04d300eeda..0ff47a239d 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -33,6 +33,13 @@ test_expect_success 'setup' '\n \techo ignored-by-tree.d export-ignore >>.gitattributes &&\n \tgit add ignored-by-tree ignored-by-tree.d .gitattributes &&\n\n+\tmkdir subdir &&\n+\t>subdir/included &&\n+\t>subdir/ignored-by-subtree &&\n+\t>subdir/ignored-by-tree &&\n+\techo ignored-by-subtree export-ignore >subdir/.gitattributes &&\n+\tgit add subdir &&\n+\n \techo ignored by worktree >ignored-by-worktree &&\n \techo ignored-by-worktree export-ignore >.gitattributes &&\n \tgit add ignored-by-worktree &&\n@@ -93,6 +100,15 @@ test_expect_exists\tarchive-pathspec-wildcard/ignored-by-worktree\n test_expect_missing\tarchive-pathspec-wildcard/excluded-by-pathspec.d\n test_expect_missing\tarchive-pathspec-wildcard/excluded-by-pathspec.d/file\n\n+test_expect_success 'git -C subdir archive' '\n+\tgit -C subdir archive HEAD >archive-subdir.tar &&\n+\textract_tar_to_dir archive-subdir\n+'\n+\n+test_expect_exists\tarchive-subdir/included\n+test_expect_missing\tarchive-subdir/ignored-by-subtree\n+test_expect_missing\tarchive-subdir/ignored-by-tree\n+\n test_expect_success 'git archive with worktree attributes' '\n \tgit archive --worktree-attributes HEAD >worktree.tar &&\n \t(mkdir worktree && cd worktree && \"$TAR\" xf -) writes:\n\n> archive.c | 71 +++++++++++++++++++++++++++++------------\n> t/t5000-tar-tree.sh | 13 ++++++++\n> t/t5001-archive-attr.sh | 16 ++++++++++\n> 3 files changed, 79 insertions(+), 21 deletions(-)\n\nThere are a handful of CI failures that can be seen at\n\n https://github.com/git/git/actions/runs/4482588035/jobs/7880821225#step:6:1803\n https://github.com/git/git/actions/runs/4482588035/jobs/7880821849#step:4:1811\n\nwhich is with this topic in 'seen'. Exactly the same 'seen' without\nthis topic seems to pass\n\n https://github.com/git/git/actions/runs/4484290871\n\nThanks.\n"},{"id":"474137","messageId":"2be99e1f-bff6-cd24-dbe8-8948e2d630c7@web.de","threadId":"59334","inReplyTo":"xmqqlejpg17g.fsf@gitster.g","subject":"Re: [PATCH] archive: improve support for running in a subdirectory","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-24T22:26:32Z","receivedAt":"2023-03-24T22:26:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.03.23 um 23:59 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> archive.c | 71 +++++++++++++++++++++++++++++------------\n>> t/t5000-tar-tree.sh | 13 ++++++++\n>> t/t5001-archive-attr.sh | 16 ++++++++++\n>> 3 files changed, 79 insertions(+), 21 deletions(-)\n>\n> There are a handful of CI failures that can be seen at\n>\n> https://github.com/git/git/actions/runs/4482588035/jobs/7880821225#step:6:1803\n> https://github.com/git/git/actions/runs/4482588035/jobs/7880821849#step:4:1811\n>\n> which is with this topic in 'seen'. Exactly the same 'seen' without\n> this topic seems to pass\n>\n> https://github.com/git/git/actions/runs/4484290871\n\nUh, nasty. The linux-asan run reveals a use of the return value of\nrelative_path() after manipulating the underlying buffer. I hope\nfixing that helps linux-musl as well.\n\nRené\n\n\n"},{"id":"474138","messageId":"7c33b01b-7b2a-25fa-9a66-1e65cd12bc84@web.de","threadId":"59334","inReplyTo":"e923e844-6891-76dc-e7db-4771b2d91792@web.de","subject":"[PATCH v2] archive: improve support for running in subdirectory","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-24T22:27:11Z","receivedAt":"2023-03-24T22:27:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When git archive is started in a subdirectory, it archives its\ncorresponding tree and its child objects, only. That is intended. It\ndoes that by effectively cd'ing into that tree and setting \"prefix\" to\nthe empty string.\n\nThis has unfortunate consequences, though: Attributes are anchored at\nthe root of the repository and git archive still applies them to\nsubtrees, causing mismatches. And when checking pathspecs it cannot\ntell the difference between one that doesn't match anthing or one that\nmatches some actual blob outside of the subdirectory, leading to a\nconfusing error message.\n\nFix that by keeping the \"prefix\" value and passing it to pathspec and\nattribute functions, and shortening it using relative_path() for paths\nwritten to the archive and (if --verbose is given) to stdout.\n\nStill reject attempts to archive files outside the current directory,\nbut print a more specific error in that case. Recognizing it requires a\nfull traversal of the subtree for each pathspec, however. Allowing them\nwould be easier, but archive entry paths starting with \"../\" can be\nproblematic to extract -- e.g. bsdtar skips them by default.\n\nReported-by: Cristian Le \nReported-by: Matthias Görgens \nSigned-off-by: René Scharfe \n---\nInterdiff:\n diff --git a/archive.c b/archive.c\n index c8d66169d1..a61615d180 100644\n --- a/archive.c\n +++ b/archive.c\n @@ -169,6 +169,7 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t}\n\n \tif (args->prefix) {\n +\t\tstatic struct strbuf new_path = STRBUF_INIT;\n \t\tstatic struct strbuf buf = STRBUF_INIT;\n \t\tconst char *rel;\n\n @@ -183,8 +184,11 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\tif (!strcmp(rel, \"./\") || starts_with(rel, \"../\"))\n \t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n\n -\t\tstrbuf_setlen(&path, args->baselen);\n -\t\tstrbuf_addstr(&path, rel);\n +\t\t/* rel can refer to path, so don't edit it in place */\n +\t\tstrbuf_reset(&new_path);\n +\t\tstrbuf_add(&new_path, args->base, args->baselen);\n +\t\tstrbuf_addstr(&new_path, rel);\n +\t\tstrbuf_swap(&path, &new_path);\n \t}\n\n \tif (args->verbose)\n\n archive.c | 75 +++++++++++++++++++++++++++++------------\n t/t5000-tar-tree.sh | 13 +++++++\n t/t5001-archive-attr.sh | 16 +++++++++\n 3 files changed, 83 insertions(+), 21 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 1c2ca78e52..a61615d180 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -168,6 +168,29 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\targs->convert = check_attr_export_subst(check);\n \t}\n\n+\tif (args->prefix) {\n+\t\tstatic struct strbuf new_path = STRBUF_INIT;\n+\t\tstatic struct strbuf buf = STRBUF_INIT;\n+\t\tconst char *rel;\n+\n+\t\trel = relative_path(path_without_prefix, args->prefix, &buf);\n+\n+\t\t/*\n+\t\t * We don't add an entry for the current working\n+\t\t * directory when we are at the root; skip it also when\n+\t\t * we're in a subdirectory or submodule. Skip entries\n+\t\t * higher up as well.\n+\t\t */\n+\t\tif (!strcmp(rel, \"./\") || starts_with(rel, \"../\"))\n+\t\t\treturn S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;\n+\n+\t\t/* rel can refer to path, so don't edit it in place */\n+\t\tstrbuf_reset(&new_path);\n+\t\tstrbuf_add(&new_path, args->base, args->baselen);\n+\t\tstrbuf_addstr(&new_path, rel);\n+\t\tstrbuf_swap(&path, &new_path);\n+\t}\n+\n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\n@@ -403,6 +426,27 @@ static int reject_entry(const struct object_id *oid UNUSED,\n \treturn ret;\n }\n\n+static int reject_outside(const struct object_id *oid UNUSED,\n+\t\t\t struct strbuf *base, const char *filename,\n+\t\t\t unsigned mode, void *context)\n+{\n+\tstruct archiver_args *args = context;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tif (S_ISDIR(mode))\n+\t\treturn READ_TREE_RECURSIVE;\n+\n+\tstrbuf_addbuf(&path, base);\n+\tstrbuf_addstr(&path, filename);\n+\tif (starts_with(relative_path(path.buf, args->prefix, &buf), \"../\"))\n+\t\tret = -1;\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&path);\n+\treturn ret;\n+}\n+\n static int path_exists(struct archiver_args *args, const char *path)\n {\n \tconst char *paths[] = { path, NULL };\n@@ -410,8 +454,13 @@ static int path_exists(struct archiver_args *args, const char *path)\n \tint ret;\n\n \tctx.args = args;\n-\tparse_pathspec(&ctx.pathspec, 0, 0, \"\", paths);\n+\tparse_pathspec(&ctx.pathspec, 0, PATHSPEC_PREFER_CWD,\n+\t\t args->prefix, paths);\n \tctx.pathspec.recursive = 1;\n+\tif (args->prefix && read_tree(args->repo, args->tree, &ctx.pathspec,\n+\t\t\t\t reject_outside, args))\n+\t\tdie(_(\"pathspec '%s' matches files outside the \"\n+\t\t \"current directory\"), path);\n \tret = read_tree(args->repo, args->tree,\n \t\t\t&ctx.pathspec,\n \t\t\treject_entry, &ctx);\n@@ -427,9 +476,8 @@ static void parse_pathspec_arg(const char **pathspec,\n \t * Also if pathspec patterns are dependent, we're in big\n \t * trouble as we test each one separately\n \t */\n-\tparse_pathspec(&ar_args->pathspec, 0,\n-\t\t PATHSPEC_PREFER_FULL,\n-\t\t \"\", pathspec);\n+\tparse_pathspec(&ar_args->pathspec, 0, PATHSPEC_PREFER_CWD,\n+\t\t ar_args->prefix, pathspec);\n \tar_args->pathspec.recursive = 1;\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n@@ -441,8 +489,7 @@ static void parse_pathspec_arg(const char **pathspec,\n }\n\n static void parse_treeish_arg(const char **argv,\n-\t\tstruct archiver_args *ar_args, const char *prefix,\n-\t\tint remote)\n+\t\t\t struct archiver_args *ar_args, int remote)\n {\n \tconst char *name = argv[0];\n \tconst struct object_id *commit_oid;\n@@ -481,20 +528,6 @@ static void parse_treeish_arg(const char **argv,\n \tif (!tree)\n \t\tdie(_(\"not a tree object: %s\"), oid_to_hex(&oid));\n\n-\tif (prefix) {\n-\t\tstruct object_id tree_oid;\n-\t\tunsigned short mode;\n-\t\tint err;\n-\n-\t\terr = get_tree_entry(ar_args->repo,\n-\t\t\t\t &tree->object.oid,\n-\t\t\t\t prefix, &tree_oid,\n-\t\t\t\t &mode);\n-\t\tif (err || !S_ISDIR(mode))\n-\t\t\tdie(_(\"current working directory is untracked\"));\n-\n-\t\ttree = parse_tree_indirect(&tree_oid);\n-\t}\n \tar_args->refname = ref;\n \tar_args->tree = tree;\n \tar_args->commit_oid = commit_oid;\n@@ -712,7 +745,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \t\tsetup_git_directory();\n \t}\n\n-\tparse_treeish_arg(argv, &args, prefix, remote);\n+\tparse_treeish_arg(argv, &args, remote);\n \tparse_pathspec_arg(argv + 1, &args);\n\n \trc = ar->write_archive(ar, &args);\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 918a2fc7c6..a60ae6145e 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -433,6 +433,19 @@ test_expect_success 'catch non-matching pathspec' '\n \ttest_must_fail git archive -v HEAD -- \"*.abc\" >/dev/null\n '\n\n+test_expect_success 'reject paths outside the current directory' '\n+\ttest_must_fail git -C a/bin archive HEAD .. >/dev/null 2>err &&\n+\tgrep \"outside the current directory\" err\n+'\n+\n+test_expect_success 'allow pathspecs that resolve to the current directory' '\n+\tgit -C a/bin archive -v HEAD ../bin >/dev/null 2>actual &&\n+\tcat >expect <<-\\EOF &&\n+\tsh\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Pull the size and date of each entry in a tarfile using the system tar.\n #\n # We'll pull out only the year from the date; that avoids any question of\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex 04d300eeda..0ff47a239d 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -33,6 +33,13 @@ test_expect_success 'setup' '\n \techo ignored-by-tree.d export-ignore >>.gitattributes &&\n \tgit add ignored-by-tree ignored-by-tree.d .gitattributes &&\n\n+\tmkdir subdir &&\n+\t>subdir/included &&\n+\t>subdir/ignored-by-subtree &&\n+\t>subdir/ignored-by-tree &&\n+\techo ignored-by-subtree export-ignore >subdir/.gitattributes &&\n+\tgit add subdir &&\n+\n \techo ignored by worktree >ignored-by-worktree &&\n \techo ignored-by-worktree export-ignore >.gitattributes &&\n \tgit add ignored-by-worktree &&\n@@ -93,6 +100,15 @@ test_expect_exists\tarchive-pathspec-wildcard/ignored-by-worktree\n test_expect_missing\tarchive-pathspec-wildcard/excluded-by-pathspec.d\n test_expect_missing\tarchive-pathspec-wildcard/excluded-by-pathspec.d/file\n\n+test_expect_success 'git -C subdir archive' '\n+\tgit -C subdir archive HEAD >archive-subdir.tar &&\n+\textract_tar_to_dir archive-subdir\n+'\n+\n+test_expect_exists\tarchive-subdir/included\n+test_expect_missing\tarchive-subdir/ignored-by-subtree\n+test_expect_missing\tarchive-subdir/ignored-by-tree\n+\n test_expect_success 'git archive with worktree attributes' '\n \tgit archive --worktree-attributes HEAD >worktree.tar &&\n \t(mkdir worktree && cd worktree && \"$TAR\" xf -) writes:\n\n> +\t\t/* rel can refer to path, so don't edit it in place */\n> +\t\tstrbuf_reset(&new_path);\n> +\t\tstrbuf_add(&new_path, args->base, args->baselen);\n> +\t\tstrbuf_addstr(&new_path, rel);\n> +\t\tstrbuf_swap(&path, &new_path);\n\nThanks. That indeed was nasty and tricky.\n\n"}]}