D950: run-tests: update back to original node after bisecting

2017-10-13 Thread ryanmce (Ryan McElroy) 
ryanmce accepted this revision as: ryanmce.
ryanmce added a comment.


  I like this overall; but I don't feel I can queue a new flag to bisect 
without input from the rest of the community.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D950

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D950: run-tests: update back to original node after bisecting

2017-10-13 Thread ryanmce (Ryan McElroy) 
ryanmce added inline comments.

INLINE COMMENTS

> quark wrote in run-tests.py:2124-2153
> During the meeting deciding to experiment with Phabricator, I think we agreed 
> that with Phabricator's ability to diff with whitespaces ignored 
>  
> and its yellow margins of "Moved from line X", the patch size standard could 
> be more flexible.

I suppose, I ended up using that ability but it would still help readability 
either way. I won't fuss too much over it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D950

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D950: run-tests: update back to original node after bisecting

2017-10-12 Thread quark (Jun Wu) 
quark added inline comments.

INLINE COMMENTS

> ryanmce wrote in run-tests.py:2124-2153
> For readability, please add the try/finally (with the finally block being 
> just `pass` for now) in a separate patch.

During the meeting deciding to experiment with Phabricator, I think we agreed 
that with Phabricator's ability to diff with whitespaces ignored 
 
and its yellow margins of "Moved from line X", the patch size standard could be 
more flexible.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D950

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D950: run-tests: update back to original node after bisecting

2017-10-12 Thread ryanmce (Ryan McElroy) 
ryanmce requested changes to this revision.
ryanmce added a comment.
This revision now requires changes to proceed.


  I'm +1 on this series and the rest looks fine but I think this one needs some 
improvements before landing.

INLINE COMMENTS

> run-tests.py:2115-2116
>  return data
> +orignode = pread(hgcmd + ['id', '--debug'])[:21]
> +if orignode.endswith('+'):
> +self.stream.writeln('skipping bisect: uncommitted changes')

It feels fragile to rely on the output of a debug flag command to determine 
state. Can you use `hg status` instead?

> run-tests.py:2120-2121
> +orignode = orignode[:20]
> +if len(orignode) != 20:
> +self.stream.writeln('skipping bisect: unknown node')
> +return

When does this happen? Add a comment, perhaps? I'd also prefer a more 
explanatory output.

> run-tests.py:2124-2153
> +try:
> +pread(bisectcmd + ['--reset']),
> +pread(bisectcmd + ['--bad', '.'])
> +pread(bisectcmd + ['--good',
> +  self._runner.options.known_good_rev])
> +# TODO: we probably need to forward more options
> +# that alter hg's behavior inside the tests.

For readability, please add the try/finally (with the finally block being just 
`pass` for now) in a separate patch.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D950

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel