{"thread":{"id":"56451","subject":"[PATCH 2/3] Die if filter is attempted without a worktree","startedAt":"2021-09-06T18:10:36Z","lastAt":"2021-09-07T20:28:07Z","messageCount":8,"participants":["Calum McConnell","Ævar Arnfjörð Bjarmason","Bagas Sanjaya","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"434797","messageId":"20210906181002.625647-2-calumlikesapplepie@gmail.com","threadId":"56451","inReplyTo":"20210906181002.625647-1-calumlikesapplepie@gmail.com","subject":"[PATCH 2/3] Die if filter is attempted without a worktree","fromName":"Calum McConnell","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2021-09-06T18:10:01Z","receivedAt":"2021-09-06T18:10:36Z","isPatch":true,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"As far as I know, this isn't possible.  Rather than add a bunch of\ncode to workarround something that might not be possible, lets just\nhalt and catch fire if it does.  This might need to be removed before\nthe change goes into master\n\nSigned-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n---\n convert.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex 5d64ccce57..df70c250b0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -646,6 +646,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tsq_quote_buf(&worktreePath, the_repository->worktree);\n \tdict[1].value = worktreePath.buf;\n \n+\t/* The results of a nonexistent worktree could be... weird.  Lets avoid*/\n+\tif(dict[1].value == NULL){\n+\t\tBUG(\"There is no worktree for this worktree substitution\");\n+\t}\n+\n \t/* expand all %f or %w with the quoted path */\n \tstrbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);\n \tstrbuf_release(&filePath);\n-- \n2.30.2\n\n"},{"id":"434798","messageId":"20210906181002.625647-1-calumlikesapplepie@gmail.com","threadId":"56451","inReplyTo":null,"subject":"[PATCH 1/3] Add support for new %w wildcard in checkout filter","fromName":"Calum McConnell","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2021-09-06T18:10:00Z","receivedAt":"2021-09-06T18:10:36Z","isPatch":true,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"When building content filters with gitattributes, for instance to ensure\ngit stores the plain-text rather than the binary form of data for certain\nformats, it is often advantageous to separate the filters into separate,\npotentially complex scripts.  However, as the $PWD where content filters\nare executed is unspecified the path to scripts needs to be specified as\nan absolute path.  That means that the guide for setting up a repository\nwhich uses scripts to filter content cannot simply consist of \"include\nthe following lines in your .git/config file\", and it means that the\notherwise safe operation of moving a git repository from one folder to\nanother is decidedly unsafe.\n\nThis %w (short for 'work tree') will allow such scripts to exist and\nbe executed on each checkout, without needing to be added to the PATH\nor be dependent upon the $PWD of the checkout call.\n\nSigned-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n---\n convert.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d6fb3410a..5d64ccce57 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -9,6 +9,7 @@\n #include \"sub-process.h\"\n #include \"utf8.h\"\n #include \"ll-merge.h\"\n+#include \"repository.h\"\n \n /*\n  * convert.c - convert a file when checking it out and checking it in.\n@@ -630,19 +631,26 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \n \t/* apply % substitution to cmd */\n \tstruct strbuf cmd = STRBUF_INIT;\n-\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf filePath = STRBUF_INIT;\n+\tstruct strbuf worktreePath = STRBUF_INIT;\n \tstruct strbuf_expand_dict_entry dict[] = {\n \t\t{ \"f\", NULL, },\n+\t\t{ \"w\", NULL, }, \n \t\t{ NULL, NULL, },\n \t};\n \n-\t/* quote the path to preserve spaces, etc. */\n-\tsq_quote_buf(&path, params->path);\n-\tdict[0].value = path.buf;\n+\t/* quote the paths to preserve spaces, etc. */\n+\tsq_quote_buf(&filePath, params->path);\n+\tdict[0].value = filePath.buf;\n+\t\n+\tsq_quote_buf(&worktreePath, the_repository->worktree);\n+\tdict[1].value = worktreePath.buf;\n \n-\t/* expand all %f with the quoted path */\n+\t/* expand all %f or %w with the quoted path */\n \tstrbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);\n-\tstrbuf_release(&path);\n+\tstrbuf_release(&filePath);\n+  \tstrbuf_release(&worktreePath);\n+\n \n \tstrvec_push(&child_process.args, cmd.buf);\n \tchild_process.use_shell = 1;\n-- \n2.30.2\n\n"},{"id":"434799","messageId":"20210906181002.625647-3-calumlikesapplepie@gmail.com","threadId":"56451","inReplyTo":"20210906181002.625647-1-calumlikesapplepie@gmail.com","subject":"[PATCH 3/3] Document the new gitattributes change","fromName":"Calum McConnell","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2021-09-06T18:10:02Z","receivedAt":"2021-09-06T18:10:36Z","isPatch":true,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"The warning I included might make this feature hard-to-use: but\npeople are clever, and there are ways around it, which also\ngrant other useful properties to smudge- and clean- filters.\n\nFor instance, if you add at the beginning of each file you filter,\nyou add GITTATTRIBUTES_FILTERED_VERSION_1, that grants the \"no\ndouble clean\" property and allows for versioning of the script.\n\nTo do this perfectly, you'd want to let the user declare the\nscript file, and then have git figure out what version of it\nto run.  But that would take an understanding of the conversion\ncode which I lack, and would also be a lot of a work for a\nlittle-used addition to an underused feature.\n\nSigned-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n---\n Documentation/gitattributes.txt | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 83fd4e19a4..33b0087d4f 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -486,6 +486,19 @@ not exist, or may have different contents. So, smudge and clean commands\n should not try to access the file on disk, but only act as filters on the\n content provided to them on standard input.\n \n+Sequence \"%w\" on the filter command line is replaced with the absolute path\n+to the root of the repository working tree.  This is useful if you have\n+complex scripts specific to your project.  Note that these scripts should\n+be mostly static for the life of the project, since the script version that \n+is run may significantly predate or follow the version of the file being\n+processed\n+\n+------------------------\n+[filter \"unzip-file\"]\n+\tclean = %w/scripts/filter-unzip-clean\n+\tsmudge = %w/scripts/filter-unzip-smudge\n+------------------------\n+\n Long Running Filter Process\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n-- \n2.30.2\n\n"},{"id":"434804","messageId":"87lf49nye4.fsf@evledraar.gmail.com","threadId":"56451","inReplyTo":"20210906181002.625647-2-calumlikesapplepie@gmail.com","subject":"Re: [PATCH 2/3] Die if filter is attempted without a worktree","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-06T22:09:11Z","receivedAt":"2021-09-06T22:16:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 06 2021, Calum McConnell wrote:\n\n> As far as I know, this isn't possible.  Rather than add a bunch of\n> code to workarround something that might not be possible, lets just\n> halt and catch fire if it does.  This might need to be removed before\n> the change goes into master\n>\n> Signed-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n> ---\n>  convert.c | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/convert.c b/convert.c\n> index 5d64ccce57..df70c250b0 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -646,6 +646,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n>  \tsq_quote_buf(&worktreePath, the_repository->worktree);\n>  \tdict[1].value = worktreePath.buf;\n>  \n> +\t/* The results of a nonexistent worktree could be... weird.  Lets avoid*/\n> +\tif(dict[1].value == NULL){\n> +\t\tBUG(\"There is no worktree for this worktree substitution\");\n> +\t}\n\nThis BUG() addition is itself buggy, elsewhere e.g. in builtin/gc.c you\ncan see where we have conditions like:\n\n    the_repository->worktree ? the_repository->worktree : the_repository->gitdir;\n\nI'm not bothering much with the greater context here, but if we suppose\nthat we have a case where worktreePath.buf is NULL, then\nthe_repository->worktree surely must have been NULL, and if you check\nwhat sq_quote_buf() does, you'll see:\n\n    void sq_quote_buf(struct strbuf *dst, const char *src)\n    [...]\n            while (*src) {\n\nI.e. we'd segfault anyway if that \"src\" were to be NULL.\n\nEven if that weren't the case then that's not the same as the\nworktreePath.buf being NULL, which even if we suppose sq_quote_buf()\nwon't segfault and just returned won't AFAICT ever be the case, see the\ncomment for strbuf_slopbuf in strbuf.c. So I think that even if you\nsomehow reached this with a NULL worktree that BUG() won't ever be\nreached.\n\nI think this can probably just be dropped, to the extent that we need\nsome check like this it seems like it should happen a lot earlier in\nconvert.c than here, i.e. during the early setup can't we detect & abort\nif we don't have a required worktree?\n\n"},{"id":"434819","messageId":"a20bd60e-7445-131c-e9f3-548bc8dcf1f4@gmail.com","threadId":"56451","inReplyTo":"20210906181002.625647-2-calumlikesapplepie@gmail.com","subject":"Re: [PATCH 2/3] Die if filter is attempted without a worktree","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-07T08:18:08Z","receivedAt":"2021-09-07T08:18:25Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 07/09/21 01.10, Calum McConnell wrote:\n> +\t/* The results of a nonexistent worktree could be... weird.  Lets avoid*/\n> +\tif(dict[1].value == NULL){\n> +\t\tBUG(\"There is no worktree for this worktree substitution\");\n> +\t}\n> +\n\nWhy don't simply print that error message without BUG() (aka using \ndie(_(\"message\"))? It can be l10n-ed if you using the approach.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"434885","messageId":"fd7b8c130d9220b3871f2aecfe80b8d342d6deb0.camel@gmail.com","threadId":"56451","inReplyTo":"87lf49nye4.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] Die if filter is attempted without a worktree","fromName":"Calum McConnell","fromEmail":"calumlikesapplepie@gmail.com","sentAt":"2021-09-07T14:56:54Z","receivedAt":"2021-09-07T14:56:59Z","isPatch":true,"sender":{"key":"calumlikesapplepie@gmail.com","avatar":null},"body":"On Tue, 2021-09-07 at 00:09 +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Sep 06 2021, Calum McConnell wrote:\n> \n> > As far as I know, this isn't possible.  Rather than add a bunch of\n> > code to workarround something that might not be possible, lets just\n> > halt and catch fire if it does.  This might need to be removed before\n> > the change goes into master\n> > \n> > Signed-off-by: Calum McConnell <calumlikesapplepie@gmail.com>\n> > ---\n> >  convert.c | 5 +++++\n> >  1 file changed, 5 insertions(+)\n> > \n> > diff --git a/convert.c b/convert.c\n> > index 5d64ccce57..df70c250b0 100644\n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -646,6 +646,11 @@ static int filter_buffer_or_fd(int in, int out,\n> > void *data)\n> >         sq_quote_buf(&worktreePath, the_repository->worktree);\n> >         dict[1].value = worktreePath.buf;\n> >  \n> > +       /* The results of a nonexistent worktree could be... weird. \n> > Lets avoid*/\n> > +       if(dict[1].value == NULL){\n> > +               BUG(\"There is no worktree for this worktree\n> > substitution\");\n> > +       }\n> \n> This BUG() addition is itself buggy, elsewhere e.g. in builtin/gc.c you\n> can see where we have conditions like:\n> \n>     the_repository->worktree ? the_repository->worktree :\n> the_repository->gitdir;\n> \n> I'm not bothering much with the greater context here, but if we suppose\n> that we have a case where worktreePath.buf is NULL, then\n> the_repository->worktree surely must have been NULL, and if you check\n> what sq_quote_buf() does, you'll see:\n> \n>     void sq_quote_buf(struct strbuf *dst, const char *src)\n>     [...]\n>             while (*src) {\n> \n> I.e. we'd segfault anyway if that \"src\" were to be NULL.\n> \n> Even if that weren't the case then that's not the same as the\n> worktreePath.buf being NULL, which even if we suppose sq_quote_buf()\n> won't segfault and just returned won't AFAICT ever be the case, see the\n> comment for strbuf_slopbuf in strbuf.c. So I think that even if you\n> somehow reached this with a NULL worktree that BUG() won't ever be\n> reached.\n> \n> I think this can probably just be dropped, to the extent that we need\n> some check like this it seems like it should happen a lot earlier in\n> convert.c than here, i.e. during the early setup can't we detect & abort\n> if we don't have a required worktree?\n\nPart of the reason I inserted this check was because I wasn't sure about\nthe ordering on a checkout into an empty directory (eg, would there\nactually be the filter script when it ran?).  On deeper thought, however,\nthat problem wouldn't even be solved by this.  Not to mention how you\nwould need to include the script on the initial commit, and such.\n\nSince the check doesn't even do what I thought it would do, I thought of a\nbetter approach: rather than having it expand to the working tree, it\nexpands to the git directory.  This means that you can place your scripts\nin a much better location: since .git/config must be modified anyways to\nuse a filter, it doesn't entail a loss of features.\n\nWhy I ever thought to use the worktree is beyond me. Patch V2 coming in a\nfew days.\n\nCalum McConnell\n\n"},{"id":"434894","messageId":"YTeUZEQayE9D08Es@coredump.intra.peff.net","threadId":"56451","inReplyTo":"20210906181002.625647-1-calumlikesapplepie@gmail.com","subject":"Re: [PATCH 1/3] Add support for new %w wildcard in checkout filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-07T16:33:40Z","receivedAt":"2021-09-07T16:33:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 06, 2021 at 02:10:00PM -0400, Calum McConnell wrote:\n\n> When building content filters with gitattributes, for instance to ensure\n> git stores the plain-text rather than the binary form of data for certain\n> formats, it is often advantageous to separate the filters into separate,\n> potentially complex scripts.  However, as the $PWD where content filters\n> are executed is unspecified the path to scripts needs to be specified as\n> an absolute path.  That means that the guide for setting up a repository\n> which uses scripts to filter content cannot simply consist of \"include\n> the following lines in your .git/config file\", and it means that the\n> otherwise safe operation of moving a git repository from one folder to\n> another is decidedly unsafe.\n> \n> This %w (short for 'work tree') will allow such scripts to exist and\n> be executed on each checkout, without needing to be added to the PATH\n> or be dependent upon the $PWD of the checkout call.\n\nI sympathize with your goal here, but I have two high-level thoughts.\n\nOne is regarding security.\n\nI was worried a bit that this might provide an opportunity for untrusted\nrepositories to trigger scripts from within their working trees without\nthe user having a chance to OK them. But the \"%w\" here would be used in\nthe config that the user must manually specify. So that gives them an\nopportunity to investigate it as much as they want.\n\nBut it does mean we're pointing to code in the working tree that could\nchange after a git-pull, etc. You _can_ be careful about that (e.g., by\nfetching and looking at the results, and then merging only if OK), but\nthe \"use from working tree by default\" model makes it easy to screw up\n(especially if you put it into your ~/.gitconfig, and then it's used\nwith every repository, trusted or not).\n\nWhen discussing similar features (like, say, taking project-level config\nafter making sure it's OK), our general thinking has been that we should\nmake sure Git can access some non-worktree location, and make it easy for\nusers to copy the data to it.\n\nSo in its most awkward state, that's asking people to copy the script\nsomewhere in their PATH (and then update it as needed, after verifying\nit each time). That's not _too_ bad as instructions go, but gets hard if\nyour audience doesn't have a consistent PATH set up.\n\nI wonder if we could do better with one or both of:\n\n  - some way of specifying the repository directory in a config option.\n    I think you can do something like that now with:\n\n      mkdir -p .git/scripts\n      cp myfilter .git/scripts/foo-filter\n      git config filter.foo.clean '$(git rev-parse --git-dir)/scripts/foo-filter'\n\n    which is admittedly a bit awkward. Some kind of syntactic candy\n    could help there (having Git recognize \"$GIT_SCRIPTS/foo-filter\" or\n    something).\n\n  - some way of specifying an in-repo blob via config. I think right now\n    you could do something like:\n\n      git config filter.foo.clean '\n        tmp=$(mktemp -t foo-filter) &&\n\ttrap \"rm -f \\\"$tmp\\\"\" 0 &&\n\tgit cat-file 1234abcd >\"$tmp\" &&\n\t\"$tmp\"\n      '\n\n    That's obviously even more horrible, but if we had some kind of\n    syntactic sugar, you could set the config to \"$oid:1234abcd\" or\n    something.\n\n    That saves you from the awkward \"mkdir/cp\" in the first example, but\n    gets around the security implications because of the immutability of\n    Git objects.\n\nMy second thought was that this is a more general problem. It's one for\nconfig itself, and for other script programs you might point to via\nconfig. So it seems like we should be able to come up with a solution\nthat is more general than just clean/smudge filters.\n\n-Peff\n"},{"id":"434931","messageId":"xmqqilzc9lm6.fsf@gitster.g","threadId":"56451","inReplyTo":"20210906181002.625647-1-calumlikesapplepie@gmail.com","subject":"Re: [PATCH 1/3] Add support for new %w wildcard in checkout filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-07T20:28:01Z","receivedAt":"2021-09-07T20:28:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> Subject: [PATCH 1/3] Add support for new %w wildcard in checkout filter\n\nPlease write your title to help readers of \"git shortlog --no-merges\".\nWith the above, readers would not know where the %w would newly be\nallowed to appear, what it does, and how it helps what use case.\n\n> When building content filters with gitattributes, for instance to ensure\n> git stores the plain-text rather than the binary form of data for certain\n> formats, it is often advantageous to separate the filters into separate,\n> potentially complex scripts.  However, as the $PWD where content filters\n> are executed is unspecified the path to scripts needs to be specified as\n> an absolute path.\n\nIsn't $PWD at least stable in a repository, relative to the top of\nworktree or something?  IOW, isn't the above raise a separate\ndocumentation issue that is better solved without any new code?\n\n> That means that the guide for setting up a repository\n> which uses scripts to filter content cannot simply consist of \"include\n> the following lines in your .git/config file\", and it means that the\n> otherwise safe operation of moving a git repository from one folder to\n> another is decidedly unsafe.\n\nIf these paths are given as absolute paths (e.g. ~/filter-scripts/),\nthe repositories that refer to these can be moved freely, as long as\nthe location of the the directory that holds these auxiliary scripts\nis kept stable, no?\n"}]}