Bug 374722 - History, compare with each other doesn't work when a resource has been renamed
Summary: History, compare with each other doesn't work when a resource has been renamed
Status: NEW
Alias: None
Product: EGit
Classification: Technology
Component: Core (show other bugs)
Version: 1.3   Edit
Hardware: Macintosh Mac OS X - Carbon (unsup.)
: P3 major with 15 votes (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
: 387183 419685 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-20 01:14 EDT by Jeremy Norris CLA
Modified: 2014-08-08 09:11 EDT (History)
15 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Norris CLA 2012-03-20 01:14:26 EDT
Build Identifier: v1.3.0.201202151440-r

If a file has been renamed, it cannot be compared with revisions in the past (via the History View).

This bug appears to be related to Bug 302549 (see comment 17).


Reproducible: Always

Steps to Reproduce:
- Enable "follow renames"
- Show history of a renamed file
- Select 2 versions
- Context menu > Compare with Each Other
results in: right side says "<filename>.java" is not in commit ...
Comment 1 Kevin Sawicki CLA 2012-03-20 14:14:29 EDT
Proposed fix pushed to https://git.eclipse.org/r/#/c/5391/
Comment 2 Stefan Lay CLA 2012-08-17 08:28:57 EDT
*** Bug 387183 has been marked as a duplicate of this bug. ***
Comment 3 Robin Stocker CLA 2012-09-30 17:41:44 EDT
Any updates on this? Compare With -> Commit (see bug 382476) would also benefit from having RenameTracker available. It would also be nice there if it could track multiple paths, not only one.
Comment 4 Christian Rösch CLA 2013-01-30 07:59:15 EST
Having noticed this bug (cannot open content of file before rename) may stop the migration to git. We have several other problems and having the history and contents of a file available after a rename is a big point.

So are there any solutions in sight?
Comment 5 Marc Herbert CLA 2013-03-14 10:02:12 EDT
Reproduced with Egit 2.1

Funny enough renames are detected by "Compare With->Previous revision" but not by "Compare With->anything else"

Workaround: Use Team->Show Annotations (= git blame)
Comment 6 Robin Rosenberg CLA 2013-05-15 14:44:43 EDT
Partial dupe of 335082, but this bugs references a patch.
Comment 7 Stephan Krull CLA 2013-05-28 08:32:59 EDT
Build Identifier: v3.0.0.201305272355

Still an issue, with a patch waiting for further operation at https://git.eclipse.org/r/#/c/5391/ for more than 12 month now.

how is the process with this ASSIGNED issue going to continue? who is in charge? as far as I can see Kevin Sawicki is not available anymore.
Comment 8 Robin Rosenberg CLA 2013-05-28 15:29:50 EDT
(In reply to comment #7)
> Build Identifier: v3.0.0.201305272355
> 
> Still an issue, with a patch waiting for further operation at
> https://git.eclipse.org/r/#/c/5391/ for more than 12 month now.
> 
> how is the process with this ASSIGNED issue going to continue? who is in
> charge? as far as I can see Kevin Sawicki is not available anymore.

I think anyone can take over stale patches, though it doesn't hurt to ask the people that worked on it before doing it. Sometimes it turns out they just forgot it and will update the patch themselves.
Comment 9 Robin Stocker CLA 2013-10-17 06:45:41 EDT
*** Bug 419685 has been marked as a duplicate of this bug. ***
Comment 10 Carsten Pfeiffer CLA 2014-07-16 03:00:07 EDT
I've been working on Robin's latest change (https://git.eclipse.org/r/#/c/5391/5) wrt Stefan's comments.

I'm having some problems in combining the PathFilter limiting the history to e.g. "all changes in parent folder of the selected resource" with the FollowFilter for the currently selected file. The result is always the same as if I had only set the PathFilter.

OTOH, I'm wondering whether this makes sense at all. Would it really be helpful to display "all changes in parent folder of the selected resource" and additionally all changes of renames of the selected resource? Same question for "Show all changes in project containing the selected resource".

Displaying the renames mostly makes sense if you only display the single file to follow, no?
Comment 11 Marc Herbert CLA 2014-07-16 03:06:34 EDT
> Displaying the renames mostly makes sense if you only display the single file to follow, no?

Not sure... did you consider a folder name change? (treated by Git as many path changes, one per file)
Comment 12 Robin Stocker CLA 2014-07-16 03:09:54 EDT
(In reply to Carsten Pfeiffer from comment #10)
> I've been working on Robin's latest change
> (https://git.eclipse.org/r/#/c/5391/5) wrt Stefan's comments.

Cool.

About your question, I'm not sure what you're asking. But I'd be happy with a solution for comment 0 (fix it for when we show the history for a single file) and worry about the complicated cases later.
Comment 13 Carsten Pfeiffer CLA 2014-07-16 03:15:37 EDT
(In reply to Marc Herbert from comment #11)
> Not sure... did you consider a folder name change? (treated by Git as many
> path changes, one per file)

The thing is, you can only follow renames of a *single file*. This is also what git log --follow says:
"Continue listing the history of a file beyond renames (works only for a single file)."

So following directory renames is not possible, AFAICS.
Comment 14 Carsten Pfeiffer CLA 2014-07-16 03:28:55 EDT
(In reply to Robin Stocker from comment #12)
> About your question, I'm not sure what you're asking. But I'd be happy with
> a solution for comment 0 (fix it for when we show the history for a single
> file) and worry about the complicated cases later.

I see -- this comment is not about displaying the rename-commits in addition to the other, but rather about the actions in the context menu ("Compare with each other").
Comment 15 Marc Herbert CLA 2014-07-16 03:47:30 EDT
> So following directory renames is not possible, AFAICS.

I think you meant: "too computationally expensive" ;-)
Comment 16 Carsten Pfeiffer CLA 2014-07-16 04:06:32 EDT
(In reply to Carsten Pfeiffer from comment #10)
> I'm having some problems in combining the PathFilter limiting the history to
> e.g. "all changes in parent folder of the selected resource" with the
> FollowFilter for the currently selected file. The result is always the same
> as if I had only set the PathFilter.

So here's the problem:
When we want to combine a FollowFilter with a PathFilter for e.g. "Show all changes in parent folder of the selected resource", we would use an OrFilter.

But this breaks the entire rename detection because of this code in RewriteTreeFiler, line 138:
  if (adds > 0 && tw.getFilter() instanceof FollowFilter) {

tw.getFilter() would in our case be the OrFilter, so the condition would never be true.

There's no way to find our nested FollowFilter inside a TreeFilter and no way to plug our own handling in there (the entire RewriteTreeFilter is package private). Creating a custom FollowFilter with an integrated OrFilter is almost possible (package private constructor), but still hackish.

Any suggestion?
Comment 17 Carsten Pfeiffer CLA 2014-07-16 04:10:45 EDT
Oh, and btw, even if we did this, the rename detection for "compare with each other" would still only work for the single selected file.

For the case where you show all commits inside a folder, project or entire repository, you might want to compare other files that were also renamed (but not selected).

So maybe we should rather perform an additional walk on demand, with the file to be compared.
Comment 18 Robin Rosenberg CLA 2014-07-16 04:56:55 EDT
(In reply to Carsten Pfeiffer from comment #17)
> Oh, and btw, even if we did this, the rename detection for "compare with
> each other" would still only work for the single selected file.
> 
> For the case where you show all commits inside a folder, project or entire
> repository, you might want to compare other files that were also renamed
> (but not selected).
> 
> So maybe we should rather perform an additional walk on demand, with the
> file to be compared.

I think we need a directory rename detector for this. I couldn't find the discussion in the Git-ML archives, but I'm sure there is a reason for not having it in C Git, such as not being reliable enough, too slow or semantically fuzzy. Those reasons may or may not be a reason for not trying it in EGit.

There may also be different interpretations of what following a directory rename means, e.g.

- Only follow the directory when we find that a lot of files are moved to another directory and the original directory is no more

or

- We could start by looking at one directory. If any file is moved to another directory we start following the destination directory too.

That we do "instanceof FollowFilter" in the code doesn't mean we can't change it. Most solutions to interesting problems in EGit has a partial solution i JGit. Following multiple paths may be a solution here.. A risk is that it's too expensive to be worth doing, but then git and EGit users may have different opinions on what is "too" expensive. We may even cap the amount of CPU being used to do this. If it's too CPU-intensive, just stop, leaving a message in the history view somewhere.
Comment 19 Carsten Pfeiffer CLA 2014-07-16 06:35:56 EDT
(In reply to Robin Rosenberg from comment #18)

Good points. I'm sure we find a solution, I'm just asking here for a direction because I think the problem is growing.

> I think we need a directory rename detector for this.

I don't think this will solve the problem with "Compare with each other". There are many different scenarios to support.

Here's a small list, for a start:
1) "Compare with previous version" (in the commit viewer): should find the previous version across renames
2a) "Compare with each other": should compare the files even if they were renamed
2b) "Compare with each other": actually, IMHO it should allow comparing completely different files (why should they have anything in common to be comparable?)
3) "Compare with each other in tree": same as above, necessary to compare single files in a multi-file commit

All this is easy if you have focused a single file and show only its commits. If you show commits of a folder, project or the entire repository, the selected file becomes mostly irrelevant. If the history view displays commits to files A,B,C and D, why should I only be able to compare A and B, but not C and D?

We cannot really walk the repository and detect and remember all renames to all files, so if we want to support the above, we have to calculate the renames on demand.

For the "Compare with each other" case, it would not even be necessary anymore, if we support 2b). Just compare the files, if the user requested it.
Comment 20 Robin Rosenberg CLA 2014-07-16 08:07:13 EDT
I think https://git.eclipse.org/r/#/c/5391/5 should be completed (anyone can grab it and continue), then we'll see what's still missing
Comment 21 Carsten Pfeiffer CLA 2014-07-16 08:16:48 EDT
(In reply to Robin Rosenberg from comment #20)
> I think https://git.eclipse.org/r/#/c/5391/5 should be completed (anyone can
> grab it and continue), then we'll see what's still missing

Well, that's the reason I'm posting here: ATM you cannot combine the different "Show all changes..." options with a follow filter. As soon as you do this, rename tracking does not work at all anymore, because of RewriteTreeFilter doing this instanceof check.

I think it would be best to continue without the requirement to combine the follow filter with the other options. That would already be quite an improvement over the current situation.

Then we can open another issue for the remaining cases.
Comment 22 Robin Rosenberg CLA 2014-07-16 08:19:59 EDT
(In reply to Carsten Pfeiffer from comment #21)
> (In reply to Robin Rosenberg from comment #20)
> > I think https://git.eclipse.org/r/#/c/5391/5 should be completed (anyone can
> > grab it and continue), then we'll see what's still missing
> 
> Well, that's the reason I'm posting here: ATM you cannot combine the
> different "Show all changes..." options with a follow filter. As soon as you
> do this, rename tracking does not work at all anymore, because of
> RewriteTreeFilter doing this instanceof check.
> 
> I think it would be best to continue without the requirement to combine the
> follow filter with the other options. That would already be quite an
> improvement over the current situation.

I can buy that.
Comment 23 Marc Herbert CLA 2014-07-16 11:02:14 EDT
Just for the record, background info about renames including Linus' "naked" opinions:

https://git.wiki.kernel.org/index.php/Git_FAQ#Why_does_Git_not_.22track.22_renames.3F
http://thread.gmane.org/gmane.comp.version-control.git/27/focus=217
http://www.wincent.com/a/about/wincent/weblog/archives/2007/07/a_look_back_bra.php


> Those reasons may or may not be a reason for not trying it in EGit.

If Egit can implement better rename detections than mainline git then great! Plus: rich graphical interfaces are here for a reason. Only fear: minor risk of breaking consistency between the two. In my team/job I find very important the ability to go back and forth between different git user interfaces, which unfortunately requires preserving the names of the original git commands and concepts, even when their choice was very poor.
Comment 24 Nobody - feel free to take it CLA 2014-07-28 17:04:46 EDT
I've installed patchset 6, which is working well.

> I think it would be best to continue without the requirement to combine the
> follow filter with the other options. That would already be quite an
> improvement over the current situation.

> Then we can open another issue for the remaining cases.

I have to agree! In respect of "comparing each other", comparing the currently opened file is IMHO the most important use case. And that is working right now with Kevin's and Carsten's implementation.
Comment 25 Stefan Lay CLA 2014-07-29 09:55:29 EDT
(In reply to Carsten Pfeiffer from comment #19)

> All this is easy if you have focused a single file and show only its
> commits. If you show commits of a folder, project or the entire repository,
> the selected file becomes mostly irrelevant. If the history view displays
> commits to files A,B,C and D, why should I only be able to compare A and B,
> but not C and D?

I do not quite agree that the selected file becomes mostly irrelevant if I show commits of a folder, project or the entire repository. The selection, or in other words the input of the History View, is relevant for all the comparison actions. I normally always want to see the history of the whole repository, but I would nevertheless want to compare the content of the file which is input of the History View also across renames.


> 
> We cannot really walk the repository and detect and remember all renames to
> all files, so if we want to support the above, we have to calculate the
> renames on demand.
> 
> For the "Compare with each other" case, it would not even be necessary
> anymore, if we support 2b). Just compare the files, if the user requested it.

Yes, that would be useful, but does not resolve the problem that per default the versions of the input of the History View should be compared.
Comment 26 Stefan Lay CLA 2014-07-29 10:01:10 EDT
(In reply to Carsten Pfeiffer from comment #21)
> (In reply to Robin Rosenberg from comment #20)
> > I think https://git.eclipse.org/r/#/c/5391/5 should be completed (anyone can
> > grab it and continue), then we'll see what's still missing
> 
> Well, that's the reason I'm posting here: ATM you cannot combine the
> different "Show all changes..." options with a follow filter. As soon as you
> do this, rename tracking does not work at all anymore, because of
> RewriteTreeFilter doing this instanceof check.
> 
> I think it would be best to continue without the requirement to combine the
> follow filter with the other options. That would already be quite an
> improvement over the current situation.
> 
> Then we can open another issue for the remaining cases.

I agree to that. The proposed change is already very useful for the case that the filter is set to "Show all changes to selected resources and its children".
Comment 27 Carsten Pfeiffer CLA 2014-07-29 10:09:12 EDT
(In reply to Stefan Lay from comment #25)
> I do not quite agree that the selected file becomes mostly irrelevant if I
> show commits of a folder, project or the entire repository. The selection,
> or in other words the input of the History View, is relevant for all the
> comparison actions. I normally always want to see the history of the whole
> repository, but I would nevertheless want to compare the content of the file
> which is input of the History View also across renames.

IMHO, the more commits you show (folder -> project -> repository), the less relevant becomes the selected file (input). If I wanted to compare the selected file with a previous version I wouldn't manually search through the commits of the entire folder/project/repository. I would only display the relevant commits.

I for one use the folder, project and repository modes to display *additional* commits, unrelated to the selected file. And hence my request to support comparisons between any kind of file in any commit.

> Yes, that would be useful, but does not resolve the problem that per default
> the versions of the input of the History View should be compared.

But is that so important given that this comparison is only possible for a fraction of all visible commits? I for one would find "compare any file in the history" much more useful.
Comment 28 Stefan Lay CLA 2014-08-08 09:11:01 EDT
Submitted https://git.eclipse.org/r/#/c/5391/.

This fixes the issue for all the menu entries in the context menu of the commit list of the history view if the filter is set to "Show all changes to selected resources and its children".

What's still missing is the entries of the submenu of the file list on the lower right ("Open Workspace Version", "Compare With Workspace").