Re: [PATCH v3] Add code to Mercurial VCS backend to show topic if there is any.

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Manuel Jacob wrote on Sun, 21 Jun 2020 13:17 +0200:
> On 2020-06-20 12:43, Daniel Shahaf wrote:
> > Manuel Jacob wrote on Sat, 20 Jun 2020 01:31 +0200:
> >> "Topics" is an experimental concept in Mercurial that augments the
> >> current branching concept (called "named branches").
> >> 
> >> For more information, see the not always up-to-date Mercurial Wiki 
> >> page
> >> https://www.mercurial-scm.org/wiki/TopicPlan.
> >> ---
> >>  Functions/VCS_Info/Backends/VCS_INFO_get_data_hg | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg 
> >> b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> >> index cd5ef321d..deeb331a0 100644
> >> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> >> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_hg
> >> @@ -5,7 +5,7 @@
> >> 
> >>  setopt localoptions extendedglob NO_shwordsplit
> >> 
> >> -local hgbase bmfile branchfile rebasefile dirstatefile mqseriesfile \
> >> +local hgbase bmfile branchfile topicfile rebasefile dirstatefile 
> >> mqseriesfile \
> >>      curbmfile curbm \
> >>      mqstatusfile mqguardsfile patchdir mergedir \
> >>      r_csetid r_lrev r_branch i_bmhash i_bmname \
> >> @@ -27,6 +27,7 @@ mergedir="${hgbase}/.hg/merge/"
> >>  bmfile="${hgbase}/.hg/bookmarks"
> >>  curbmfile="${hgbase}/.hg/bookmarks.current"
> >>  branchfile="${hgbase}/.hg/branch"
> >> +topicfile="${hgbase}/.hg/topic"
> >>  rebasefile="${hgbase}/.hg/rebasestate"
> >>  dirstatefile="${hgbase}/.hg/dirstate"
> >>  mqstatusfile="${patchdir}/status" # currently applied patches
> >> @@ -69,6 +70,13 @@ fi
> >>  # If we still don't know the branch it's safe to assume default
> >>  [[ -n ${r_branch} ]] || r_branch="default"
> >> 
> >> +# Show topic if there is any (the UI for this experimental concept is 
> >> not yet
> >> +# final, but for a long time the convention has been to join the 
> >> branch name
> >> +# and the topic name by a colon)
> >> +if [[ -f ${topicfile} && -r ${topicfile} && -s ${topicfile} ]] ; then
> >> +    r_branch=${r_branch}:$(head -n 1 ${topicfile})
> >> +fi
> > 
> > I'm happy to read the file directly, despite experimentality, given the
> > discussion elsethread that you'll propose a corresponding unit test to
> > the upstream maintainers.
> > 
> > Using $(head) forks, and we try to avoid forks because they're 
> > expensive
> > on some platforms.  In this case, you could use «IFS= read -r REPLY
> > <$topicfile» instead.  (The construct «$(<foo)», just above the hunk
> > context, is special-cased to not fork.)
> 
> Is there anything special about `REPLY`? It seems to be a convention. 
> Could I also use e.g. TOPIC?

Yes, $REPLY is merely a convention (see zshparam(1)).  You could use
$topic (lowercase) instead; $TOPIC (uppercase) would not be idiomatic.

> > As written, existing set-branch-format hooks would see the 
> > ${hook_com[branch]}
> > input argument set to strings such as "default:mytopic".  Would this
> > behaviour be expected, or would people rather expect 
> > ${hook_com[branch]}
> > to be set to strings such as "default" even when .hg/topic exists, and
> > to have a new ${hook_com[foo]} key whose value would be "mytopic" (or
> > possibly "default:mytopic")?  This would affect the API to hooks but
> > would not affect the final output.
> 
> If the topic extension is enabled, the meaning of places using branch 
> names is changed.
> When determining how many heads a branch has (e.g. when warning that 
> another head will be created), each "branch:topic" (or simply "branch" 
> if there’s no topic) is considered separately.
> When specifying a revision, "branch:topic" refers to the head of the 
> branch with branch name "branch" and topic name "topic", while "branch" 
> refers to the headmost revision with branch name "branch" but no topic.
> 
> An example:
> 
> B C
> |/
> A
> 
> Assume that all three changesets have the "branch" field set to 
> "develop". B has topic "feature1" and C has topic "feature2".
> 
> * "develop" refers to A
> * "develop:feature1" refers to B
> * "develop:feature2" refers to C
> 
> If the user updated to B, but the prompt shows "develop" (this is 
> currently the case), it would be misleading. I don’t know all 
> possibilities in which the hooks use `${hook_com[branch]}`, but I’d 
> expect that including the topic would be correct in more cases than not 
> including it.
> 

Thanks for the walkthrough.  Agreed.  Might want to state on
Doc/Zsh/contrib.yo:1331 that in the hg backend the topic is included,
then.

> > Similarly, as written, the %b expando in "formats", "actionformats", 
> > and
> > "branchformat" will always be replaced with a string such as
> > "default:mytopic".  If someone wants to change the format,¹ they won't
> > be able to achieve that using zstyle settings alone; they'd have to
> > write an appropriate vcs_info hook in their zshrc.  What do you think
> > of, say, making the topic name available as a separate expando in
> > branchformat?  Again, this would not affect the default output, but 
> > make
> > customizations easier to implement.
> > 
> > ¹ Use-cases for changing the format include: colouring "default" and
> > "mytopic" differently (compare «zstyle '*' branchformat '%B%b%%b 
> > %U%r%u'»
> > when the get-revision style is set); changing the colon to a space for
> > readability or copypasteability; and perhaps even eliding the topic
> > name entirely(?).
> 
> For the reasons stated above, the topic name should be included. The 
> colon should also stay because the combined "branch:topic" can be passed 
> as a revision identifier to various places. Coloring branch and topic 
> differently wouldn’t be "wrong", but since the "branch:topic" mostly 
> makes sense together, personally I’d like to have it in the same 
> appearance.

I see: if "branch:topic" mostly appears as an atomic unit, it makes
sense that most people will want to color it atomically.

>

That's all, I think.  I don't foresee any blocks to merging v4 when you
post it.

Cheers,

Daniel