D943: chg: move only first time relevant if condition out of loop

2017-10-06 Thread simpkins (Adam Simpkins) 
simpkins added inline comments.

INLINE COMMENTS

> chg.c:368-397
>   enum {
>   SERVE = 1,
>   DAEMON = 2,
>   SERVEDAEMON = SERVE | DAEMON,
>   TIME = 4,
>   };
>   unsigned int state = 0;

I agree with Yuya that the original code is more readable, and that this won't
have any measurable perf impact.  Users presumably aren't running commands with
many thousands of arguments that we have to loop through here (and even if they
were, the CPU branch predictor would do a good job of handling the i == 0
check).

If you wanted to simplify this function, I personally think this would be 
better:

  int i;
  for (i = 0; i < argc; ++i) {
  if (strcmp(argv[i], "--") == 0) {
  break;
  } else if (strcmp("-d", argv[i]) == 0 ||
  strcmp("--daemon", argv[i]) == 0) {
  if (strcmp("serve", argv[0]) == 0)
  return 1;
  } else if (strcmp("--time", argv[i]) == 0) {
  return 1;
  }
  }
  return 0;

or perhaps

  if (argc == 0)
  return 0;
  
  int isserve = (strcmp(argv[0], "serve") == 0);
  for (i = 0; i < argc; ++i) {
  if (strcmp(argv[i], "--") == 0)
  break;
  if (isserve && (strcmp("-d", argv[i]) == 0 ||
  strcmp("--daemon", argv[i]) == 0))
  return 1;
  if (strcmp("--time", argv[i]) == 0)
  return 1;
  }
  return 0;

REPOSITORY
  rHG Mercurial

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

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

D864: commands: rename clone --uncompressed to --stream and document

2017-10-01 Thread simpkins (Adam Simpkins) 
simpkins accepted this revision.
simpkins added a comment.


  The code and test changes seem fine to me.  I don't have context around the 
argument naming discussion, but it seems reasonable.

REPOSITORY
  rHG Mercurial

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

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

D863: commands: remove suggestion to clone via `cp -al`

2017-10-01 Thread simpkins (Adam Simpkins) 
simpkins accepted this revision.
simpkins added a comment.


  killitwithfire 


REPOSITORY
  rHG Mercurial

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

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

D850: hgweb: add HTML elements to control whitespace settings for annotate

2017-10-01 Thread simpkins (Adam Simpkins) 
simpkins accepted this revision.
simpkins added a comment.


  Looks fine to me, but I'm no HTML/JS expert.
  dogscience 


REPOSITORY
  rHG Mercurial

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

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

D849: hgweb: query string arguments to control whitespace for annotate

2017-10-01 Thread simpkins (Adam Simpkins) 
simpkins accepted this revision.
simpkins added a comment.


  Looks good to me apart from one minor question.

INLINE COMMENTS

> webutil.py:180-183
> +try:
> +v = bool(int(v))
> +except ValueError:
> +v = True

Would it be better to use `util.parsebool()` here?

REPOSITORY
  rHG Mercurial

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

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

D171: phabricator: include the suggested arc config in the repo

2017-07-24 Thread simpkins (Adam Simpkins) 
simpkins accepted this revision.
simpkins added a comment.


  This looks good to me too.
  
  Personally I would vote for setting `history.immutable` to `false` too.  This 
mostly affects the behavior of the `arc diff` and `arc amend` commands.  This 
probably won't matter at all for people that submit diffs using the `phabsend` 
extension instead of `arc diff`.
  When set to false, `arc diff` will rewrite your local commit message to 
include the differential revision ID when submitting a diff.  `arc amend` will 
update your commit message to add a `Reviewed By` line once your diff has been 
accepted.
  Personally I find it useful to have this information in my local commit 
messages.  (The `phabdiff` and `phabstatus` extensions in the rFBHGEXT 
repository can also report some useful information if they find this field 
present in your commit message.)
  
  The only downside of leaving `history.immutable` as `true` is that you don't 
