{"thread":{"id":"51612","subject":"[PATCH] apply: reload .gitattributes after patching it","startedAt":"2019-08-09T10:02:32Z","lastAt":"2019-08-26T16:41:48Z","messageCount":41,"participants":["brian m. carlson","Taylor Blau","Jeff King","SZEDER Gábor","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380200","messageId":"20190809100217.427178-1-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":null,"subject":"[PATCH] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-09T10:02:17Z","receivedAt":"2019-08-09T10:02:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When applying multiple patches with git am, or when rebasing using the\nam backend, it's possible that one of our patches has updated a\ngitattributes file. Currently, we cache this information, so if a\nfile in a subsequent patch has attributes applied, the file will be\nwritten out with the attributes in place as of the time we started the\nrebase or am operation, not with the attributes applied by the previous\npatch. This problem does not occur when using the -m or -i flags to\nrebase.\n\nTo ensure we write the correct data into the working tree, expire the\ncache after each patch that touches a path ending in \".gitattributes\".\n\nSigned-off-by: brian m. carlson \n---\n apply.c | 11 +++++++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex cde95369bb..b959b88b8e 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \tint res = 0;\n+\tint flush_attributes = 0;\n \n \tstate->patch_input_file = filename;\n \tif (read_patch_file(&buf, fd) < 0)\n@@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,\n \t\t\tpatch_stats(state, patch);\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n+\n+\t\t\tif (!flush_attributes && patch->new_name) {\n+\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n+\t\t\t\t/* The patch applied to a .gitattributes file. */\n+\t\t\t\tif (dummy)\n+\t\t\t\t\tflush_attributes = 1;\n+\t\t\t\tfree(dummy);\n+\t\t\t}\n \t\t}\n \t\telse {\n \t\t\tif (state->apply_verbosity > verbosity_normal)\n@@ -4746,6 +4755,8 @@ static int apply_patch(struct apply_state *state,\n \tif (state->summary && state->apply_verbosity > verbosity_silent)\n \t\tsummary_patch_list(list);\n \n+\tif (flush_attributes)\n+\t\treset_parsed_attributes();\n end:\n \tfree_patch_list(list);\n \tstrbuf_release(&buf);\ndiff --git a/convert.c b/convert.c\nindex 94ff837649..030e9b81b9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1293,10 +1293,11 @@ struct conv_attrs {\n \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n };\n \n+static struct attr_check *check;\n+\n static void convert_attrs(const struct index_state *istate,\n \t\t\t struct conv_attrs *ca, const char *path)\n {\n-\tstatic struct attr_check *check;\n \tstruct attr_check_item *ccheck = NULL;\n \n \tif (!check) {\n@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n \t\tca->crlf_action = CRLF_AUTO_INPUT;\n }\n \n+void reset_parsed_attributes(void)\n+{\n+\tattr_check_free(check);\n+\tcheck = NULL;\n+}\n+\n int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n {\n \tstruct conv_attrs ca;\ndiff --git a/convert.h b/convert.h\nindex 831559f10d..3710969d43 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n int would_convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t\t const char *path);\n \n+/*\n+ * Reset the internal list of attributes used by convert_to_git and\n+ * convert_to_working_tree.\n+ */\n+void reset_parsed_attributes(void);\n+\n /*****************************************************************\n *\n * Streaming conversion support\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 80b23fd326..062dc41df7 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -301,6 +301,29 @@ test_expect_success 'rebase --am and --show-current-patch' '\n \t)\n '\n \n+test_expect_success 'rebase --am and .gitattributes' '\n+\ttest_create_repo attributes &&\n+\t(\n+\t\tcd attributes &&\n+\t\ttest_commit init &&\n+\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n+\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n+\n+\t\ttest_commit second &&\n+\t\tgit checkout -b test HEAD^ &&\n+\n+\t\techo \"*.txt filter=test\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\ttest_commit third &&\n+\n+\t\techo \"This text is smudged.\" >a.txt &&\n+\t\tgit add a.txt &&\n+\t\ttest_commit fourth &&\n+\t\tgit rebase master &&\n+\t\tgrep \"smudged\" a.txt\n+\t)\n+'\n+\n test_expect_success 'rebase--merge.sh and --show-current-patch' '\n \ttest_create_repo conflict-merge &&\n \t(\n"},{"id":"380201","messageId":"20190809111452.GA93559@syl.local","threadId":"51612","inReplyTo":"20190809100217.427178-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-08-09T11:14:52Z","receivedAt":"2019-08-09T11:14:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi brian,\n\nOn Fri, Aug 09, 2019 at 10:02:17AM +0000, brian m. carlson wrote:\n> When applying multiple patches with git am, or when rebasing using the\n> am backend, it's possible that one of our patches has updated a\n> gitattributes file. Currently, we cache this information, so if a\n> file in a subsequent patch has attributes applied, the file will be\n> written out with the attributes in place as of the time we started the\n> rebase or am operation, not with the attributes applied by the previous\n> patch. This problem does not occur when using the -m or -i flags to\n> rebase.\n\nAh. Thanks for a description of the issue.\n\n> To ensure we write the correct data into the working tree, expire the\n> cache after each patch that touches a path ending in \".gitattributes\".\n\nOK, that seems like a sensible direction...\n\n> Signed-off-by: brian m. carlson \n> ---\n> apply.c | 11 +++++++++++\n> convert.c | 9 ++++++++-\n> convert.h | 6 ++++++\n> t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n> 4 files changed, 48 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index cde95369bb..b959b88b8e 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n> \tstruct patch *list = NULL, **listp = &list;\n> \tint skipped_patch = 0;\n> \tint res = 0;\n> +\tint flush_attributes = 0;\n>\n> \tstate->patch_input_file = filename;\n> \tif (read_patch_file(&buf, fd) < 0)\n> @@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,\n> \t\t\tpatch_stats(state, patch);\n> \t\t\t*listp = patch;\n> \t\t\tlistp = &patch->next;\n> +\n> +\t\t\tif (!flush_attributes && patch->new_name) {\n> +\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n\nIt's a shame that 'strip_path_suffix' doesn't take a 'char *out', and\naccept NULL for that (which would save us the assignment and subsequent\n'free'). In either case, this is certainly the appropriate function.\n\nBut, I'm not sure if 'dummy' is the best name for this variable or not.\nMy first thought was that I'd expect this to be named 'suffix' (or the\nless descriptive 'p'), but I don't know if the change is warranted.\nWhat *are* we supposed to call this variable?\n\"path_after_gitattributes\" ;-)?\n\n> +\t\t\t\t/* The patch applied to a .gitattributes file. */\n> +\t\t\t\tif (dummy)\n> +\t\t\t\t\tflush_attributes = 1;\n> +\t\t\t\tfree(dummy);\n> +\t\t\t}\n> \t\t}\n> \t\telse {\n> \t\t\tif (state->apply_verbosity > verbosity_normal)\n> @@ -4746,6 +4755,8 @@ static int apply_patch(struct apply_state *state,\n> \tif (state->summary && state->apply_verbosity > verbosity_silent)\n> \t\tsummary_patch_list(list);\n>\n> +\tif (flush_attributes)\n> +\t\treset_parsed_attributes();\n> end:\n> \tfree_patch_list(list);\n> \tstrbuf_release(&buf);\n> diff --git a/convert.c b/convert.c\n> index 94ff837649..030e9b81b9 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -1293,10 +1293,11 @@ struct conv_attrs {\n> \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n> };\n>\n> +static struct attr_check *check;\n> +\n> static void convert_attrs(const struct index_state *istate,\n> \t\t\t struct conv_attrs *ca, const char *path)\n> {\n> -\tstatic struct attr_check *check;\n> \tstruct attr_check_item *ccheck = NULL;\n>\n> \tif (!check) {\n> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n> \t\tca->crlf_action = CRLF_AUTO_INPUT;\n> }\n>\n> +void reset_parsed_attributes(void)\n> +{\n> +\tattr_check_free(check);\n> +\tcheck = NULL;\n> +}\n> +\n\nMakes sense.\n\n> int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n> {\n> \tstruct conv_attrs ca;\n> diff --git a/convert.h b/convert.h\n> index 831559f10d..3710969d43 100644\n> --- a/convert.h\n> +++ b/convert.h\n> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n> int would_convert_to_git_filter_fd(const struct index_state *istate,\n> \t\t\t\t const char *path);\n>\n> +/*\n> + * Reset the internal list of attributes used by convert_to_git and\n> + * convert_to_working_tree.\n> + */\n> +void reset_parsed_attributes(void);\n> +\n> /*****************************************************************\n> *\n> * Streaming conversion support\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 80b23fd326..062dc41df7 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -301,6 +301,29 @@ test_expect_success 'rebase --am and --show-current-patch' '\n> \t)\n> '\n>\n> +test_expect_success 'rebase --am and .gitattributes' '\n> +\ttest_create_repo attributes &&\n> +\t(\n> +\t\tcd attributes &&\n> +\t\ttest_commit init &&\n> +\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n> +\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n> +\n> +\t\ttest_commit second &&\n> +\t\tgit checkout -b test HEAD^ &&\n> +\n> +\t\techo \"*.txt filter=test\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\ttest_commit third &&\n> +\n> +\t\techo \"This text is smudged.\" >a.txt &&\n> +\t\tgit add a.txt &&\n> +\t\ttest_commit fourth &&\n> +\t\tgit rebase master &&\n> +\t\tgrep \"smudged\" a.txt\n> +\t)\n> +'\n> +\n\nThis definitely exercise the behavior here. Thanks for adding a test\nwhile you're at it.\n\n> test_expect_success 'rebase--merge.sh and --show-current-patch' '\n> \ttest_create_repo conflict-merge &&\n> \t(\n\nI wouldn't be opposed to renaming 'dummy' to be something else, but in\nthe case that you don't feel a re-roll is necessary, please consider my:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"380205","messageId":"20190809112552.GM118825@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809111452.GA93559@syl.local","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-09T11:25:52Z","receivedAt":"2019-08-09T11:26:02Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-09 at 11:14:52, Taylor Blau wrote:\n> > diff --git a/apply.c b/apply.c\n> > index cde95369bb..b959b88b8e 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n> > \tstruct patch *list = NULL, **listp = &list;\n> > \tint skipped_patch = 0;\n> > \tint res = 0;\n> > +\tint flush_attributes = 0;\n> >\n> > \tstate->patch_input_file = filename;\n> > \tif (read_patch_file(&buf, fd) < 0)\n> > @@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,\n> > \t\t\tpatch_stats(state, patch);\n> > \t\t\t*listp = patch;\n> > \t\t\tlistp = &patch->next;\n> > +\n> > +\t\t\tif (!flush_attributes && patch->new_name) {\n> > +\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n> \n> It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and\n> accept NULL for that (which would save us the assignment and subsequent\n> 'free'). In either case, this is certainly the appropriate function.\n\nYeah, I felt the same way. We could refactor this out into two separate\nfunctions, one which returns an ssize_t and one which actually\nallocates, but I'm not sure it makes a huge amount of sense with just\none caller. The allocation is relatively small, and I've tried to make\nsure it's called exactly once per patch so as not to be wasteful and\ninefficient.\n\n> But, I'm not sure if 'dummy' is the best name for this variable or not.\n> My first thought was that I'd expect this to be named 'suffix' (or the\n> less descriptive 'p'), but I don't know if the change is warranted.\n> What *are* we supposed to call this variable?\n> \"path_after_gitattributes\" ;-)?\n\nI struggled with this variable name, as I'm sure you guessed. I could\ncall it \"has_suffix\" or something similar. I have mixed feelings about\nnaming pointer variables like booleans since it makes things prone to\nmisreading, but I think that might be the sanest thing to do here. I can\ncertainly do \"p\" as well.\n\nI'll see if there are other comments about directions to take here, and\nthen reroll.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380206","messageId":"20190809113613.GA3957@sigill.intra.peff.net","threadId":"51612","inReplyTo":"20190809112552.GM118825@genre.crustytoothpaste.net","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-09T11:36:14Z","receivedAt":"2019-08-09T11:36:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 09, 2019 at 11:25:52AM +0000, brian m. carlson wrote:\n\n> > > +\t\t\tif (!flush_attributes && patch->new_name) {\n> > > +\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n> > \n> > It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and\n> > accept NULL for that (which would save us the assignment and subsequent\n> > 'free'). In either case, this is certainly the appropriate function.\n> \n> Yeah, I felt the same way. We could refactor this out into two separate\n> functions, one which returns an ssize_t and one which actually\n> allocates, but I'm not sure it makes a huge amount of sense with just\n> one caller. The allocation is relatively small, and I've tried to make\n> sure it's called exactly once per patch so as not to be wasteful and\n> inefficient.\n\nI think you could do this with:\n\n size_t len;\n if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&\n len > 0 && is_dir_sep(patch->new_name[len-1]))\n flush_attributes = 1;\n\nNot sure if that is better or worse. It avoids the extra memory\nboilerplate, but the logic is a slightly more subtle.\n\n-Peff\n"},{"id":"380208","messageId":"20190809114723.GN118825@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809113613.GA3957@sigill.intra.peff.net","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-09T11:47:24Z","receivedAt":"2019-08-09T11:47:34Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-09 at 11:36:14, Jeff King wrote:\n> I think you could do this with:\n> \n> size_t len;\n> if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&\n> len > 0 && is_dir_sep(patch->new_name[len-1]))\n> flush_attributes = 1;\n> \n> Not sure if that is better or worse. It avoids the extra memory\n> boilerplate, but the logic is a slightly more subtle.\n\nI think an approach like that would be fine with a comment. I'm not sure\nthis exact approach works with -p0, though, so I'll have to tweak it a\nbit.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380209","messageId":"20190809124318.GJ20404@szeder.dev","threadId":"51612","inReplyTo":"20190809113613.GA3957@sigill.intra.peff.net","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-09T12:43:18Z","receivedAt":"2019-08-09T12:43:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Aug 09, 2019 at 07:36:14AM -0400, Jeff King wrote:\n> On Fri, Aug 09, 2019 at 11:25:52AM +0000, brian m. carlson wrote:\n> \n> > > > +\t\t\tif (!flush_attributes && patch->new_name) {\n> > > > +\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n> > > \n> > > It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and\n> > > accept NULL for that (which would save us the assignment and subsequent\n> > > 'free'). In either case, this is certainly the appropriate function.\n> > \n> > Yeah, I felt the same way. We could refactor this out into two separate\n> > functions, one which returns an ssize_t and one which actually\n> > allocates, but I'm not sure it makes a huge amount of sense with just\n> > one caller. The allocation is relatively small, and I've tried to make\n> > sure it's called exactly once per patch so as not to be wasteful and\n> > inefficient.\n> \n> I think you could do this with:\n> \n> size_t len;\n> if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&\n> len > 0 && is_dir_sep(patch->new_name[len-1]))\n> flush_attributes = 1;\n> \n> Not sure if that is better or worse. It avoids the extra memory\n> boilerplate, but the logic is a slightly more subtle.\n\nSubtle indeed :) because you have to allow len == 0 to catch the\ntoplevel .gitattributes file.\n\nBut there is an other subtlety here: when I read the commit message\nsaying \"patch that touches a path ending in \".gitattributes\".\" and saw\nthe new call to strip_path_suffix(), I immediately thought what would\nhappen with a file called 'foo.gitattributes'. Only when I looked\ninto strip_path_suffix() became it clear that it only removes full\npath components, so such a filename won't cause any trouble (though\nperhaps the worst thing that could happen is that we unnecessarily\nflush the attributes cache).\n\n"},{"id":"380210","messageId":"20190809135148.GA7495@sigill.intra.peff.net","threadId":"51612","inReplyTo":"20190809124318.GJ20404@szeder.dev","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-09T13:51:49Z","receivedAt":"2019-08-09T13:51:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 09, 2019 at 02:43:18PM +0200, SZEDER Gábor wrote:\n\n> > I think you could do this with:\n> > \n> > size_t len;\n> > if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&\n> > len > 0 && is_dir_sep(patch->new_name[len-1]))\n> > flush_attributes = 1;\n> > \n> > Not sure if that is better or worse. It avoids the extra memory\n> > boilerplate, but the logic is a slightly more subtle.\n> \n> Subtle indeed :) because you have to allow len == 0 to catch the\n> toplevel .gitattributes file.\n\nI'll pretend I was leaving that as a puzzle for the readers. :)\n\n> But there is an other subtlety here: when I read the commit message\n> saying \"patch that touches a path ending in \".gitattributes\".\" and saw\n> the new call to strip_path_suffix(), I immediately thought what would\n> happen with a file called 'foo.gitattributes'. Only when I looked\n> into strip_path_suffix() became it clear that it only removes full\n> path components, so such a filename won't cause any trouble (though\n> perhaps the worst thing that could happen is that we unnecessarily\n> flush the attributes cache).\n\nRight. I think the term we want here is really \"basename\". So in fact:\n\n if (!strcmp(basename(patch->new_name), GITATTRIBUTES_FILE))\n\nwould do what we want, except for the annoying caveat that basename() is\nallowed to modify its parameter (to remove trailing directory\nseparators, but we know we wouldn't have them here).\n\n-Peff\n"},{"id":"380250","messageId":"20190809220848.GO118825@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809135148.GA7495@sigill.intra.peff.net","subject":"Re: [PATCH] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-09T22:08:48Z","receivedAt":"2019-08-09T22:08:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-09 at 13:51:49, Jeff King wrote:\n> On Fri, Aug 09, 2019 at 02:43:18PM +0200, SZEDER Gábor wrote:\n> > But there is an other subtlety here: when I read the commit message\n> > saying \"patch that touches a path ending in \".gitattributes\".\" and saw\n> > the new call to strip_path_suffix(), I immediately thought what would\n> > happen with a file called 'foo.gitattributes'. Only when I looked\n> > into strip_path_suffix() became it clear that it only removes full\n> > path components, so such a filename won't cause any trouble (though\n> > perhaps the worst thing that could happen is that we unnecessarily\n> > flush the attributes cache).\n> \n> Right. I think the term we want here is really \"basename\". So in fact:\n> \n> if (!strcmp(basename(patch->new_name), GITATTRIBUTES_FILE))\n> \n> would do what we want, except for the annoying caveat that basename() is\n> allowed to modify its parameter (to remove trailing directory\n> separators, but we know we wouldn't have them here).\n\nI think this is exactly the function I'm looking for. I'm a little\nuncomfortable relying on the fact that typical implementations don't\nmodify the string when there's no trailing slash, though.\n\nI think I'm going to end up refactoring out the strip_path_suffix\nfunction into a has_path_suffix and use that. That will avoid the\nallocation and it doesn't rely on the generosity of OS implementers.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380286","messageId":"20190811174748.33552-1-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809100217.427178-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 0/2] Honor .gitattributes with rebase --am","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-11T17:47:46Z","receivedAt":"2019-08-11T17:48:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series makes rebase --am honor the .gitattributes file for\nsubsequent patches when a patch changes it.\n\nChanges from v1:\n* Add has_path_suffix in a separate commit.\n\nbrian m. carlson (2):\n path: add a function to check for path suffix\n apply: reload .gitattributes after patching it\n\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n 6 files changed, 77 insertions(+), 10 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: e865872fc3 :rebase-gitattributes\n-: ---------- > 2: 125fda966c path: add a function to check for path suffix\n1: d7ea19aeef ! 3: f54af7e595 apply: reload .gitattributes after patching it\n @@ apply.c: static int apply_patch(struct apply_state *state,\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n +\n -+\t\t\tif (!flush_attributes && patch->new_name) {\n -+\t\t\t\tchar *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);\n -+\t\t\t\t/* The patch applied to a .gitattributes file. */\n -+\t\t\t\tif (dummy)\n -+\t\t\t\t\tflush_attributes = 1;\n -+\t\t\t\tfree(dummy);\n -+\t\t\t}\n ++\t\t\tif (!flush_attributes && patch->new_name &&\n ++\t\t\t has_path_suffix(patch->new_name, GITATTRIBUTES_FILE))\n ++\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n \t\t\tif (state->apply_verbosity > verbosity_normal)\n"},{"id":"380287","messageId":"20190811174748.33552-2-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190811174748.33552-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/2] path: add a function to check for path suffix","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-11T17:47:47Z","receivedAt":"2019-08-11T17:48:18Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We have a function to strip the path suffix from a commit, but we don't\nhave one to check for a path suffix. For a plain filename, we can use\nbasename, but that requires an allocation, since POSIX allows it to\nmodify its argument. Refactor strip_path_suffix into a helper function\nand a new function, has_path_suffix to meet this need.\n\nSigned-off-by: brian m. carlson \n---\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 25e97b8c3f..e193c62b7d 100644\n--- a/path.c\n+++ b/path.c\n@@ -1221,31 +1221,52 @@ static inline int chomp_trailing_dir_sep(const char *path, int len)\n }\n \n /*\n- * If path ends with suffix (complete path components), returns the\n- * part before suffix (sans trailing directory separators).\n- * Otherwise returns NULL.\n+ * If path ends with suffix (complete path components), returns the offset of\n+ * the last character in the path before the suffix (sans trailing directory\n+ * separators), and -1 otherwise.\n */\n-char *strip_path_suffix(const char *path, const char *suffix)\n+static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n {\n \tint path_len = strlen(path), suffix_len = strlen(suffix);\n \n \twhile (suffix_len) {\n \t\tif (!path_len)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n \t\tif (is_dir_sep(path[path_len - 1])) {\n \t\t\tif (!is_dir_sep(suffix[suffix_len - 1]))\n-\t\t\t\treturn NULL;\n+\t\t\t\treturn -1;\n \t\t\tpath_len = chomp_trailing_dir_sep(path, path_len);\n \t\t\tsuffix_len = chomp_trailing_dir_sep(suffix, suffix_len);\n \t\t}\n \t\telse if (path[--path_len] != suffix[--suffix_len])\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \t}\n \n \tif (path_len && !is_dir_sep(path[path_len - 1]))\n-\t\treturn NULL;\n-\treturn xstrndup(path, chomp_trailing_dir_sep(path, path_len));\n+\t\treturn -1;\n+\treturn chomp_trailing_dir_sep(path, path_len);\n+}\n+\n+/*\n+ * Returns true if the path ends with suffix, considering only complete path\n+ * components and false otherwise.\n+ */\n+int has_path_suffix(const char *path, const char *suffix)\n+{\n+\treturn stripped_path_suffix_offset(path, suffix) != -1;\n+}\n+\n+/*\n+ * If path ends with suffix (complete path components), returns the\n+ * part before suffix (sans trailing directory separators).\n+ * Otherwise returns NULL.\n+ */\n+char *strip_path_suffix(const char *path, const char *suffix)\n+{\n+\tssize_t offset = stripped_path_suffix_offset(path, suffix);\n+\n+\treturn offset == -1 ? NULL : xstrndup(path, offset);\n }\n \n int daemon_avoid_alias(const char *p)\ndiff --git a/path.h b/path.h\nindex 2ba6ca58c8..c01d045786 100644\n--- a/path.h\n+++ b/path.h\n@@ -193,4 +193,7 @@ const char *git_path_merge_head(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n+\n+int has_path_suffix(const char *path, const char *suffix);\n+\n #endif /* PATH_H */\n"},{"id":"380288","messageId":"20190811174748.33552-3-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190811174748.33552-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-11T17:47:48Z","receivedAt":"2019-08-11T17:48:18Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When applying multiple patches with git am, or when rebasing using the\nam backend, it's possible that one of our patches has updated a\ngitattributes file. Currently, we cache this information, so if a\nfile in a subsequent patch has attributes applied, the file will be\nwritten out with the attributes in place as of the time we started the\nrebase or am operation, not with the attributes applied by the previous\npatch. This problem does not occur when using the -m or -i flags to\nrebase.\n\nTo ensure we write the correct data into the working tree, expire the\ncache after each patch that touches a path ending in \".gitattributes\".\n\nSigned-off-by: brian m. carlson \n---\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex cde95369bb..201ee12e21 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \tint res = 0;\n+\tint flush_attributes = 0;\n \n \tstate->patch_input_file = filename;\n \tif (read_patch_file(&buf, fd) < 0)\n@@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n \t\t\tpatch_stats(state, patch);\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n+\n+\t\t\tif (!flush_attributes && patch->new_name &&\n+\t\t\t has_path_suffix(patch->new_name, GITATTRIBUTES_FILE))\n+\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n \t\t\tif (state->apply_verbosity > verbosity_normal)\n@@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,\n \tif (state->summary && state->apply_verbosity > verbosity_silent)\n \t\tsummary_patch_list(list);\n \n+\tif (flush_attributes)\n+\t\treset_parsed_attributes();\n end:\n \tfree_patch_list(list);\n \tstrbuf_release(&buf);\ndiff --git a/convert.c b/convert.c\nindex 94ff837649..030e9b81b9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1293,10 +1293,11 @@ struct conv_attrs {\n \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n };\n \n+static struct attr_check *check;\n+\n static void convert_attrs(const struct index_state *istate,\n \t\t\t struct conv_attrs *ca, const char *path)\n {\n-\tstatic struct attr_check *check;\n \tstruct attr_check_item *ccheck = NULL;\n \n \tif (!check) {\n@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n \t\tca->crlf_action = CRLF_AUTO_INPUT;\n }\n \n+void reset_parsed_attributes(void)\n+{\n+\tattr_check_free(check);\n+\tcheck = NULL;\n+}\n+\n int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n {\n \tstruct conv_attrs ca;\ndiff --git a/convert.h b/convert.h\nindex 831559f10d..3710969d43 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n int would_convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t\t const char *path);\n \n+/*\n+ * Reset the internal list of attributes used by convert_to_git and\n+ * convert_to_working_tree.\n+ */\n+void reset_parsed_attributes(void);\n+\n /*****************************************************************\n *\n * Streaming conversion support\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 80b23fd326..062dc41df7 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -301,6 +301,29 @@ test_expect_success 'rebase --am and --show-current-patch' '\n \t)\n '\n \n+test_expect_success 'rebase --am and .gitattributes' '\n+\ttest_create_repo attributes &&\n+\t(\n+\t\tcd attributes &&\n+\t\ttest_commit init &&\n+\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n+\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n+\n+\t\ttest_commit second &&\n+\t\tgit checkout -b test HEAD^ &&\n+\n+\t\techo \"*.txt filter=test\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\ttest_commit third &&\n+\n+\t\techo \"This text is smudged.\" >a.txt &&\n+\t\tgit add a.txt &&\n+\t\ttest_commit fourth &&\n+\t\tgit rebase master &&\n+\t\tgrep \"smudged\" a.txt\n+\t)\n+'\n+\n test_expect_success 'rebase--merge.sh and --show-current-patch' '\n \ttest_create_repo conflict-merge &&\n \t(\n"},{"id":"380293","messageId":"xmqq1rxrcjfp.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190811174748.33552-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-12T00:32:26Z","receivedAt":"2019-08-12T00:32:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> We have a function to strip the path suffix from a commit, but we don't\n> have one to check for a path suffix. For a plain filename, we can use\n> basename, but that requires an allocation, since POSIX allows it to\n> modify its argument. Refactor strip_path_suffix into a helper function\n> and a new function, has_path_suffix to meet this need.\n\nI wish we did not use a crazy phrase \"path suffix\", that would\ninevitably confuse ourselves with things like \".exe\".\n\n> /*\n> + * If path ends with suffix (complete path components), returns the offset of\n> + * the last character in the path before the suffix (sans trailing directory\n> + * separators), and -1 otherwise.\n\ni.e. this is offset to the last path component.\n\n> +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n\nPerhaps\n\n static ssize_t last_path_component_offset(const char *path, const char *name)\n\nI am tempted to also call the second parameter to this function\n\"basename\", as we know from the proposed log message that you wish\n\"basename\" were usable for this purpose, but basename itself has\nanother confusing factor (i.e. \"are we stripping \".exe\" extension?\",\nto which the answer is no in the context of these functions).\n\nIf we agree with the \"last path component\" phrasing, has_path_suffix()\nwould become something like:\n\n int last_path_component_equals(const char *path, const char *name);\n\nperhaps.\n"},{"id":"380296","messageId":"20190812011054.GA9180@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"xmqq1rxrcjfp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-12T01:10:54Z","receivedAt":"2019-08-12T01:11:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-12 at 00:32:26, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n> \n> Perhaps\n> \n> static ssize_t last_path_component_offset(const char *path, const char *name)\n> \n> I am tempted to also call the second parameter to this function\n> \"basename\", as we know from the proposed log message that you wish\n> \"basename\" were usable for this purpose, but basename itself has\n> another confusing factor (i.e. \"are we stripping \".exe\" extension?\",\n> to which the answer is no in the context of these functions).\n> \n> If we agree with the \"last path component\" phrasing, has_path_suffix()\n> would become something like:\n> \n> int last_path_component_equals(const char *path, const char *name);\n\nExcept this is not necessarily the last path component. It could match\none or more path components with the way the function is written. If you\nwant to ignore that and name the function accordingly, I won't object,\nbut we could theoretically handle a name like \"foo/.gitattributes\" as\nwell.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380297","messageId":"20190812043622.GL20404@szeder.dev","threadId":"51612","inReplyTo":"20190812011054.GA9180@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-12T04:36:22Z","receivedAt":"2019-08-12T04:36:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 12, 2019 at 01:10:54AM +0000, brian m. carlson wrote:\n> On 2019-08-12 at 00:32:26, Junio C Hamano wrote:\n> > \"brian m. carlson\" writes:\n> > > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n> > \n> > Perhaps\n> > \n> > static ssize_t last_path_component_offset(const char *path, const char *name)\n> > \n> > I am tempted to also call the second parameter to this function\n> > \"basename\", as we know from the proposed log message that you wish\n> > \"basename\" were usable for this purpose, but basename itself has\n> > another confusing factor (i.e. \"are we stripping \".exe\" extension?\",\n> > to which the answer is no in the context of these functions).\n> > \n> > If we agree with the \"last path component\" phrasing, has_path_suffix()\n> > would become something like:\n> > \n> > int last_path_component_equals(const char *path, const char *name);\n> \n> Except this is not necessarily the last path component. It could match\n> one or more path components with the way the function is written. If you\n> want to ignore that and name the function accordingly, I won't object,\n> but we could theoretically handle a name like \"foo/.gitattributes\" as\n> well.\n\nends_with_path_components(), perhaps?\n\nI think having \"path_component\" in some form in the function name\nwould have avoided my confusion mentioned earlier in a reply to the\nfirst version.\n\n\n"},{"id":"380317","messageId":"xmqq7e7iba7j.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190812011054.GA9180@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-12T16:49:20Z","receivedAt":"2019-08-12T16:49:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2019-08-12 at 00:32:26, Junio C Hamano wrote:\n>> \"brian m. carlson\" writes:\n>> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n>> \n>> Perhaps\n>> \n>> static ssize_t last_path_component_offset(const char *path, const char *name)\n>> \n>> I am tempted to also call the second parameter to this function\n>> \"basename\", as we know from the proposed log message that you wish\n>> \"basename\" were usable for this purpose, but basename itself has\n>> another confusing factor (i.e. \"are we stripping \".exe\" extension?\",\n>> to which the answer is no in the context of these functions).\n>> \n>> If we agree with the \"last path component\" phrasing, has_path_suffix()\n>> would become something like:\n>> \n>> int last_path_component_equals(const char *path, const char *name);\n>\n> Except this is not necessarily the last path component. It could match\n> one or more path components with the way the function is written.\n\nThat's fair. Is the feature that allows the function called\nends_with_component*S* like Szeder suggests designed one, i.e. with\nan explicit purpose of supporting callers that pass \"foo/bar\" as the\n\"suffix\" to it, or is it merely that the implementation happens to\nwork that way, even though the expected use that is supported is to\npass only one level component but the implementation did not even\nbother asserting that the \"suffix\" does not have a slash in it?\n\n"},{"id":"380353","messageId":"20190812224021.GA229582@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"xmqq7e7iba7j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-12T22:40:21Z","receivedAt":"2019-08-12T22:40:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-12 at 16:49:20, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > On 2019-08-12 at 00:32:26, Junio C Hamano wrote:\n> >> \"brian m. carlson\" writes:\n> >> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n> >> \n> >> Perhaps\n> >> \n> >> static ssize_t last_path_component_offset(const char *path, const char *name)\n> >> \n> >> I am tempted to also call the second parameter to this function\n> >> \"basename\", as we know from the proposed log message that you wish\n> >> \"basename\" were usable for this purpose, but basename itself has\n> >> another confusing factor (i.e. \"are we stripping \".exe\" extension?\",\n> >> to which the answer is no in the context of these functions).\n> >> \n> >> If we agree with the \"last path component\" phrasing, has_path_suffix()\n> >> would become something like:\n> >> \n> >> int last_path_component_equals(const char *path, const char *name);\n> >\n> > Except this is not necessarily the last path component. It could match\n> > one or more path components with the way the function is written.\n> \n> That's fair. Is the feature that allows the function called\n> ends_with_component*S* like Szeder suggests designed one, i.e. with\n> an explicit purpose of supporting callers that pass \"foo/bar\" as the\n> \"suffix\" to it, or is it merely that the implementation happens to\n> work that way, even though the expected use that is supported is to\n> pass only one level component but the implementation did not even\n> bother asserting that the \"suffix\" does not have a slash in it?\n\nWell, I split it out from a function that handles multiple path\ncomponents, mostly so that I could leverage existing work (and not have\nto worry about getting it wrong). It wasn't explicitly intended that it\nsupport multiple components, since I don't require that for my\nimplementation, but I could see future users taking advantage of that.\n\nI think \"ends_with_path_components\" might be the way forward, unless\nyou think something else would be better.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380355","messageId":"20190813011259.GA18065@sigill.intra.peff.net","threadId":"51612","inReplyTo":"20190812224021.GA229582@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-13T01:13:00Z","receivedAt":"2019-08-13T01:13:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 12, 2019 at 10:40:21PM +0000, brian m. carlson wrote:\n\n> I think \"ends_with_path_components\" might be the way forward, unless\n> you think something else would be better.\n\nFWIW, having read the rest of the thread, that was the name that clicked\nfor me.\n\n-Peff\n"},{"id":"380360","messageId":"20190813063618.GO20404@szeder.dev","threadId":"51612","inReplyTo":"xmqq7e7iba7j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-13T06:36:18Z","receivedAt":"2019-08-13T06:36:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 12, 2019 at 09:49:20AM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > On 2019-08-12 at 00:32:26, Junio C Hamano wrote:\n> >> \"brian m. carlson\" writes:\n> >> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n> >> \n> >> Perhaps\n> >> \n> >> static ssize_t last_path_component_offset(const char *path, const char *name)\n> >> \n> >> I am tempted to also call the second parameter to this function\n> >> \"basename\", as we know from the proposed log message that you wish\n> >> \"basename\" were usable for this purpose, but basename itself has\n> >> another confusing factor (i.e. \"are we stripping \".exe\" extension?\",\n> >> to which the answer is no in the context of these functions).\n> >> \n> >> If we agree with the \"last path component\" phrasing, has_path_suffix()\n> >> would become something like:\n> >> \n> >> int last_path_component_equals(const char *path, const char *name);\n> >\n> > Except this is not necessarily the last path component. It could match\n> > one or more path components with the way the function is written.\n> \n> That's fair. Is the feature that allows the function called\n> ends_with_component*S* like Szeder suggests designed one, i.e. with\n> an explicit purpose of supporting callers that pass \"foo/bar\" as the\n> \"suffix\" to it, or is it merely that the implementation happens to\n> work that way, even though the expected use that is supported is to\n> pass only one level component but the implementation did not even\n> bother asserting that the \"suffix\" does not have a slash in it?\n\nThe plural in the suggested function name was intentional on my part,\neven though in this callsite in question we are only interested in the\nfilename, i.e. only a single path component.\n\nI was hoping that the names of these related functions will be kept in\nsync, and all will somehow contain the \"path_components\" substring,\ne.g. strip_path_suffix() becomes strip_suffix_path_components() or\nsomething. And that function must be able to handle multiple path\ncomponents, becase there is this callsite:\n\n exec-cmd.c: !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n\nand the build sets '-DGIT_EXEC_PATH=\"libexec/git-core\"' by default.\n\n"},{"id":"380379","messageId":"20190813024307.705016-1-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809100217.427178-1-sandals@crustytoothpaste.net","subject":"[PATCH v3 0/2] Honor .gitattributes with rebase --am","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-13T02:43:05Z","receivedAt":"2019-08-13T12:45:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series makes rebase --am honor the .gitattributes file for\nsubsequent patches when a patch changes it.\n\nChanges from v2:\n* Rename has_path_suffix to ends_with_path_components.\n\nChanges from v1:\n* Add has_path_suffix in a separate commit.\n\nbrian m. carlson (2):\n path: add a function to check for path suffix\n apply: reload .gitattributes after patching it\n\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n 6 files changed, 77 insertions(+), 10 deletions(-)\n\nRange-diff against v2:\n1: 125fda966c ! 1: 14c853420b path: add a function to check for path suffix\n @@ Commit message\n have one to check for a path suffix. For a plain filename, we can use\n basename, but that requires an allocation, since POSIX allows it to\n modify its argument. Refactor strip_path_suffix into a helper function\n - and a new function, has_path_suffix to meet this need.\n + and a new function, ends_with_path_components, to meet this need.\n \n Signed-off-by: brian m. carlson \n \n @@ path.c: static inline int chomp_trailing_dir_sep(const char *path, int len)\n +}\n +\n +/*\n -+ * Returns true if the path ends with suffix, considering only complete path\n -+ * components and false otherwise.\n ++ * Returns true if the path ends with components, considering only complete path\n ++ * components, and false otherwise.\n + */\n -+int has_path_suffix(const char *path, const char *suffix)\n ++int ends_with_path_components(const char *path, const char *components)\n +{\n -+\treturn stripped_path_suffix_offset(path, suffix) != -1;\n ++\treturn stripped_path_suffix_offset(path, components) != -1;\n +}\n +\n /*\n @@ path.h: const char *git_path_merge_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n +\n -+int has_path_suffix(const char *path, const char *suffix);\n ++int ends_with_path_components(const char *path, const char *components);\n +\n #endif /* PATH_H */\n2: f54af7e595 ! 2: 5f4402b38d apply: reload .gitattributes after patching it\n @@ apply.c: static int apply_patch(struct apply_state *state,\n \t\t\tlistp = &patch->next;\n +\n +\t\t\tif (!flush_attributes && patch->new_name &&\n -+\t\t\t has_path_suffix(patch->new_name, GITATTRIBUTES_FILE))\n ++\t\t\t ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))\n +\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n"},{"id":"380380","messageId":"20190813024307.705016-2-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190813024307.705016-1-sandals@crustytoothpaste.net","subject":"[PATCH v3 1/2] path: add a function to check for path suffix","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-13T02:43:06Z","receivedAt":"2019-08-13T12:45:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We have a function to strip the path suffix from a commit, but we don't\nhave one to check for a path suffix. For a plain filename, we can use\nbasename, but that requires an allocation, since POSIX allows it to\nmodify its argument. Refactor strip_path_suffix into a helper function\nand a new function, ends_with_path_components, to meet this need.\n\nSigned-off-by: brian m. carlson \n---\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 25e97b8c3f..e3da1f3c4e 100644\n--- a/path.c\n+++ b/path.c\n@@ -1221,31 +1221,52 @@ static inline int chomp_trailing_dir_sep(const char *path, int len)\n }\n \n /*\n- * If path ends with suffix (complete path components), returns the\n- * part before suffix (sans trailing directory separators).\n- * Otherwise returns NULL.\n+ * If path ends with suffix (complete path components), returns the offset of\n+ * the last character in the path before the suffix (sans trailing directory\n+ * separators), and -1 otherwise.\n */\n-char *strip_path_suffix(const char *path, const char *suffix)\n+static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n {\n \tint path_len = strlen(path), suffix_len = strlen(suffix);\n \n \twhile (suffix_len) {\n \t\tif (!path_len)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n \t\tif (is_dir_sep(path[path_len - 1])) {\n \t\t\tif (!is_dir_sep(suffix[suffix_len - 1]))\n-\t\t\t\treturn NULL;\n+\t\t\t\treturn -1;\n \t\t\tpath_len = chomp_trailing_dir_sep(path, path_len);\n \t\t\tsuffix_len = chomp_trailing_dir_sep(suffix, suffix_len);\n \t\t}\n \t\telse if (path[--path_len] != suffix[--suffix_len])\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \t}\n \n \tif (path_len && !is_dir_sep(path[path_len - 1]))\n-\t\treturn NULL;\n-\treturn xstrndup(path, chomp_trailing_dir_sep(path, path_len));\n+\t\treturn -1;\n+\treturn chomp_trailing_dir_sep(path, path_len);\n+}\n+\n+/*\n+ * Returns true if the path ends with components, considering only complete path\n+ * components, and false otherwise.\n+ */\n+int ends_with_path_components(const char *path, const char *components)\n+{\n+\treturn stripped_path_suffix_offset(path, components) != -1;\n+}\n+\n+/*\n+ * If path ends with suffix (complete path components), returns the\n+ * part before suffix (sans trailing directory separators).\n+ * Otherwise returns NULL.\n+ */\n+char *strip_path_suffix(const char *path, const char *suffix)\n+{\n+\tssize_t offset = stripped_path_suffix_offset(path, suffix);\n+\n+\treturn offset == -1 ? NULL : xstrndup(path, offset);\n }\n \n int daemon_avoid_alias(const char *p)\ndiff --git a/path.h b/path.h\nindex 2ba6ca58c8..14d6dcad16 100644\n--- a/path.h\n+++ b/path.h\n@@ -193,4 +193,7 @@ const char *git_path_merge_head(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n+\n+int ends_with_path_components(const char *path, const char *components);\n+\n #endif /* PATH_H */\n"},{"id":"380381","messageId":"20190813024307.705016-3-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190813024307.705016-1-sandals@crustytoothpaste.net","subject":"[PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-13T02:43:07Z","receivedAt":"2019-08-13T12:46:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When applying multiple patches with git am, or when rebasing using the\nam backend, it's possible that one of our patches has updated a\ngitattributes file. Currently, we cache this information, so if a\nfile in a subsequent patch has attributes applied, the file will be\nwritten out with the attributes in place as of the time we started the\nrebase or am operation, not with the attributes applied by the previous\npatch. This problem does not occur when using the -m or -i flags to\nrebase.\n\nTo ensure we write the correct data into the working tree, expire the\ncache after each patch that touches a path ending in \".gitattributes\".\n\nSigned-off-by: brian m. carlson \n---\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n t/t3400-rebase.sh | 23 +++++++++++++++++++++++\n 4 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex cde95369bb..e5373452ab 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \tint res = 0;\n+\tint flush_attributes = 0;\n \n \tstate->patch_input_file = filename;\n \tif (read_patch_file(&buf, fd) < 0)\n@@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n \t\t\tpatch_stats(state, patch);\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n+\n+\t\t\tif (!flush_attributes && patch->new_name &&\n+\t\t\t ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))\n+\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n \t\t\tif (state->apply_verbosity > verbosity_normal)\n@@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,\n \tif (state->summary && state->apply_verbosity > verbosity_silent)\n \t\tsummary_patch_list(list);\n \n+\tif (flush_attributes)\n+\t\treset_parsed_attributes();\n end:\n \tfree_patch_list(list);\n \tstrbuf_release(&buf);\ndiff --git a/convert.c b/convert.c\nindex 94ff837649..030e9b81b9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1293,10 +1293,11 @@ struct conv_attrs {\n \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n };\n \n+static struct attr_check *check;\n+\n static void convert_attrs(const struct index_state *istate,\n \t\t\t struct conv_attrs *ca, const char *path)\n {\n-\tstatic struct attr_check *check;\n \tstruct attr_check_item *ccheck = NULL;\n \n \tif (!check) {\n@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n \t\tca->crlf_action = CRLF_AUTO_INPUT;\n }\n \n+void reset_parsed_attributes(void)\n+{\n+\tattr_check_free(check);\n+\tcheck = NULL;\n+}\n+\n int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n {\n \tstruct conv_attrs ca;\ndiff --git a/convert.h b/convert.h\nindex 831559f10d..3710969d43 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n int would_convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t\t const char *path);\n \n+/*\n+ * Reset the internal list of attributes used by convert_to_git and\n+ * convert_to_working_tree.\n+ */\n+void reset_parsed_attributes(void);\n+\n /*****************************************************************\n *\n * Streaming conversion support\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 80b23fd326..062dc41df7 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -301,6 +301,29 @@ test_expect_success 'rebase --am and --show-current-patch' '\n \t)\n '\n \n+test_expect_success 'rebase --am and .gitattributes' '\n+\ttest_create_repo attributes &&\n+\t(\n+\t\tcd attributes &&\n+\t\ttest_commit init &&\n+\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n+\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n+\n+\t\ttest_commit second &&\n+\t\tgit checkout -b test HEAD^ &&\n+\n+\t\techo \"*.txt filter=test\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\ttest_commit third &&\n+\n+\t\techo \"This text is smudged.\" >a.txt &&\n+\t\tgit add a.txt &&\n+\t\ttest_commit fourth &&\n+\t\tgit rebase master &&\n+\t\tgrep \"smudged\" a.txt\n+\t)\n+'\n+\n test_expect_success 'rebase--merge.sh and --show-current-patch' '\n \ttest_create_repo conflict-merge &&\n \t(\n"},{"id":"380394","messageId":"xmqq4l2l9fxh.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190812224021.GA229582@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T16:40:58Z","receivedAt":"2019-08-13T16:41:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Well, I split it out from a function that handles multiple path\n> components, mostly so that I could leverage existing work (and not have\n> to worry about getting it wrong). It wasn't explicitly intended that it\n> support multiple components, since I don't require that for my\n> implementation, but I could see future users taking advantage of that.\n>\n> I think \"ends_with_path_components\" might be the way forward, unless\n> you think something else would be better.\n\nGood; thanks.\n"},{"id":"380395","messageId":"xmqqzhkd81ag.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190813063618.GO20404@szeder.dev","subject":"Re: [PATCH v2 1/2] path: add a function to check for path suffix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T16:42:31Z","receivedAt":"2019-08-13T16:42:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> ... And that function must be able to handle multiple path\n> components, becase there is this callsite:\n>\n> exec-cmd.c: !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n>\n> and the build sets '-DGIT_EXEC_PATH=\"libexec/git-core\"' by default.\n\nOK, that answers my earlier question. We do want to support such a\ncaller with one or more components at the end.\n\nThanks.\n"},{"id":"380403","messageId":"xmqq7e7h7xap.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190813024307.705016-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T18:08:46Z","receivedAt":"2019-08-13T18:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> When applying multiple patches with git am, or when rebasing using the\n> am backend, it's possible that one of our patches has updated a\n> gitattributes file. Currently, we cache this information, so if a\n> file in a subsequent patch has attributes applied, the file will be\n> written out with the attributes in place as of the time we started the\n> rebase or am operation, not with the attributes applied by the previous\n> patch. This problem does not occur when using the -m or -i flags to\n> rebase.\n\nBack when \"am\" was a script that repeatedly calls \"apply --index &&\ncommit-tree\", caching the original contents of the attributes file\nand using it throughout the series would have been impossible to\nbegin with, so this must be a regression when we rewrote it in C and\nmoved to do everything in a single process without clearly the state\nbetween the steps, I guess.\n\n\"rebase -m\" and \"rebase -i\" are not repeated run_command() calls\nthat invoke \"git cherry-pick\" or \"git merge\" these days, either, so\nI am somewhat curious how they avoid fallilng into the same trap.\n\nThanks for the fix. Will queue.\n"},{"id":"380424","messageId":"xmqq36i47ku3.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"20190813024307.705016-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-13T22:37:56Z","receivedAt":"2019-08-13T22:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> +test_expect_success 'rebase --am and .gitattributes' '\n> +\ttest_create_repo attributes &&\n> +\t(\n> +\t\tcd attributes &&\n> +\t\ttest_commit init &&\n> +\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n> +\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n\nThese eventually invokes test-when-finished for cleaning things up,\nthat cannot be called inside a subshell. The \"attributes\" test\nrepository is a throw-away reopsitory, so we should be able to just\nuse \"git config\" to set the variables locally in it, no?\n\n> +\t\ttest_commit second &&\n> +\t\tgit checkout -b test HEAD^ &&\n> +\n> +\t\techo \"*.txt filter=test\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\ttest_commit third &&\n> +\n> +\t\techo \"This text is smudged.\" >a.txt &&\n> +\t\tgit add a.txt &&\n> +\t\ttest_commit fourth &&\n> +\t\tgit rebase master &&\n> +\t\tgrep \"smudged\" a.txt\n> +\t)\n> +'\n> +\n> test_expect_success 'rebase--merge.sh and --show-current-patch' '\n> \ttest_create_repo conflict-merge &&\n> \t(\n"},{"id":"380425","messageId":"20190813224834.GB229582@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"xmqq36i47ku3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-13T22:48:34Z","receivedAt":"2019-08-13T22:49:02Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-13 at 22:37:56, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > +test_expect_success 'rebase --am and .gitattributes' '\n> > +\ttest_create_repo attributes &&\n> > +\t(\n> > +\t\tcd attributes &&\n> > +\t\ttest_commit init &&\n> > +\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n> > +\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n> \n> These eventually invokes test-when-finished for cleaning things up,\n> that cannot be called inside a subshell. The \"attributes\" test\n> repository is a throw-away reopsitory, so we should be able to just\n> use \"git config\" to set the variables locally in it, no?\n\nYup, that's safe. I'll reroll with that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380523","messageId":"xmqqftm22i7e.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"xmqq7e7h7xap.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-15T22:10:29Z","receivedAt":"2019-08-15T22:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>> When applying multiple patches with git am, or when rebasing using the\n>> am backend, it's possible that one of our patches has updated a\n>> gitattributes file. Currently, we cache this information, so if a\n>> file in a subsequent patch has attributes applied, the file will be\n>> written out with the attributes in place as of the time we started the\n>> rebase or am operation, not with the attributes applied by the previous\n>> patch. This problem does not occur when using the -m or -i flags to\n>> rebase.\n> ...\n> \"rebase -m\" and \"rebase -i\" are not repeated run_command() calls\n> that invoke \"git cherry-pick\" or \"git merge\" these days, either, so\n> I am somewhat curious how they avoid fallilng into the same trap.\n>\n> Thanks for the fix. Will queue.\n\nActually there still is one more thing I wasn't clear about the\nchange.\n\n> To ensure we write the correct data into the working tree, expire the\n> cache after each patch that touches a path ending in \".gitattributes\".\n> ...\n> +\t\t\tif (!flush_attributes && patch->new_name &&\n> +\t\t\t ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))\n> +\t\t\t\tflush_attributes = 1;\n\nWhen an attribute file is removed by a patch, we should forget what\nwe read earlier from the file before it got removed. Would such a\ncase, where patch->new_name would be NULL, be handled correctly?\n\nThe call to ends_with_path_components() is almost no cost, and I\nwould suspect that this call is easier to reason about without the\n\"!flush_attributes &&\" in the conditional part, by the way.\n\nThanks.\n"},{"id":"380530","messageId":"20190816002253.GA365197@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"xmqqftm22i7e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-16T00:22:53Z","receivedAt":"2019-08-16T00:23:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-15 at 22:10:29, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > \"brian m. carlson\" writes:\n> >\n> >> When applying multiple patches with git am, or when rebasing using the\n> >> am backend, it's possible that one of our patches has updated a\n> >> gitattributes file. Currently, we cache this information, so if a\n> >> file in a subsequent patch has attributes applied, the file will be\n> >> written out with the attributes in place as of the time we started the\n> >> rebase or am operation, not with the attributes applied by the previous\n> >> patch. This problem does not occur when using the -m or -i flags to\n> >> rebase.\n> > ...\n> > \"rebase -m\" and \"rebase -i\" are not repeated run_command() calls\n> > that invoke \"git cherry-pick\" or \"git merge\" these days, either, so\n> > I am somewhat curious how they avoid fallilng into the same trap.\n> >\n> > Thanks for the fix. Will queue.\n> \n> Actually there still is one more thing I wasn't clear about the\n> change.\n> \n> > To ensure we write the correct data into the working tree, expire the\n> > cache after each patch that touches a path ending in \".gitattributes\".\n> > ...\n> > +\t\t\tif (!flush_attributes && patch->new_name &&\n> > +\t\t\t ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))\n> > +\t\t\t\tflush_attributes = 1;\n> \n> When an attribute file is removed by a patch, we should forget what\n> we read earlier from the file before it got removed. Would such a\n> case, where patch->new_name would be NULL, be handled correctly?\n\nThat's a good question. I don't think we handle that case, so I'll\ninclude that (and an appropriate test) with my reroll.\n\nI suspect we probably want something like:\n\n if ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n\nThis should cause at most one flush per patch, and I think this should\ncover both cases, as well as any we haven't thought of. It's also\npossible that we could get a copy or rename that causes a false\npositive, but considering the contents of a .gitattributes file, that\nseems unlikely enough in practice that it's probably not worth worrying\nabout for now.\n\n> The call to ends_with_path_components() is almost no cost, and I\n> would suspect that this call is easier to reason about without the\n> \"!flush_attributes &&\" in the conditional part, by the way.\n\nYeah, it probably is. It was added to avoid the allocation, but now that\nwe don't have one, it shouldn't be a problem.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380630","messageId":"20190818184403.861907-1-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190809100217.427178-1-sandals@crustytoothpaste.net","subject":"[PATCH v4 0/2] Honor .gitattributes with rebase --am","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-18T18:44:01Z","receivedAt":"2019-08-18T18:44:19Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series makes rebase --am honor the .gitattributes file for\nsubsequent patches when a patch changes it.\n\nChanges from v3:\n* Check for both addition and removal of .gitattributes files.\n* Switch from \"test_config\" to \"git config\".\n\nChanges from v2:\n* Rename has_path_suffix to ends_with_path_components.\n\nChanges from v1:\n* Add has_path_suffix in a separate commit.\n\nbrian m. carlson (2):\n path: add a function to check for path suffix\n apply: reload .gitattributes after patching it\n\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 90 insertions(+), 10 deletions(-)\n\nRange-diff against v3:\n1: 5f4402b38d ! 1: fa825e4b40 apply: reload .gitattributes after patching it\n @@ apply.c: static int apply_patch(struct apply_state *state,\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n +\n -+\t\t\tif (!flush_attributes && patch->new_name &&\n -+\t\t\t ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))\n ++\t\t\tif ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n ++\t\t\t (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n +\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' '\n +\t(\n +\t\tcd attributes &&\n +\t\ttest_commit init &&\n -+\t\ttest_config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n -+\t\ttest_config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n ++\t\tgit config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n ++\t\tgit config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n +\n +\t\ttest_commit second &&\n +\t\tgit checkout -b test HEAD^ &&\n @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' '\n +\t\techo \"This text is smudged.\" >a.txt &&\n +\t\tgit add a.txt &&\n +\t\ttest_commit fourth &&\n ++\n ++\t\tgit checkout -b removal HEAD^ &&\n ++\t\tgit rm .gitattributes &&\n ++\t\tgit add -u &&\n ++\t\ttest_commit fifth &&\n ++\t\tgit cherry-pick test &&\n ++\n ++\t\tgit checkout test &&\n +\t\tgit rebase master &&\n -+\t\tgrep \"smudged\" a.txt\n ++\t\tgrep \"smudged\" a.txt &&\n ++\n ++\t\tgit checkout removal &&\n ++\t\tgit reset --hard &&\n ++\t\tgit rebase master &&\n ++\t\tgrep \"clean\" a.txt\n +\t)\n +'\n +\n"},{"id":"380631","messageId":"20190818184403.861907-2-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190818184403.861907-1-sandals@crustytoothpaste.net","subject":"[PATCH v4 1/2] path: add a function to check for path suffix","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-18T18:44:02Z","receivedAt":"2019-08-18T18:44:20Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We have a function to strip the path suffix from a commit, but we don't\nhave one to check for a path suffix. For a plain filename, we can use\nbasename, but that requires an allocation, since POSIX allows it to\nmodify its argument. Refactor strip_path_suffix into a helper function\nand a new function, ends_with_path_components, to meet this need.\n\nSigned-off-by: brian m. carlson \n---\n path.c | 39 ++++++++++++++++++++++++++++++---------\n path.h | 3 +++\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 25e97b8c3f..e3da1f3c4e 100644\n--- a/path.c\n+++ b/path.c\n@@ -1221,31 +1221,52 @@ static inline int chomp_trailing_dir_sep(const char *path, int len)\n }\n \n /*\n- * If path ends with suffix (complete path components), returns the\n- * part before suffix (sans trailing directory separators).\n- * Otherwise returns NULL.\n+ * If path ends with suffix (complete path components), returns the offset of\n+ * the last character in the path before the suffix (sans trailing directory\n+ * separators), and -1 otherwise.\n */\n-char *strip_path_suffix(const char *path, const char *suffix)\n+static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)\n {\n \tint path_len = strlen(path), suffix_len = strlen(suffix);\n \n \twhile (suffix_len) {\n \t\tif (!path_len)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n \t\tif (is_dir_sep(path[path_len - 1])) {\n \t\t\tif (!is_dir_sep(suffix[suffix_len - 1]))\n-\t\t\t\treturn NULL;\n+\t\t\t\treturn -1;\n \t\t\tpath_len = chomp_trailing_dir_sep(path, path_len);\n \t\t\tsuffix_len = chomp_trailing_dir_sep(suffix, suffix_len);\n \t\t}\n \t\telse if (path[--path_len] != suffix[--suffix_len])\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \t}\n \n \tif (path_len && !is_dir_sep(path[path_len - 1]))\n-\t\treturn NULL;\n-\treturn xstrndup(path, chomp_trailing_dir_sep(path, path_len));\n+\t\treturn -1;\n+\treturn chomp_trailing_dir_sep(path, path_len);\n+}\n+\n+/*\n+ * Returns true if the path ends with components, considering only complete path\n+ * components, and false otherwise.\n+ */\n+int ends_with_path_components(const char *path, const char *components)\n+{\n+\treturn stripped_path_suffix_offset(path, components) != -1;\n+}\n+\n+/*\n+ * If path ends with suffix (complete path components), returns the\n+ * part before suffix (sans trailing directory separators).\n+ * Otherwise returns NULL.\n+ */\n+char *strip_path_suffix(const char *path, const char *suffix)\n+{\n+\tssize_t offset = stripped_path_suffix_offset(path, suffix);\n+\n+\treturn offset == -1 ? NULL : xstrndup(path, offset);\n }\n \n int daemon_avoid_alias(const char *p)\ndiff --git a/path.h b/path.h\nindex 2ba6ca58c8..14d6dcad16 100644\n--- a/path.h\n+++ b/path.h\n@@ -193,4 +193,7 @@ const char *git_path_merge_head(struct repository *r);\n const char *git_path_fetch_head(struct repository *r);\n const char *git_path_shallow(struct repository *r);\n \n+\n+int ends_with_path_components(const char *path, const char *components);\n+\n #endif /* PATH_H */\n"},{"id":"380632","messageId":"20190818184403.861907-3-sandals@crustytoothpaste.net","threadId":"51612","inReplyTo":"20190818184403.861907-1-sandals@crustytoothpaste.net","subject":"[PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-18T18:44:03Z","receivedAt":"2019-08-18T18:44:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"When applying multiple patches with git am, or when rebasing using the\nam backend, it's possible that one of our patches has updated a\ngitattributes file. Currently, we cache this information, so if a\nfile in a subsequent patch has attributes applied, the file will be\nwritten out with the attributes in place as of the time we started the\nrebase or am operation, not with the attributes applied by the previous\npatch. This problem does not occur when using the -m or -i flags to\nrebase.\n\nTo ensure we write the correct data into the working tree, expire the\ncache after each patch that touches a path ending in \".gitattributes\".\n\nSigned-off-by: brian m. carlson \n---\n apply.c | 7 +++++++\n convert.c | 9 ++++++++-\n convert.h | 6 ++++++\n t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 57 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex cde95369bb..d57bc635e4 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \tint res = 0;\n+\tint flush_attributes = 0;\n \n \tstate->patch_input_file = filename;\n \tif (read_patch_file(&buf, fd) < 0)\n@@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n \t\t\tpatch_stats(state, patch);\n \t\t\t*listp = patch;\n \t\t\tlistp = &patch->next;\n+\n+\t\t\tif ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n+\t\t\t (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n+\t\t\t\tflush_attributes = 1;\n \t\t}\n \t\telse {\n \t\t\tif (state->apply_verbosity > verbosity_normal)\n@@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,\n \tif (state->summary && state->apply_verbosity > verbosity_silent)\n \t\tsummary_patch_list(list);\n \n+\tif (flush_attributes)\n+\t\treset_parsed_attributes();\n end:\n \tfree_patch_list(list);\n \tstrbuf_release(&buf);\ndiff --git a/convert.c b/convert.c\nindex 94ff837649..030e9b81b9 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1293,10 +1293,11 @@ struct conv_attrs {\n \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n };\n \n+static struct attr_check *check;\n+\n static void convert_attrs(const struct index_state *istate,\n \t\t\t struct conv_attrs *ca, const char *path)\n {\n-\tstatic struct attr_check *check;\n \tstruct attr_check_item *ccheck = NULL;\n \n \tif (!check) {\n@@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n \t\tca->crlf_action = CRLF_AUTO_INPUT;\n }\n \n+void reset_parsed_attributes(void)\n+{\n+\tattr_check_free(check);\n+\tcheck = NULL;\n+}\n+\n int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n {\n \tstruct conv_attrs ca;\ndiff --git a/convert.h b/convert.h\nindex 831559f10d..3710969d43 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n int would_convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t\t const char *path);\n \n+/*\n+ * Reset the internal list of attributes used by convert_to_git and\n+ * convert_to_working_tree.\n+ */\n+void reset_parsed_attributes(void);\n+\n /*****************************************************************\n *\n * Streaming conversion support\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 80b23fd326..23469cc789 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -301,6 +301,42 @@ test_expect_success 'rebase --am and --show-current-patch' '\n \t)\n '\n \n+test_expect_success 'rebase --am and .gitattributes' '\n+\ttest_create_repo attributes &&\n+\t(\n+\t\tcd attributes &&\n+\t\ttest_commit init &&\n+\t\tgit config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n+\t\tgit config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n+\n+\t\ttest_commit second &&\n+\t\tgit checkout -b test HEAD^ &&\n+\n+\t\techo \"*.txt filter=test\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\ttest_commit third &&\n+\n+\t\techo \"This text is smudged.\" >a.txt &&\n+\t\tgit add a.txt &&\n+\t\ttest_commit fourth &&\n+\n+\t\tgit checkout -b removal HEAD^ &&\n+\t\tgit rm .gitattributes &&\n+\t\tgit add -u &&\n+\t\ttest_commit fifth &&\n+\t\tgit cherry-pick test &&\n+\n+\t\tgit checkout test &&\n+\t\tgit rebase master &&\n+\t\tgrep \"smudged\" a.txt &&\n+\n+\t\tgit checkout removal &&\n+\t\tgit reset --hard &&\n+\t\tgit rebase master &&\n+\t\tgrep \"clean\" a.txt\n+\t)\n+'\n+\n test_expect_success 'rebase--merge.sh and --show-current-patch' '\n \ttest_create_repo conflict-merge &&\n \t(\n"},{"id":"380697","messageId":"9b940950-666a-0c4c-58c0-1e61ac9e654c@gmail.com","threadId":"51612","inReplyTo":"20190818184403.861907-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-19T09:41:42Z","receivedAt":"2019-08-19T09:41:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Brian\n\nOn 18/08/2019 19:44, brian m. carlson wrote:\n> When applying multiple patches with git am, or when rebasing using the\n> am backend, it's possible that one of our patches has updated a\n> gitattributes file. Currently, we cache this information, so if a\n> file in a subsequent patch has attributes applied, the file will be\n> written out with the attributes in place as of the time we started the\n> rebase or am operation, not with the attributes applied by the previous\n> patch. This problem does not occur when using the -m or -i flags to\n> rebase.\n\nDo you know why -m and -i aren't affected?\n\n> To ensure we write the correct data into the working tree, expire the\n> cache after each patch that touches a path ending in \".gitattributes\".\n> \n> Signed-off-by: brian m. carlson \n> ---\n> apply.c | 7 +++++++\n> convert.c | 9 ++++++++-\n> convert.h | 6 ++++++\n> t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++++++\n> 4 files changed, 57 insertions(+), 1 deletion(-)\n> \n> diff --git a/apply.c b/apply.c\n> index cde95369bb..d57bc635e4 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n> \tstruct patch *list = NULL, **listp = &list;\n> \tint skipped_patch = 0;\n> \tint res = 0;\n> +\tint flush_attributes = 0;\n> \n> \tstate->patch_input_file = filename;\n> \tif (read_patch_file(&buf, fd) < 0)\n> @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n> \t\t\tpatch_stats(state, patch);\n> \t\t\t*listp = patch;\n> \t\t\tlistp = &patch->next;\n> +\n> +\t\t\tif ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n> +\t\t\t (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n> +\t\t\t\tflush_attributes = 1;\n\nstyle nit - these lines are very long compared to 80 characters\n\n> \t\t}\n> \t\telse {\n> \t\t\tif (state->apply_verbosity > verbosity_normal)\n> @@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,\n> \tif (state->summary && state->apply_verbosity > verbosity_silent)\n> \t\tsummary_patch_list(list);\n> \n> +\tif (flush_attributes)\n> +\t\treset_parsed_attributes();\n> end:\n> \tfree_patch_list(list);\n> \tstrbuf_release(&buf);\n> diff --git a/convert.c b/convert.c\n> index 94ff837649..030e9b81b9 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -1293,10 +1293,11 @@ struct conv_attrs {\n> \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n> };\n> \n> +static struct attr_check *check;\n\nI was concerned about the impact adding a file global if we ever want to \nmulti-thread this for submodules, but looking through the file there are \na couple of others already so this isn't creating a new problem.\n\nBest Wishes\n\nPhillip\n\n> +\n> static void convert_attrs(const struct index_state *istate,\n> \t\t\t struct conv_attrs *ca, const char *path)\n> {\n> -\tstatic struct attr_check *check;\n> \tstruct attr_check_item *ccheck = NULL;\n> \n> \tif (!check) {\n> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n> \t\tca->crlf_action = CRLF_AUTO_INPUT;\n> }\n> \n> +void reset_parsed_attributes(void)\n> +{\n> +\tattr_check_free(check);\n> +\tcheck = NULL;\n> +}\n> +\n> int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)\n> {\n> \tstruct conv_attrs ca;\n> diff --git a/convert.h b/convert.h\n> index 831559f10d..3710969d43 100644\n> --- a/convert.h\n> +++ b/convert.h\n> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n> int would_convert_to_git_filter_fd(const struct index_state *istate,\n> \t\t\t\t const char *path);\n> \n> +/*\n> + * Reset the internal list of attributes used by convert_to_git and\n> + * convert_to_working_tree.\n> + */\n> +void reset_parsed_attributes(void);\n> +\n> /*****************************************************************\n> *\n> * Streaming conversion support\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 80b23fd326..23469cc789 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -301,6 +301,42 @@ test_expect_success 'rebase --am and --show-current-patch' '\n> \t)\n> '\n> \n> +test_expect_success 'rebase --am and .gitattributes' '\n> +\ttest_create_repo attributes &&\n> +\t(\n> +\t\tcd attributes &&\n> +\t\ttest_commit init &&\n> +\t\tgit config filter.test.clean \"sed -e '\\''s/smudged/clean/g'\\''\" &&\n> +\t\tgit config filter.test.smudge \"sed -e '\\''s/clean/smudged/g'\\''\" &&\n> +\n> +\t\ttest_commit second &&\n> +\t\tgit checkout -b test HEAD^ &&\n> +\n> +\t\techo \"*.txt filter=test\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\ttest_commit third &&\n> +\n> +\t\techo \"This text is smudged.\" >a.txt &&\n> +\t\tgit add a.txt &&\n> +\t\ttest_commit fourth &&\n> +\n> +\t\tgit checkout -b removal HEAD^ &&\n> +\t\tgit rm .gitattributes &&\n> +\t\tgit add -u &&\n> +\t\ttest_commit fifth &&\n> +\t\tgit cherry-pick test &&\n> +\n> +\t\tgit checkout test &&\n> +\t\tgit rebase master &&\n> +\t\tgrep \"smudged\" a.txt &&\n> +\n> +\t\tgit checkout removal &&\n> +\t\tgit reset --hard &&\n> +\t\tgit rebase master &&\n> +\t\tgrep \"clean\" a.txt\n> +\t)\n> +'\n> +\n> test_expect_success 'rebase--merge.sh and --show-current-patch' '\n> \ttest_create_repo conflict-merge &&\n> \t(\n> \n"},{"id":"380698","messageId":"2c33a668-4c7d-2114-41b1-8907c4384a15@gmail.com","threadId":"51612","inReplyTo":"9b940950-666a-0c4c-58c0-1e61ac9e654c@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-19T09:55:27Z","receivedAt":"2019-08-19T09:55:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 19/08/2019 10:41, Phillip Wood wrote:\n> [...]\n>> diff --git a/convert.c b/convert.c\n>> index 94ff837649..030e9b81b9 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -1293,10 +1293,11 @@ struct conv_attrs {\n>>       const char *working_tree_encoding; /* Supported encoding or \n>> default encoding if NULL */\n>>   };\n>> +static struct attr_check *check;\n> \n> I was concerned about the impact adding a file global if we ever want to \n> multi-thread this for submodules, but looking through the file there are \n> a couple of others already so this isn't creating a new problem.\n\nDoh, I've just realized it was static already - ignore that.\n\nOne thing did occur to me though - does this patch reset attributes like \nthe merge marker length (they're less critical though if there is a \nconflict after an attribute change it would be nice to have the correct \nlength) or just the ones for filtering files?\n\nBest Wishes\n\nPhillip\n\n> \n> Best Wishes\n> \n> Phillip\n> \n>> +\n>>   static void convert_attrs(const struct index_state *istate,\n>>                 struct conv_attrs *ca, const char *path)\n>>   {\n>> -    static struct attr_check *check;\n>>       struct attr_check_item *ccheck = NULL;\n>>       if (!check) {\n>> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct \n>> index_state *istate,\n>>           ca->crlf_action = CRLF_AUTO_INPUT;\n>>   }\n>> +void reset_parsed_attributes(void)\n>> +{\n>> +    attr_check_free(check);\n>> +    check = NULL;\n>> +}\n>> +\n>>   int would_convert_to_git_filter_fd(const struct index_state *istate, \n>> const char *path)\n>>   {\n>>       struct conv_attrs ca;\n>> diff --git a/convert.h b/convert.h\n>> index 831559f10d..3710969d43 100644\n>> --- a/convert.h\n>> +++ b/convert.h\n>> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct \n>> index_state *istate,\n>>   int would_convert_to_git_filter_fd(const struct index_state *istate,\n>>                      const char *path);\n>> +/*\n>> + * Reset the internal list of attributes used by convert_to_git and\n>> + * convert_to_working_tree.\n>> + */\n>> +void reset_parsed_attributes(void);\n>> +\n>>   /*****************************************************************\n>>    *\n>>    * Streaming conversion support\n>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n>> index 80b23fd326..23469cc789 100755\n>> --- a/t/t3400-rebase.sh\n>> +++ b/t/t3400-rebase.sh\n>> @@ -301,6 +301,42 @@ test_expect_success 'rebase --am and \n>> --show-current-patch' '\n>>       )\n>>   '\n>> +test_expect_success 'rebase --am and .gitattributes' '\n>> +    test_create_repo attributes &&\n>> +    (\n>> +        cd attributes &&\n>> +        test_commit init &&\n>> +        git config filter.test.clean \"sed -e \n>> '\\''s/smudged/clean/g'\\''\" &&\n>> +        git config filter.test.smudge \"sed -e \n>> '\\''s/clean/smudged/g'\\''\" &&\n>> +\n>> +        test_commit second &&\n>> +        git checkout -b test HEAD^ &&\n>> +\n>> +        echo \"*.txt filter=test\" >.gitattributes &&\n>> +        git add .gitattributes &&\n>> +        test_commit third &&\n>> +\n>> +        echo \"This text is smudged.\" >a.txt &&\n>> +        git add a.txt &&\n>> +        test_commit fourth &&\n>> +\n>> +        git checkout -b removal HEAD^ &&\n>> +        git rm .gitattributes &&\n>> +        git add -u &&\n>> +        test_commit fifth &&\n>> +        git cherry-pick test &&\n>> +\n>> +        git checkout test &&\n>> +        git rebase master &&\n>> +        grep \"smudged\" a.txt &&\n>> +\n>> +        git checkout removal &&\n>> +        git reset --hard &&\n>> +        git rebase master &&\n>> +        grep \"clean\" a.txt\n>> +    )\n>> +'\n>> +\n>>   test_expect_success 'rebase--merge.sh and --show-current-patch' '\n>>       test_create_repo conflict-merge &&\n>>       (\n>>\n"},{"id":"380757","messageId":"20190820024505.GH365197@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"9b940950-666a-0c4c-58c0-1e61ac9e654c@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-20T02:45:05Z","receivedAt":"2019-08-20T02:45:15Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-19 at 09:41:42, Phillip Wood wrote:\n> Hi Brian\n> \n> On 18/08/2019 19:44, brian m. carlson wrote:\n> > When applying multiple patches with git am, or when rebasing using the\n> > am backend, it's possible that one of our patches has updated a\n> > gitattributes file. Currently, we cache this information, so if a\n> > file in a subsequent patch has attributes applied, the file will be\n> > written out with the attributes in place as of the time we started the\n> > rebase or am operation, not with the attributes applied by the previous\n> > patch. This problem does not occur when using the -m or -i flags to\n> > rebase.\n> \n> Do you know why -m and -i aren't affected?\n\nI had to look, but I believe the answer is because they use the\nsequencer, and the sequencer calls git merge-recursive as a separate\nprocess, and so the writing of the tree is only done in a subprocess,\nwhich can't persist state.\n\nShould we move the merge-recursive code into the main process, we'll\nlikely have the same problem there.\n\n> > diff --git a/apply.c b/apply.c\n> > index cde95369bb..d57bc635e4 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n> > \tstruct patch *list = NULL, **listp = &list;\n> > \tint skipped_patch = 0;\n> > \tint res = 0;\n> > +\tint flush_attributes = 0;\n> > \tstate->patch_input_file = filename;\n> > \tif (read_patch_file(&buf, fd) < 0)\n> > @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n> > \t\t\tpatch_stats(state, patch);\n> > \t\t\t*listp = patch;\n> > \t\t\tlistp = &patch->next;\n> > +\n> > +\t\t\tif ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n> > +\t\t\t (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n> > +\t\t\t\tflush_attributes = 1;\n> \n> style nit - these lines are very long compared to 80 characters\n\nThey are. They aren't two different from other lines in the file, and I\nthought that leaving them that way would preserve the similarities, but\nI can also wrap them. I'll send out a v5 with wrapped lines.\n\n> > diff --git a/convert.c b/convert.c\n> > index 94ff837649..030e9b81b9 100644\n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -1293,10 +1293,11 @@ struct conv_attrs {\n> > \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n> > };\n> > +static struct attr_check *check;\n> \n> I was concerned about the impact adding a file global if we ever want to\n> multi-thread this for submodules, but looking through the file there are a\n> couple of others already so this isn't creating a new problem.\n> > +\n> > static void convert_attrs(const struct index_state *istate,\n> > \t\t\t struct conv_attrs *ca, const char *path)\n> > {\n> > -\tstatic struct attr_check *check;\n> > \tstruct attr_check_item *ccheck = NULL;\n> > \tif (!check) {\n> > @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n> > \t\tca->crlf_action = CRLF_AUTO_INPUT;\n> > }\n\nThe portion I've included above demonstrates that this was already a\nstatic variable, just one hidden in a function. So yeah, this is no\nworse than it already was.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380759","messageId":"20190820030544.GI365197@genre.crustytoothpaste.net","threadId":"51612","inReplyTo":"2c33a668-4c7d-2114-41b1-8907c4384a15@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-08-20T03:05:44Z","receivedAt":"2019-08-20T03:05:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-08-19 at 09:55:27, Phillip Wood wrote:\n> On 19/08/2019 10:41, Phillip Wood wrote:\n> > [...]\n> > > diff --git a/convert.c b/convert.c\n> > > index 94ff837649..030e9b81b9 100644\n> > > --- a/convert.c\n> > > +++ b/convert.c\n> > > @@ -1293,10 +1293,11 @@ struct conv_attrs {\n> > >       const char *working_tree_encoding; /* Supported encoding or\n> > > default encoding if NULL */\n> > >   };\n> > > +static struct attr_check *check;\n> > \n> > I was concerned about the impact adding a file global if we ever want to\n> > multi-thread this for submodules, but looking through the file there are\n> > a couple of others already so this isn't creating a new problem.\n> \n> Doh, I've just realized it was static already - ignore that.\n\nAnd I just realized that I didn't read the entire thread before\nresponding. Sorry about that.\n\n> One thing did occur to me though - does this patch reset attributes like the\n> merge marker length (they're less critical though if there is a conflict\n> after an attribute change it would be nice to have the correct length) or\n> just the ones for filtering files?\n\nIt resets \"crlf\", \"ident\", \"filter\", \"eol\", \"text\", and\n\"working-tree-encoding\". Things it doesn't reset include \"whitespace\",\n\"export-ignore\", \"export-subst\", \"merge\", and \"conflict-marker-size\".\nOf these, I think only the latter two are relevant.\n\nI'll update that in v5.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"380791","messageId":"18fcc7db-7c09-3fbf-1e3f-81be99f4bb17@gmail.com","threadId":"51612","inReplyTo":"20190820024505.GH365197@genre.crustytoothpaste.net","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-20T08:52:48Z","receivedAt":"2019-08-20T08:52:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Brian\n\nOn 20/08/2019 03:45, brian m. carlson wrote:\n> On 2019-08-19 at 09:41:42, Phillip Wood wrote:\n>> Hi Brian\n>>\n>> On 18/08/2019 19:44, brian m. carlson wrote:\n>>> When applying multiple patches with git am, or when rebasing using the\n>>> am backend, it's possible that one of our patches has updated a\n>>> gitattributes file. Currently, we cache this information, so if a\n>>> file in a subsequent patch has attributes applied, the file will be\n>>> written out with the attributes in place as of the time we started the\n>>> rebase or am operation, not with the attributes applied by the previous\n>>> patch. This problem does not occur when using the -m or -i flags to\n>>> rebase.\n>>\n>> Do you know why -m and -i aren't affected?\n> \n> I had to look, but I believe the answer is because they use the\n> sequencer, and the sequencer calls git merge-recursive as a separate\n> process, and so the writing of the tree is only done in a subprocess,\n> which can't persist state.\n\nThe sequencer has been running in a single process for a while now. We \ndo fork for 'git merge' sometimes when processing 'merge' commands but \n'pick' commands are all done in a single process by calling \ndo_recursive_merge().\n\nBest Wishes\n\nPhillip\n\n> Should we move the merge-recursive code into the main process, we'll\n> likely have the same problem there.\n> \n>>> diff --git a/apply.c b/apply.c\n>>> index cde95369bb..d57bc635e4 100644\n>>> --- a/apply.c\n>>> +++ b/apply.c\n>>> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,\n>>> \tstruct patch *list = NULL, **listp = &list;\n>>> \tint skipped_patch = 0;\n>>> \tint res = 0;\n>>> +\tint flush_attributes = 0;\n>>> \tstate->patch_input_file = filename;\n>>> \tif (read_patch_file(&buf, fd) < 0)\n>>> @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,\n>>> \t\t\tpatch_stats(state, patch);\n>>> \t\t\t*listp = patch;\n>>> \t\t\tlistp = &patch->next;\n>>> +\n>>> +\t\t\tif ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||\n>>> +\t\t\t (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))\n>>> +\t\t\t\tflush_attributes = 1;\n>>\n>> style nit - these lines are very long compared to 80 characters\n> \n> They are. They aren't two different from other lines in the file, and I\n> thought that leaving them that way would preserve the similarities, but\n> I can also wrap them. I'll send out a v5 with wrapped lines.\n> \n>>> diff --git a/convert.c b/convert.c\n>>> index 94ff837649..030e9b81b9 100644\n>>> --- a/convert.c\n>>> +++ b/convert.c\n>>> @@ -1293,10 +1293,11 @@ struct conv_attrs {\n>>> \tconst char *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n>>> };\n>>> +static struct attr_check *check;\n>>\n>> I was concerned about the impact adding a file global if we ever want to\n>> multi-thread this for submodules, but looking through the file there are a\n>> couple of others already so this isn't creating a new problem.\n>>> +\n>>> static void convert_attrs(const struct index_state *istate,\n>>> \t\t\t struct conv_attrs *ca, const char *path)\n>>> {\n>>> -\tstatic struct attr_check *check;\n>>> \tstruct attr_check_item *ccheck = NULL;\n>>> \tif (!check) {\n>>> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,\n>>> \t\tca->crlf_action = CRLF_AUTO_INPUT;\n>>> }\n> \n> The portion I've included above demonstrates that this was already a\n> static variable, just one hidden in a function. So yeah, this is no\n> worse than it already was.\n> \n"},{"id":"380792","messageId":"facdfb93-bbf5-cca7-7970-b68efed7cfd3@gmail.com","threadId":"51612","inReplyTo":"20190820030544.GI365197@genre.crustytoothpaste.net","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-20T08:56:26Z","receivedAt":"2019-08-20T08:56:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/08/2019 04:05, brian m. carlson wrote:\n> On 2019-08-19 at 09:55:27, Phillip Wood wrote:\n>> On 19/08/2019 10:41, Phillip Wood wrote:\n>>> [...]\n>>>> diff --git a/convert.c b/convert.c\n>>>> index 94ff837649..030e9b81b9 100644\n>>>> --- a/convert.c\n>>>> +++ b/convert.c\n>>>> @@ -1293,10 +1293,11 @@ struct conv_attrs {\n>>>>       const char *working_tree_encoding; /* Supported encoding or\n>>>> default encoding if NULL */\n>>>>   };\n>>>> +static struct attr_check *check;\n>>>\n>>> I was concerned about the impact adding a file global if we ever want to\n>>> multi-thread this for submodules, but looking through the file there are\n>>> a couple of others already so this isn't creating a new problem.\n>>\n>> Doh, I've just realized it was static already - ignore that.\n> \n> And I just realized that I didn't read the entire thread before\n> responding. Sorry about that.\n> \n>> One thing did occur to me though - does this patch reset attributes like the\n>> merge marker length (they're less critical though if there is a conflict\n>> after an attribute change it would be nice to have the correct length) or\n>> just the ones for filtering files?\n> \n> It resets \"crlf\", \"ident\", \"filter\", \"eol\", \"text\", and\n> \"working-tree-encoding\". Things it doesn't reset include \"whitespace\",\n> \"export-ignore\", \"export-subst\", \"merge\", and \"conflict-marker-size\".\n> Of these, I think only the latter two are relevant.\n> \n> I'll update that in v5.\n\nThanks, one other thought I had was that this is really a bug in 'am' \nrather than 'rebase'. There has been some talk of switching the default \nrabase backend to the sequencer so maybe it would be better to test 'am' \nrather than 'rebase' to ensure we still check 'am' is working properly \nafter any switch in the rebase backend. (perhaps we should be testing \nrebase and rebase --interactive separately as well to prevent any future \nregressions I'm not sure)\n\nBest Wishes\n\nPhillip\n\n"},{"id":"380826","messageId":"xmqqd0gzvgo3.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"18fcc7db-7c09-3fbf-1e3f-81be99f4bb17@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T18:24:28Z","receivedAt":"2019-08-20T18:24:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> Do you know why -m and -i aren't affected?\n>>\n>> I had to look, but I believe the answer is because they use the\n>> sequencer, and the sequencer calls git merge-recursive as a separate\n>> process, and so the writing of the tree is only done in a subprocess,\n>> which can't persist state.\n>\n> The sequencer has been running in a single process for a while now. We\n> do fork for 'git merge' sometimes when processing 'merge' commands but\n> 'pick' commands are all done in a single process by calling\n> do_recursive_merge().\n>\n> Best Wishes\n>\n> Phillip\n>\n>> Should we move the merge-recursive code into the main process, we'll\n>> likely have the same problem there.\n\nSo we actually have the same issue already?\n"},{"id":"380828","messageId":"d59de3db-13e6-35d5-2cb1-b38dc8854c60@gmail.com","threadId":"51612","inReplyTo":"xmqqd0gzvgo3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-20T18:32:02Z","receivedAt":"2019-08-20T18:32:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/08/2019 19:24, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>>> Do you know why -m and -i aren't affected?\n>>>\n>>> I had to look, but I believe the answer is because they use the\n>>> sequencer, and the sequencer calls git merge-recursive as a separate\n>>> process, and so the writing of the tree is only done in a subprocess,\n>>> which can't persist state.\n>>\n>> The sequencer has been running in a single process for a while now. We\n>> do fork for 'git merge' sometimes when processing 'merge' commands but\n>> 'pick' commands are all done in a single process by calling\n>> do_recursive_merge().\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>> Should we move the merge-recursive code into the main process, we'll\n>>> likely have the same problem there.\n> \n> So we actually have the same issue already?\n\nI don't think so, I modified Brian's test to call 'rebase -i' and it \npasses but no one seems to know why.\n\nBest Wishes\n\nPhillip\n"},{"id":"381187","messageId":"e8196c6e-7f3c-1b29-73ba-40094f1e0280@gmail.com","threadId":"51612","inReplyTo":"d59de3db-13e6-35d5-2cb1-b38dc8854c60@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-26T15:09:20Z","receivedAt":"2019-08-26T15:09:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/08/2019 19:32, Phillip Wood wrote:\n> On 20/08/2019 19:24, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>>\n>>>>> Do you know why -m and -i aren't affected?\n>>>>\n>>>> I had to look, but I believe the answer is because they use the\n>>>> sequencer, and the sequencer calls git merge-recursive as a separate\n>>>> process, and so the writing of the tree is only done in a subprocess,\n>>>> which can't persist state.\n>>>\n>>> The sequencer has been running in a single process for a while now. We\n>>> do fork for 'git merge' sometimes when processing 'merge' commands but\n>>> 'pick' commands are all done in a single process by calling\n>>> do_recursive_merge().\n>>>\n>>> Best Wishes\n>>>\n>>> Phillip\n>>>\n>>>> Should we move the merge-recursive code into the main process, we'll\n>>>> likely have the same problem there.\n>>\n>> So we actually have the same issue already?\n> \n> I don't think so, I modified Brian's test to call 'rebase -i' and it \n> passes but no one seems to know why.\n\nI spent some time digging into this and the attributes are reloaded \nbefore each pick. This is because check_updates() called by \nunpack_trees() calls git_attr_set_direction(GIT_ATTR_CHECKOUT) at the \nstart of the function and git_attr_set_direction(GIT_ATTR_CHECKIN) at \nthe end of the function. This has the effect of dropping all loaded \nattributes as git_attr_set_direction() calls drop_all_attr_stacks() when \nthe direction is changed.\n\nBest Wishes\n\nPhillip\n\n> Best Wishes\n> \n> Phillip\n"},{"id":"381209","messageId":"xmqqk1azj2ux.fsf@gitster-ct.c.googlers.com","threadId":"51612","inReplyTo":"e8196c6e-7f3c-1b29-73ba-40094f1e0280@gmail.com","subject":"Re: [PATCH v4 2/2] apply: reload .gitattributes after patching it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T16:41:42Z","receivedAt":"2019-08-26T16:41:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I spent some time digging into this and the attributes are reloaded\n> before each pick. This is because check_updates() called by\n> unpack_trees() calls git_attr_set_direction(GIT_ATTR_CHECKOUT) at the\n> start of the function and git_attr_set_direction(GIT_ATTR_CHECKIN) at\n> the end of the function. This has the effect of dropping all loaded\n> attributes as git_attr_set_direction() calls drop_all_attr_stacks()\n> when the direction is changed.\n\nAh, OK (and thanks), so it was working by accident ;-)\n"}]}