From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 29 Nov 2018 04:05:55 GMT
Subject: Re: [PATCH 0/5] Add a new "sparse" tree walk algorithm
Message-ID: <d739ad00-d4f7-2a1b-7e03-3b69d74bdb5f@gmail.com>
In-Reply-To: <874lc0zw0p.fsf@evledraar.gmail.com>

On 11/28/2018 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
> This is really interesting. I tested this with:
>
>      diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>      index 124b1bafc4..5c7615f06c 100644
>      --- a/builtin/pack-objects.c
>      +++ b/builtin/pack-objects.c
>      @@ -3143 +3143 @@ static void get_object_list(int ac, const char **av)
>      -       mark_edges_uninteresting(&revs, show_edge, sparse);
>      +       mark_edges_uninteresting(&revs, show_edge, 1);
>
> To emulate having a GIT_TEST_* mode for this, which seems like a good
> idea since it turned up a lot of segfaults in pack-objects. I wasn't
> able to get a backtrace for that since it always happens indirectly, and
> I didn't dig enough to see how to manually invoke it the right way.

Thanks for double-checking this. I had run a similar test in my 
prototype implementation, but over-simplified some code when rewriting 
it for submission (and then forgot to re-run that test).

Specifically, these null checks are important:

diff --git a/list-objects.c b/list-objects.c
index 9bb93d1640..7e864b4db8 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -232,6 +232,10 @@ static void add_edge_parents(struct commit *commit,
         for (parents = commit->parents; parents; parents = parents->next) {
                 struct commit *parent = parents->item;
                 struct tree *tree = get_commit_tree(parent);
+
+               if (!tree)
+                       continue;
+
                 oidset_insert(set, &tree->object.oid);

                 if (!(parent->object.flags & UNINTERESTING))
@@ -261,6 +265,8 @@ void mark_edges_uninteresting(struct rev_info *revs,

                 if (sparse) {
                         struct tree *tree = get_commit_tree(commit);
+                       if (!tree)
+                               continue;

                         if (commit->object.flags & UNINTERESTING)
                                 tree->object.flags |= UNINTERESTING;

I will definitely include a GIT_TEST_* variable in v2.

Thanks,

-Stolee