[v2,0/3] progress.c API users: fix bogus counting

Message

Ævar Arnfjörð Bjarmason Aug. 5, 2021, 11:01 a.m. UTC

As a split-off from the larger topic these were submitted as part of
[1] and which didn't get picked up. As I pointed out in [2] that
larger topic had some hidden untested-for flaws.

But these patches are just fixes to bogus progress bar output from
that topic. Let's consider them in isolation...

Since v1 the only changes are to the commit messages, in response to
SZEDER's feedback at
https://lore.kernel.org/git/[e-mail]/ and
https://lore.kernel.org/git/[e-mail]/;
Hopefully this update addresses all of those outstanding comments.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/

SZEDER Gábor (2):
  commit-graph: fix bogus counter in "Scanning merged commits" progress
    line
  entry: show finer-grained counter in "Filtering content" progress line

Ævar Arnfjörð Bjarmason (1):
  midx: don't provide a total for QSORT() progress

 commit-graph.c | 2 +-
 entry.c        | 7 +++----
 midx.c         | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  832a6c1f78 ! 1:  bcb13be500 commit-graph: fix bogus counter in "Scanning merged commits" progress line
    @@ Commit message
         This happens because while iterating over an array the loop variable
         is passed to display_progress() as-is, but while C arrays (and thus
         the loop variable) start at 0 and end at N-1, the progress counter
    -    must end at N.  This causes the failures of the tests
    -    'fetch.writeCommitGraph' and 'fetch.writeCommitGraph with submodules'
    -    in 't5510-fetch.sh' when run with GIT_TEST_CHECK_PROGRESS=1.
    +    must end at N. Fix this by passing 'i + 1' to display_progress(), like
    +    most other callsites do.
     
    -    Fix this by passing 'i + 1' to display_progress(), like most other
    -    callsites do.
    +    There's an RFC series to add a GIT_TEST_CHECK_PROGRESS=1 mode[1] which
    +    catches this issue in the 'fetch.writeCommitGraph' and
    +    'fetch.writeCommitGraph with submodules' tests in
    +    't5510-fetch.sh'. The GIT_TEST_CHECK_PROGRESS=1 mode is not part of
    +    this series, but future changes to progress.c may add it or similar
    +    assertions to catch this and similar bugs elsewhere.
    +
    +    1. https://lore.kernel.org/git/[e-mail]/
     
         Signed-off-by: SZEDER Gábor <[e-mail]>
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
2:  3411fe0515 = 2:  8e67712c48 midx: don't provide a total for QSORT() progress
3:  f65001627a ! 3:  c70e554e46 entry: show finer-grained counter in "Filtering content" progress line
    @@ Commit message
             paths to process, then the counter shown in the "done" progress
             line will not match the expected total.
     
    -        This would cause a BUG() in an upcoming change that adds an
    -        assertion checking if the "total" at the end matches the last
    -        progress bar update..
    -
    -        This is because both use only one filter.  (The test 'delayed
    -        checkout in process filter' uses two filters but the first one
    -        does all the work, so that test already happens to succeed even
    -        with such an assertion.)
    +        The partially-RFC series to add a GIT_TEST_CHECK_PROGRESS=1
    +        mode[1] helps spot this issue. Under it the 'missing file in
    +        delayed checkout' and 'invalid file in delayed checkout' tests in
    +        't0021-conversion.sh' fail, because both use only one
    +        filter.  (The test 'delayed checkout in process filter' uses two
    +        filters but the first one does all the work, so that test already
    +        happens to succeed even with GIT_TEST_CHECK_PROGRESS=1.)
     
           - The progress counter is updated only once per filter, not once per
             processed path, so if a filter has a lot of paths to process, then
    @@ Commit message
         path.
     
         After this change the 'invalid file in delayed checkout' in
    -    't0021-conversion.sh' would succeed with the future BUG() assertion
    -    discussed above but the 'missing file in delayed checkout' test would
    -    still fail, because its purposefully buggy filter doesn't process any
    -    paths, so we won't execute that inner loop at all (this will be fixed
    -    in a subsequent commit).
    +    't0021-conversion.sh' would succeed with the GIT_TEST_CHECK_PROGRESS=1
    +    assertion discussed above, but the 'missing file in delayed checkout'
    +    test would still fail.
    +
    +    It'll fail because its purposefully buggy filter doesn't process any
    +    paths, so we won't execute that inner loop at all, see [2] for how to
    +    spot that issue without GIT_TEST_CHECK_PROGRESS=1. It's not
    +    straightforward to fix it with the current progress.c library (see [3]
    +    for an attempt), so let's leave it for now.
    +
    +    1. https://lore.kernel.org/git/[e-mail]/
    +    2. http://lore.kernel.org/git/[e-mail]
    +    3. https://lore.kernel.org/git/[e-mail]/
     
         Signed-off-by: SZEDER Gábor <[e-mail]>
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>