martinvonz added a comment.
I know this has already been queue and I'm just slow, but could you send
follow-up patches to address my comments?
INLINE COMMENTS
> debugcommands.py:3416
> + ]
> + + cmdutil.logopts,
> + _(b"hg debugbackupbundle [--recover HASH]"),
Do we really want to support all of these? (FYI, they include `--stat`,
`--graph`, `--style`, `--patch`, and a few more.)
> debugcommands.py:3432
> + backups = filter(
> + os.path.isfile, glob.glob(repo.vfs.join("strip-backup") + "/*.hg")
> + )
I think Windows doesn't care much about `/` vs `\`, but should we ideally use
`os.path.join(..., ".hg")` here?
> debugcommands.py:3436
> +
> + opts["bundle"] = ""
> + opts["force"] = None
Clearer to pass `bundlename=""` in the call to `getremotechanges()`?
> debugcommands.py:3437
> + opts["bundle"] = ""
> + opts["force"] = None
> + limit = logcmdutil.getlimit(opts)
Clearer to pass `force=None` (or `False`?) in the call to `getremotechanges()`?
> debugcommands.py:3441
> + def display(other, chlist, displayer):
> + if opts.get("newest_first"):
> + chlist.reverse()
Always false, so delete it?
> debugcommands.py:3518-3528
> + backupdate = time.strftime(
> + "%a %H:%M, %Y-%m-%d",
> + time.localtime(os.path.getmtime(source)),
> + )
> + ui.status("\n%s\n" % (backupdate.ljust(50)))
> + if ui.verbose:
> + ui.status("%s%s\n" % ("bundle:".ljust(13), source))
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).
> debugcommands.py:3528
> + "template"
> + ] = "{label('status.modified', node|short)}
> {desc|firstline}\n"
> + displayer = logcmdutil.changesetdisplayer(
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?
> debugcommands.py:3530
> + displayer = logcmdutil.changesetdisplayer(
> + ui, other, opts, False
> + )
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`)?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7932/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7932
To: pulkit, #hg-reviewers, durin42
Cc: martinvonz, marmoute, durin42, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
