Page MenuHomeSoftware Heritage

Use a shorter format for the git logs shown on Phabricator.
Changes PlannedPublic

Authored by vlorentz on Mar 31 2020, 4:28 PM.

Details

Reviewers
None
Group Reviewers
Reviewers
Summary

The current output can make the page of a diff very long.

Diff Detail

Repository
rCJSWH Jenkins jobs
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11507
Build 17440: arc lint + arc unit

Event Timeline

vlorentz created this revision.Mar 31 2020, 4:28 PM

that might be a tad too consise...

vlorentz added a comment.EditedMar 31 2020, 4:51 PM

but the current format is so long... just look at D2879 with the comment history uncollapsed. The comment history is almost as long as the diff's content itself

but the current format is so long... just look at D2879 with the comment history uncollapsed. The comment history is almost as long as the diff's content itself

yes, i know.
But i think that's an edge case...

of 1. that diff should be splitted if possible

  1. Also the pb might not be with the git commit too long...

More with how phabricator decided to show the actual diff after the comments.
That makes it impossible to find the start of the diff determinic...
And scroll and scroll you go...

It'd be great if we could have the diff first and the comments after....

Anyway...
Let's see what other people thinks of this.
I'll comply with whatever the team decides.

yes, i know.
But i think that's an edge case...

of 1. that diff should be splitted if possible

  1. Also the pb might not be with the git commit too long...

I agree with @ardumont, this is an edge case.

Nervertheless, maybe a simple solution would be (quoting Remarkup reference)

You can use lines=N to limit the vertical size of a chunk of code, and name=some_name.ext to give it a name. For example, this:

lang=html, name=example.html, lines=12, counterexample
...

A scrollbar will then appear if the git log is too long.

Nervertheless, maybe a simple solution would be (quoting Remarkup reference)

You can use lines=N to limit the vertical size of a chunk of code, and name=some_name.ext to give it a name. For example, this:

lang=html, name=example.html, lines=12, counterexample
...

A scrollbar will then appear if the git log is too long.

Excellent!

A scrollbar will then appear if the git log is too long.

Excellent!

quite!

olasd added a subscriber: olasd.Apr 1 2020, 10:11 AM

Having the full commit history displayed when a patch is applied is very important to me. I agree that the display could be improved. However here's my rationale for showing it in full:

It shows exactly what stack of diffs has been applied (when you previously had to fish the information out of a console log, trying to decrypt what arcanist was trying to do), so you know what's actually been tested. We also have a history of poorly wording and poorly separating git commits, and I'd like us to improve that. Phabricator's UI is awful in that regard, hiding commit messages away in a default collapsed box in a default hidden tab.

Until everyone starts really splitting their diffs commit by commit I don't think we should do oneline logs.

I'd be +0 for collapsing the box if it's too long; I feel people will keep ignoring it when I really would like it to jump at them.

It'd be great if we could have the diff first and the comments after....

On a Diff, you can use J to jump to the next code change (see ? for other shortcuts). But I'd argue that comments are more important than the code, in that they show you what's been reviewed and what hasn't been, as well as how the diff came to be in that state.

olasd added a comment.Apr 1 2020, 10:31 AM

I just thought of a possible compromise. #ShowerThoughts

The current display mixes the "dependency stack" (of diffs that need to be applied before the current one) and the diff itself. I think it would be appropriate to display a full log of the commits associated with the current diff (letting us review full commit messages conveniently), while only showing a oneline diff of the dependency stack (reminding us of the explicit+implicit dependencies needed to apply the current diff).

I'm also ok to keep the verbosity of the merge/rebase process out of the common message, only showing it when it fails.

This should cut down on the comment length significantly.

vlorentz planned changes to this revision.Apr 1 2020, 11:34 AM

...
Until everyone starts really splitting their diffs commit by commit I don't think we should do oneline logs.

I think we are doing that already.
Except for some edge case usage like 2879 here (i intended to split once green, and i did btw).

I'd be +0 for collapsing the box if it's too long; I feel people will keep ignoring it when I really would like it to jump at them.

fine by me.

I knew there was a good reason behind the fact you activated this configuration in the first place. I just did not know what exactly.
Thanks for clarifying.

On a Diff, you can use to jump to the next code change (see for other shortcuts).

Well that does not work for me unfortunately yet. That's on me most though (well my browser's configuration i think).
Thanks for the hint!

But I'd argue that comments are more important than the code, in that they show you what's been reviewed and what hasn't been, as well as how the diff came to be in that state.

yes, theoritically that's true.

But pragmatically, as soon as the diff is a tad old with too many comments on it.
That becomes unreadable.

The comments are still there, but no longer in their initial context...

...
This should cut down on the comment length significantly.

Sounds like a good compromise ;)

On a Diff, you can use to jump to the next code change (see for other shortcuts).

Well that does not work for me unfortunately yet. That's on me most though (well my browser's configuration i think).
Thanks for the hint!

That's now fixed on my side (i removed my browser's unused default binding
in my config). And it works. Thanks again.