Re: [PATCH v2 2/2] commit-graph: fix writing first commit-graph during fetch

> On Wed, Oct 23, 2019 at 01:01:35PM +0000, Derrick Stolee via GitGitGadget wrote:
>>
>> You may ask: did this feature ever work at all? Yes, it did, as long as
>> you had a commit-graph covering all of your local refs. My testing was
>> unfortunately limited to this scenario. The UNINTERESTING commits are
>> always part of the "old" commit-graph, and when we add new commits to a
>> top layer of the commit-graph chain those are not needed. If we happen
>> to merge layers of the chain, then the commits are added as a list, not
>> using a commit walk. Further, the test added for this config option in
>> t5510-fetch.sh uses local filesystem clones, which somehow avoids this
>> logic.
> 
> Does this last sentence still holds, given that a submodule plays a
> crucial role in triggering this bug?  I think it either doesn't, or
> I still don't completely understand the situation.

>> I tested running clear_commit_marks_many() to clear the UNINTERESTING
>> flag inside close_reachable(), but the tips did not have the flag, so
>> that did nothing.
>>
>> It turns out that the calculate_changed_submodule_paths() method is at
>> fault. Thanks, Peff, for pointing out this detail! More specifically,
>> for each submodule, the collect_changed_submodules() runs a revision
>> walk to essentially do file-history on the list of submodules. That
>> revision walk marks commits UNININTERESTING if they are simiplified away
> 
> s/simiplified/simplified/
> 
>> by not changing the submodule.
>>
>> Instead, I finally arrived on the conclusion that I should use a flag
>> that is not used in any other part of the code. In commit-reach.c, a
>> number of flags were defined for commit walk algorithms. The REACHABLE
>> flag seemed like it made the most sense, and it seems it was not
>> actually used in the file. The REACHABLE flag was used in early versions
>> of commit-reach.c, but was removed by 4fbcca4 (commit-reach: make
>> can_all_from_reach... linear, 2018-07-20).
>>
>> Add the REACHABLE flag to commit-graph.c and use it instead of
>> UNINTERESTING in close_reachable(). This fixes the bug in manual
>> testing.
> 
> I'm inclined to agree that using a flag that is not used anywhere else
> is the safest thing to do, and at -rcX time safest is good.  I'm not
> sure whether it's the right thing to do in the long term, though.
> 
> Furthermore, calling this flag REACHABLE is misleading, because the
> code actually means SEEN.
> Consider the following sequence of commands:
> 
>   # Create a pack with two commits
>   $ git commit --allow-empty -m one &&
>   $ git commit --allow-empty -m two &&
>   $ git repack -ad &&
>   # Make one of those commits unreachable
>   $ git reset --hard HEAD^ &&
>   # Not even from reflogs!
>   $ git reflog expire --expire-unreachable=now --all
>   # Now write a commit-graph from that pack file
>   $ git commit-graph write
>   Computing commit graph generation numbers: 100% (2/2), done.
> 
> It added two commits to the commit-graph, although one of them is
> clearly not reachable anymore, so marking it as REACHABLE while
> enumerating all commits feels wrong.