Page MenuHomePhabricator

Merge "expandshortcommitquery" and "stablecommitnamequery" into "resolverefs"
ClosedPublic

Authored by epriestley on Nov 2 2013, 10:02 PM.
Tags
None
Referenced Files
F14064253: D7484.diff
Mon, Nov 18, 10:39 PM
F14052814: D7484.diff
Fri, Nov 15, 11:02 AM
F14040901: D7484.diff
Mon, Nov 11, 2:07 PM
F14024452: D7484.diff
Thu, Nov 7, 9:36 AM
F14012525: D7484.id16864.diff
Fri, Nov 1, 2:27 PM
F14010160: D7484.id16864.diff
Thu, Oct 31, 6:04 AM
F14009860: D7484.id16864.diff
Thu, Oct 31, 12:41 AM
F14001772: D7484.id16864.diff
Fri, Oct 25, 11:53 AM
Subscribers

Details

Summary

Ref T1493. Diffusion has some garbagey behavior for things we can't resolve. Common cases are:

  • Looking at a branch that doesn't exist.
  • Looking at a repository with no branches.
  • Looking at a commit that doesn't exist.
  • Looking at an empty repository.

In these cases, we generally fatal unhelpfully. I want to untangle this mess.

This doesn't help much, but does clean things up a bit. We currently have two separate query paths, "stablecommitname" and "expandshortcommit". These are pretty much doing the same thing -- taking some ref like "master" or "default" or a tag name or part of a commit name, and turning it into a full commit name. Merge them into a single "resolverefs" method.

This simplifies the code a fair bit, and gives us better error messages. They still aren't great, but they're like this now:

Ref "7498aec194ecf2d333e0e2baddd9d5cdf922d7f1" is ambiguous or does not exist.

...instead of just:

ERR-INVALID-COMMIT
Test Plan

Looked at Git, Mercurial and Subversion repositories that were empty and non-empty. Looked at branches/heads. Tried to look at invalid commits. Looked at tags. All of this still works, and some behaviors are a bit better than they used to be.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

src/applications/diffusion/conduit/ConduitAPI_diffusion_browsequery_Method.php
201

This fixes the browse view for branch names which are also revsets, like a:b.

src/applications/diffusion/controller/DiffusionBrowseController.php
151 โ†—(On Diff #16864)

I'll fix this in the next diff.

src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php
139โ€“143

After finding and fixing the other hsprintf() issue in this diff, I'm like 99% sure this is correct and might just remove this comment entirely.

Resolve refs is approximately infinitely better than having the two sillier APIs.