D4312: New bookflow extension for bookmark-based branching

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D4312#79581, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D4312#79573, @smf wrote:
  >
  > > In https://phab.mercurial-scm.org/D4312#79509, @durin42 wrote:
  > >
  > > > There's been some good discussion on this. I'm sympathetic to both 
arguments here, namely: "we could improve bookmarks and make them less bad" and 
"bookmarks are a dead end and nobody should use them and we shouldn't improve 
them" (or thereabouts - I'm summarizing complicated positions to less than a 
sentence, so bear with me.) I think not following up on our plans to at least 
make plausible incremental improvements to bookmarks serves our users poorly, 
and this extension merits landing as an experimental extension. We can always 
spin it back out if we get unhappy with it.
  > >
  > >
  > > Obviously, I can't say I'm too happy with this. Allowing users to shoot 
themselves in the foot even more is pretty bad.
  >
  >
  > That ship has sailed: bookmarks exist, and are more visible than features 
like rebase.
  >
  > > 
  > > 
  > >> I'm going to land this patch largely as-is on default, with the 
following tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension 
so we can iterate its behavior, some misc. check-code fixes, tweak the commit 
message slightly to make check-commit happy.
  > > 
  > > When I bring up community issues (including at this sprint) I thought 
this project was more than just one person. Having to constantly put up the 
good fight is completely negated if no one is going to listen and just ship 
this anyways. Bookmark related features belong as a third-party extension just 
like hgsubversion, hg-git, etc.
  >
  > ...it is? You're the only voice of objection that I'm seeing. I see some 
mostly indifference from BitBucket and some pretty good enthusiasm from the 
contributor, rhodecode, Kevin, etc. And it's experimental, so we can dump it if 
I've made the wrong choice. If I've missed some other /objections/ (as 
contrasted with what I perceive to be indifference - maybe I'm misreading 
Erik?) please point out my error.
  
  
  I hung out with Erik yesterday and he said he was too frustrated and tired of 
explaining himself to reply to this thread. I can't say I really blame him. The 
previous discussion outlines why having (and encouraging) two branching models 
is bad for the ecosystem. This is based on Erik and mine’s years of experience 
working on Bitbucket.
  
  The more important and pressing issues are exchanging obs markers and 
improving named branching. A cash donation in those directions has gone unused 
and it is frustrating that the weight of Bitbucket’s experience and resources 
has gone ignored.
  
  This will not help the *average* user and sends a mixed (and dangerous) 
message that bookmarks should be used. For a team that truly wants this 
feature, hosting this extension as a package on pypi is the best solution.

REPOSITORY
  rHG Mercurial

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

To: idlsoft, #hg-reviewers, pulkit, marcink
Cc: evzijst, krbullock, mharbison72, smf, markand, marcink, durin42, jwatt, 
pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D4312: New bookflow extension for bookmark-based branching

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D4312#79509, @durin42 wrote:
  
  > There's been some good discussion on this. I'm sympathetic to both 
arguments here, namely: "we could improve bookmarks and make them less bad" and 
"bookmarks are a dead end and nobody should use them and we shouldn't improve 
them" (or thereabouts - I'm summarizing complicated positions to less than a 
sentence, so bear with me.) I think not following up on our plans to at least 
make plausible incremental improvements to bookmarks serves our users poorly, 
and this extension merits landing as an experimental extension. We can always 
spin it back out if we get unhappy with it.
  
  
  Obviously, I can't say I'm too happy with this. Allowing users to shoot 
themselves in the foot even more is pretty bad.
  
  > I'm going to land this patch largely as-is on default, with the following 
tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension so we can 
iterate its behavior, some misc. check-code fixes, tweak the commit message 
slightly to make check-commit happy.
  
  When I bring up community issues (including at this sprint) I thought this 
project was more than just one person. Having to constantly put up the good 
fight is completely negated if no one is going to listen and just ship this 
anyways. Bookmark related features belong as a third-party extension just like 
hgsubversion, hg-git, etc.

REPOSITORY
  rHG Mercurial

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

