git-arc: Improve 'list' when a review exists

[jlduran]

Summary

When a review exists in Phabricator, git-arc matches the first line of
the commit log with the title of the review. If there is a match, it
grabs the differential ID to query:

echo '{"names":["D12345"]}' | arc call-conduit -- phid.lookup

and gets the "status" (e.g., open, closed) as reported by the API
instead of displaying the output similar to the one produced by:

arc list --ansi

which appropriately shows a more accurate colored status (e.g.,
Accepted, Changes Planned, etc.)

Improve this by querying differential.revision.search, and
filtering the '.response.data[0].fields.status' response.

Also, bump the width to accommodate for "Changes Planned", which
contains 15 characters.

[jlduran]

Final patch in the series.

% git arc list main..fix-git-arc-consolidated
33118f4d8db4 No Review             : git-arc.sh: Fix typo s/Truning/Turning/
775a91cc76f6 Accepted        D46767: git-arc: Do not echo unescaped literals to jq
c305f7eb5215 Accepted        D46781: git-arc: Make patch with reviewers more portable
a75479fe401f Accepted        D46789: git-arc: Fix find_author() for external users
23587ac312ed Needs Review    D46804: git-arc: Improve 'list' when a review exists

I'm not really good at painting in the terminal. I hope there is a shorter way to achieve the same result.

The branch with all the commits (plus one trivial typo fix) is on GitHub: https://github.com/jlduran/freebsd-src/tree/fix-git-arc-consolidated

[jlduran]

Minor fixes

• Use arc_call_conduit() instead of arc call-conduit
• Use the color name

[markj]

If I understand correctly, "when a review exists" means that the commit log already has a "Differential Revision" tag?

I don't use git arc list so don't have strong feelings about this, but the patch looks okay.

tools/tools/git/git-arc.sh

242–254

Does this do the right thing if the revision doesn't exist?

[jlduran]

In D46804#1067197, @markj wrote:

If I understand correctly, "when a review exists" means that the commit log already has a "Differential Revision" tag?

This may happen when a reviewer gets the patch via git arc patch -c D12345, as git-arc automatically includes Differential Revision and Reviewed by, if applicable.
Otherwise, for instance, from the point of view of the author, it will grep the output of arc list --ansi that matches the commit title.
This patch attempts to match the output in both cases.

I don't use git arc list so don't have strong feelings about this, but the patch looks okay.

I'm following jhb's article. If you are proficient with arcanist, there is really no need.
In case someone else stumbles upon this review, in order to use arcanist with a recent PHP version, you'll need a patch that turns an error into a warning, this is the smart thing to do. However, I have opted to fix the error as I am getting acquainted with the tools.

tools/tools/git/git-arc.sh

242–254

One of the advantages of Phabricator not being actively maintained is that the API won't change! But, in all seriousness, yes, let's add a check.

[jlduran]

git arc list playbook (for my future reference)

1. Build a list of commit ids (reversed).
2. Get a list of open revisions with colors (arc list --ansi).
3. For each commit:
   1. Print the commit short hash.
   2. Look for "Differential Revision:" in the commit body (log2diff).
   3. If there is only one result, print the Differential ID, if any ( diff2status). This may happen when a reviewer uses git arc patch -c to download the review and commit locally, as this text is added automatically by git-arc:
      1. Sanity check for the Differential Revision ID.
      2. Capture the output of echo '{"names":["D12345"]}' | arc call-conduit -- phid.lookup. To check online, visit https://reviews.freebsd.org/conduit/method/phid.lookup/ and type an array of the Differential Review IDs ["D12345"] [^1].
      3. Grab the Phabricator ID. This string must exist, but check just in case.
      4. Grab the "detailed status" (printf "%s" '{"constraints": {"phids": ["PHID-DREV-bxzw4f4pkmzlyha4gosr"]}}' | arc call-conduit -- differential.revision.search | jq -r '.response.data[0].fields.status').
      5. Pretty-print the "detailed status" and the title.
      6. Stop.
   4. Check if the title of the commit matches one from the list obtained in step 2. This can yield one of three outcomes:
      1. No Review : <title>.
      2. Ambiguous Reviews: D12345, D54321. I wonder if it should also print the title
      3. The output of the line matched in step 2, without the initial * (note that in my terminal, this also removes the bold font and coloring of the Differential Revision ID).
      4. Stop.

[^1]:

NOTE: This output contains a status object, however, it does not match the "detailed status", and it is the main purpose of this revision.

[0mp]

tools/tools/git/git-arc.sh

153

Should we make those variables local?

[jlduran]

tools/tools/git/git-arc.sh

153

The problem is, we need these to be global, so they can be consumed by diff2status() below.
I can just inline the entire function within diff2status()? I just wasn't sure this is the best way to paint in the terminal.
I'll escape the variables when passed to printf in the meantime.

[jlduran]

Rebase.

[markj]

tools/tools/git/git-arc.sh

153

I don't quite understand why the colour variables need to be local when we're returning their values.

IMO a nicer helper function would look something like this:

printf_color()
{
    local esc

    color=$1
    shift
    case $color in
    red)
        esc="\033[31m"
        ;;
    ...
    esac

    printf "$esc"
    printf $@
    printf "\033[0m"
}

So one can write printf_color red "%s" "Hello, world". This way, the caller doesn't need to fuss with resetting the colour.

[imp]

tools/tools/git/git-arc.sh

153

I'd be tempted to call 'esc' 'sgr' instead, since esc is properly just '\033' in my thinking.
SGR is the name of the escape sequence. It's short for Set Graphics Rendendition.
Otherwise that's a great idea.

[jlduran]

• Update with suggestions

The suggestions were so nice, I decided to expand printf_color to accept any color name, just in case someone™ might decide to reuse it in the future.

[jlduran]

• Use a compact switch statement

[markj]

tools/tools/git/git-arc.sh

152

[jlduran]

• Add missing local variable declaration