Page MenuHomePhabricator

`arc land` should work even when the repo has untracked files
Closed, ResolvedPublic

Description

Since D11995, it looks like arc diff has to be run with --allow-untracked if I have temporary untracked files in the repo, because refusing to add them to the diff aborts the process. arc land has similar implications, but no equivalent flag, so the only way to land stuff is to add the temp files to .git/info/exclude. This pretty onerous, especially for temp files, which may have different names and pop up in various places just due to the nature of development.

How can I land a diff without dedicating a ton of time to carefully curating the untracked files?

Event Timeline

staticshock raised the priority of this task from to Needs Triage.
staticshock updated the task description. (Show Details)
staticshock added a project: Arcanist.
staticshock added a subscriber: staticshock.

As far as I know, the reason arc diff warns about untracked files is because untracked files could impact the result of unit testing or linting. The only way to know if those results are correct are for the working directory to exactly match what you're landing.

The other issue with untracked files on arc land is that they won't play nicely with rebasing / merging / etc, especially if someone else adds an untracked file in the remote and you have it still untracked in your local repo.

You can add arc.autostash to .arcconfig to have Arcanist automatically stash and pop untracked or modified working files when running Arcanist commands.

Maybe I'm misunderstanding how arc.autostash works, but I don't think it does what you claim. It looks like it just runs git stash, which ignores untracked files.

A lot of engineers I know rely on arc diff not failing when untracked files are around. Auto-stashing files still requires a change in behavior (for example, unstashing once the diff has gone out).

Context on this:

  • The behavioral change in D11995 was not intentional.
  • I believe the current behavior is unambiguously desirable and correct, and that all other behaviors are evil and dangerous.
  • Specifically:
    • I believe there is no legitimate reason to ever have untracked files in your working copy. I've asked a number of engineers about why they do this in the past and never received an answer I found compelling.
    • Every VCS has a wide array of trivial mechanisms available to ignore untracked files.
    • I believe untracked files in your working copy are essentially evil and dangerous by their nature.

Here's the specific story behind this check:

  • An engineer at a large social media company pushed a change which forgot to add an untracked file.
  • This went to production and basically disabled ads by accident, costing the company a very huge amount of revenue.
  • I added the check.
  • Engineers hated it, and were adamant that untracked files were important to their workflows and that they never made mistakes like this and the one mistake was a freak accident.
  • The most vocal engineer who hated it the most made a serious error with an untracked file later that week.

The old behavior was a sort of uneasy middle ground between my ideological stance that this behavior is evil and the stance of most engineers that dirty working copies full of random untracked files are normal and desirable.

In general, D11995 combined three separate prompts which interacted in buggy ways into a single prompt. We can't restore the old behavior, per se, without separating these prompts back out, at least partially. I think the new behavior is much better so I'm hesitant to move backward on D11995.

While I feel strongly that this practice is pure evil, I also don't think the magnitude of the evil is very large. It's just a poor development practice which avoids a tiny inconvenience (cleaning or using ignore mechanisms for your working copy) but risks a huge failure (not publishing a required file).

Generally, I think there are several ways to move forward here:

  • Take the hardline ideological ground and decline to change the behavior.
  • Keep fighting the good fight; maybe add arc ignore to try to make it easier to do the right thing. Maybe split this prompt back up partially, so it's kind of worse for everyone but we're still trying.
  • Give up and accept defeat. Remove checks for untracked files, remove --allow-untracked, etc. The new behavior for arc would be to ignore untracked files in all cases.

I'm inclined to just give this one up. It would let us remove a bunch of code, remove flags, and give engineers a better experience with the tool 99.99% of the time. The other 0.01% of the time they will have a disastrously bad experience, but we don't bear any of the costs of these bad cases.

I think a lot of the pain caused by D11995 (in SVN world at least) is just caused by bad wording. Consider:

seth@fr-dev9:~/update/p$ svn st
?       run.sh
M       ld.cpp
seth@fr-dev9:~/update/p1$ arc diff
You have uncommitted changes in this working copy.

  Working copy: /home/seth/update/p1/

  Untracked changes in working copy:
  (To ignore this change, add it to "svn:ignore".)
    run.sh


    Do you want to create a new commit with this change? [y/N] ^C
seth@fr-dev9:~/update/p1$

There are two specific problems here:

  1. The prompt is confusing. I don't know what it means to "create a new commit with this change" in svn. It turns out that this really means to just svn add the file.
  2. The prompt doesn't mention the --allow-untracked flag. I actually created T7608: arcanist - allow untracked files because I was unaware of the flag.

I like this check. It protects me from accidentally forgetting to add files.

I think the current behavior may be a bit heavy-handed when it forces you to auto-add or abort, but it's certainly not a showstopper. I think the ideal case would be that arc would just require user to verify the untracked files were okay before proceeding, but the current behavior achieves the same thing semantically and is only a bit more inconvenient (barring documentation issues).

  • D12256 is the "give up" approach.
  • D12258 is the "partially back out of it" approach.

I would prefer the "fight the good fight / make it easier to do the right thing" approach.

Could this be fixed by allowing the --allow-untracked flag to be specified via config, and applied to arc diff and arc land? That would encourage using the current strict behavior, but give teams an escape hatch in case of mutiny.

angie added a project: Restricted Project.Jun 3 2015, 8:19 PM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jun 3 2015, 8:35 PM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Oct 14 2015, 6:40 PM