Hi Brian
On 20/08/2019 03:45, brian m. carlson wrote:
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.
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.
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.
