[0/6] Kill the_repository in tree-walk.c

Message

Duy Nguyen June 24, 2019, 9:55 a.m. UTC

This is the continuation of nd/sha1-name-c-wo-the-repository. In that
series I sealed off one place in sha1-name.c that cannot walk trees
from arbitrary repositories. With tree-walk.c taking 'struct
repository *' directly, that check in there can now be removed.

Nguyễn Thái Ngọc Duy (6):
  sha1-file.c: remove the_repo from read_object_with_reference()
  tree-walk.c: remove the_repo from fill_tree_descriptor()
  tree-walk.c: remove the_repo from get_tree_entry()
  tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks()
  match-trees.c: remove the_repo from shift_tree*()
  Use the right 'struct repository' instead of the_repository

 archive.c                   |  4 +++-
 blame.c                     |  4 ++--
 builtin/cat-file.c          |  3 ++-
 builtin/grep.c              |  6 ++++--
 builtin/merge-tree.c        | 22 +++++++++++--------
 builtin/pack-objects.c      |  3 ++-
 builtin/rebase.c            |  4 ++--
 builtin/reset.c             |  4 ++--
 builtin/rm.c                |  2 +-
 builtin/update-index.c      |  2 +-
 cache.h                     |  7 +++---
 fast-import.c               |  9 +++++---
 line-log.c                  |  7 +++---
 match-trees.c               | 12 ++++++-----
 merge-recursive.c           | 43 +++++++++++++++++++++----------------
 notes.c                     |  4 ++--
 sequencer.c                 |  6 +++---
 sha1-file.c                 |  5 +++--
 sha1-name.c                 | 25 +++++++--------------
 shallow.c                   |  3 ++-
 t/helper/test-match-trees.c |  2 +-
 tree-diff.c                 |  4 ++--
 tree-walk.c                 | 35 ++++++++++++++++++++----------
 tree-walk.h                 |  8 ++++---
 unpack-trees.c              |  2 +-
 25 files changed, 129 insertions(+), 97 deletions(-)

Comments

Junio C Hamano June 26, 2019, 5:20 p.m. UTC | #1

Nguyễn Thái Ngọc Duy  <[e-mail]> writes:

> This is the continuation of nd/sha1-name-c-wo-the-repository. In that
> series I sealed off one place in sha1-name.c that cannot walk trees
> from arbitrary repositories. With tree-walk.c taking 'struct
> repository *' directly, that check in there can now be removed.

Thanks.

With these queued on 'master', t7814 seems to become flaky (tried
running it with --stress, with and without these patches).  Are we
touching a wrong index file in some codepaths or something?

Johannes Schindelin June 27, 2019, 1:04 p.m. UTC | #2

Hi Junio,

On Wed, 26 Jun 2019, Junio C Hamano wrote:

> Nguyễn Thái Ngọc Duy  <[e-mail]> writes:
>
> > This is the continuation of nd/sha1-name-c-wo-the-repository. In that
> > series I sealed off one place in sha1-name.c that cannot walk trees
> > from arbitrary repositories. With tree-walk.c taking 'struct
> > repository *' directly, that check in there can now be removed.
>
> Thanks.
>
> With these queued on 'master', t7814 seems to become flaky (tried
> running it with --stress, with and without these patches).  Are we
> touching a wrong index file in some codepaths or something?

It's not flaky, as it fails consistently, and yes, we're touching the
wrong repository in at least this one code path. I think I would have
wished for a more careful conversion in this patch series, as it does
touch critical code paths.

Given that this bug was only caught by a failing CI build, it does make me
wonder what other bugs are hidden and would slip into our code base just
because of gaps in the code coverage.

Ciao,
Dscho

Derrick Stolee June 27, 2019, 5:09 p.m. UTC | #3

On 6/27/2019 9:04 AM, Johannes Schindelin wrote:
> Given that this bug was only caught by a failing CI build, it does make me
> wonder what other bugs are hidden and would slip into our code base just
> because of gaps in the code coverage.

Here are the lines introduced by this series that are not covered by the
test suite. I'm not asking you to write tests to cover these lines, but
please re-examine the lines to be sure the correct conversion was made.

Thanks,
-Stolee

> Uncovered code in 'pu' not in 'jch'
> --------------------------------------------------------
> 
> archive.c
> 47f956bd 421) err = get_tree_entry(ar_args->repo,
> 47f956bd 422)      &tree->object.oid,
> 
> fast-import.c
> 35d7cdbe 2565) char *buf = read_object_with_reference(the_repository,
> 35d7cdbe 2566)        &n->oid,
> 
> match-trees.c
> 3fe87a7f 294) if (get_tree_entry(r, hash2, del_prefix, shifted, &mode))
> 
> t/helper/test-match-trees.c
> 3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1);
>
> Nguyễn Thái Ngọc Duy	35d7cdbe sha1-file.c: remove the_repo from read_object_with_reference()
> Nguyễn Thái Ngọc Duy	3fe87a7f match-trees.c: remove the_repo from shift_tree*()
> Nguyễn Thái Ngọc Duy	47f956bd tree-walk.c: remove the_repo from get_tree_entry()