[prev in list] [next in list] [prev in thread] [next in thread]
List: git
Subject: Re: Fix a pathological case in git detecting proper renames
From: Kumar Gala <galak () kernel ! crashing ! org>
Date: 2007-11-30 1:18:04
Message-ID: AC8DD8B5-C41E-4195-9CD6-E1A327F1878A () kernel ! crashing ! org
[Download RAW message or body]
On Nov 29, 2007, at 6:41 PM, Linus Torvalds wrote:
>
>
> On Thu, 29 Nov 2007, Jeff King wrote:
>>
>> I think it will get worse, because you are simultaneously calculating
>> all of the similarity scores bit by bit rather than doing a loop.
>> Though
>> perhaps you mean at the end you will end up with a list of src/dst
>> pairs
>> sorted by score, and you can loop over that.
>
> Well, after thinking about this a bit, I think there's a solution
> that may
> work well with the current thing too: instead of looping just *once*
> over
> the list of rename pairs, loop twice - and simply refuse to do
> copies on
> the first loop.
>
> This trivial patch does that, and turns Kumar's test-case into a
> perfect
> rename list.
Glad I can be of use for something :)
> It's not pretty, it's not smart, but it seems to work. There's
> something
> to be said for keeping it simple and stupid.
>
> And it should not be nearly as expensive as it may _look_. Yes, the
> loop
> is "(i = 0; i < num_create * num_src; i++)", but the important part is
> that the whole array is sorted by rename score, and we have a
>
> if (mx[i].score < minimum_score)
> break;
>
> in it, so uthe loop actually would tend to terminate rather quickly.
>
> Anyway, Kumar, the thing to take away from this is:
>
> - git really doesn't even *care* about the whole "rename detection"
> internally, and any commits you have done with renames are totally
> independent of the heuristics we then use to *show* the renames.
>
> - the rename detection really is for just two reasons: (a) keep humans
> happy, and keep the diffs small and (b) help automatic merging
> across
> renames. So getting renames right is certainly good, but it's more
> of a
> "politeness" issue than a "correctness" issue, although the merge
> portion of it does matter a lot sometimes.
Yeah both of these dawned on me after my brain started working and
thinking about what you were talking about when you say git manages
'content' and thinking about how the content management correlates to
the diffs we generate.
> - the important thing here is that you can commit your changes and not
> worry about them being somehow "corrupted" by lack of rename
> detection,
> even if you commit them with a version of git that doesn't do rename
> detection the way you expected it. The rename detection is an
> "after-the-fact" thing, not something that actually gets saved in
> the
> repository, which is why we can change the heuristics _after_ seeing
> examples, and the examples magically correct themselves!
>
> - try out the two patches I've posted, and see if they work for you.
> They
> pass the test-suite, and the output for your example commit looks
> sane,
> but hey, if you have other test-cases, try them out.
Only started using -M when a co-worker informed me about it. But if I
see other cases in the future I'll let you guys know.
> Here's Kumar's pretty diffstat with both my patches:
>
> Makefile | 6 +++---
> board/{cds => freescale}/common/cadmus.c | 0
> board/{cds => freescale}/common/cadmus.h | 0
> board/{cds => freescale}/common/eeprom.c | 0
> board/{cds => freescale}/common/eeprom.h | 0
> board/{cds => freescale}/common/ft_board.c | 0
> board/{cds => freescale}/common/via.c | 0
> board/{cds => freescale}/common/via.h | 0
> board/{cds => freescale}/mpc8541cds/Makefile | 0
> board/{cds => freescale}/mpc8541cds/config.mk | 0
> board/{cds => freescale}/mpc8541cds/init.S | 0
> board/{cds => freescale}/mpc8541cds/mpc8541cds.c | 0
> board/{cds => freescale}/mpc8541cds/u-boot.lds | 4 ++--
> board/{cds => freescale}/mpc8548cds/Makefile | 0
> board/{cds => freescale}/mpc8548cds/config.mk | 0
> board/{cds => freescale}/mpc8548cds/init.S | 0
> board/{cds => freescale}/mpc8548cds/mpc8548cds.c | 0
> board/{cds => freescale}/mpc8548cds/u-boot.lds | 4 ++--
> board/{cds => freescale}/mpc8555cds/Makefile | 0
> board/{cds => freescale}/mpc8555cds/config.mk | 0
> board/{cds => freescale}/mpc8555cds/init.S | 0
> board/{cds => freescale}/mpc8555cds/mpc8555cds.c | 0
> board/{cds => freescale}/mpc8555cds/u-boot.lds | 4 ++--
> 23 files changed, 9 insertions(+), 9 deletions(-)
>
> and here it is before:
>
> Makefile | 6 +-
> board/cds/mpc8548cds/Makefile | 60 -----
> board/cds/mpc8555cds/Makefile | 60 -----
> board/cds/mpc8555cds/init.S | 255
> --------------------
> board/cds/mpc8555cds/u-boot.lds | 150
> ------------
> board/{cds => freescale}/common/cadmus.c | 0
> board/{cds => freescale}/common/cadmus.h | 0
> board/{cds => freescale}/common/eeprom.c | 0
> board/{cds => freescale}/common/eeprom.h | 0
> board/{cds => freescale}/common/ft_board.c | 0
> board/{cds => freescale}/common/via.c | 0
> board/{cds => freescale}/common/via.h | 0
> board/{cds => freescale}/mpc8541cds/Makefile | 0
> board/{cds => freescale}/mpc8541cds/config.mk | 0
> board/{cds => freescale}/mpc8541cds/init.S | 0
> board/{cds => freescale}/mpc8541cds/mpc8541cds.c | 0
> board/{cds => freescale}/mpc8541cds/u-boot.lds | 4 +-
> .../mpc8541cds => freescale/mpc8548cds}/Makefile | 0
> board/{cds => freescale}/mpc8548cds/config.mk | 0
> board/{cds => freescale}/mpc8548cds/init.S | 0
> board/{cds => freescale}/mpc8548cds/mpc8548cds.c | 0
> board/{cds => freescale}/mpc8548cds/u-boot.lds | 4 +-
> .../mpc8541cds => freescale/mpc8555cds}/Makefile | 0
> board/{cds => freescale}/mpc8555cds/config.mk | 0
> .../mpc8541cds => freescale/mpc8555cds}/init.S | 0
> board/{cds => freescale}/mpc8555cds/mpc8555cds.c | 0
> .../mpc8541cds => freescale/mpc8555cds}/u-boot.lds | 4 +-
> 27 files changed, 9 insertions(+), 534 deletions(-)
>
> so it certainly makes the diffs prettier.
Much better. I love with open source development works.
- k
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic