patchapply: reload .gitattributes after patching it

> When applying multiple patches with git am, or when rebasing using the
> am backend, it's possible that one of our patches has updated a
> gitattributes file. Currently, we cache this information, so if a
> file in a subsequent patch has attributes applied, the file will be
> written out with the attributes in place as of the time we started the
> rebase or am operation, not with the attributes applied by the previous
> patch. This problem does not occur when using the -m or -i flags to
> rebase.

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  apply.c           | 11 +++++++++++
>  convert.c         |  9 ++++++++-
>  convert.h         |  6 ++++++
>  t/t3400-rebase.sh | 23 +++++++++++++++++++++++
>  4 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index cde95369bb..b959b88b8e 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
>  	struct patch *list = NULL, **listp = &list;
>  	int skipped_patch = 0;
>  	int res = 0;
> +	int flush_attributes = 0;
>
>  	state->patch_input_file = filename;
>  	if (read_patch_file(&buf, fd) < 0)
> @@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,
>  			patch_stats(state, patch);
>  			*listp = patch;
>  			listp = &patch->next;
> +
> +			if (!flush_attributes && patch->new_name) {
> +				char *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);

> +				/* The patch applied to a .gitattributes file. */
> +				if (dummy)
> +					flush_attributes = 1;
> +				free(dummy);
> +			}
>  		}
>  		else {
>  			if (state->apply_verbosity > verbosity_normal)
> @@ -4746,6 +4755,8 @@ static int apply_patch(struct apply_state *state,
>  	if (state->summary && state->apply_verbosity > verbosity_silent)
>  		summary_patch_list(list);
>
> +	if (flush_attributes)
> +		reset_parsed_attributes();
>  end:
>  	free_patch_list(list);
>  	strbuf_release(&buf);
> diff --git a/convert.c b/convert.c
> index 94ff837649..030e9b81b9 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -1293,10 +1293,11 @@ struct conv_attrs {
>  	const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */
>  };
>
> +static struct attr_check *check;
> +
>  static void convert_attrs(const struct index_state *istate,
>  			  struct conv_attrs *ca, const char *path)
>  {
> -	static struct attr_check *check;
>  	struct attr_check_item *ccheck = NULL;
>
>  	if (!check) {
> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,
>  		ca->crlf_action = CRLF_AUTO_INPUT;
>  }
>
> +void reset_parsed_attributes(void)
> +{
> +	attr_check_free(check);
> +	check = NULL;
> +}
> +

>  int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)
>  {
>  	struct conv_attrs ca;
> diff --git a/convert.h b/convert.h
> index 831559f10d..3710969d43 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,
>  int would_convert_to_git_filter_fd(const struct index_state *istate,
>  				   const char *path);
>
> +/*
> + * Reset the internal list of attributes used by convert_to_git and
> + * convert_to_working_tree.
> + */
> +void reset_parsed_attributes(void);
> +
>  /*****************************************************************
>   *
>   * Streaming conversion support
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 80b23fd326..062dc41df7 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -301,6 +301,29 @@ test_expect_success 'rebase --am and --show-current-patch' '
>  	)
>  '
>
> +test_expect_success 'rebase --am and .gitattributes' '
> +	test_create_repo attributes &&
> +	(
> +		cd attributes &&
> +		test_commit init &&
> +		test_config filter.test.clean "sed -e '\''s/smudged/clean/g'\''" &&
> +		test_config filter.test.smudge "sed -e '\''s/clean/smudged/g'\''" &&
> +
> +		test_commit second &&
> +		git checkout -b test HEAD^ &&
> +
> +		echo "*.txt filter=test" >.gitattributes &&
> +		git add .gitattributes &&
> +		test_commit third &&
> +
> +		echo "This text is smudged." >a.txt &&
> +		git add a.txt &&
> +		test_commit fourth &&
> +		git rebase master &&
> +		grep "smudged" a.txt
> +	)
> +'
> +

> > diff --git a/apply.c b/apply.c
> > index cde95369bb..b959b88b8e 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
> >  	struct patch *list = NULL, **listp = &list;
> >  	int skipped_patch = 0;
> >  	int res = 0;
> > +	int flush_attributes = 0;
> >
> >  	state->patch_input_file = filename;
> >  	if (read_patch_file(&buf, fd) < 0)
> > @@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,
> >  			patch_stats(state, patch);
> >  			*listp = patch;
> >  			listp = &patch->next;
> > +
> > +			if (!flush_attributes && patch->new_name) {
> > +				char *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);
> 
> It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and
> accept NULL for that (which would save us the assignment and subsequent
> 'free'). In either case, this is certainly the appropriate function.

> But, I'm not sure if 'dummy' is the best name for this variable or not.
> My first thought was that I'd expect this to be named 'suffix' (or the
> less descriptive 'p'), but I don't know if the change is warranted.
> What *are* we supposed to call this variable?
> "path_after_gitattributes" ;-)?

> > > +			if (!flush_attributes && patch->new_name) {
> > > +				char *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);
> > 
> > It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and
> > accept NULL for that (which would save us the assignment and subsequent
> > 'free'). In either case, this is certainly the appropriate function.
> 
> Yeah, I felt the same way. We could refactor this out into two separate
> functions, one which returns an ssize_t and one which actually
> allocates, but I'm not sure it makes a huge amount of sense with just
> one caller. The allocation is relatively small, and I've tried to make
> sure it's called exactly once per patch so as not to be wasteful and
> inefficient.

> I think you could do this with:
> 
>   size_t len;
>   if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&
>       len > 0 && is_dir_sep(patch->new_name[len-1]))
>           flush_attributes = 1;
> 
> Not sure if that is better or worse. It avoids the extra memory
> boilerplate, but the logic is a slightly more subtle.

> On Fri, Aug 09, 2019 at 11:25:52AM +0000, brian m. carlson wrote:
> 
> > > > +			if (!flush_attributes && patch->new_name) {
> > > > +				char *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);
> > > 
> > > It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and
> > > accept NULL for that (which would save us the assignment and subsequent
> > > 'free'). In either case, this is certainly the appropriate function.
> > 
> > Yeah, I felt the same way. We could refactor this out into two separate
> > functions, one which returns an ssize_t and one which actually
> > allocates, but I'm not sure it makes a huge amount of sense with just
> > one caller. The allocation is relatively small, and I've tried to make
> > sure it's called exactly once per patch so as not to be wasteful and
> > inefficient.
> 
> I think you could do this with:
> 
>   size_t len;
>   if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&
>       len > 0 && is_dir_sep(patch->new_name[len-1]))
>           flush_attributes = 1;
> 
> Not sure if that is better or worse. It avoids the extra memory
> boilerplate, but the logic is a slightly more subtle.

> > I think you could do this with:
> > 
> >   size_t len;
> >   if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&
> >       len > 0 && is_dir_sep(patch->new_name[len-1]))
> >           flush_attributes = 1;
> > 
> > Not sure if that is better or worse. It avoids the extra memory
> > boilerplate, but the logic is a slightly more subtle.
> 
> Subtle indeed :) because you have to allow len == 0 to catch the
> toplevel .gitattributes file.

> But there is an other subtlety here: when I read the commit message
> saying "patch that touches a path ending in ".gitattributes"." and saw
> the new call to strip_path_suffix(), I immediately thought what would
> happen with a file called 'foo.gitattributes'.  Only when I looked
> into strip_path_suffix() became it clear that it only removes full
> path components, so such a filename won't cause any trouble (though
> perhaps the worst thing that could happen is that we unnecessarily
> flush the attributes cache).

> On Fri, Aug 09, 2019 at 02:43:18PM +0200, SZEDER Gábor wrote:
> > But there is an other subtlety here: when I read the commit message
> > saying "patch that touches a path ending in ".gitattributes"." and saw
> > the new call to strip_path_suffix(), I immediately thought what would
> > happen with a file called 'foo.gitattributes'.  Only when I looked
> > into strip_path_suffix() became it clear that it only removes full
> > path components, so such a filename won't cause any trouble (though
> > perhaps the worst thing that could happen is that we unnecessarily
> > flush the attributes cache).
> 
> Right. I think the term we want here is really "basename". So in fact:
> 
>   if (!strcmp(basename(patch->new_name), GITATTRIBUTES_FILE))
> 
> would do what we want, except for the annoying caveat that basename() is
> allowed to modify its parameter (to remove trailing directory
> separators, but we know we wouldn't have them here).

> We have a function to strip the path suffix from a commit, but we don't
> have one to check for a path suffix. For a plain filename, we can use
> basename, but that requires an allocation, since POSIX allows it to
> modify its argument. Refactor strip_path_suffix into a helper function
> and a new function, has_path_suffix to meet this need.

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)
> 
> Perhaps
> 
>     static ssize_t last_path_component_offset(const char *path, const char *name)
> 
> I am tempted to also call the second parameter to this function
> "basename", as we know from the proposed log message that you wish
> "basename" were usable for this purpose, but basename itself has
> another confusing factor (i.e. "are we stripping ".exe" extension?",
> to which the answer is no in the context of these functions).
> 
> If we agree with the "last path component" phrasing, has_path_suffix()
> would become something like:
> 
>     int last_path_component_equals(const char *path, const char *name);

> On 2019-08-12 at 00:32:26, Junio C Hamano wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)
> > 
> > Perhaps
> > 
> >     static ssize_t last_path_component_offset(const char *path, const char *name)
> > 
> > I am tempted to also call the second parameter to this function
> > "basename", as we know from the proposed log message that you wish
> > "basename" were usable for this purpose, but basename itself has
> > another confusing factor (i.e. "are we stripping ".exe" extension?",
> > to which the answer is no in the context of these functions).
> > 
> > If we agree with the "last path component" phrasing, has_path_suffix()
> > would become something like:
> > 
> >     int last_path_component_equals(const char *path, const char *name);
> 
> Except this is not necessarily the last path component. It could match
> one or more path components with the way the function is written. If you
> want to ignore that and name the function accordingly, I won't object,
> but we could theoretically handle a name like "foo/.gitattributes" as
> well.

> On 2019-08-12 at 00:32:26, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)
>> 
>> Perhaps
>> 
>>     static ssize_t last_path_component_offset(const char *path, const char *name)
>> 
>> I am tempted to also call the second parameter to this function
>> "basename", as we know from the proposed log message that you wish
>> "basename" were usable for this purpose, but basename itself has
>> another confusing factor (i.e. "are we stripping ".exe" extension?",
>> to which the answer is no in the context of these functions).
>> 
>> If we agree with the "last path component" phrasing, has_path_suffix()
>> would become something like:
>> 
>>     int last_path_component_equals(const char *path, const char *name);
>
> Except this is not necessarily the last path component. It could match
> one or more path components with the way the function is written.

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2019-08-12 at 00:32:26, Junio C Hamano wrote:
> >> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)
> >> 
> >> Perhaps
> >> 
> >>     static ssize_t last_path_component_offset(const char *path, const char *name)
> >> 
> >> I am tempted to also call the second parameter to this function
> >> "basename", as we know from the proposed log message that you wish
> >> "basename" were usable for this purpose, but basename itself has
> >> another confusing factor (i.e. "are we stripping ".exe" extension?",
> >> to which the answer is no in the context of these functions).
> >> 
> >> If we agree with the "last path component" phrasing, has_path_suffix()
> >> would become something like:
> >> 
> >>     int last_path_component_equals(const char *path, const char *name);
> >
> > Except this is not necessarily the last path component. It could match
> > one or more path components with the way the function is written.
> 
> That's fair.  Is the feature that allows the function called
> ends_with_component*S* like Szeder suggests designed one, i.e. with
> an explicit purpose of supporting callers that pass "foo/bar" as the
> "suffix" to it, or is it merely that the implementation happens to
> work that way, even though the expected use that is supported is to
> pass only one level component but the implementation did not even
> bother asserting that the "suffix" does not have a slash in it?

> Well, I split it out from a function that handles multiple path
> components, mostly so that I could leverage existing work (and not have
> to worry about getting it wrong). It wasn't explicitly intended that it
> support multiple components, since I don't require that for my
> implementation, but I could see future users taking advantage of that.
>
> I think "ends_with_path_components" might be the way forward, unless
> you think something else would be better.

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2019-08-12 at 00:32:26, Junio C Hamano wrote:
> >> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >> > +static ssize_t stripped_path_suffix_offset(const char *path, const char *suffix)
> >> 
> >> Perhaps
> >> 
> >>     static ssize_t last_path_component_offset(const char *path, const char *name)
> >> 
> >> I am tempted to also call the second parameter to this function
> >> "basename", as we know from the proposed log message that you wish
> >> "basename" were usable for this purpose, but basename itself has
> >> another confusing factor (i.e. "are we stripping ".exe" extension?",
> >> to which the answer is no in the context of these functions).
> >> 
> >> If we agree with the "last path component" phrasing, has_path_suffix()
> >> would become something like:
> >> 
> >>     int last_path_component_equals(const char *path, const char *name);
> >
> > Except this is not necessarily the last path component. It could match
> > one or more path components with the way the function is written.
> 
> That's fair.  Is the feature that allows the function called
> ends_with_component*S* like Szeder suggests designed one, i.e. with
> an explicit purpose of supporting callers that pass "foo/bar" as the
> "suffix" to it, or is it merely that the implementation happens to
> work that way, even though the expected use that is supported is to
> pass only one level component but the implementation did not even
> bother asserting that the "suffix" does not have a slash in it?

> ...  And that function must be able to handle multiple path
> components, becase there is this callsite:
>
>   exec-cmd.c:         !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&
>
> and the build sets '-DGIT_EXEC_PATH="libexec/git-core"' by default.

> When applying multiple patches with git am, or when rebasing using the
> am backend, it's possible that one of our patches has updated a
> gitattributes file. Currently, we cache this information, so if a
> file in a subsequent patch has attributes applied, the file will be
> written out with the attributes in place as of the time we started the
> rebase or am operation, not with the attributes applied by the previous
> patch. This problem does not occur when using the -m or -i flags to
> rebase.

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> When applying multiple patches with git am, or when rebasing using the
>> am backend, it's possible that one of our patches has updated a
>> gitattributes file. Currently, we cache this information, so if a
>> file in a subsequent patch has attributes applied, the file will be
>> written out with the attributes in place as of the time we started the
>> rebase or am operation, not with the attributes applied by the previous
>> patch. This problem does not occur when using the -m or -i flags to
>> rebase.
> ...
> "rebase -m" and "rebase -i" are not repeated run_command() calls
> that invoke "git cherry-pick" or "git merge" these days, either, so
> I am somewhat curious how they avoid fallilng into the same trap.
>
> Thanks for the fix.  Will queue.

> To ensure we write the correct data into the working tree, expire the
> cache after each patch that touches a path ending in ".gitattributes".
> ...
> +			if (!flush_attributes && patch->new_name &&
> +			    ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))
> +				flush_attributes = 1;

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> >> When applying multiple patches with git am, or when rebasing using the
> >> am backend, it's possible that one of our patches has updated a
> >> gitattributes file. Currently, we cache this information, so if a
> >> file in a subsequent patch has attributes applied, the file will be
> >> written out with the attributes in place as of the time we started the
> >> rebase or am operation, not with the attributes applied by the previous
> >> patch. This problem does not occur when using the -m or -i flags to
> >> rebase.
> > ...
> > "rebase -m" and "rebase -i" are not repeated run_command() calls
> > that invoke "git cherry-pick" or "git merge" these days, either, so
> > I am somewhat curious how they avoid fallilng into the same trap.
> >
> > Thanks for the fix.  Will queue.
> 
> Actually there still is one more thing I wasn't clear about the
> change.
> 
> > To ensure we write the correct data into the working tree, expire the
> > cache after each patch that touches a path ending in ".gitattributes".
> > ...
> > +			if (!flush_attributes && patch->new_name &&
> > +			    ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE))
> > +				flush_attributes = 1;
> 
> When an attribute file is removed by a patch, we should forget what
> we read earlier from the file before it got removed.  Would such a
> case, where patch->new_name would be NULL, be handled correctly?

> +test_expect_success 'rebase --am and .gitattributes' '
> +	test_create_repo attributes &&
> +	(
> +		cd attributes &&
> +		test_commit init &&
> +		test_config filter.test.clean "sed -e '\''s/smudged/clean/g'\''" &&
> +		test_config filter.test.smudge "sed -e '\''s/clean/smudged/g'\''" &&

> +		test_commit second &&
> +		git checkout -b test HEAD^ &&
> +
> +		echo "*.txt filter=test" >.gitattributes &&
> +		git add .gitattributes &&
> +		test_commit third &&
> +
> +		echo "This text is smudged." >a.txt &&
> +		git add a.txt &&
> +		test_commit fourth &&
> +		git rebase master &&
> +		grep "smudged" a.txt
> +	)
> +'
> +
>  test_expect_success 'rebase--merge.sh and --show-current-patch' '
>  	test_create_repo conflict-merge &&
>  	(

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > +test_expect_success 'rebase --am and .gitattributes' '
> > +	test_create_repo attributes &&
> > +	(
> > +		cd attributes &&
> > +		test_commit init &&
> > +		test_config filter.test.clean "sed -e '\''s/smudged/clean/g'\''" &&
> > +		test_config filter.test.smudge "sed -e '\''s/clean/smudged/g'\''" &&
> 
> These eventually invokes test-when-finished for cleaning things up,
> that cannot be called inside a subshell.  The "attributes" test
> repository is a throw-away reopsitory, so we should be able to just
> use "git config" to set the variables locally in it, no?

> When applying multiple patches with git am, or when rebasing using the
> am backend, it's possible that one of our patches has updated a
> gitattributes file. Currently, we cache this information, so if a
> file in a subsequent patch has attributes applied, the file will be
> written out with the attributes in place as of the time we started the
> rebase or am operation, not with the attributes applied by the previous
> patch. This problem does not occur when using the -m or -i flags to
> rebase.

> To ensure we write the correct data into the working tree, expire the
> cache after each patch that touches a path ending in ".gitattributes".
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>   apply.c           |  7 +++++++
>   convert.c         |  9 ++++++++-
>   convert.h         |  6 ++++++
>   t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/apply.c b/apply.c
> index cde95369bb..d57bc635e4 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
>   	struct patch *list = NULL, **listp = &list;
>   	int skipped_patch = 0;
>   	int res = 0;
> +	int flush_attributes = 0;
>   
>   	state->patch_input_file = filename;
>   	if (read_patch_file(&buf, fd) < 0)
> @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,
>   			patch_stats(state, patch);
>   			*listp = patch;
>   			listp = &patch->next;
> +
> +			if ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
> +			    (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
> +				flush_attributes = 1;

>   		}
>   		else {
>   			if (state->apply_verbosity > verbosity_normal)
> @@ -4746,6 +4751,8 @@ static int apply_patch(struct apply_state *state,
>   	if (state->summary && state->apply_verbosity > verbosity_silent)
>   		summary_patch_list(list);
>   
> +	if (flush_attributes)
> +		reset_parsed_attributes();
>   end:
>   	free_patch_list(list);
>   	strbuf_release(&buf);
> diff --git a/convert.c b/convert.c
> index 94ff837649..030e9b81b9 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -1293,10 +1293,11 @@ struct conv_attrs {
>   	const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */
>   };
>   
> +static struct attr_check *check;

> +
>   static void convert_attrs(const struct index_state *istate,
>   			  struct conv_attrs *ca, const char *path)
>   {
> -	static struct attr_check *check;
>   	struct attr_check_item *ccheck = NULL;
>   
>   	if (!check) {
> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,
>   		ca->crlf_action = CRLF_AUTO_INPUT;
>   }
>   
> +void reset_parsed_attributes(void)
> +{
> +	attr_check_free(check);
> +	check = NULL;
> +}
> +
>   int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path)
>   {
>   	struct conv_attrs ca;
> diff --git a/convert.h b/convert.h
> index 831559f10d..3710969d43 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct index_state *istate,
>   int would_convert_to_git_filter_fd(const struct index_state *istate,
>   				   const char *path);
>   
> +/*
> + * Reset the internal list of attributes used by convert_to_git and
> + * convert_to_working_tree.
> + */
> +void reset_parsed_attributes(void);
> +
>   /*****************************************************************
>    *
>    * Streaming conversion support
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 80b23fd326..23469cc789 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -301,6 +301,42 @@ test_expect_success 'rebase --am and --show-current-patch' '
>   	)
>   '
>   
> +test_expect_success 'rebase --am and .gitattributes' '
> +	test_create_repo attributes &&
> +	(
> +		cd attributes &&
> +		test_commit init &&
> +		git config filter.test.clean "sed -e '\''s/smudged/clean/g'\''" &&
> +		git config filter.test.smudge "sed -e '\''s/clean/smudged/g'\''" &&
> +
> +		test_commit second &&
> +		git checkout -b test HEAD^ &&
> +
> +		echo "*.txt filter=test" >.gitattributes &&
> +		git add .gitattributes &&
> +		test_commit third &&
> +
> +		echo "This text is smudged." >a.txt &&
> +		git add a.txt &&
> +		test_commit fourth &&
> +
> +		git checkout -b removal HEAD^ &&
> +		git rm .gitattributes &&
> +		git add -u &&
> +		test_commit fifth &&
> +		git cherry-pick test &&
> +
> +		git checkout test &&
> +		git rebase master &&
> +		grep "smudged" a.txt &&
> +
> +		git checkout removal &&
> +		git reset --hard &&
> +		git rebase master &&
> +		grep "clean" a.txt
> +	)
> +'
> +
>   test_expect_success 'rebase--merge.sh and --show-current-patch' '
>   	test_create_repo conflict-merge &&
>   	(
> 

> [...]
>> diff --git a/convert.c b/convert.c
>> index 94ff837649..030e9b81b9 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -1293,10 +1293,11 @@ struct conv_attrs {
>>       const char *working_tree_encoding; /* Supported encoding or 
>> default encoding if NULL */
>>   };
>> +static struct attr_check *check;
> 
> I was concerned about the impact adding a file global if we ever want to 
> multi-thread this for submodules, but looking through the file there are 
> a couple of others already so this isn't creating a new problem.

> 
> Best Wishes
> 
> Phillip
> 
>> +
>>   static void convert_attrs(const struct index_state *istate,
>>                 struct conv_attrs *ca, const char *path)
>>   {
>> -    static struct attr_check *check;
>>       struct attr_check_item *ccheck = NULL;
>>       if (!check) {
>> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct 
>> index_state *istate,
>>           ca->crlf_action = CRLF_AUTO_INPUT;
>>   }
>> +void reset_parsed_attributes(void)
>> +{
>> +    attr_check_free(check);
>> +    check = NULL;
>> +}
>> +
>>   int would_convert_to_git_filter_fd(const struct index_state *istate, 
>> const char *path)
>>   {
>>       struct conv_attrs ca;
>> diff --git a/convert.h b/convert.h
>> index 831559f10d..3710969d43 100644
>> --- a/convert.h
>> +++ b/convert.h
>> @@ -94,6 +94,12 @@ void convert_to_git_filter_fd(const struct 
>> index_state *istate,
>>   int would_convert_to_git_filter_fd(const struct index_state *istate,
>>                      const char *path);
>> +/*
>> + * Reset the internal list of attributes used by convert_to_git and
>> + * convert_to_working_tree.
>> + */
>> +void reset_parsed_attributes(void);
>> +
>>   /*****************************************************************
>>    *
>>    * Streaming conversion support
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 80b23fd326..23469cc789 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -301,6 +301,42 @@ test_expect_success 'rebase --am and 
>> --show-current-patch' '
>>       )
>>   '
>> +test_expect_success 'rebase --am and .gitattributes' '
>> +    test_create_repo attributes &&
>> +    (
>> +        cd attributes &&
>> +        test_commit init &&
>> +        git config filter.test.clean "sed -e 
>> '\''s/smudged/clean/g'\''" &&
>> +        git config filter.test.smudge "sed -e 
>> '\''s/clean/smudged/g'\''" &&
>> +
>> +        test_commit second &&
>> +        git checkout -b test HEAD^ &&
>> +
>> +        echo "*.txt filter=test" >.gitattributes &&
>> +        git add .gitattributes &&
>> +        test_commit third &&
>> +
>> +        echo "This text is smudged." >a.txt &&
>> +        git add a.txt &&
>> +        test_commit fourth &&
>> +
>> +        git checkout -b removal HEAD^ &&
>> +        git rm .gitattributes &&
>> +        git add -u &&
>> +        test_commit fifth &&
>> +        git cherry-pick test &&
>> +
>> +        git checkout test &&
>> +        git rebase master &&
>> +        grep "smudged" a.txt &&
>> +
>> +        git checkout removal &&
>> +        git reset --hard &&
>> +        git rebase master &&
>> +        grep "clean" a.txt
>> +    )
>> +'
>> +
>>   test_expect_success 'rebase--merge.sh and --show-current-patch' '
>>       test_create_repo conflict-merge &&
>>       (
>>

> On 19/08/2019 10:41, Phillip Wood wrote:
> > [...]
> > > diff --git a/convert.c b/convert.c
> > > index 94ff837649..030e9b81b9 100644
> > > --- a/convert.c
> > > +++ b/convert.c
> > > @@ -1293,10 +1293,11 @@ struct conv_attrs {
> > >       const char *working_tree_encoding; /* Supported encoding or
> > > default encoding if NULL */
> > >   };
> > > +static struct attr_check *check;
> > 
> > I was concerned about the impact adding a file global if we ever want to
> > multi-thread this for submodules, but looking through the file there are
> > a couple of others already so this isn't creating a new problem.
> 
> Doh, I've just realized it was static already - ignore that.

> On 2019-08-19 at 09:55:27, Phillip Wood wrote:
>> On 19/08/2019 10:41, Phillip Wood wrote:
>>> [...]
>>>> diff --git a/convert.c b/convert.c
>>>> index 94ff837649..030e9b81b9 100644
>>>> --- a/convert.c
>>>> +++ b/convert.c
>>>> @@ -1293,10 +1293,11 @@ struct conv_attrs {
>>>>        const char *working_tree_encoding; /* Supported encoding or
>>>> default encoding if NULL */
>>>>    };
>>>> +static struct attr_check *check;
>>>
>>> I was concerned about the impact adding a file global if we ever want to
>>> multi-thread this for submodules, but looking through the file there are
>>> a couple of others already so this isn't creating a new problem.
>>
>> Doh, I've just realized it was static already - ignore that.
> 
> And I just realized that I didn't read the entire thread before
> responding.  Sorry about that.
> 
>> One thing did occur to me though - does this patch reset attributes like the
>> merge marker length (they're less critical though if there is a conflict
>> after an attribute change it would be nice to have the correct length) or
>> just the ones for filtering files?
> 
> It resets "crlf", "ident", "filter", "eol", "text", and
> "working-tree-encoding".  Things it doesn't reset include "whitespace",
> "export-ignore", "export-subst", "merge", and "conflict-marker-size".
> Of these, I think only the latter two are relevant.
> 
> I'll update that in v5.

> Hi Brian
> 
> On 18/08/2019 19:44, brian m. carlson wrote:
> > When applying multiple patches with git am, or when rebasing using the
> > am backend, it's possible that one of our patches has updated a
> > gitattributes file. Currently, we cache this information, so if a
> > file in a subsequent patch has attributes applied, the file will be
> > written out with the attributes in place as of the time we started the
> > rebase or am operation, not with the attributes applied by the previous
> > patch. This problem does not occur when using the -m or -i flags to
> > rebase.
> 
> Do you know why -m and -i aren't affected?

> > diff --git a/apply.c b/apply.c
> > index cde95369bb..d57bc635e4 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
> >   	struct patch *list = NULL, **listp = &list;
> >   	int skipped_patch = 0;
> >   	int res = 0;
> > +	int flush_attributes = 0;
> >   	state->patch_input_file = filename;
> >   	if (read_patch_file(&buf, fd) < 0)
> > @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,
> >   			patch_stats(state, patch);
> >   			*listp = patch;
> >   			listp = &patch->next;
> > +
> > +			if ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
> > +			    (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
> > +				flush_attributes = 1;
> 
> style nit - these lines are very long compared to 80 characters

> > diff --git a/convert.c b/convert.c
> > index 94ff837649..030e9b81b9 100644
> > --- a/convert.c
> > +++ b/convert.c
> > @@ -1293,10 +1293,11 @@ struct conv_attrs {
> >   	const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */
> >   };
> > +static struct attr_check *check;
> 
> I was concerned about the impact adding a file global if we ever want to
> multi-thread this for submodules, but looking through the file there are a
> couple of others already so this isn't creating a new problem.
> > +
> >   static void convert_attrs(const struct index_state *istate,
> >   			  struct conv_attrs *ca, const char *path)
> >   {
> > -	static struct attr_check *check;
> >   	struct attr_check_item *ccheck = NULL;
> >   	if (!check) {
> > @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,
> >   		ca->crlf_action = CRLF_AUTO_INPUT;
> >   }

> On 2019-08-19 at 09:41:42, Phillip Wood wrote:
>> Hi Brian
>>
>> On 18/08/2019 19:44, brian m. carlson wrote:
>>> When applying multiple patches with git am, or when rebasing using the
>>> am backend, it's possible that one of our patches has updated a
>>> gitattributes file. Currently, we cache this information, so if a
>>> file in a subsequent patch has attributes applied, the file will be
>>> written out with the attributes in place as of the time we started the
>>> rebase or am operation, not with the attributes applied by the previous
>>> patch. This problem does not occur when using the -m or -i flags to
>>> rebase.
>>
>> Do you know why -m and -i aren't affected?
> 
> I had to look, but I believe the answer is because they use the
> sequencer, and the sequencer calls git merge-recursive as a separate
> process, and so the writing of the tree is only done in a subprocess,
> which can't persist state.

> Should we move the merge-recursive code into the main process, we'll
> likely have the same problem there.
> 
>>> diff --git a/apply.c b/apply.c
>>> index cde95369bb..d57bc635e4 100644
>>> --- a/apply.c
>>> +++ b/apply.c
>>> @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
>>>    	struct patch *list = NULL, **listp = &list;
>>>    	int skipped_patch = 0;
>>>    	int res = 0;
>>> +	int flush_attributes = 0;
>>>    	state->patch_input_file = filename;
>>>    	if (read_patch_file(&buf, fd) < 0)
>>> @@ -4670,6 +4671,10 @@ static int apply_patch(struct apply_state *state,
>>>    			patch_stats(state, patch);
>>>    			*listp = patch;
>>>    			listp = &patch->next;
>>> +
>>> +			if ((patch->new_name && ends_with_path_components(patch->new_name, GITATTRIBUTES_FILE)) ||
>>> +			    (patch->old_name && ends_with_path_components(patch->old_name, GITATTRIBUTES_FILE)))
>>> +				flush_attributes = 1;
>>
>> style nit - these lines are very long compared to 80 characters
> 
> They are.  They aren't two different from other lines in the file, and I
> thought that leaving them that way would preserve the similarities, but
> I can also wrap them.  I'll send out a v5 with wrapped lines.
> 
>>> diff --git a/convert.c b/convert.c
>>> index 94ff837649..030e9b81b9 100644
>>> --- a/convert.c
>>> +++ b/convert.c
>>> @@ -1293,10 +1293,11 @@ struct conv_attrs {
>>>    	const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */
>>>    };
>>> +static struct attr_check *check;
>>
>> I was concerned about the impact adding a file global if we ever want to
>> multi-thread this for submodules, but looking through the file there are a
>> couple of others already so this isn't creating a new problem.
>>> +
>>>    static void convert_attrs(const struct index_state *istate,
>>>    			  struct conv_attrs *ca, const char *path)
>>>    {
>>> -	static struct attr_check *check;
>>>    	struct attr_check_item *ccheck = NULL;
>>>    	if (!check) {
>>> @@ -1339,6 +1340,12 @@ static void convert_attrs(const struct index_state *istate,
>>>    		ca->crlf_action = CRLF_AUTO_INPUT;
>>>    }
> 
> The portion I've included above demonstrates that this was already a
> static variable, just one hidden in a function.  So yeah, this is no
> worse than it already was.
> 

>>> Do you know why -m and -i aren't affected?
>>
>> I had to look, but I believe the answer is because they use the
>> sequencer, and the sequencer calls git merge-recursive as a separate
>> process, and so the writing of the tree is only done in a subprocess,
>> which can't persist state.
>
> The sequencer has been running in a single process for a while now. We
> do fork for 'git merge' sometimes when processing 'merge' commands but
> 'pick' commands are all done in a single process by calling
> do_recursive_merge().
>
> Best Wishes
>
> Phillip
>
>> Should we move the merge-recursive code into the main process, we'll
>> likely have the same problem there.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>> Do you know why -m and -i aren't affected?
>>>
>>> I had to look, but I believe the answer is because they use the
>>> sequencer, and the sequencer calls git merge-recursive as a separate
>>> process, and so the writing of the tree is only done in a subprocess,
>>> which can't persist state.
>>
>> The sequencer has been running in a single process for a while now. We
>> do fork for 'git merge' sometimes when processing 'merge' commands but
>> 'pick' commands are all done in a single process by calling
>> do_recursive_merge().
>>
>> Best Wishes
>>
>> Phillip
>>
>>> Should we move the merge-recursive code into the main process, we'll
>>> likely have the same problem there.
> 
> So we actually have the same issue already?

> On 20/08/2019 19:24, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>>>> Do you know why -m and -i aren't affected?
>>>>
>>>> I had to look, but I believe the answer is because they use the
>>>> sequencer, and the sequencer calls git merge-recursive as a separate
>>>> process, and so the writing of the tree is only done in a subprocess,
>>>> which can't persist state.
>>>
>>> The sequencer has been running in a single process for a while now. We
>>> do fork for 'git merge' sometimes when processing 'merge' commands but
>>> 'pick' commands are all done in a single process by calling
>>> do_recursive_merge().
>>>
>>> Best Wishes
>>>
>>> Phillip
>>>
>>>> Should we move the merge-recursive code into the main process, we'll
>>>> likely have the same problem there.
>>
>> So we actually have the same issue already?
> 
> I don't think so, I modified Brian's test to call 'rebase -i' and it 
> passes but no one seems to know why.

> I spent some time digging into this and the attributes are reloaded
> before each pick. This is because check_updates() called by
> unpack_trees() calls git_attr_set_direction(GIT_ATTR_CHECKOUT) at the
> start of the function and git_attr_set_direction(GIT_ATTR_CHECKIN) at
> the end of the function. This has the effect of dropping all loaded
> attributes as git_attr_set_direction() calls drop_all_attr_stacks()
> when the direction is changed.