To: idlsoft, #hg-reviewers, pulkit, marcink
Cc: evzijst, krbullock, mharbison72, smf, markand, marcink, durin42, jwatt, 
pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D4312: New bookflow extension for bookmark-based branching

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D4312#77596, @idlsoft wrote:
  
  > @smf I just noticed your name on 
https://www.mercurial-scm.org/wiki/BookmarkUpdatePlan, which puts your comments 
into a larger context.
  >  This would definitely be an improvement, and reduce the scope of what this 
extension does.
  
  
  I think that page has a typo: it should be "discussed at the London sprint." 
Ryan and I were quite eager to change this behavior of not moving bookmarks. It 
was one of the experiments I mentioned in my previous comment. Over the next 
year or so, I learned there was a big difference between Ryan's use-case (the 
monorepo) and the everyday repo (average repo on Bitbucket): having one (and 
only one) centrally controlled repo is easy to set a policy like "don't move 
bookmarks."
  
  In the wild, there are tons of unknown variables of how people are currently 
relying on the behavior; in particular, scripts. In fact, I have a few scripts 
that do rely on this behavior. Changing that at this point (by default, without 
a flag) is ill-advised in my opinion.
  
  > Would you consider also addressing the `hg bookmark NAME` doing two very 
different things depending on the bookmark already existing?
  
  At this point, I would prefer not to change default behavior for similar 
reasons as above.

REPOSITORY
  rHG Mercurial

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

To: idlsoft, #hg-reviewers, pulkit, marcink
Cc: mharbison72, smf, markand, marcink, durin42, jwatt, pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D4312: New bookflow extension for bookmark-based branching

[smf (Sean Farley)]

smf added a comment.


  Thanks for submitting your patch, it’s clearly a work of passion and I 
appreciate that. I know feature branches is a much sought-after behavior. 
However, I must express my objection to this direction in bookmarks. (apologies 
in advance)
  
  I think the core issue at hand is the difference between data models. In 
particular, there are two huge differences:
  
  - mapping (one-to-many vs one-to-one) and
  - garbage collection
  
  A bit of background: I've written many versions of this ref-like behavior 
over the years, including one during my time at Bitbucket. My experience with 
each of these extensions never felt right and I ended up throwing them all away.
  
  As programmers, we like to abstract and create functional mappings around 
data. But we can't hack our way around the difference between a one-to-many 
relation vs a one-to-one relation. At a fundamental mathematical level, the two 
relations are not the same. For the last ten years, millions of Mercurial repos 
have been created that have (named) branches that are stored in the metadata. 
This is pretty ingrained in each developer's mind: one commit <-> one branch 
name. I think that changing this at this point would cause unnecessary 
frustration for our users.
  
  For hosting sites like Bitbucket, this is even worse. Having two branching 
models is just impossible. A simple example is writing the branch api: should 
it return named branches? bookmarks? both? What happens when there is a call to 
create a branch? Create a branch? bookmark? both??? There are other rough edges 
as well: documentation, user tutorials, UI/UX, etc. The lessons I learned at 
Bitbucket helped me understand that core Mercurial should not recommend 
bookmarks as feature branches.
  
  It doesn't stop there, though. Possibly the biggest misunderstanding is what 
and how anonymous heads work and why they don't disappear like they do in git. 
It is straight-up impossible to remove anonymous heads in Mercurial and trying 
to paper over them just makes things worse. I've tried assigning auto-generated 
bookmarks, auto-marking obsolescence markers, auto-hiding commits locally, etc; 
anything I could think of. Nothing ever really worked in my experience.
  
  Perhaps the show stopper, though, is evolve. Much like Mercurial, evolve 
views the world through  changesets. Changesets and their successors / 
predecessors are related by obsolescence markers and form a DAG, just like 
changesets of a repo are in a DAG. These relations have a starting point and an 
ending point and the most natural fit to that is, of course, storing the branch 
name in the actual changeset. When needing to evolve changsets, the only 
practical (perhaps even only possible) way to determine which branch it lived 
on is with named branches. Bookmarks / refs lose that information. Therefore, 
trying to shoehorn that model into evolve or even core is reinventing what 
branches already achieve.
  
  Everything I tried in the bookmarks world fought against the core design 
principles of Mercurial. For those reasons, I think it is best to stick to one 
branching model and keep bookmarks (as much as possible) in third-party 
extensions (or if my dreams come true: in hg-git). As I've witnessed over the 
years, while at both Mercurial and Bitbucket, users will assume an implicit 
blessing for behavior when a feature is included in hgext. See for example, the 
nightmare of largefiles (and that need causing Facebook's rewrite in lfs), eol, 
keyword, etc, etc. We can put as many "features of last resort" warnings that 
we want but the ultimate protection we have to save ourselves the headache is 
to keep things out of hgext.
  
  All of this is not to say I don't want feature branches in Mercurial, I do. 
It's something Erik and I have thought a lot about and we both want to improve 
on (named) branches that allow feature workflows and work better with phases 
and evolve. I have even worked over the last few months on this exact approach 
and, on my side, I will use this as a fire to get my own work out the door. I 
would welcome and appreciate any kind of help in planning, coding, or even just 
spitballing.

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

To: idlsoft, #hg-reviewers, pulkit, marcink
Cc: smf, markand, marcink, durin42, jwatt, pulkit, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D3665: graph: improve graph output by using Unicode characters

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D3665#58976, @smf wrote:
  
  > In https://phab.mercurial-scm.org/D3665#58973, @johnstiles wrote:
  >
  > > Thanks for the assist, @smf ! I appreciate it.
  >
  >
  > Sure, no problem :-)
  >
  > By the way, here's the diff of what I changed:
  >
  >   diff --git a/hgext/beautifygraph.py b/hgext/beautifygraph.py
  >   index 7ff3c08..254d2cc 100644
  >   --- a/hgext/beautifygraph.py
  >   +++ b/hgext/beautifygraph.py
  >   @@ -4,23 +4,23 @@
  ># Copyright 2018 John Stiles 
  >#
  ># This software may be used and distributed according to the terms of the
  ># GNU General Public License version 2 or any later version.
  >   
  >   -'''This extension beautifies log -G output by using Unicode characters.
  >   +'''beautify log -G output by using Unicode characters (EXPERIMENTAL)
  
  
  In the Mercurial project, we use "EXPERIMENTAL" as a flag to mean "we have 
