Page MenuHomePhabricator

Diffusion doesn't display file moves/renames in an easily consumable/reviewable way
Closed, WontfixPublic

Assigned To
Authored By
Mnkras
Feb 19 2017, 6:08 PM
Referenced Files
F3102825: pasted_file
Feb 19 2017, 7:06 PM
F3102815: pasted_file
Feb 19 2017, 7:06 PM
F3102465: pasted_file
Feb 19 2017, 6:08 PM
F3102495: example.diff
Feb 19 2017, 6:08 PM
F3102501: pasted_file
Feb 19 2017, 6:08 PM
Subscribers

Description

I have noticed that when viewing commits that (for example) move an entire directory, Diffusion does not do a great job at displaying this in a consumable/reviewable way (especially if there are minor changes). The UI does not distinguish a file move enough from removing/adding files and doesn't clearly communicate where a file moved from, or to.

For example:

pasted_file (1×1 px, 197 KB)

I moved the folder up one directory, from this list, is is very confusing what happened, when hovering over the "V" it either says "Moved Here" or "Moved Away" and in the path, you can maybe figure out what actually happened (ui->/<-ui).

Also when viewing the actual changes in the commit, I get a bunch of added/deleted files, which is super unhelpful, as I see all the unchanged moved files contents, with the modified, moved files.

This is the diff for the commit I used in the example:

and just for reference, this is how my git client shows this diff:

pasted_file (454×1 px, 91 KB)

phabricator 9f3cde4db7654499c1ed565fbf048b8e8710f70a (Sat, Feb 18)
arcanist 3b6b523c2b236e3724a1e115f126cb6fd05fa128 (Sat, Feb 18)
phutil 5e14a5b17809be93c79554bc87bb5b6fe0f70bf4 (Sat, Feb 18)

Event Timeline

I'm not sure I understand what changes you're suggesting.

It looks like your client just does not show this information:

  • Which paths have been moved away.
  • Where moved-to files were moved from.

...and it spells out "renamed" in text instead of using a "V" code tooltip.

We once rendered these renames like this:

new/path/to/file.c
  Moved From: old/path/to/file.c

This was changed in D5708 because a contributor found this confusing.

I'm particularly not sure about these concerns:

The UI does not distinguish a file move enough from removing/adding files

It uses "V" to indicate a move, versus "A" for add and "D" for delete. This is similar to svn status, git diff --raw, etc.

and doesn't clearly communicate where a file moved from, or to.

In Phabricator you can read the path to identify both the source and destination, while the screenshot you offer for comparison does not contain the source path! It sounds like not communicating this information at all is preferable to communicating it with {src -> dst} notation?

This is the same path rename syntax that git itself uses, which is why we have selected it:

$ git commit -m wip
[master 16e0b3fbf3] wip
 1 file changed, 0 insertions( ), 0 deletions(-)
 rename src/applications/{macro/controller => }/PhabricatorMacroViewController.php (100%)

We once rendered these renames like this:

new/path/to/file.c
  Moved From: old/path/to/file.c

Personally I like that, I think a compromise to make it easier to consume, would be to try and group the moves together and showing the relation, for example
now:

a ->
b ->
a <-
b <-

to

a ->
a <-
b ->
b <-

In Phabricator you can read the path to identify both the source and destination, while the screenshot you offer for comparison does not contain the source path! It sounds like not communicating this information at all is preferable to communicating it with {src -> dst} notation?

For me, the point of showing that screenshot, is that I can clearly see that it was a move, and it was consolidated in one area, compared to my example suggestion above, seeing the src -> dst is definitely needed, (I am curious if it would be possible to do a diff of sorts, purely on the paths, where it would be like: /some/path/src/pathdest/path/file.html)

This is the same path rename syntax that git itself uses, which is why we have selected it:

$ git commit -m wip
[master 16e0b3fbf3] wip
 1 file changed, 0 insertions( ), 0 deletions(-)
 rename src/applications/{macro/controller => }/PhabricatorMacroViewController.php (100%)

I did not realize that, I understand why its done this way, and the masses like it, so who am I to argue

My other issue was with the actual diff displayed, it gave no indication of the move (probably due to the 1 line change) but it makes the diff unusable, as its the entire file:

pasted_file (678×3 px, 144 KB)

pasted_file (616×3 px, 109 KB)

The in-diff issue is described in T11482.

epriestley claimed this task.

This seems to mostly be a matter of taste, and we already have behavior similar to the behavior of git, which seems reasonable.