git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 6/9] t4058: explore duplicate tree entry handling in a bit more detail

From
Elijah Newren <newren@gmail.com>
Date
Apr 21, 2021, 17:38 UTC
Message-ID
<CABPp-BHoCron-aNvjwwDkoAcvi_5X5uoZdO639eGg9XnoJUqdQ@mail.gmail.com>
In-Reply-To
<87fszj3jni.fsf@evledraar.gmail.com>

On Wed, Apr 21, 2021 at 5:29 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 61 quoted lines
>
>
> On Fri, Dec 11 2020, Elijah Newren via GitGitGadget wrote:
>
> > While creating the last commit, I found a number of other cases where
> > git would segfault when faced with trees that have duplicate entries.
> > None of these segfaults are in the diffcore-rename code (they all occur
> > in cache-tree and unpack-trees).  Further, to my knowledge, no one has
> > ever been adversely affected by these bugs, and given that it has been
> > 15 years and folks have fixed a few other issues with historical
> > duplicate entries (as noted in the last commit), I am not sure we will
> > ever run into anyone having problems with these.  So I am not sure these
> > are worth fixing, but it doesn't hurt to at least document these
> > failures in the same test file that is concerned with duplicate tree
> > entries.
> > [...]
> > +test_expect_failure 'fast-forward from duplicate entries to non-duplicate' '
> > +     git merge update
> > +'
> > +
> >  test_done
>
> Per https://lore.kernel.org/git/87lf9b3mth.fsf@evledraar.gmail.com/
> isn't the noise of having a segfault from "git" worth fixing in itself
> though? I.e. something like this, so we at least se why it fails:
>
> diff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh
> index 54614b814db..ed91d9f7fe9 100755
> --- a/t/t4058-diff-duplicates.sh
> +++ b/t/t4058-diff-duplicates.sh
> @@ -182,8 +182,10 @@ test_expect_success 'switch to base branch and force status to be clean' '
>         test_must_be_empty actual
>  '
>
> -test_expect_failure 'fast-forward from duplicate entries to non-duplicate' '
> -       git merge update
> +test_expect_success 'fast-forward from duplicate entries to non-duplicate' '
> +       ! git merge update 2>err &&
> +       grep "^BUG: " err &&
> +       grep -F "should have entry at o->src_index->cache[1]" err
>  '
>
>  test_done
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8a1afbc1e49..230cb073fe1 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -789,8 +789,11 @@ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names,
>          */
>         for (i = 0; i < nr_entries; i++) {
>                 int new_ce_len, len, rc;
> +               int j = pos + i;
>
> -               src[0] = o->src_index->cache[pos + i];
> +               src[0] = o->src_index->cache[j];
> +               if (!src[0])
> +                       BUG("should have entry at o->src_index->cache[%d]", j);
>
>                 len = ce_namelen(src[0]);
>                 new_ce_len = cache_entry_size(len);
>

Seems reasonable to me. Are you planning to add a commit message and turn it into a proper patch? If so, I'll give my Thumbs-up-by or whatever we need. :-)

Previous: Ævar Arnfjörð BjarmasonNext: Elijah Newren via GitGitGadget
Message 41 of 55 in “diffcore-rename improvements”
  1. 0/7 diffcore-rename improvementsElijah Newren via GitGitGadget, Dec 6, 2020
  2. 4/7 diffcore-rename: change a few comments to use 'add' instead of 'create'Elijah Newren via GitGitGadget, Dec 6, 2020
  3. Junio C HamanoDec 10, 2020
  4. 2/7 diffcore-rename: remove unnecessary if-clauseElijah Newren via GitGitGadget, Dec 6, 2020
  5. Taylor BlauDec 9, 2020
  6. Elijah NewrenDec 10, 2020
  7. Junio C HamanoDec 10, 2020
  8. Elijah NewrenDec 10, 2020
  9. Junio C HamanoDec 10, 2020
  10. Elijah NewrenDec 6, 2020
  11. 7/7 Accelerate rename_dst setupElijah Newren via GitGitGadget, Dec 6, 2020
  12. Taylor BlauDec 9, 2020
  13. Elijah NewrenDec 10, 2020
  14. Junio C HamanoDec 10, 2020
  15. 6/7 diffcore-rename: simplify and accelerate register_rename_src()Elijah Newren via GitGitGadget, Dec 6, 2020
  16. Taylor BlauDec 9, 2020
  17. Elijah NewrenDec 10, 2020
  18. Taylor BlauDec 10, 2020
  19. Junio C HamanoDec 10, 2020
  20. 5/7 diffcore-rename: reduce jumpiness in progress countersElijah Newren via GitGitGadget, Dec 6, 2020
  21. Taylor BlauDec 9, 2020
  22. Junio C HamanoDec 10, 2020
  23. 3/7 diffcore-rename: rename num_create to num_targetsElijah Newren via GitGitGadget, Dec 6, 2020
  24. Junio C HamanoDec 10, 2020
  25. Elijah NewrenDec 10, 2020
  26. 1/7 diffcore-rename: avoid usage of global in too_many_rename_candidates()Elijah Newren via GitGitGadget, Dec 6, 2020
  27. Taylor BlauDec 9, 2020
  28. 0/9 diffcore-rename improvementsElijah Newren via GitGitGadget, Dec 11, 2020
  29. 1/9 diffcore-rename: rename num_create to num_destinationsElijah Newren via GitGitGadget, Dec 11, 2020
  30. 2/9 diffcore-rename: avoid usage of global in too_many_rename_candidates()Elijah Newren via GitGitGadget, Dec 11, 2020
  31. 3/9 diffcore-rename: simplify limit checkElijah Newren via GitGitGadget, Dec 11, 2020
  32. 8/9 diffcore-rename: accelerate rename_dst setupElijah Newren via GitGitGadget, Dec 11, 2020
  33. 7/9 diffcore-rename: simplify and accelerate register_rename_src()Elijah Newren via GitGitGadget, Dec 11, 2020
  34. 4/9 diffcore-rename: reduce jumpiness in progress countersElijah Newren via GitGitGadget, Dec 11, 2020
  35. 9/9 diffcore-rename: remove unneccessary duplicate entry checksElijah Newren via GitGitGadget, Dec 11, 2020
  36. Christian CouderDec 29, 2020
  37. Elijah NewrenDec 29, 2020
  38. 5/9 t4058: add more tests and documentation for duplicate tree entry handlingElijah Newren via GitGitGadget, Dec 11, 2020
  39. 6/9 t4058: explore duplicate tree entry handling in a bit more detailElijah Newren via GitGitGadget, Dec 11, 2020
  40. Ævar Arnfjörð BjarmasonApr 21, 2021
  41. Elijah NewrenApr 21, 2021
  42. 0/9 diffcore-rename improvementsElijah Newren via GitGitGadget, Dec 29, 2020
  43. 1/9 diffcore-rename: rename num_create to num_destinationsElijah Newren via GitGitGadget, Dec 29, 2020
  44. 8/9 diffcore-rename: accelerate rename_dst setupElijah Newren via GitGitGadget, Dec 29, 2020
  45. 7/9 diffcore-rename: simplify and accelerate register_rename_src()Elijah Newren via GitGitGadget, Dec 29, 2020
  46. 2/9 diffcore-rename: avoid usage of global in too_many_rename_candidates()Elijah Newren via GitGitGadget, Dec 29, 2020
  47. 3/9 diffcore-rename: simplify limit checkElijah Newren via GitGitGadget, Dec 29, 2020
  48. Başar UğurNov 9, 2021
  49. Elijah NewrenNov 10, 2021
  50. Başar UğurNov 11, 2021
  51. Elijah NewrenNov 11, 2021
  52. 4/9 diffcore-rename: reduce jumpiness in progress countersElijah Newren via GitGitGadget, Dec 29, 2020
  53. 9/9 diffcore-rename: remove unnecessary duplicate entry checksElijah Newren via GitGitGadget, Dec 29, 2020
  54. 6/9 t4058: explore duplicate tree entry handling in a bit more detailElijah Newren via GitGitGadget, Dec 29, 2020
  55. 5/9 t4058: add more tests and documentation for duplicate tree entry handlingElijah Newren via GitGitGadget, Dec 29, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.