the option to remove this later." In general, we have a very strong breaking 
change policy (as in, we try to never have one). The only exceptions to that 
rule that I'm aware of are if it's a security problem (e.g. default clone of 
subrepositories changed due to major security flaws) or was broken from day 1.
  
  >  A terminal with UTF-8 support and monospace narrow text are required.
  >   '''
  >   
  >   from __future__ import absolute_import
  >   
  >   from mercurial.i18n import _
  >   from mercurial import (
  >   encoding,
  >   extensions,
  > 
  > +graphmod,
  > 
  >   templatekw,
  > 
  > - graphmod )
  
  I'll go through and briefly explain each hunk (just for reference and if any 
other newcomers find this patch). The `graphmod` import needed to be earlier 
due to our code style checker.
  
  > 1. Note for extension authors: ONLY specify testedwith = 
'ships-with-hg-core' for
  > 2. extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
  > 3. be specifying the version(s) of Mercurial they are tested with, or diff 
--git a/tests/test-duplicateoptions.py b/tests/test-duplicateoptions.py index 
4511a89..397eca4 100644
  >   - a/tests/test-duplicateoptions.py +++ b/tests/test-duplicateoptions.py 
@@ -4,11 +4,11 @@ from mercurial import ( commands, extensions, ui as uimod, )
  > 
  > -ignore = {b'highlight', b'win32text', b'factotum'} +ignore = 
{b'highlight', b'win32text', b'factotum', b'beautifygraph'}
  
  I only found this test breaking on the gcc compile farm with the output 
changing to "beautifygraph: unsupported encoding, UTF-8 required" so I choose 
to skip importing that extension in this test.
  
  >   if os.name != 'nt':
  >   ignore.add(b'win32mbcs')
  >   
  >   disabled = [ext for ext in extensions.disabled().keys() if ext not in 
ignore]
  > 
  > diff --git a/tests/test-glog-beautifygraph.t 
b/tests/test-glog-beautifygraph.t
  >  index c3d1fb7..e62334f 100644
  > 
  > - a/tests/test-glog-beautifygraph.t +++ b/tests/test-glog-beautifygraph.t 
@@ -89,10 +89,14 @@ o  (0) root >   logcmdutil, >   revsetlang, >   smartset, > 
) > +  > from mercurial.utils import ( +  >   stringutil, +  > )
  
  Ah, yes, a classic race condition with another dev changing something that 
you're working on.
  
  >   > def logrevset(repo, pats, opts):
  >   > revs = logcmdutil._initialrevs(repo, opts)
  >   > if not revs:
  >   > return None
  >   > match, pats, slowpath = logcmdutil._makematcher(repo, revs, pats, 
opts)
  > 
  > @@ -109,11 +113,11 @@ o  (0) root
  > 
  >   > else:
  >   > tree = []
  >   > ui = repo.ui
  >   > ui.write(b'%r\n' % (opts.get(b'rev', []),))
  >   > ui.write(revsetlang.prettyformat(tree) + b'\n')
  > 
  > - > ui.write(smartset.prettyformat(revs) + b'\n') +  >  
   ui.write(stringutil.prettyrepr(revs) + b'\n')
  
  As I mentioned above, it seems Yuya changed this and it landed before your 
patch.
  
  >   > revs = smartset.baseset()  # display no revisions
  >   > return revs, filematcher
  >   > extensions.wrapfunction(logcmdutil, 'getrevs', printrevset)
  >   > aliases, entry = cmdutil.findcmd(b'log', commands.table)
  >   > entry[1].append((b'', b'print-revset', False,
  > 
  > @@ -324,1003 +328,1001 @@ The extension should not turn on if we'r
  > 
  > 
  >   
  >   The rest of our tests will use the default narrow text UTF-8.
  >   
  > $ hg log -G -q
  > 
  > - \xe2\x8c\xbe  34:fea3ac5810e0 (esc) +  \xe2\x97\x8d  34:fea3ac5810e0 (esc)
  
  Ah, I think you forgot to update the test output when changing some of the 
characters you used (at least I hope that was the problem here). All I did to 
update the whole test at large was run with: `run-tests.py -l 
tests/test-glog-beautifygraph.t -i`. The `-i` meaning "interactively ask the 
user to accept this change" which will automatically change the test file for 
you.
  
  >   \xe2\x94\x82 (esc)
  > 
  > - \xe2\x94\x82 

D3665: graph: improve graph output by using Unicode characters

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D3665#58973, @johnstiles wrote:
  
  > Thanks for the assist, @smf ! I appreciate it.
  
  
  Sure, no problem :-)
  
  By the way, here's the diff of what I changed:
  
diff --git a/hgext/beautifygraph.py b/hgext/beautifygraph.py
index 7ff3c08..254d2cc 100644
--- a/hgext/beautifygraph.py
+++ b/hgext/beautifygraph.py
@@ -4,23 +4,23 @@
 # Copyright 2018 John Stiles 
 #
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-'''This extension beautifies log -G output by using Unicode characters.
+'''beautify log -G output by using Unicode characters (EXPERIMENTAL)
 
A terminal with UTF-8 support and monospace narrow text are required.
 '''
 
 from __future__ import absolute_import
 
 from mercurial.i18n import _
 from mercurial import (
 encoding,
 extensions,
+graphmod,
 templatekw,
-graphmod
 )
 
 # Note for extension authors: ONLY specify testedwith = 
'ships-with-hg-core' for
 # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
 # be specifying the version(s) of Mercurial they are tested with, or
