Page MenuHomePhabricator

arc land doesn't work if visual diff tool is configured
Closed, ResolvedPublic

Description

arc land outputs:
Merging local "master" into "origin/master" produces an empty diff. This usually means these changes have already landed.

If I run git status, I get:

On branch master
Your branch is ahead of 'origin/master' by 1 commit.

The reason arc land is failing is because git diff HEAD -- returns no result, as if there was no change.
The reason THAT happened is that I've configured it to work with a different diff tool (Meld - a great visual diff).
In my .gitconfig I have:

[diff]
external = /home/ofer/opt/git_meld.py

Once I reverted back to the default diff tool, it worked!

Here's the fix to make it work even with a different diff tool configured:
In:
https://github.com/phacility/arcanist/blob/3d7ac867f53892b8210339bbb0c5fd91e0e36d78/src/land/ArcanistGitLandEngine.php#L184
Change:
'diff HEAD --'
Into:
'diff HEAD --no-ext-diff --'

Aviv Eyal says, "There are a few other places where we just git diff, so maybe there's a better solution, or maybe we just need to find them all."

Thanks!

Event Timeline

avivey added a project: Arcanist.
avivey added a subscriber: avivey.

Looking at the history of git, it looks like --no-ext-diff was introduced circa 1.5.3 (2007).

I think we already add this flag in most cases (via ArcanistGitAPI->getDiffBaseOptions()), but the specific check in arc land doesn't go through that code (since it doesn't really care about the diff, just if the are any changes at all).

A better call might be getUncommittedStatus(), basically this diff:

diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php
index 0e79850..8ac0126 100644
--- a/src/land/ArcanistGitLandEngine.php
+++ b/src/land/ArcanistGitLandEngine.php
@@ -181,9 +181,9 @@ final class ArcanistGitLandEngine
           $this->getTargetFullRef()));
     }
 
-    list($changes) = $api->execxLocal('diff HEAD --');
-    $changes = trim($changes);
-    if (!strlen($changes)) {
+    $api->reloadWorkingCopy();
+    $changes= $api->getUncommittedStatus();
+    if (!$changes) {
       throw new Exception(
         pht(
           'Merging local "%s" into "%s" produces an empty diff. '.

This might be relatively expensive compared to just running git diff, though, as it will also rebuild untracked/unstaged files.

I'll just stick --no-ext-diff in here for now and maybe we can clean this up as part of broader upcoming arc changes.

Thanks!
Evan, do I get an extra free month for using phacility?
;-)

I'd theoretically be open to doing that kind of thing, but I worry it would waste a lot of our time in arguing over whether or not something is really a good bug report or not. We already see a lot of this with our security bounty program and the definition of a valid report there is very narrow.

We also don't really want to incentivize users to report bugs to us purely because they exist. For example, if we make a typo somewhere and 5 people immediately report as a "bug" to earn service credit, a lot of time is being wasted and no one is really benefiting. Currently, users benefit primarily by getting bugs fixed, so they mostly report only bugs which affect them. I think this is about the right balance: if a bug isn't important enough that you're willing to describe it to get it fixed, it's probably usually not the highest priority thing we could be working on. (Stuff like typos will get fixed sooner or later when we notice them, and doesn't really impair anyone's ability to use the software in the meantime.)

More generally, I'm not aware of any projects which run actual "any-bug" bounties (for reports of non-security bugs). If you could point at an existing, well-run, generally successful program which gives users a direct financial reward of some type for reporting any bug, I'd be more open to thinking about this. It's possible that they exist and I'm simply not aware of them, but I suspect the administrative overhead of such a program would make it infeasible to really run seriously in the long term.

So, at least for now, you just get your bug fixed.

That's perfectly fine. Thanks for the thorough answer! I see your point and indeed, I think this strikes a better balance.
Keep on doing this great work and building a product that helps everyone have fewer bugs! ;-)

I think this should be fixed in HEAD of master now. It should promote to stable pretty shortly, probably. Let us know if you're still seeing issues after updating, and thanks for the report!