Page MenuHomePhabricator

Fix several safety issues with repository URIs
ClosedPublic

Authored by epriestley on Nov 30 2017, 8:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 9:32 AM
Unknown Object (File)
Sat, Nov 16, 9:41 PM
Unknown Object (File)
Fri, Nov 15, 11:46 PM
Unknown Object (File)
Mon, Nov 11, 6:54 PM
Unknown Object (File)
Fri, Nov 8, 4:02 AM
Unknown Object (File)
Oct 18 2024, 8:49 PM
Unknown Object (File)
Oct 14 2024, 1:41 AM
Unknown Object (File)
Sep 25 2024, 11:24 AM
Subscribers
None

Details

Summary

See PHI234. Several issues here:

  • The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
  • There's just an actual typo which prevents the "Observe" version of this error from triggering properly.

Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to replace a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).

Test Plan

Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:

Screen Shot 2017-11-30 at 12.09.30 PM.png (988×1 px, 160 KB)

Diff Detail

Repository
rP Phabricator
Branch
overwrite1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 18903
Build 25489: Run Core Tests
Build 25488: arc lint + arc unit

Event Timeline

  • Use a more conventional spelling of "unrecoverably".
amckinley added inline comments.
src/docs/user/userguide/diffusion_uris.diviner
244–245

Technically this isn't true though, right? Even if the remote has a branch that would conflict with a local name, the commits stick around but just become unreachable? I understand if we want to gloss over this in the docs to set expectations; I just want to make sure I understand what's actually happening.

259–262

And I guess this is remote implementation specific.

This revision is now accepted and ready to land.Nov 30 2017, 9:29 PM

Yeah, I'm glossing for simplicity -- the actual behavior is that the commits remain on disk until Git garbage collects them. With normal/default config, I think that's 14 days ("gc.pruneExpire"?) but it could be "never" or something like "instantly" depending on config, I think. I'm also not totally sure that any server-side stuff triggers GC behavior. We also don't currently ever GC explicitly, although this might change. My understanding of the Git GC is generally a bit fuzzy.

So the complete explanation is more like "... they will be unrecoverably destroyed [probably after a couple weeks unless you've changed Git settings]."

For mirror, I'm simplifying git push --mirror:

--mirror
    Instead of naming each ref to push, specifies that all refs under refs/ (which includes but is not limited to refs/heads/, refs/remotes/, and
    refs/tags/) be mirrored to the remote repository. Newly created local refs will be pushed to the remote end, locally updated refs will be force
    updated on the remote end, and deleted refs will be removed from the remote end. This is the default if the configuration option
    remote.<remote>.mirror is set.

The remote there also needs to GC to really destroy stuff, and it's possible that someone might implement a git which does something magic here and doesn't actually delete anything when a client says "delete master", but I don't know of any Git which works like that today, at least. (Except maybe Phabricator, which says "no, I'm not going to do that" by default, but doesn't have any fancier sort of behaviors.)

This revision was automatically updated to reflect the committed changes.