diff --git a/tests/test-duplicateoptions.py b/tests/test-duplicateoptions.py
index 4511a89..397eca4 100644
--- a/tests/test-duplicateoptions.py
+++ b/tests/test-duplicateoptions.py
@@ -4,11 +4,11 @@ from mercurial import (
 commands,
 extensions,
 ui as uimod,
 )
 
-ignore = {b'highlight', b'win32text', b'factotum'}
+ignore = {b'highlight', b'win32text', b'factotum', b'beautifygraph'}
 
 if os.name != 'nt':
 ignore.add(b'win32mbcs')
 
 disabled = [ext for ext in extensions.disabled().keys() if ext not in 
ignore]
diff --git a/tests/test-glog-beautifygraph.t 
b/tests/test-glog-beautifygraph.t
index c3d1fb7..e62334f 100644
--- a/tests/test-glog-beautifygraph.t
+++ b/tests/test-glog-beautifygraph.t
@@ -89,10 +89,14 @@ o  (0) root
   >   logcmdutil,
   >   revsetlang,
   >   smartset,
   > )
   > 
+  > from mercurial.utils import (
+  >   stringutil,
+  > )
+  > 
   > def logrevset(repo, pats, opts):
   > revs = logcmdutil._initialrevs(repo, opts)
   > if not revs:
   > return None
   > match, pats, slowpath = logcmdutil._makematcher(repo, revs, pats, 
opts)
@@ -109,11 +113,11 @@ o  (0) root
   > else:
   > tree = []
   > ui = repo.ui
   > ui.write(b'%r\n' % (opts.get(b'rev', []),))
   > ui.write(revsetlang.prettyformat(tree) + b'\n')
-  > ui.write(smartset.prettyformat(revs) + b'\n')
+  > ui.write(stringutil.prettyrepr(revs) + b'\n')
   > revs = smartset.baseset()  # display no revisions
   > return revs, filematcher
   > extensions.wrapfunction(logcmdutil, 'getrevs', printrevset)
   > aliases, entry = cmdutil.findcmd(b'log', commands.table)
   > entry[1].append((b'', b'print-revset', False,
@@ -324,1003 +328,1001 @@ The extension should not turn on if we'r
   
 
 The rest of our tests will use the default narrow text UTF-8.
 
   $ hg log -G -q
-  \xe2\x8c\xbe  34:fea3ac5810e0 (esc)
+  \xe2\x97\x8d  34:fea3ac5810e0 (esc)
   \xe2\x94\x82 (esc)
-  \xe2\x94\x82 \xe2\x97\xaf  33:68608f5145f9 (esc)
+  \xe2\x94\x82 \xe2\x97\x8b  33:68608f5145f9 (esc)

[... elided the rest]

REPOSITORY
  rHG Mercurial

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

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

D3665: graph: improve graph output by using Unicode characters

[smf (Sean Farley)]

smf added a comment.


  Greg and Jun,
  
  Before anyone else requests more changes or anything, this is a first time 
contributor and I was trying to be a bit more flexible as a reviewer. I 
actually queued the patch yesterday but due to the test failures, I haven't 
pushed. I'll personally fix up the tests and add the experimental flag.
  
  John,
  
  Don't worry about updating this patch or rebasing. I've already (locally) 
added "EXPERIMENTAL" and am fixing the other test failures, too. I just haven't 
finished yet because I'm trying to also finish my household chores :-P I'll 
post a diff of what I tweaked when I finish this weekend. Thanks for bearing 
with me on this discussion!

REPOSITORY
  rHG Mercurial

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

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

D3665: graph: improve graph output by using Unicode characters

[smf (Sean Farley)]

smf added a comment.


  Could you send me the output of `hg export REV | head` or do you just want me 
to add your name and not worry about the date/time, etc.?

REPOSITORY
  rHG Mercurial

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

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

D3665: graph: improve graph output by using Unicode characters

[smf (Sean Farley)]

smf added a comment.


  I've looked this over today and have queued this up :-) Unfortunately, 
though, the metadata doesn't seem right? I'm not getting your name or email 
(nor timestamp) for the patch. Do you want me to use the same name from 
https://phab.mercurial-scm.org/rHG24e517600b2986a3d5855456b3cdf0830ea0a79e 
(John Stiles )?

REPOSITORY
  rHG Mercurial

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

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

D3715: namespaces: allow namespaces whose symbols resolve to many nodes (API)

[smf (Sean Farley)]

smf added a comment.


  It seems I'm having email sending trouble ... going to attempt to send again

REPOSITORY
  rHG Mercurial

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

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

D2873: remotenames: add functionality to override -B flag of push

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D2873#52100, @indygreg wrote:
  
  > In https://phab.mercurial-scm.org/D2873#52025, @smf wrote:
  >
  > > In https://phab.mercurial-scm.org/D2873#52021, @indygreg wrote:
  > >
  > > > Looks good!
  > >
  > >
  > > I'm very heavily against this direction. Changing the behavior of push 
(even in this extension) is something I've always considered outside the scope 
of remotenames. Having another extension that changes push behavior (e.g. 
bookmark-push) is where I think this should go so that remotenames is just 
that: keeping track of remote names.
  >
  >
  > I think there's room for this feature to live outside of remotenames. But 
currently I think it is the best place for it, since remotenames is the closest 
thing we have to... tracking remote names. We can always alias the old config 
option in the future if we move this functionality elsewhere.
  
  
  One of the biggest regrets I have about the original remotenames, is the 
modifying of push (and some default) behavior. The `--to` is small enough (and 
I guess fine enough) for now but I absolutely and strongly believe that all the 
other behavior modifications should stay in an out-of-core repo for now. My 
goal is to split my remotenames repo to be based off of core's remotenames 
(basically only having the behavior changing patches there).

REPOSITORY
  rHG Mercurial

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

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

D2873: remotenames: add functionality to override -B flag of push

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D2873#52021, @indygreg wrote:
  
  > Looks good!
  
  
  I'm very heavily against this direction. Changing the behavior of push (even 
in this extension) is something I've always considered outside the scope of 
remotenames. Having another extension that changes push behavior (e.g. 
bookmark-push) is where I think this should go so that remotenames is just 
that: keeping track of remote names.

REPOSITORY
  rHG Mercurial

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

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

D612: directaccess: add a hiddenlevel argument to registrar.command

[smf (Sean Farley)]

smf added a comment.


  Has there been discussion around extensions using this? Should there be 
discussion now? Specifically, I'm trying to figure out how external things will 
use this feature. Should it be a try/except? Or should modules set the access 
level before looking for a (potentially) hidden commit?

REPOSITORY
  rHG Mercurial

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

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

D464: commit: use an unambiguous path suffix for the commit editor file

[smf (Sean Farley)]

smf added inline comments.

INLINE COMMENTS

> durin42 wrote in cmdutil.py:3214
> I agree with smf: this patch is accomplishing a reasonable thing, but we 
> should clean up this interface if we're going to use it. Let's add an action= 
> parameter that's optional in the 4.4 cycle, with a devel warning if it's not 
> specified. Then in 4.4 we can make it a mandatory parameter for ui.edit().
> 
> Right now the only client of this poke-something-in-extra API is histedit, so 
> the time is right to clean up the API before more consumers do silly things. 
> For your immediate goal in this patch, here's what I'd suggest as a 
> compromise:
> 
> 1. Add the new action=None business to ui.edit(), which then makes 
> suffix='.hg$ACTION.txt' or similar
> 2. Update histedit to use that instead of screwing around with 'suffix' in 
> extra
> 3. Remove support for 'suffix' in extra
> 4. Do this patch, but with action='commit' instead of poking '.hgcommit.txt' 
> in the extra
> 
> How does that sound to everyone?

Yeah, this is pretty much what I had in mind (sorry again for the confusion!). 
I also don't have any preference for '.hg.$ACTION.txt' just for it to be 
something sane / stable.

REPOSITORY
  rHG Mercurial

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

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

D464: commit: use an unambiguous path suffix for the commit editor file

[smf (Sean Farley)]

smf added a comment.


  Everything I was talking about in https://phab.mercurial-scm.org/D468 was 
meant to be posted here. (I am not doing well with phabricator it seems)

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7920, @mbolin wrote:
  
  > @smf so are you OK with this patch as-is?
  
  
  Yeah, should be fine.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7915, @mbolin wrote:
  
  > @smf Personally, I think that https://phab.mercurial-scm.org/D464 is a 
better place to have that discussion.
  
  
  Sigh. Yes, this is what I was confused about.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7916, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D468#7914, @smf wrote:
  >
  > > Basically, I was pondering aloud if '~' would be enough to future-proof 
us and if we shouldn't just rename all temp files to something unique 
(HG_EDITOR for commits, HG_HISTEDIT for histedit, etc). What I was hoping for 
was a discussion on that.
  >
  >
  > This sounds like a good idea. However, if I'm understanding @mbolin 
correctly, we /also/ want a ~ suffix (or similar) so that devtools don't need 
to grow hg-specific logic for tempfiles (since, in the case of a ~ suffix, 
we'll be enough like vim that they will already correctly ignore things). Do I 
have a reasonable handle on this, or is there more nuance that I'm missing?
  
  
  Yeah, I don't know what happened. I guess I misread this and thought it was 
similar to work I did before. I withdraw my concerns and apologize for 
derailing this.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7912, @mbolin wrote:
  
  > @smf As I put in the summary, I think this use of `tempfile.mkstemp()` is 
different than the others in the codebase because it uses the `dir=` argument 
to create a file in the working copy. As such, I'd argue that it's reasonable 
to consider it separately from the others.
  >
  > In particular, I think it's independent of how temp files are created for 
things like commit messages as those are paths that are intended to be exposed 
to the user insofar as they are opened in the user's editor. As it stands, I 
have https://phab.mercurial-scm.org/D464 out for review as a first step to 
impose some order on path names for those types of files.
  >
  > As far as this change is concerned, the focus is to preserve the existing 
behavior of `mktempcopy()` while minimizing its impact on other developer tools.
  
  
  Jesus, I confused myself on this patch. Yes, you're right. I completely 
mistook this for other patches I worked on mucking around with the tmp 
directories.
  
  That's definitely on me, sorry!

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7910, @smf wrote:
  
  > That seems a bit over complicated to me. Why not just just use the
  >  random tmp as a directory instead of a file while renaming the file at
  >  the same time:
  >
  > /tmp/XYZ123/hg-editor (or HG_EDITOR or whatever)
  >
  > This would allow 'editortmpinhg'[1] to just be .hg/hg-editor (or
  >  HG_EDITOR etc). The '~' seems a bit like a quick hack and I think I'd
  >  prefer to do this cleanly. My logic here is:
  >
  > 1. if we can append a character to the suffix, then we should be able to 
change the directory
  > 2. we might want different 'file types' for different tmp files and '~' 
seems that it might not get us far enough. For instance: a) editor, b) 
conflicts c) histedit etc
  >
  >   For (a) and (c), I hacked something together for my fork of magit (called 
mahgic) so that I can have different modes for commit (to show the diff) and 
for histedit (so that 'tab' and 'enter' will show the commit at the point).
  >
  >   [1] I planned on making editortmpinhg a more thorough thing so that 
