dlax added a comment.
In https://phab.mercurial-scm.org/D985#17380, @pulkit wrote:
> The next two patches https://phab.mercurial-scm.org/D1042 and
https://phab.mercurial-scm.org/D1043, though sent by me are patches by Denis.
They were some good cleanups which I think was wrong to fold
dlax added a comment.
As an end user, I'd probably not use this; it's too much magic.
INLINE COMMENTS
> ui.py:56
> +# Destination revset explanation:
> +# - max(roots(ALLSRC) & ::SRC)^
> +# The obsoleted changeset that the SRC stack is based on.
Maybe also explain what `ALLSRC` and `SRC`
dlax added inline comments.
INLINE COMMENTS
> commands.py:809
> +hbisect.resetstate(repo)
> +ui.warn(_('bisect aborted\n'))
> +return
This "bisect aborted" message is not very useful (especially at "warning"
level): either the command fails with a message or it succeeds
dlax added inline comments.
INLINE COMMENTS
> pulkit wrote in cmdutil.py:558
> Well I agree with you on this. Yesterday I accidentally stumbled on your repo
> at hg.logilab.org/ by clicking a link in one of the commit message and saw
> that you have the above feedback implemented yourself.
dlax added inline comments.
INLINE COMMENTS
> ryanmce wrote in remotenames.py:11-16
> nit: for multiline docblocks, I prefer to leave the first line empty:
>
> """
> pulls bookmarks and branches information of the remote repo during a
> pull or clone operation.
>
> localrepo is our
dlax added inline comments.
INLINE COMMENTS
> pulkit wrote in commands.py:4794
> I can implement your suggestion using following ways:
>
> 1. add 'clean' and 'unknown' to `show` variable and take them out later as
> that variable is used later also
>
> 2. add a new variable which handles
dlax added inline comments.
INLINE COMMENTS
> repoview.py:104
> -
> -Secret and hidden changeset should not pretend to be here."""
> assert not repo.changelog.filteredrevs
Maybe update the docstring instead of removing?
REPOSITORY
rHG Mercurial
REVISION DETAIL
dlax added a comment.
In https://phab.mercurial-scm.org/D1012#17020, @dlax wrote:
> (Also having an actual use of the new behavior in the patch series would
help understanding the intent.)
Hm, I now see it's used in some other differentials but they do not appear in
this stack
dlax requested changes to this revision.
dlax added a comment.
This revision now requires changes to proceed.
The idea of using different names to control the behavior of the filter
function sounds strange to me.
Maybe `repoview.filterrevs()` could accept `**kwargs` that would be passed to
dlax added inline comments.
INLINE COMMENTS
> repoview.py:38
> pinned.update([par.rev() for par in repo[None].parents()])
> pinned.update([cl.rev(bm) for bm in repo._bookmarks.values()])
>
Updating a "private" attribute of an object outside the class is not nice.
Not sure about
dlax requested changes to this revision.
dlax added a comment.
This revision now requires changes to proceed.
Just a couple of comments (mostly Python things) based on a first quick read.
I haven't closely looked at the algorithm yet.
Thanks for working back on this!
INLINE COMMENTS
>
dlax accepted this revision.
dlax added a comment.
This patch looks good to me. But I have no idea if the overall idea in the
series is good or not.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D933
To: pulkit, #hg-reviewers, dlax
Cc: dlax, quark, yuja,
dlax requested changes to this revision.
dlax added a comment.
This revision now requires changes to proceed.
Overall, the series only introduces helper functions. So it's hard to tell if
the implementation is meaningful without any use of the code...
INLINE COMMENTS
> remotenames.py:31
>
dlax requested changes to this revision.
dlax added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> remotenames.py:31
>
> +def saveremotebookmarks(repo, vfs, remotepath, bookmarks):
> +""" save remote bookmarks in .hg/remotenames/bookmarks.
`repo`
dlax added a comment.
Also, it'd be useful to indicate where the code comes from (i.e. what is the
"remoterepo" mentioned in the first line of the commit message).
Then, a module docstring explaining the concept at stake (starting by what is
a "remotename") and the purpose of the
dlax added inline comments.
INLINE COMMENTS
> run-tests.py:2116
> +pread(bisectcmd + ['--good',
>self._runner.options.known_good_rev])
> # TODO: we probably need to forward more options
Perhaps also update the indentation of continuation line?
dlax requested changes to this revision.
dlax added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> remotenames.py:28
> +if node in repo and not repo[node].obsolete():
> +bmap[branch].append(node)
I don't get the point of the
dlax added inline comments.
INLINE COMMENTS
> dlax wrote in scmutil.py:695
> A "correct" indentation would be:
>
> nodesdict = {short(pred): [short(s) for s in succs]
>for pred, succs in replacements.iteritems()}
> fm.write('hashchanges', '%s\n',
>
dlax requested changes to this revision.
dlax added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> dlax wrote in scmutil.py:695
> Indentation still weird (though no longer wrong).
> Could you align continuation lines with the opening delimiter (`{` for
>
dlax added inline comments.
INLINE COMMENTS
> scmutil.py:695
> +fm.formatdict(nodesdict, fmt='%r --> %r', sep='\n'))
> +
> def addremove(repo, matcher, prefix, opts=None, dry_run=None,
> similarity=None):
Indentation still weird (though no longer wrong).
Could you
dlax added a comment.
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>
> - a/mercurial/scmutil.py +++ b/mercurial/scmutil.py @@ -685,6 +685,18 @@ if
tostrip: repair.delayedstrip(repo.ui, repo, tostrip, operation)
>
> +def showchanges(replacements, fm): +""" output
101 - 121 of 121 matches
Mail list logo