get this information in your commit message when running `arc diff` / `arc 
amend`; it shouldn't impact any other behavior.
  One arguable downside of setting `history.immutable` to `false` is that your 
local commit IDs do change after running `arc diff` or `arc amend` if it needed 
to update your commit message.

REPOSITORY
  rHG Mercurial

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

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

[Differential] [Closed] D68: dirstate: update backup functions to take full backup filename

2017-07-14 Thread simpkins (Adam Simpkins) 
This revision was automatically updated to reflect the committed changes.
Closed by commit rHGec306bc6915b: dirstate: update backup functions to take 
full backup filename (authored by simpkins).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D68?vs=96&id=138

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

AFFECTED FILES
  hgext/shelve.py
  mercurial/dirstate.py
  mercurial/dirstateguard.py
  mercurial/localrepo.py

CHANGE DETAILS

Index: mercurial/localrepo.py
===
--- mercurial/localrepo.py
+++ mercurial/localrepo.py
@@ -1202,7 +1202,7 @@
 else:
 # discard all changes (including ones already written
 # out) in this transaction
-repo.dirstate.restorebackup(None, prefix='journal.')
+repo.dirstate.restorebackup(None, 'journal.dirstate')
 
 repo.invalidate(clearfilecache=True)
 
@@ -1262,7 +1262,7 @@
 
 @unfilteredmethod
 def _writejournal(self, desc):
-self.dirstate.savebackup(None, prefix='journal.')
+self.dirstate.savebackup(None, 'journal.dirstate')
 self.vfs.write("journal.branch",
   encoding.fromlocal(self.dirstate.branch()))
 self.vfs.write("journal.desc",
@@ -1350,7 +1350,7 @@
 # prevent dirstateguard from overwriting already restored one
 dsguard.close()
 
-self.dirstate.restorebackup(None, prefix='undo.')
+self.dirstate.restorebackup(None, 'undo.dirstate')
 try:
 branch = self.vfs.read('undo.branch')
 self.dirstate.setbranch(encoding.tolocal(branch))
Index: mercurial/dirstateguard.py
===
--- mercurial/dirstateguard.py
+++ mercurial/dirstateguard.py
@@ -31,8 +31,8 @@
 self._repo = repo
 self._active = False
 self._closed = False
-self._suffix = '.backup.%s.%d' % (name, id(self))
-repo.dirstate.savebackup(repo.currenttransaction(), self._suffix)
+self._backupname = 'dirstate.backup.%s.%d' % (name, id(self))
+repo.dirstate.savebackup(repo.currenttransaction(), self._backupname)
 self._active = True
 
 def __del__(self):
@@ -45,25 +45,24 @@
 
 def close(self):
 if not self._active: # already inactivated
-msg = (_("can't close already inactivated backup: dirstate%s")
-   % self._suffix)
+msg = (_("can't close already inactivated backup: %s")
+   % self._backupname)
 raise error.Abort(msg)
 
 self._repo.dirstate.clearbackup(self._repo.currenttransaction(),
- self._suffix)
+ self._backupname)
 self._active = False
 self._closed = True
 
 def _abort(self):
 self._repo.dirstate.restorebackup(self._repo.currenttransaction(),
-   self._suffix)
+   self._backupname)
 self._active = False
 
 def release(self):
 if not self._closed:
 if not self._active: # already inactivated
-msg = (_("can't release already inactivated backup:"
- " dirstate%s")
-   % self._suffix)
+msg = (_("can't release already inactivated backup: %s")
+   % self._backupname)
 raise error.Abort(msg)
 self._abort()
Index: mercurial/dirstate.py
===
--- mercurial/dirstate.py
+++ mercurial/dirstate.py
@@ -1300,10 +1300,10 @@
 else:
 return self._filename
 
-def savebackup(self, tr, suffix='', prefix=''):
-'''Save current dirstate into backup file with suffix'''
-assert len(suffix) > 0 or len(prefix) > 0
+def savebackup(self, tr, backupname):
+'''Save current dirstate into backup file'''
 filename = self._actualfilename(tr)
