Re: [PATCH] commit-graph: don't show progress percentages while expanding reachable commits

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Mar 22, 2019 at 04:49:43PM +0100, SZEDER Gábor wrote:
> On Fri, Mar 22, 2019 at 04:11:24PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 
> > On Fri, Mar 22 2019, SZEDER Gábor wrote:
> > 
> > > On Fri, Mar 22, 2019 at 03:28:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > >>
> > >> On Fri, Mar 22 2019, SZEDER Gábor wrote:
> > >>
> > >> > On Fri, Mar 22, 2019 at 12:11:26PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > >> >>
> > >> >> On Fri, Mar 22 2019, SZEDER Gábor wrote:
> > >> >>
> > >> >> > Commit 49bbc57a57 (commit-graph write: emit a percentage for all
> > >> >> > progress, 2019-01-19) was a bit overeager when it added progress
> > >> >> > percentages to the "Expanding reachable commits in commit graph" phase
> > >> >> > as well, because most of the time the number of commits that phase has
> > >> >> > to iterate over is not known in advance and grows significantly, and,
> > >> >> > consequently, we end up with nonsensical numbers:
> > >> >> >
> > >> >> >   $ git commit-graph write --reachable
> > >> >> >   Expanding reachable commits in commit graph: 138606% (824706/595), done.
> > >> >> >   [...]
> > >> >> >
> > >> >> >   $ git rev-parse v5.0 | git commit-graph write --stdin-commits
> > >> >> >   Expanding reachable commits in commit graph: 81264400% (812644/1), done.
> > >> >> >   [...]
> > >> >> >
> > >> >> > Therefore, don't show progress percentages in the "Expanding reachable
> > >> >> > commits in commit graph" phase.
> > >> >>
> > >> >> There's indeed a bug here as your examples show, but there *are* cases
> > >> >> where it's correct, as the commit message for my patch on "master" shows
> > >> >> there's cases where we correctly.
> > >> >>
> > >> >> So this "fixes" things by always removing the progress, why not instead
> > >> >> pass down the state to close_reachable() about what we're walking over,
> > >> >> so we can always show progress, or at least in some cases?
> > >> >
> > >> > The cases where it does display correct percentages are exceptional,
> > >> > and doesn't worth the effort to try to find out whether ther current
> > >> > operation happens to be such a case.
> > >>
> > >> It's the "write" entry point without arguments that displays the correct
> > >> progress. So not exceptional, but yeah, it's not what we use on "gc".
> > >
> > > Bit it displays the correct number only if all the reachable commits
> > > are in packfiles, which is not necessarily the case (e.g. unpacked
> > > small packs during 'git fetch').
> > 
> > No, argument-less "write" only considers packed commits.
> 
> No, it considers packed commits as starting points, and then expands
> to all reachable commits, that's what that loop in question is about.
> 
>   $ git init
>   Initialized empty Git repository in /tmp/test/.git/
>   $ echo >file
>   $ git add file
>   $ git commit -q -m initial
>   $ echo 1 >file
>   $ git commit -q -m 1 file
>   $ git rev-parse HEAD | git pack-objects
>   .git/objects/pack/pack
>   Enumerating objects: 1, done.
>   Counting objects: 100% (1/1), done.
>   ece3ff72952af2b28e048fa5c58db88c44312876
>   Writing objects: 100% (1/1), done.
>   Total 1 (delta 0), reused 0 (delta 0)
>   $ git commit-graph write
>   Computing commit graph generation numbers: 100% (2/2), done.
> 
> 
> > >> In any case, the problem is that sometimes we've walked the full set of
> > >> commits already, and some other times we haven't.
> > >
> > > ... and that we can't really be sure whether we've walked the full set
> > > of commits until after this loop.
> > 
> > I'm fairly sure we can when we start with a full walk. See my
> > explanation in <87imwbc6x8.fsf@evledraar.gmail.com>, but I may have
> > missed something.

BTW, if we know for _sure_ that we started with the full set of
commits, then we don't have to iterate over all the history in this
"Expanding reachable commits..." phase in the first place, because
there is nowhere to expand anyway.  Maybe we wouldn't even have to
call close_reachable() in that case.