Re: [PATCH 5/7] diffcore-rename: reduce jumpiness in progress counters

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 10, 2020, 02:36 UTC
Message-ID: <xmqqh7ouxu8l.fsf@gitster.c.googlers.com>
In-Reply-To: <e8257516c1f983d590b8a81fb32ecd5cb91dc737.1607223276.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Inexact rename detection works by comparing all sources to all
> destinations, computing similarities, and then finding the best matches
> among those that are sufficiently similar.

Here, you are using <sources, destinations> as contrasting pair of words, which supports my comment on 1/7 and 3/7 ;-)

Show 20 quoted lines

> However, it is preceded by exact rename detection that works by
> checking if there are files with identical hashes.  If exact renames are
> found, we can exclude some files from inexact rename detection.
>
> The inexact rename detection loops over the full set of files, but
> immediately skips those for which rename_dst[i].is_rename is true and
> thus doesn't compare any sources to that destination.  As such, these
> paths shouldn't be included in the progress counter.
> ...
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 7270eb6af48..3d637ba4645 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -587,7 +587,7 @@ void diffcore_rename(struct diff_options *options)
>  	if (options->show_rename_progress) {
>  		progress = start_delayed_progress(
>  				_("Performing inexact rename detection"),
> -				(uint64_t)rename_dst_nr * (uint64_t)rename_src_nr);
> +				(uint64_t)num_targets * (uint64_t)rename_src_nr);
>  	}

The num_targets (number of destinations) holds the "remaining" candidates after exact renames are taken care of, so this reduces the size of the matrix used to count the progress meter. OK.

Show 8 quoted lines

>  	mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_targets), sizeof(*mx));
> @@ -626,7 +626,8 @@ void diffcore_rename(struct diff_options *options)
>  			diff_free_filespec_blob(two);
>  		}
>  		dst_cnt++;
> -		display_progress(progress, (uint64_t)(i+1)*(uint64_t)rename_src_nr);
> +		display_progress(progress,
> +				 (uint64_t)dst_cnt * (uint64_t)rename_src_nr);

And this fills the progress meter by using the number of destinations that are actually considered. Between the two hunks, there is a "if the source of this destination is already known, move to next 'i'" continue, that does not update the progress meter. Changing (i+1) to dst_cnt here compensates for the reduction of the matrix size we see above.

Makes sense.  This looks good.

>  	}
>  	stop_progress(&progress);