Page MenuHomePhabricator

push: added clear warning message when pushing closed branches(issue6080)
ClosedPublic

Authored by taapas1128 on Feb 28 2019, 2:15 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Feb 28 2019, 2:15 PM
taapas1128 updated this revision to Diff 14276.Feb 28 2019, 3:46 PM
mharbison72 added inline comments.
tests/test-push-warn.t
235

Minor nit, but it looks like the existing space before the branch name was lost. And it would probably be a little more readable if there was a space between the branch name and (closed).

taapas1128 updated this revision to Diff 14290.Mar 2 2019, 2:14 AM
pulkit added a subscriber: pulkit.Mar 6 2019, 8:16 AM
pulkit added inline comments.
mercurial/discovery.py
356

This is adding (closed) unconditionally even if the branch we are pushing is open.

tests/test-push-warn.t
256

Here none of the branch c and d are closed, but it still appends (closed).

taapas1128 updated this revision to Diff 14401.Mar 8 2019, 1:47 PM
taapas1128 marked an inline comment as done.Mar 8 2019, 1:49 PM
taapas1128 marked an inline comment as done.Mar 8 2019, 2:20 PM
taapas1128 updated this revision to Diff 14412.Mar 8 2019, 2:25 PM
mharbison72 added inline comments.Mar 11 2019, 12:28 PM
mercurial/discovery.py
358

I had assumed from the bug report that the request was to annotate each closed branch as closed, so I interpreted the output incorrectly. It might be too much output to put (closed) after each closed branch in the list if it is long. But maybe "(%d closed)" % len(closedbranches) will make it less ambiguous? See what others think.

taapas1128 added inline comments.Mar 11 2019, 12:38 PM
mercurial/discovery.py
358

maybe we can just add (contains closed branches) at the end of the message.

pulkit added inline comments.Mar 15 2019, 9:37 AM
mercurial/discovery.py
351

This is calculating all the closed branches in a repo. We need to find the closed branches which are in the newbranches list.

358

But maybe "(%d closed)" % len(closedbranches) will make it less ambiguous

This is a nice idea. Let's do it this way. What do you think?

taapas1128 added inline comments.Mar 15 2019, 9:42 AM
mercurial/discovery.py
358

okay i will update the patch .

taapas1128 marked 3 inline comments as done.Mar 15 2019, 10:03 AM

@pulkit done.

taapas1128 retitled this revision from push: added clear warning message when pushing a closed branch(issue6080) to push: added clear warning message when pushing closed branches(issue6080).Mar 15 2019, 10:07 AM
taapas1128 updated this revision to Diff 14507.

Nice work, I have left some inline comments. Can you also add a test where we pushing multiple branches and not every branch is a closed branch?

mercurial/discovery.py
348

How about having this as a set so that we don't need to convert it later.

350

if isclosed will work.

352

No need to convert closedbranches back to list.

356

if closedbranches will work here.

taapas1128 marked 4 inline comments as done.Mar 17 2019, 9:19 AM
taapas1128 updated this revision to Diff 14534.
pulkit accepted this revision.Mar 17 2019, 11:03 AM

@taapas1128 when you get time, can you see whether reading the branchmap can slow down things or not. It other words, can you test some performance numbers with it?

A good way to do that is to enable contrib/perf.py as perf extension and use hg perfdiscovery command.

Queuing this, many thanks!

This revision was automatically updated to reflect the committed changes.