Page MenuHomePhabricator

arc diff: relax handling of submodule diffs in working copy
Closed, InvalidPublic

Description

arc diff does not deal well with submodule diffs in the working copy.

For example, if your working copy has submodule changes, and you run arc diff, it will present the option to ignore the diffs.

If you accept the offer, arc diff will try to git stash pop an unrelated stash into your working copy, and fail hard when it (probably) doesn't merge correctly, or if you have no commits in your git stash. For example:

You have uncommitted changes in this working copy.

  Working copy: /u/j/editor/

  Unstaged changes in working copy:
    Frameworks     # this is a submodule!


    Do you want to create a new commit with these changes? [y/N]  N

Stashing uncommitted changes. (You can restore them with `git stash pop`).


    You have not specified any reviewers. Continue anyway? [y/N] y

Linting...
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
Exception
Command failed with error #1!
COMMAND
git stash pop

[...lots of output from failed merges...]

STDERR
(empty)
(Run with `--trace` for a full exception trace.)

I am currently running with a patch which relaxes the handling of submodule diffs by passing --ignore-submodules to git diff and git diff-files in two cases. A config option is provided for anyone who really likes the current behavior. However, the current behavior is not considered useful by anyone in my organization, so I'd be happy to make --ignore-submodules the default.

On the flip side, this change is very useful to us, as we often are running with submodule modifications, and undoing all of them in order to create each arc diff is burdensome and a big time sink. Pretty much everyone in my organization has some form of this patch in their arcanist clone.

Event Timeline

skrap updated the task description. (Show Details)
skrap added a project: Arcanist.
skrap added a subscriber: skrap.

A preliminary patch which accomplishes my goals is below. It's provided as an example, and I'm happy to discuss other approaches or make any modifications deemed appropriate.

diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index f5ceae9..c6324dc 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -447,6 +447,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       // output in the visual diff.
       '--no-textconv',
     );
+    if (!$this->getConfigurationManager()->getConfigFromAnySource('git.strict-submodule-diffs-in-working-copy')) {
+        $options[] = '--ignore-submodules';
+    }
     return implode(' ', $options);
   }
 
@@ -650,10 +653,13 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       ));
 
     // Unstaged changes
-    $unstaged_future = $this->buildLocalFuture(
-      array(
-        'diff-files --name-only',
-      ));
+    $diff_files_cmd = array(
+        'diff-files --name-only'
+      );
+    if (!$this->getConfigurationManager()->getConfigFromAnySource('git.strict-submodule-diffs-in-working-copy')) {
+      $diff_files_cmd[0] .= ' --ignore-submodules'; 
+    }
+    $unstaged_future = $this->buildLocalFuture($diff_files_cmd);
 
     $futures = array(
       $uncommitted_future,
epriestley edited subscribers, added: skrap; removed: xdxdxd.
epriestley claimed this task.
epriestley added a subscriber: epriestley.

Please describe the root problem you are facing when filing feature requests, not just your desired solution:

https://secure.phabricator.com/book/phabcontrib/article/feature_requests/#describe-problems

In particular, this request does not explain why submodules are frequently dirty. The best solution to this problem completely depends on that.

Some dirtiness is handled more broadly and more cleanly by .gitignore rules, and I can not exclude that as a solution based on this report. See T9455 for more.

Some dirtiness is the result of poor practices or general lack of discipline, and I hesitate to encourage these behaviors in the upstream. See T7521#104969 for more.

Thanks for the feedback.

While I did delve somewhat into my proposed solution, I do think the core of this issue is not a feature request. The core issue is that arcanist offers to stash my changes for me, and when I accept its offer, it throws an uncaught exception and crashes.

I'll refile with a different title and without the two sentences where I discuss a possible solution.