everything would be in the .hg directory so that programs (like emacs) would be 
able to find the correct repo given only the tmp file.
  
  
  Basically, I was pondering aloud if '~' would be enough to future-proof us 
and if we shouldn't just rename all temp files to something unique (HG_EDITOR 
for commits, HG_HISTEDIT for histedit, etc). What I was hoping for was a 
discussion on that.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7909, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D468#7908, @smf wrote:
  >
  > > In https://phab.mercurial-scm.org/D468#7836, @quark wrote:
  > >
  > > > In https://phab.mercurial-scm.org/D468#7833, @smf wrote:
  > > >
  > > > > Basically, I'd like a more unified approach for all types of temp 
files (commit message, histedit, conflicts, etc).
  > > >
  > > >
  > > > I think this patch is about low-level `util` function that shouldn't be 
coupled with `ui` or config. It has value on its own and I don't think such 
"unified approach" should block this patch - we can always add configs and make 
callers of `util.mktempcopy` pass `suffix` down here AFTER this patch.
  > >
  > >
  > > Um, what? That's the whole point of review. Yes, I know you think you can 
do this AFTER (why are we yelling?). I don't like this patch as-is and am 
frankly tired of trying to jam stuff through review and promise to clean up 
later. I am against this patch.
  >
  >
  > As a bystander here, I'm not even sure what the proposals are. Can one of 