+assert backupname != filename
 
 # use '_writedirstate' instead of 'write' to write changes certainly,
 # because the latter omits writing out if transaction is running.
@@ -1324,27 +1324,20 @@
 # end of this transaction
 tr.registertmp(filename, location='plain')
 
-backupname = prefix + self._filename + suffix
-assert backupname != filename
 self._opener.tryunlink(backupname)
 # hardlink backup is okay because _writedirstate is always called
 # with an "atomictemp=True" file.
 util.copyfile(self._opener.join(filename),
   self._opener.join(backupname), hardlink=True)
 
-def restorebackup(self, tr, suffix='', prefix=''):
-'''Restore dirstate by backup file with suffix'''
-

[Differential] [Abandoned] D69: tests: put temporary files in $TESTTMP

2017-07-13 Thread simpkins (Adam Simpkins) 
simpkins abandoned this revision.
simpkins added a comment.


  https://phab.mercurial-scm.org/D67 has landed in hgcommitted, so this isn't 
really necessary any more.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

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

[Differential] [Request, 5 lines] D72: httpclient: honor the timeout when setting up the connection

2017-07-12 Thread simpkins (Adam Simpkins) 
simpkins created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously HTTPConnection._connect() called socket.create_connection() without
  specifying a timeout.  This could cause the code to hang forever trying to
  establish a connection, even if a timeout parameter was specified when
  creating the HTTPConnection object.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/httpclient/__init__.py

CHANGE DETAILS

