Do not blame folders #9392

gerhardol · 2021-07-17T20:42:48Z

Needed with #9056

Follow up to #9335
See long term goals in #7598 (comment)
(FileHistory is a sidetrack)

Proposed changes

It is possible to blame Git Tree objects (folders) from File tree.
GE shows the history for all commits affecting the folder, but just display the file and blame for one of them (the first reported file).
The name for the displayed/blamed file is added to the title bar.

If no files are seen for a certain commit, GE uses the "FileHistory" path for blame, which is not possible for a folder.
Also the first file in the folder is viewed, not matching the folder name in the title bar.
Diff tab could have been displayed but parsing of multiple diffs fail why this is hidden too.
(So no tabs shown for folder and aritificial commits.)

With #9056, this will raise an exception (that PR will show many issues in GE, a number has already been handled...)
(no popup though)

Note: That the filenames for a commit is not reported is a problem itself that likely occurs due to "--follow" option.
See https://stackoverflow.com/questions/46487476/git-log-follow-graph-skips-commits
That is ignored in this PR, the core case should be handled regardless.

Note 2: It is still possible to get the old behavior from the command line, that is ignored in this PR.

Screenshots

"Bin/" is used in these examples

Before #9335

(for all commits, no file viewed)

Before in master

#9056

After

Slightly different commits, changes in HEAD so artificial commits are shown - no tabs available

Test methodology

Manual

✒️ I contribute this code under The Developer Certificate of Origin.

mstv

👍 much clearer to me

mstv · 2021-07-17T21:35:25Z

GitUI/CommandsDialogs/FormFileHistory.cs

+            if (tabControl1.SelectedTab?.Parent is null)
+            {
+                tabControl1.SelectedTab = DiffTab;
+            }


What is the purpose of the DiffTab in FormFileHistory?

It just duplicates what can be seen on the CommitTab - with a little more space, of course.

It does not really apply when showing the history of a folder.

So, it should be hidden as the BlameTab and the ViewTab - or be removed completely.

Diff tab should show all changes to the folder. GE parsing fails to present this, probably the only place that is used.
To fix the parsing would require that the path is specially handled, which could be done, but as the info is available I will remove that.

mstv

Thank you

gerhardol · 2021-07-18T21:43:42Z

To be squash merged in one day or so.
(rebased and squashed, only refreshed the commit message.)

mstv · 2021-07-19T07:35:29Z

It seemed to work well in a short test with the GE repo.
For a private repo, the tabs "Diff", "View" and "Blame" are shown for some commits, for some not - unsystematically at the first and second glance.

OT: How to enable/disable the Artificial Commits for FormFileHistory? Why is the CommitInfoTab hidden, too?

gerhardol · 2021-07-19T21:43:19Z

It seemed to work well in a short test with the GE repo.
For a private repo, the tabs "Diff", "View" and "Blame" are shown for some commits, for some not - unsystematically at the first and second glance.

git-log --follow is not behavi9ng as most expect, it is a hack.
(Linus Torvalds want to track code changes instead, he has a point but I would preferred if something like --follow worked better.)
https://stackoverflow.com/questions/46487476/git-log-follow-graph-skips-commits

OT: How to enable/disable the Artificial Commits for FormFileHistory? Why is the CommitInfoTab hidden, too?

See also #9393

There is no commit info ever for artificial commits, compare to Browse.
It would be cleaner if CommitInfo was always merged (compare to gitk).

For FileHistory I would not bother. It is not that many steps left to get to #7598 (comment) or so.

mstv · 2021-07-19T22:16:46Z

I am not voting against merging this.

But it looks confusing:

mstv · 2021-07-19T22:21:04Z

There is no commit info ever for artificial commits, compare to Browse.

But if files of the folder have changes, would they be displayed?

At work, artificial commits were displayed although no changes.

For FileHistory I would not bother. It is not that many steps left to get to #7598 (comment) or so.

👍 Good to hear / read.

gerhardol · 2021-07-20T14:45:28Z

I am not voting against merging this.

But it looks confusing:

The current behavior creates internal exceptions, so that is definitely worse.

A third git-log would get the commits lost with --follow, adding the filenames for the commits from the full git-log parsing.
That is mentioned in the description.
I do not want to add that.

Is it better to always drop the Diff/View/Blame tabs for object type tree (folders)?
The Diff (and indirectly View) you can get from Commit, Blame for one of the files is lost.

There is no commit info ever for artificial commits, compare to Browse.

But if files of the folder have changes, would they be displayed?

In FileHistory, the Diff tab is "fixed" to one file so the Diff tab only contain info if there are changes to that specific file.
For a "folder" there is no info about a certain commit (as that is from git-log). But the artificial commits should show the same name as HEAD, updating that.

(There is a theoretical possibility that a file is renamed in HEAD, then the wrong file would be blamed, so this makes sense also for non folders).
(And again, CommitInfo could be redesigned to include artificial but I do not want to do that.)

At work, artificial commits were displayed although no changes.

Then you only viewed the history for files that are changed in HEAD.

Good to hear / read.

Blame in FileTree is likely some work, not got into details, especially if some "view-only" mode is needed.

It is also open how to enable/disable the Advanced filter for paths. That is more a design issue than work though.
You submitted #7598 and may have some thoughts if the functionality is implemented with Advanced filter...

mstv · 2021-07-20T21:17:42Z

Is it better to always drop the Diff/View/Blame tabs for object type tree (folders)?

IMO, yes.

The Diff (and indirectly View) you can get from Commit, Blame for one of the files is lost.

According to Murphy's Law, the blamed file will be the wrong one.
The blame should be opened from the (missing) context menu in the FileStatusList.

have some thoughts if the functionality is implemented with Advanced filter...

... only the abstract view from user's perspective yet
The data source for revisions may be different. But the presentation and most of the interactions should be common.

RussKie

👀 no specific comments

gerhardol · 2021-07-22T22:46:35Z

Is it better to always drop the Diff/View/Blame tabs for object type tree (folders)?

IMO, yes.

Changed this.
Note that if HEAD is not displayed, there is no initial selection and Diff etc is displayed anyway.
Will look at that in #9393, more complicated than I wish it would be.

Plan to squash and merge tomorrow.

It is possible to blame Git Tree objects (folders) from File tree, then the Diff/View/Blame could possibly only be shown for one "random" file in each commit (or none as git-log --follow do not report all commits). To decrease confusion only one of the files are shown. For artificial commits, this means that no tabs are displayed. * If a file object was blamed, diff the same file for Artificial commits as for HEAD.

gerhardol requested a review from mstv July 17, 2021 20:42

ghost assigned gerhardol Jul 17, 2021

mstv reviewed Jul 17, 2021

View reviewed changes

mstv approved these changes Jul 18, 2021

View reviewed changes

gerhardol force-pushed the feature/do-not-blame-folder branch from 4156f77 to 339c41f Compare July 18, 2021 21:42

gerhardol mentioned this pull request Jul 18, 2021

Show artificial commits also for filtered grid #9393

Merged

RussKie reviewed Jul 22, 2021

View reviewed changes

gerhardol force-pushed the feature/do-not-blame-folder branch from 9e9a239 to 1a92b61 Compare July 23, 2021 18:16

gerhardol merged commit b4c6305 into gitextensions:master Jul 23, 2021

gerhardol deleted the feature/do-not-blame-folder branch July 23, 2021 19:25

ghost added this to the 3.6 milestone Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not blame folders #9392

Do not blame folders #9392

gerhardol commented Jul 17, 2021 •

edited

mstv left a comment

mstv Jul 17, 2021

gerhardol Jul 17, 2021

mstv left a comment

gerhardol commented Jul 18, 2021

mstv commented Jul 19, 2021

gerhardol commented Jul 19, 2021

mstv commented Jul 19, 2021

mstv commented Jul 19, 2021

gerhardol commented Jul 20, 2021

mstv commented Jul 20, 2021

RussKie left a comment

gerhardol commented Jul 22, 2021

Do not blame folders #9392

Do not blame folders #9392

Conversation

gerhardol commented Jul 17, 2021 • edited

Proposed changes

Screenshots

Before #9335

Before in master

#9056

After

Test methodology

mstv left a comment

Choose a reason for hiding this comment

mstv Jul 17, 2021

Choose a reason for hiding this comment

gerhardol Jul 17, 2021

Choose a reason for hiding this comment

mstv left a comment

Choose a reason for hiding this comment

gerhardol commented Jul 18, 2021

mstv commented Jul 19, 2021

gerhardol commented Jul 19, 2021

mstv commented Jul 19, 2021

mstv commented Jul 19, 2021

gerhardol commented Jul 20, 2021

mstv commented Jul 20, 2021

RussKie left a comment

Choose a reason for hiding this comment

gerhardol commented Jul 22, 2021

gerhardol commented Jul 17, 2021 •

edited