@matheusbsbs / matheus-bagliano / commits / 45bde58ef8

grep: demonstrate bug with textconv attributes and submodules

In some circumstances, "git grep --textconv --recurse-submodules" ignores the textconv attributes from the submodules and erroneously applies the attributes defined in the superproject on the submodules' files. The textconv cache is also saved on the superproject, even for submodule objects. A fix for these problems will probably require at least three changes: - Some textconv and attributes functions (as well as their callees) will have to be adjusted to work with arbitrary repositories. Note that "fill_textconv()", for example, already receives a "struct repository" but it writes the textconv cache using "write_loose_object()", which implicitly works on "the_repository". - grep.c functions will have to call textconv/userdiff routines passing the "repo" field from "struct grep_source" instead of the one from "struct grep_opt". The latter always points to "the_repository" on "git grep" executions (see its initialization in builtin/grep.c), but the former points to the correct repository that each source (an object, file, or buffer) comes from. - "userdiff_find_by_path()" might need to use a different attributes stack for each repository it works on or reset its internal static stack when the repository is changed throughout the calls. For now, let's add some tests to demonstrate these problems, and also update a NEEDSWORK comment in grep.h that mentions this bug to reference the added tests. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Matheus Tavares committed Sep 29, 2021 at 09:24 UTC 45bde58ef8f521e7deb730ffe90040b58d11af64
2 files changed +106 -3
grep.h
+3 -3
@@ -128,9 +128,9 @@ struct grep_opt {
128 * instead.
129 *
130 * This is potentially the cause of at least one bug - "git grep"
131 - * ignoring the textconv attributes from submodules. See [1] for more
132 - * information.
133 - * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/
131 + * using the textconv attributes from the superproject on the
132 + * submodules. See the failing "git grep --textconv" tests in
133 + * t7814-grep-recurse-submodules.sh for more information.
134 */
135 struct repository *repo;
136
t/t7814-grep-recurse-submodules.sh
+103
@@ -441,4 +441,107 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo
441 test_must_fail git grep --recurse-submodules --cached "A modified line in submodule" >actual 2>&1 &&
442 test_must_be_empty actual
443 '
444 +
445 +test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '
446 + reset_and_clean &&
447 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
448 + echo "a diff=d2x" >.gitattributes &&
449 +
450 + cat >expect <<-\EOF &&
451 + a:(1|2)x(3|4)
452 + EOF
453 + git grep --textconv --recurse-submodules x >actual &&
454 + test_cmp expect actual
455 +'
456 +
457 +test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '
458 + reset_and_clean &&
459 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
460 + echo "a diff=d2x" >.gitattributes &&
461 + git add .gitattributes &&
462 + rm .gitattributes &&
463 +
464 + cat >expect <<-\EOF &&
465 + a:(1|2)x(3|4)
466 + EOF
467 + git grep --textconv --recurse-submodules x >actual &&
468 + test_cmp expect actual
469 +'
470 +
471 +test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '
472 + reset_and_clean &&
473 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
474 + super_attr="$(git rev-parse --git-path info/attributes)" &&
475 + test_when_finished "rm -f \"$super_attr\"" &&
476 + echo "a diff=d2x" >"$super_attr" &&
477 +
478 + cat >expect <<-\EOF &&
479 + a:(1|2)x(3|4)
480 + EOF
481 + git grep --textconv --recurse-submodules x >actual &&
482 + test_cmp expect actual
483 +'
484 +
485 +# Note: what currently prevents this test from passing is not that the
486 +# .gitattributes file from "./submodule" is being ignored, but that it is being
487 +# propagated to the nested "./submodule/sub" files.
488 +#
489 +test_expect_failure 'grep --textconv correctly reads submodule .gitattributes' '
490 + reset_and_clean &&
491 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
492 + echo "a diff=d2x" >submodule/.gitattributes &&
493 +
494 + cat >expect <<-\EOF &&
495 + submodule/a:(1|2)x(3|4)
496 + EOF
497 + git grep --textconv --recurse-submodules x >actual &&
498 + test_cmp expect actual
499 +'
500 +
501 +test_expect_failure 'grep --textconv correctly reads submodule .gitattributes (from index)' '
502 + reset_and_clean &&
503 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
504 + echo "a diff=d2x" >submodule/.gitattributes &&
505 + git -C submodule add .gitattributes &&
506 + rm submodule/.gitattributes &&
507 +
508 + cat >expect <<-\EOF &&
509 + submodule/a:(1|2)x(3|4)
510 + EOF
511 + git grep --textconv --recurse-submodules x >actual &&
512 + test_cmp expect actual
513 +'
514 +
515 +test_expect_failure 'grep --textconv correctly reads submodule .git/info/attributes' '
516 + reset_and_clean &&
517 + test_config_global diff.d2x.textconv "sed -e \"s/d/x/\"" &&
518 +
519 + submodule_attr="$(git -C submodule rev-parse --path-format=absolute --git-path info/attributes)" &&
520 + test_when_finished "rm -f \"$submodule_attr\"" &&
521 + echo "a diff=d2x" >"$submodule_attr" &&
522 +
523 + cat >expect <<-\EOF &&
524 + submodule/a:(1|2)x(3|4)
525 + EOF
526 + git grep --textconv --recurse-submodules x >actual &&
527 + test_cmp expect actual
528 +'
529 +
530 +test_expect_failure 'grep saves textconv cache in the appropriate repository' '
531 + reset_and_clean &&
532 + test_config_global diff.d2x_cached.textconv "sed -e \"s/d/x/\"" &&
533 + test_config_global diff.d2x_cached.cachetextconv true &&
534 + echo "a diff=d2x_cached" >submodule/.gitattributes &&
535 +
536 + # We only read/write to the textconv cache when grepping from an OID,
537 + # as the working tree file might have modifications.
538 + git grep --textconv --cached --recurse-submodules x &&
539 +
540 + super_textconv_cache="$(git rev-parse --git-path refs/notes/textconv/d2x_cached)" &&
541 + sub_textconv_cache="$(git -C submodule rev-parse \
542 + --path-format=absolute --git-path refs/notes/textconv/d2x_cached)" &&
543 + test_path_is_missing "$super_textconv_cache" &&
544 + test_path_is_file "$sub_textconv_cache"
545 +'
546 +
547 test_done