Index: mercurial/httpclient/__init__.py
===
--- mercurial/httpclient/__init__.py
+++ mercurial/httpclient/__init__.py
@@ -517,7 +517,7 @@
 logger.info('Connecting to http proxy %s:%s',
 self._proxy_host, self._proxy_port)
 sock = socket.create_connection((self._proxy_host,
- self._proxy_port))
+ self._proxy_port), self.timeout)
 if self.ssl:
 data = self._buildheaders(b'CONNECT', b'%s:%d' % (self.host,
   self.port),
@@ -549,7 +549,8 @@
 logger.info('CONNECT (for SSL) to %s:%s via proxy succeeded.',
 self.host, self.port)
 else:
-sock = socket.create_connection((self.host, self.port))
+sock = socket.create_connection((self.host, self.port),
+self.timeout)
 if self.ssl:
 # This is the default, but in the case of proxied SSL
 # requests the proxy logic above will have cleared


EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

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

[Differential] [Request, 6 lines] D69: tests: put temporary files in $TESTTMP

2017-07-12 Thread simpkins (Adam Simpkins) 
simpkins created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Update test-check-pyflakes.t to write test.py in the $TESTTMP directory.
  Previously when using "run_tests.py -l" it would write test.py into your local
  repository, leaving it behind as an untracked file after the test finished.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  tests/test-check-pyflakes.t

CHANGE DETAILS

Index: tests/test-check-pyflakes.t
===
--- tests/test-check-pyflakes.t
+++ tests/test-check-pyflakes.t
@@ -6,11 +6,11 @@
 run pyflakes on all tracked files ending in .py or without a file ending
 (skipping binary file random-seed)
 
-  $ cat > test.py < "$TESTTMP/test.py" < print(undefinedname)
   > EOF
-  $ pyflakes test.py 2>/dev/null | "$TESTDIR/filterpyflakes.py"
-  test.py:1: undefined name 'undefinedname'
+  $ pyflakes "$TESTTMP/test.py" 2>/dev/null | "$TESTDIR/filterpyflakes.py"
+  $TESTTMP/test.py:1: undefined name 'undefinedname'
   
 
   $ testrepohg locate 'set:**.py or grep("^#!.*python")' \


EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

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

[Differential] [Request, 53 lines] D68: dirstate: update backup functions to take full backup filename

2017-07-12 Thread simpkins (Adam Simpkins) 
simpkins created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Update the dirstate functions so that the caller supplies the full backup
  filename rather than just a prefix and suffix.
  
  The localrepo code was already hard-coding the fact that the backup name must
  be (exactly prefix + "dirstate" + suffix): it relied on this in 
_journalfiles()
  and undofiles().  Making the caller responsible for specifying the full backup
  name removes the need for the localrepo code to assume that dirstate._filename
  is always "dirstate".

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/shelve.py
  mercurial/dirstate.py
  mercurial/dirstateguard.py
  mercurial/localrepo.py

CHANGE DETAILS

Index: mercurial/localrepo.py
===
--- mercurial/localrepo.py
+++ mercurial/localrepo.py
@@ -1122,7 +1122,7 @@
 else:
 # discard all changes (including ones already written
 # out) in this transaction
-repo.dirstate.restorebackup(None, prefix='journal.')
+repo.dirstate.restorebackup(None, 'journal.dirstate')
 
 repo.invalidate(clearfilecache=True)
 
@@ -1182,7 +1182,7 @@
 
 @unfilteredmethod
 def _writejournal(self, desc):
-self.dirstate.savebackup(None, prefix='journal.')
+self.dirstate.savebackup(None, 'journal.dirstate')
 self.vfs.write("journal.branch",
   encoding.fromlocal(self.dirstate.branch()))
 self.vfs.write("journal.desc",
@@ -1270,7 +1270,7 @@
 # prevent dirstateguard from overwriting already restored one
 dsguard.close()
 
-self.dirstate.restorebackup(None, prefix='undo.')
+self.dirstate.restorebackup(None, 'undo.dirstate')
 try:
 branch = self.vfs.read('undo.branch')
 self.dirstate.setbranch(encoding.tolocal(branch))
Index: mercurial/dirstateguard.py
===
--- mercurial/dirstateguard.py
+++ mercurial/dirstateguard.py
@@ -31,8 +31,8 @@
 self._repo = repo
 self._active = False
 self._closed = False
-self._suffix = '.backup.%s.%d' % (name, id(self))
-repo.dirstate.savebackup(repo.currenttransaction(), self._suffix)
+self._backupname = 'dirstate.backup.%s.%d' % (name, id(self))
+repo.dirstate.savebackup(repo.currenttransaction(), self._backupname)
 self._active = True
 
 def __del__(self):
@@ -45,25 +45,24 @@
 
 def close(self):
 if not self._active: # already inactivated
-msg = (_("can't close already inactivated backup: dirstate%s")
-   % self._suffix)
+msg = (_("can't close already inactivated backup: %s")
+   % self._backupname)
 raise error.Abort(msg)
 
 self._repo.dirstate.clearbackup(self._repo.currenttransaction(),
- self._suffix)
+ self._backupname)
 self._active = False
 self._closed = True
 
 def _abort(self):
 self._repo.dirstate.restorebackup(self._repo.currenttransaction(),
-   self._suffix)
+   self._backupname)
 self._active = False
 
 def release(self):
 if not self._closed:
 if not self._active: # already inactivated
-msg = (_("can't release already inactivated backup:"
- " dirstate%s")
-   % self._suffix)
+msg = (_("can't release already inactivated backup: %s")
+   % self._backupname)
 raise error.Abort(msg)
 self._abort()
Index: mercurial/dirstate.py
===
--- mercurial/dirstate.py
+++ mercurial/dirstate.py
@@ -1300,10 +1300,10 @@
 else:
 return self._filename
 
-def savebackup(self, tr, suffix='', prefix=''):
-'''Save current dirstate into backup file with suffix'''
-assert len(suffix) > 0 or len(prefix) > 0
+def savebackup(self, tr, backupname):
+'''Save current dirstate into backup file'''
 filename = self._actualfilename(tr)
+assert backupname != filename
 
 # use '_writedirstate' instead of 'write' to write changes certainly,
 # because the latter omits writing out if transaction is running.
@@ -1324,27 +1324,20 @@
 # end of this transaction
 tr.registertmp(filename, location='plain')
 
-backupname = prefix + self._filename + suffix
-assert backupname != filename
 self._opener.tryunlink(backupname)
 # hardlink