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

Assigned To

Authored By

	skrap
	Sep 10 2015, 2:32 PM

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.

Related Objects

Mentioned Here: T9457: arc diff crashes when trying to stash submodule diffs
T7521: `arc land` should work even when the repo has untracked files
T9455: Arc meaninglessly prompts users to amend changes in submodules into parent repositories

Event Timeline

skrap created this task.Sep 10 2015, 2:32 PM

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,

• m0m0x01d claimed this task.Sep 11 2015, 4:43 PM

• m0m0x01d edited subscribers, added: • m0m0x01d; removed: skrap.Sep 11 2015, 4:47 PM

• xdxdxd removed a subscriber: • m0m0x01d.Sep 11 2015, 4:54 PM

• xdxdxd added a subscriber: • xdxdxd.

epriestley removed • m0m0x01d as the assignee of this task.Sep 11 2015, 5:01 PM

epriestley edited subscribers, added: skrap; removed: • xdxdxd.

joshuaspence added a subscriber: joshuaspence.Sep 11 2015, 11:34 PM

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.

Herald added a subscriber: cspeckmim. · View Herald TranscriptSep 21 2015, 6:46 PM

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.

See T9457.

arc diff: relax handling of submodule diffs in working copyClosed, InvalidPublicActions

Description

Related Objects

Event Timeline

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