Page MenuHomePhabricator

debugbackupbundle: introduce command to interact with strip backups
ClosedPublic

Authored by pulkit on Jan 17 2020, 1:25 PM.

Details

Summary

This vendors backups extension from hg-experimental.

Listing backups and having some utility to apply them is nice. I know we have
obsmarkers now, but this will help a lot of end users who still uses strip until
we get evolve out of experimental.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Jan 17 2020, 1:25 PM

I'm +1 on having this functionality in core, FYI.

Can we call it debugbackupbundle for clarify?

pulkit retitled this revision from [RFC]debugbackups: introduce command to interact with strip backups to debugstripbackups: introduce command to interact with strip backups.Feb 4 2020, 8:03 AM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 19868.
durin42 requested changes to this revision.Feb 12 2020, 2:29 PM

I think I'd prefer debugbackupbundle per marmoute's suggestion.

This revision now requires changes to proceed.Feb 12 2020, 2:29 PM
pulkit retitled this revision from debugstripbackups: introduce command to interact with strip backups to debugbackupbundle: introduce command to interact with strip backups.Feb 24 2020, 1:11 PM
pulkit updated this revision to Diff 20288.
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

I know this has already been queue and I'm just slow, but could you send follow-up patches to address my comments?

mercurial/debugcommands.py
3416

Do we really want to support all of these? (FYI, they include --stat, --graph, --style, --patch, and a few more.)

3432

I think Windows doesn't care much about / vs \, but should we ideally use os.path.join(..., ".hg") here?

3436

Clearer to pass bundlename="" in the call to getremotechanges()?

3437

Clearer to pass force=None (or False?) in the call to getremotechanges()?

3441

Always false, so delete it?

3518–3528

Perhaps the template here should apply at the bundle level instead of the changeset level, so the user can also template the date (and maybe bundle name).

3528

heh, status.modified seems like quite an abuse of the labeling system (I assume it was picked because the author thought that the color they had configured for that looked good in this context too). Could you switch to a different label?

3530

Does False (for the differ argument) produce different output than the default (None) would? If not, remove the False? If it does, could you pass it as a keyword argument instead (differ=False)?

mharbison72 added inline comments.
mercurial/debugcommands.py
3432

I thought we were trying to avoid os.path, so maybe just repo.vfs.join("strip-backup", ".hg")?

(And don't these need to be b'' prefixed for py3?)

marmoute added inline comments.Tue, Mar 10, 8:22 PM
mercurial/debugcommands.py
3432

+1 on using vfs here, this is the abstraction we use everywhere else.