Page MenuHomePhabricator

Autoclose and "Fixes/Ref" are too difficult for users to debug
Closed, ResolvedPublic

Description

Our workflow is generally:

  1. Differential is submitted.
  2. Differential is accepted.
  3. Someone with access to the master branch merges the branch specified in the differential and pushes to the remote.

It seems that after upgrading Phabricator last week, differentials are no longer being autoclosed. A general description of what is happening is as follows:

  1. Branch X consists of two commits (A and B).
  2. Branch X was submitted to Phabricator as a differential (lets say D123).
  3. The differential was accepted.
  4. Branch X was merged and pushed. Let's say that commit C is the merge commit ("Merge branch X into master").

The differential has been updated with links to the two commits (A and B) on the master branch, but the differential has not been closed.

Related Objects

StatusAssignedTask
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedbtrahan
ResolvedNone

Event Timeline

joshuaspence assigned this task to epriestley.
joshuaspence raised the priority of this task from to High.
joshuaspence updated the task description. (Show Details)
joshuaspence added a subscriber: joshuaspence.

This is probably related to T4746

Another potentially useful detail is that the repository is only 99.99% imported. This is because 1 commit could not be processed (I had to kill the worker task) due to the commit message containing a UTF8 character outside of BMP.

Autoclose doesn't activate until the repository is imported. You can use bin/repository mark-imported to force the flag on.

Ah! Right, okay. That would explain it.

So I've now marked the repository as imported... is it possible to reprocess these commits so that the diffs are closed?

You can use scripts/repository/reparse.php --message with either --all X (where "X" is the callsign) or individual rXnnnnn commit identifiers to reparse things, e.g.:

# Reparse one commit.
./scripts/repository/reparse.php --message rXnnnnnnnn

# Reparse every commit.
./scripts/repository/reparse.php --message --all X

Hmm, ok. So I ran ./scripts/repository/reparse.php --message --all X, but the diff is still in the "Accepted" state.

I'm not sure why that is. I'll need to add more diagnostic information; see T4746.

OK, I can provide some more insight into this.

I noticed that autoclosing has started working again (after I executed ./bin/repository mark-imported X). However ./scripts/repository/reparse.php --message --all X still does not close existing (accepted and merged) differentials.

I don't completely understand the source code here, but I think that the issue lies in the PhabricatorRepository::shouldAutocloseCommit method. It seems that the $commit->isPartiallyImported($closeable_flag) logic returns false, when I am expecting it to return true. Whilst I don't think my logic is correct either, I added an additional condition to the method which has allowed prior accepted differentials to be autoclosed:

  public function shouldAutocloseCommit(PhabricatorRepositoryCommit $commit, PhabricatorRepositoryCommitData $data) {
    if ($this->getDetail('disable-autoclose', false)) {
      return false;
    }

    ...

    $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE;
    if ($commit->isPartiallyImported($closeable_flag)) {
      return true;
    }

+    if ($data->getCommitDetail('differential.revisionID')) { 
+      return true;
+    }

    ...

    return false;
  }

@epriestley, any thoughts on this? I'm happy to work on a fix but I'm not 100% clear on what the issue is.

The first-order issue here is that the UI and tooling don't expose enough information to diagnose or resolve this problem. I want to fix that first, as it has effects beyond this specific problem.

Based on your analysis, my guess is that all branches were considered non-autoclosing during import by the first clause of shouldAutocloseBranch(), which meant nothing got flagged as closeable, and the likely fix in the general case is to move that clause from shouldAutocloseBranch() to shouldAutocloseCommit() (and likely remove the disable-autoclose check, as well, as it already exists in shouldAutocloseCommit()). But even if that's right, it should be evident from the UI/tooling, and not require digging around and adding debug statements.

epriestley edited this Maniphest Task.May 14 2014, 10:10 PM

FWIW, I don't think that I am experiencing this problem anymore. Its been a while and I can't quite remember how/when this was solved.

@epriestley, feel free to close this. Or don't, either way.

I'm still planning to improve diagnostics here, we've seen a few "this doesn't work / does work when I don't expect it to" issues and I don't have a great answer for them right now.

This sometimes works for me. It seems like sometimes commits get hung importing so revisions are never closed for those commits. Especially seems to be the case with encoding issues.

rdpascua added a comment.EditedJul 3 2014, 6:06 AM

Weird stuff. I gave up and dumped the database and installing again. suddenly it works.

◀ Merged tasks: T5556.

epriestley renamed this task from Autoclosing of differentials is not working to Autoclose and "Fixes/Ref" are too difficult for users to debug.Jul 4 2014, 2:31 PM
ryanlopez310 closed this task as Invalid.Jul 21 2014, 2:19 PM
epriestley reopened this task as Open.Jul 21 2014, 2:22 PM
  • I think T5771 fixed some of this. At least two of the reports fell under that umbrella, and it was the least obvious reason for autoclose to fail.
  • Issues related to commits not importing because of emoji or non-utf8 encodings are covered in T1191 (and maybe T4045). This task will not resolve them, and there is no real workaround until T1191.
  • I'm not fixing the (presumed) original issue here, where message reparsing didn't unstick the "not on autoclose branch" flag. At some point in the future I'll move scripts/repository/reparse.php to bin/repository reparse and add a --force-autoclose flag to cover this. I think the behavior is correct in the general case: if a commit did not autoclose when initially imported, it should not autoclose when reparsed (there are a bunch of reasons to reparse that are not related to autoclose). This is filed in T5966.
  • Other stuff in this task should now be more reasonable to debug. If you're still having issues you can't figure out after D10348, file separate tasks or come hunt us down in IRC and we'll walk you through the new stuff.
btrahan closed subtask Restricted Maniphest Task as Resolved.Oct 16 2014, 5:09 PM
poop removed a subscriber: poop.Oct 17 2014, 4:26 PM
eadler added a subscriber: eadler.May 29 2015, 8:42 PM