Page MenuHomeFreeBSD

git-arc: Add a diff subcommand
ClosedPublic

Authored by dteske on Tue, Aug 11, 6:44 PM.
Tags
None
Referenced Files
F166657854: D58789.id184001.diff
Sat, Aug 15, 7:06 AM
F166654337: D58789.id183952.diff
Sat, Aug 15, 6:18 AM
F166635309: D58789.diff
Sat, Aug 15, 2:05 AM
Unknown Object (File)
Fri, Aug 14, 8:36 AM
Unknown Object (File)
Thu, Aug 13, 8:47 PM
Unknown Object (File)
Thu, Aug 13, 8:47 PM
Unknown Object (File)
Thu, Aug 13, 8:47 PM
Unknown Object (File)
Thu, Aug 13, 8:47 PM
Subscribers

Details

Summary

Show the differences between local commits and their associated
Phabricator reviews, i.e., what "git arc update" would upload. For
each commit, the review's current raw diff is applied to the commit's
parent in a temporary index and the resulting tree is compared against
the commit itself. An empty diff means the commit and the review are
in sync.

This makes it easy to check whether local amendments have diverged
from the posted review before updating it, or to confirm that a
review is current before landing.

Test Plan
  1. Hoist a review (git arc create HEAD) [from a branch]
  2. Make some changes
  3. Amend those changes to the tip of your branch (git commit --amend)
  4. Show what's different before you update: git arc diff HEAD
  5. Optionally update (git arc update HEAD) or keep amending

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dteske edited the test plan for this revision. (Show Details)

Oh man, this is a feature I have wanted!

Add copyright for diff addition and output "Comparing" line to stderr

Moving the initial "Comparing ..." line on stderr allows tools
like vimcat to properly detect syntax highlighting

Oh man, this is a feature I have wanted!

me too! I'm using it a dozen times a day now!

tools/tools/git/git-arc.sh
445

Prevailing style in this file is not to have whitespace in the subshell invocation, here and below.

463

Informational messages are conventionally printed to stdout, e.g., in gitarc__patch().

tools/tools/git/git-arc.sh
445

Busted! You caught me (trying to slow walk a style change).

I personally prefer whitespace for two reasons:

  1. It looks better
  2. vi/vim optimization [*]
  • If you're a vi/vim user, instead of pressing w twice to get at the inner contents, you can use a single W -- compare abc=$(123) to abc=$( 123 ) wherein W skips from a to next line versus the latter where W skips from a to 1. Without the whitespace, W loses its power, and your only recourse to quickly jump from a to 1 is either two w's or six l's.

I can conform to the existing style if it's really that important.

tools/tools/git/git-arc.sh
463

I have to push back here. There is system-wide precedent for sending informational messages to stderr. Least of all for user experience data points:

  • git arc diff HEAD (for example) can take time. Time to talk to remote, etc. Therefore the user experience is better even when you do git arc diff HEAD | less (or | vim or | vimcat or whatever)

But user experience aside, git arc diff ... produces a patch. The informational message does not belong in the patch for a number of reasons.

I would rather remove the informational message completely and leave the user wondering what is happening than pollute the output stream from becoming a pure diff.

tools/tools/git/git-arc.sh
445

Every style has (often subjective) justifications. To me it looks better without extra whitespace, and an extra keystroke here and there is not a big deal. More important IMO is to keep the style consistent. So I prefer to do exactly that.

463

Ok, fair enough.

Address inline comments and fix for 80c

  • Remove informational message (~markj)
  • Elide sub-shell whitespace (markj)
  • Fix an 80c issue (long-lines)
dteske marked 2 inline comments as done.

Resurrect informational message (~markj)

This revision is now accepted and ready to land.Thu, Aug 13, 5:21 PM
tools/tools/git/git-arc.sh
445

Finger/muscle memory is what it is -- and fighting your own muscle memory makes working in a foreign style abrasive. It is why you will always find files created by me fostering the embrace of W instead of letting its value fall by the wayside, ... not because the difference between one keystroke versus two, but because fighting your own fingers leads to frustration.

In reality, when your fingers jump to W and that erroneously jumps too far, you then have to reach for b or B then w w, so it's much worse than saving a single keystroke, it's saving many keystrokes, which throughout the course of a day can really stack up.

Meanwhile, vi/nvi/vim users that work in files without the whitespace never learn W and never train their fingers to use it, and thusly never have a problem because with the whitespace, two w's still produces the desired results.

So to recap, no whitespace, w users don't notice but W users suffer. With whitespace, w users again don't notice, but it enables W users to not suffer.

This revision was automatically updated to reflect the committed changes.