you summarize what the competing ideas are?
  >
  > (In general I agree with smf that a more unified error-resistant API should 
be a goal, and I'm -0 on adding complexity to hg in the name of buck 
performance if there's a better proposal on the table (but if there is I've 
missed it.))
  
  
  From the mailing list (just posting this here for now so I can reply to it in 
the next message):
  
  Jun Wu  writes:
  
  > I think this patch is about changing the suffix. I guess some less powerful
  >  editors may only support matching file types by suffixes.
  
  Yeah, it seems that way.
  
  > Please correct me if I'm wrong, but I failed to see how the
  >  "experimental.editortmpinhg" setting could be used as-is to change the
  >  suffix. Therefore I think the patch is still needed.
  > 
  > Maybe we can change it to a config option? Like, instead of saying:
  > 
  >   extra_defaults = {
  >   'prefix': 'editor',
  >   'suffix': '.txt',
  >   }
  > 
  > Changing ".txt" there to be a config option (maybe
  >  experimental.editorsuffix).
  
  That seems a bit over complicated to me. Why not just just use the
  random tmp as a directory instead of a file while renaming the file at
  the same time:
  
  /tmp/XYZ123/hg-editor (or HG_EDITOR or whatever)
  
  This would allow 'editortmpinhg'[1] to just be .hg/hg-editor (or
  HG_EDITOR etc). The '~' seems a bit like a quick hack and I think I'd
  prefer to do this cleanly. My logic here is:
  
  1. if we can append a character to the suffix, then we should be able to
  
  change the directory
  
  2. we might want different 'file types' for different tmp files and '~' seems 
that it might not get us far enough. For instance: a) editor, b) conflicts c) 
histedit etc
  
  For (a) and (c), I hacked something together for my fork of magit
  (called mahgic) so that I can have different modes for commit (to show
  the diff) and for histedit (so that 'tab' and 'enter' will show the
  commit at the point).
  
  [1] I planned on making editortmpinhg a more thorough thing so that
  everything would be in the .hg directory so that programs (like emacs)
  would be able to find the correct repo given only the tmp file.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7836, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D468#7833, @smf wrote:
  >
  > > Basically, I'd like a more unified approach for all types of temp files 
(commit message, histedit, conflicts, etc).
  >
  >
  > I think this patch is about low-level `util` function that shouldn't be 
coupled with `ui` or config. It has value on its own and I don't think such 
"unified approach" should block this patch - we can always add configs and make 
callers of `util.mktempcopy` pass `suffix` down here AFTER this patch.
  
  
  Um, what? That's the whole point of review. Yes, I know you think you can do 
this AFTER (why are we yelling?). I don't like this patch as-is and am frankly 
tired of trying to jam stuff through review and promise to clean up later. I am 
against this patch.

REPOSITORY
  rHG Mercurial

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

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

D468: util: use ~ as a suffix for a temp file in the same directory as a source file

[smf (Sean Farley)]

smf added a comment.


  In https://phab.mercurial-scm.org/D468#7748, @durin42 wrote:
  
  > I don't object to this, but maybe others do. Reviewers not on vacation, 
please feel encouraged to push this.
  
  
  Well, I had some objections on the mailing list. It's sad that those don't 
show up in phabricator. Basically, I'd like a more unified approach for all 
types of temp files (commit message, histedit, conflicts, etc).

REPOSITORY
  rHG Mercurial

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

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