Page MenuHomePhabricator

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

Authored by epriestley on Nov 2 2013, 10:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 18, 4:20 AM
Unknown Object (File)
Thu, Apr 18, 4:20 AM
Unknown Object (File)
Thu, Apr 18, 3:44 AM
Unknown Object (File)
Thu, Apr 11, 5:56 AM
Unknown Object (File)
Mar 19 2024, 1:38 AM
Unknown Object (File)
Feb 1 2024, 9:13 PM
Unknown Object (File)
Jan 18 2024, 10:59 AM
Unknown Object (File)
Dec 7 2023, 8:19 PM
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.