[3/7] diffcore-rename: rename num_create to num_targets

Commit Message

Elijah Newren Dec. 6, 2020, 2:54 a.m. UTC

From: Elijah Newren <[e-mail]>

Files added since the base commit serve as targets for rename detection.
While it is true that added files can be thought of as being "created"
when they are added IF they have no pairing file that they were renamed
from, and it is true we start out not knowing what the pairings are, it
seems a little odd to think in terms of "file creation" when we are
looking for "file renames".  Rename the variable to avoid this minor
point of confusion.

Signed-off-by: Elijah Newren <[e-mail]>
---
 diffcore-rename.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Dec. 10, 2020, 2:20 a.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> From: Elijah Newren <[e-mail]>
>
> Files added since the base commit serve as targets for rename detection.
> While it is true that added files can be thought of as being "created"
> when they are added IF they have no pairing file that they were renamed
> from, and it is true we start out not knowing what the pairings are, it
> seems a little odd to think in terms of "file creation" when we are
> looking for "file renames".  Rename the variable to avoid this minor
> point of confusion.

This is probably subjective.  

I've always viewed the rename detection as first collecting a set of
deleted paths and a set of created paths, and then trying to find a
good mapping from the former into the latter, so I find num_create a
lot more intuitive than num_targets.

But the remaining elements in the latter set are counted in the
counter "rename_dst_nr", so we clearly are OK to call the elements
of the latter set "the destination" (of a rename), which contrasts
very well with "the source" (of a rename), which is how the deleted
paths are counted with rename_src_nr.

When doing -C and -C -C, the "source" set has not just deleted but
also the preimage of the modified paths, so "source" is a more
appropriate name than "delete".  From that point of view,
"destination" is a more appropriate name for "create" because it
contrasts well with "source".

You silently renamed num_create to num_targets in 1/7 without
justification while adding num_sources.  Perhaps we should go back
to that step and use num_destinations to match?  The result would be
using words <dst, src> that pair with each other much better than
introducing "target" to an existing mix of <create==dst, src> to
make it <target==dst, src>.

Elijah Newren Dec. 10, 2020, 2:25 a.m. UTC | #2

On Wed, Dec 9, 2020 at 6:20 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > From: Elijah Newren <[e-mail]>
> >
> > Files added since the base commit serve as targets for rename detection.
> > While it is true that added files can be thought of as being "created"
> > when they are added IF they have no pairing file that they were renamed
> > from, and it is true we start out not knowing what the pairings are, it
> > seems a little odd to think in terms of "file creation" when we are
> > looking for "file renames".  Rename the variable to avoid this minor
> > point of confusion.
>
> This is probably subjective.
>
> I've always viewed the rename detection as first collecting a set of
> deleted paths and a set of created paths, and then trying to find a
> good mapping from the former into the latter, so I find num_create a
> lot more intuitive than num_targets.
>
> But the remaining elements in the latter set are counted in the
> counter "rename_dst_nr", so we clearly are OK to call the elements
> of the latter set "the destination" (of a rename), which contrasts
> very well with "the source" (of a rename), which is how the deleted
> paths are counted with rename_src_nr.
>
> When doing -C and -C -C, the "source" set has not just deleted but
> also the preimage of the modified paths, so "source" is a more
> appropriate name than "delete".  From that point of view,
> "destination" is a more appropriate name for "create" because it
> contrasts well with "source".
>
> You silently renamed num_create to num_targets in 1/7 without
> justification while adding num_sources.  Perhaps we should go back
> to that step and use num_destinations to match?  The result would be
> using words <dst, src> that pair with each other much better than
> introducing "target" to an existing mix of <create==dst, src> to
> make it <target==dst, src>.

Ooh, I like it.  num_destinations it is.

Patch

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 0f8fce9293e..655a67759c8 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -502,7 +502,7 @@  void diffcore_rename(struct diff_options *options)
 	struct diff_queue_struct outq;
 	struct diff_score *mx;
 	int i, j, rename_count, skip_unmodified = 0;
-	int num_create, dst_cnt;
+	int num_targets, dst_cnt;
 	struct progress *progress = NULL;
 
 	if (!minimum_score)
@@ -567,13 +567,13 @@  void diffcore_rename(struct diff_options *options)
 	 * Calculate how many renames are left (but all the source
 	 * files still remain as options for rename/copies!)
 	 */
-	num_create = (rename_dst_nr - rename_count);
+	num_targets = (rename_dst_nr - rename_count);
 
 	/* All done? */
-	if (!num_create)
+	if (!num_targets)
 		goto cleanup;
 
-	switch (too_many_rename_candidates(num_create, rename_src_nr, options)) {
+	switch (too_many_rename_candidates(num_targets, rename_src_nr, options)) {
 	case 1:
 		goto cleanup;
 	case 2:
@@ -590,7 +590,7 @@  void diffcore_rename(struct diff_options *options)
 				(uint64_t)rename_dst_nr * (uint64_t)rename_src_nr);
 	}
 
-	mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));
+	mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_targets), sizeof(*mx));
 	for (dst_cnt = i = 0; i < rename_dst_nr; i++) {
 		struct diff_filespec *two = rename_dst[i].two;
 		struct diff_score *m;