Jeff King [off-list ref] writes:
OK. What next? Did the patch I sent make sense? Do you want a cleaned up
version with a commit message and signoff, or does it need work?
It just hit me that breaking (as in diffcore-break) a filepair that is a
typechange may yield the same result, and if it works, that would be
conceptually cleaner. After all, a typechange is the ultimate form of
total rewriting (the similarity between the preimage and the postimage
is very low -- even their types are different, let alone contents).
Compared to that, the rename_used++ in that codepath you touched feels
more magic to me.
On Thu, Nov 29, 2007 at 05:10:45PM -0800, Junio C Hamano wrote:
quoted
OK. What next? Did the patch I sent make sense? Do you want a cleaned up
version with a commit message and signoff, or does it need work?
It just hit me that breaking (as in diffcore-break) a filepair that is a
typechange may yield the same result, and if it works, that would be
conceptually cleaner. After all, a typechange is the ultimate form of
total rewriting (the similarity between the preimage and the postimage
is very low -- even their types are different, let alone contents).
Compared to that, the rename_used++ in that codepath you touched feels
more magic to me.
I have always been a bit confused about diffcore-break, so I am probably
misunderstanding what you mean. But are you saying that
diffcore-break.c:should_break should return 1 for typechanges? If so,
that does not have the desired effect.
-Peff
Jeff King [off-list ref] writes:
I have always been a bit confused about diffcore-break, so I am probably
misunderstanding what you mean. But are you saying that
diffcore-break.c:should_break should return 1 for typechanges?
What I had in mind was to do something like that in spirit, but instead
break such a filepair inside diffcore-rename (iow, even when the user
did not say -B) early on.
But after re-reading your patch and the surrounding code, that is
more or less what you are doing (without actually recording the extra
broken pair to be merged back later).
If we did the "automatic break of typechange" early, instead of your
patch, when we come to the register_rename_src() loop, one half of the
broken pair (i.e. "create a new symlink here") will be processed
in this part of the loop:
if (!DIFF_FILE_VALID(p->one)) {
if (!DIFF_FILE_VALID(p->two))
continue; /* unmerged */
else if (options->single_follow &&
strcmp(options->single_follow, p->two->path))
continue; /* not interested */
else
locate_rename_dst(p->two, 1);
}
and rename_dst is registered here. The other half (i.e. "remove the
regular file") will be caught by this part in the loop:
else if (!DIFF_FILE_VALID(p->two)) {
/*
* If the source is a broken "delete", and
* they did not really want to get broken,
* that means the source actually stays.
* So we increment the "rename_used" score
* by one, to indicate ourselves as a user
*/
if (p->broken_pair && !p->score)
p->one->rename_used++;
register_rename_src(p->one, p->score);
}
to register a source candidate.
Instead your patch does that with a single:
+ else if (DIFF_PAIR_TYPE_CHANGED(p)) {
+ p->one->rename_used++;
+ register_rename_src(p->one, p->score);
+ }
which is essentially doing the same thing but only for the "remove the
regular file" half. One has to wonder how the lack of handling the
other half affects the outcome and still produce a result more intuitive
than the current code.
In your test case, the "new" symlink won't have any similar symlink that
is removed from the preimage, so registering it as a rename destination
would not make a difference (it will say "no match found, so create this
as usual"), but I am not convinced if that would work well in general.
On Fri, Nov 30, 2007 at 06:36:56PM -0800, Junio C Hamano wrote:
quoted
I have always been a bit confused about diffcore-break, so I am probably
misunderstanding what you mean. But are you saying that
diffcore-break.c:should_break should return 1 for typechanges?
What I had in mind was to do something like that in spirit, but instead
break such a filepair inside diffcore-rename (iow, even when the user
did not say -B) early on.
Ah, I see. BTW, I totally screwed up the tests I did earlier. Returning
1 from should_break _does_ produce the same results for my simple case
(copy + typechange).
But after re-reading your patch and the surrounding code, that is
more or less what you are doing (without actually recording the extra
broken pair to be merged back later).
I don't think we need to, because they are never actually "broken"; we
simply consider the source a candidate for renaming, but keep the pair
together to note the typechange.
which is essentially doing the same thing but only for the "remove the
regular file" half. One has to wonder how the lack of handling the
other half affects the outcome and still produce a result more intuitive
than the current code.
AIUI, because we never broke the pair in the first place, we don't need
to look for a source for that dest (the "add a new symlink" half). It's
already part of the same filepair.
Whether this is by design or simply a happy accident that we record both
renames and typechanges in diff_filepairs, I'm not sure. Or perhaps I'm
totally misunderstanding how the breaking works.
In your test case, the "new" symlink won't have any similar symlink that
is removed from the preimage, so registering it as a rename destination
would not make a difference (it will say "no match found, so create this
as usual"), but I am not convinced if that would work well in general.
I don't know that it makes a difference. We are impacting only a
'typechange', which implies that we have a filepair in which both p->one
and p->two are valid; thus, the current code doesn't use it as a rename
dst at all.
-Peff