| Message ID | 20190825233340.10894-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Honor .gitattributes with rebase --am | expand |
Hi Brian On 26/08/2019 00:33, brian m. carlson wrote: > This series makes rebase --am honor the .gitattributes file for > subsequent patches when a patch changes it. > > Note that there are two places we load attributes in ll-merge.c, but > this code only handles the one that git am uses. The two cannot be > unified because the one in ll_merge_marker_size intentionally doesn't > load the merge attribute, since it wants to always use the recursive > strategy. Loading it anyway causes t4017 to fail. > > Changes from v4: > * Wrap lines in apply.c. > * Handle merge and conflict-marker-size attributes. > * Add tests for am and am -3 in addition to rebase. I've only had time for a quick look at these but the changes look good to me. Thanks for taking the time to add the tests for am and the code for handling the merge attributes Best Wishes Phillip > Changes from v3: > * Check for both addition and removal of .gitattributes files. > * Switch from "test_config" to "git config". > > Changes from v2: > * Rename has_path_suffix to ends_with_path_components. > > Changes from v1: > * Add has_path_suffix in a separate commit. > > brian m. carlson (2): > path: add a function to check for path suffix > am: reload .gitattributes after patching it > > apply.c | 11 ++++++++++ > convert.c | 11 +++++++++- > convert.h | 6 ++++++ > ll-merge.c | 19 +++++++++++++---- > ll-merge.h | 1 + > path.c | 39 +++++++++++++++++++++++++++-------- > path.h | 3 +++ > t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++ > t/t4150-am.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++ > 9 files changed, 164 insertions(+), 14 deletions(-) > > Range-diff against v4: > 1: fa825e4b40 ! 1: 2077a0829e apply: reload .gitattributes after patching it > @@ Metadata > Author: brian m. carlson <sandals@crustytoothpaste.net> > > ## Commit message ## > - apply: reload .gitattributes after patching it > + am: 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 > @@ Commit message > > To ensure we write the correct data into the working tree, expire the > cache after each patch that touches a path ending in ".gitattributes". > + Since we load these attributes in multiple separate files, we must > + expire them accordingly. > + > + Verify that both the am and rebase code paths work correctly, including > + the conflict marker size with am -3. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > > @@ apply.c: static int apply_patch(struct apply_state *state, > *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))) > ++ 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 { > @@ apply.c: static int apply_patch(struct apply_state *state, > strbuf_release(&buf); > > ## convert.c ## > +@@ > + #include "pkt-line.h" > + #include "sub-process.h" > + #include "utf8.h" > ++#include "ll-merge.h" > + > + /* > + * convert.c - convert a file when checking it out and checking it in. > @@ convert.c: struct conv_attrs { > const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */ > }; > @@ convert.c: static void convert_attrs(const struct index_state *istate, > +{ > + attr_check_free(check); > + check = NULL; > ++ reset_merge_attributes(); > +} > + > int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path) > @@ convert.h: void convert_to_git_filter_fd(const struct index_state *istate, > * > * Streaming conversion support > > + ## ll-merge.c ## > +@@ ll-merge.c: struct ll_merge_driver { > + char *cmdline; > + }; > + > ++static struct attr_check *merge_attributes; > ++static struct attr_check *load_merge_attributes(void) > ++{ > ++ if (!merge_attributes) > ++ merge_attributes = attr_check_initl("merge", "conflict-marker-size", NULL); > ++ return merge_attributes; > ++} > ++ > ++void reset_merge_attributes(void) > ++{ > ++ attr_check_free(merge_attributes); > ++ merge_attributes = NULL; > ++} > ++ > + /* > + * Built-in low-levels > + */ > +@@ ll-merge.c: int ll_merge(mmbuffer_t *result_buf, > + struct index_state *istate, > + const struct ll_merge_options *opts) > + { > +- static struct attr_check *check; > ++ struct attr_check *check = load_merge_attributes(); > + static const struct ll_merge_options default_opts; > + const char *ll_driver_name = NULL; > + int marker_size = DEFAULT_CONFLICT_MARKER_SIZE; > +@@ ll-merge.c: int ll_merge(mmbuffer_t *result_buf, > + normalize_file(theirs, path, istate); > + } > + > +- if (!check) > +- check = attr_check_initl("merge", "conflict-marker-size", NULL); > +- > + git_check_attr(istate, path, check); > + ll_driver_name = check->items[0].value; > + if (check->items[1].value) { > + > + ## ll-merge.h ## > +@@ ll-merge.h: int ll_merge(mmbuffer_t *result_buf, > + const struct ll_merge_options *opts); > + > + int ll_merge_marker_size(struct index_state *istate, const char *path); > ++void reset_merge_attributes(void); > + > + #endif > + > ## t/t3400-rebase.sh ## > @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' ' > ) > @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' ' > test_expect_success 'rebase--merge.sh and --show-current-patch' ' > test_create_repo conflict-merge && > ( > + > + ## t/t4150-am.sh ## > +@@ t/t4150-am.sh: test_expect_success 'am --quit keeps HEAD where it is' ' > + test_cmp expected actual > + ' > + > ++test_expect_success 'am and .gitattibutes' ' > ++ 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 conflict-marker-size=10" >.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 -b conflict third && > ++ echo "This text is different." >a.txt && > ++ git add a.txt && > ++ test_commit sixth && > ++ > ++ git checkout test && > ++ git format-patch --stdout master..HEAD >patches && > ++ git reset --hard master && > ++ git am patches && > ++ grep "smudged" a.txt && > ++ > ++ git checkout removal && > ++ git reset --hard && > ++ git format-patch --stdout master..HEAD >patches && > ++ git reset --hard master && > ++ git am patches && > ++ grep "clean" a.txt && > ++ > ++ git checkout conflict && > ++ git reset --hard && > ++ git format-patch --stdout master..HEAD >patches && > ++ git reset --hard fourth && > ++ test_must_fail git am -3 patches && > ++ grep "<<<<<<<<<<" a.txt > ++ ) > ++' > ++ > + test_done >
This series makes rebase --am honor the .gitattributes file for subsequent patches when a patch changes it. Note that there are two places we load attributes in ll-merge.c, but this code only handles the one that git am uses. The two cannot be unified because the one in ll_merge_marker_size intentionally doesn't load the merge attribute, since it wants to always use the recursive strategy. Loading it anyway causes t4017 to fail. Changes from v4: * Wrap lines in apply.c. * Handle merge and conflict-marker-size attributes. * Add tests for am and am -3 in addition to rebase. Changes from v3: * Check for both addition and removal of .gitattributes files. * Switch from "test_config" to "git config". Changes from v2: * Rename has_path_suffix to ends_with_path_components. Changes from v1: * Add has_path_suffix in a separate commit. brian m. carlson (2): path: add a function to check for path suffix am: reload .gitattributes after patching it apply.c | 11 ++++++++++ convert.c | 11 +++++++++- convert.h | 6 ++++++ ll-merge.c | 19 +++++++++++++---- ll-merge.h | 1 + path.c | 39 +++++++++++++++++++++++++++-------- path.h | 3 +++ t/t3400-rebase.sh | 36 ++++++++++++++++++++++++++++++++ t/t4150-am.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 164 insertions(+), 14 deletions(-) Range-diff against v4: 1: fa825e4b40 ! 1: 2077a0829e apply: reload .gitattributes after patching it @@ Metadata Author: brian m. carlson <sandals@crustytoothpaste.net> ## Commit message ## - apply: reload .gitattributes after patching it + am: 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 @@ Commit message To ensure we write the correct data into the working tree, expire the cache after each patch that touches a path ending in ".gitattributes". + Since we load these attributes in multiple separate files, we must + expire them accordingly. + + Verify that both the am and rebase code paths work correctly, including + the conflict marker size with am -3. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> @@ apply.c: static int apply_patch(struct apply_state *state, *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))) ++ 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 { @@ apply.c: static int apply_patch(struct apply_state *state, strbuf_release(&buf); ## convert.c ## +@@ + #include "pkt-line.h" + #include "sub-process.h" + #include "utf8.h" ++#include "ll-merge.h" + + /* + * convert.c - convert a file when checking it out and checking it in. @@ convert.c: struct conv_attrs { const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */ }; @@ convert.c: static void convert_attrs(const struct index_state *istate, +{ + attr_check_free(check); + check = NULL; ++ reset_merge_attributes(); +} + int would_convert_to_git_filter_fd(const struct index_state *istate, const char *path) @@ convert.h: void convert_to_git_filter_fd(const struct index_state *istate, * * Streaming conversion support + ## ll-merge.c ## +@@ ll-merge.c: struct ll_merge_driver { + char *cmdline; + }; + ++static struct attr_check *merge_attributes; ++static struct attr_check *load_merge_attributes(void) ++{ ++ if (!merge_attributes) ++ merge_attributes = attr_check_initl("merge", "conflict-marker-size", NULL); ++ return merge_attributes; ++} ++ ++void reset_merge_attributes(void) ++{ ++ attr_check_free(merge_attributes); ++ merge_attributes = NULL; ++} ++ + /* + * Built-in low-levels + */ +@@ ll-merge.c: int ll_merge(mmbuffer_t *result_buf, + struct index_state *istate, + const struct ll_merge_options *opts) + { +- static struct attr_check *check; ++ struct attr_check *check = load_merge_attributes(); + static const struct ll_merge_options default_opts; + const char *ll_driver_name = NULL; + int marker_size = DEFAULT_CONFLICT_MARKER_SIZE; +@@ ll-merge.c: int ll_merge(mmbuffer_t *result_buf, + normalize_file(theirs, path, istate); + } + +- if (!check) +- check = attr_check_initl("merge", "conflict-marker-size", NULL); +- + git_check_attr(istate, path, check); + ll_driver_name = check->items[0].value; + if (check->items[1].value) { + + ## ll-merge.h ## +@@ ll-merge.h: int ll_merge(mmbuffer_t *result_buf, + const struct ll_merge_options *opts); + + int ll_merge_marker_size(struct index_state *istate, const char *path); ++void reset_merge_attributes(void); + + #endif + ## t/t3400-rebase.sh ## @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' ' ) @@ t/t3400-rebase.sh: test_expect_success 'rebase --am and --show-current-patch' ' test_expect_success 'rebase--merge.sh and --show-current-patch' ' test_create_repo conflict-merge && ( + + ## t/t4150-am.sh ## +@@ t/t4150-am.sh: test_expect_success 'am --quit keeps HEAD where it is' ' + test_cmp expected actual + ' + ++test_expect_success 'am and .gitattibutes' ' ++ 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 conflict-marker-size=10" >.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 -b conflict third && ++ echo "This text is different." >a.txt && ++ git add a.txt && ++ test_commit sixth && ++ ++ git checkout test && ++ git format-patch --stdout master..HEAD >patches && ++ git reset --hard master && ++ git am patches && ++ grep "smudged" a.txt && ++ ++ git checkout removal && ++ git reset --hard && ++ git format-patch --stdout master..HEAD >patches && ++ git reset --hard master && ++ git am patches && ++ grep "clean" a.txt && ++ ++ git checkout conflict && ++ git reset --hard && ++ git format-patch --stdout master..HEAD >patches && ++ git reset --hard fourth && ++ test_must_fail git am -3 patches && ++ grep "<<<<<<<<<<" a.txt ++ ) ++' ++ + test_done