Page MenuHomePhabricator

statecheck: added support for STATES
ClosedPublic

Authored by taapas1128 on Jun 8 2019, 4:58 PM.

Details

Summary

This removes STATES from state.py and adds support to
statecheck class to handle its features.
getrepostate() function is modified accordingly.

This adds a method 'cmdutil.addunfinished()' for appending to
the unfinishedstate list so as to keep 'merge' and 'bisect' at the last.

This also makes two separate message formats for checkunfinished() and
getrepostate() as there were previously present.

Results of test changed are shown.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
martinvonz added inline comments.Jun 17 2019, 12:44 AM
mercurial/state.py
135–143

Why does bisect have to be treated differently?

213

Why was the comment above marked as done? Did I just miss the answer?

tests/test-graft.t
281

I think we probably we want separate messages for morestatus and checkunfinished so this doesn't have to change.

tests/test-strip.t
276 ↗(On Diff #15529)

This doesn't seem right. Did you forget to update this patch after your other patch got landed?

taapas1128 added inline comments.Jun 17 2019, 3:30 AM
mercurial/state.py
135–143

This was suggested by @pulkit to keep bisect last too.

tests/test-strip.t
276 ↗(On Diff #15529)

yes I haven't updated this diff since that patch because as I said on irc I wanted inclusion of merge in checkunfinished() as seperate patch because :

  1. This would require cleanup of existing code which was detecting merge in various operations.
  2. The operations would lose there personalized messages I dont want them to be lost and develop a method in which registered message can be changed accordingly.
taapas1128 added inline comments.Jun 17 2019, 3:33 AM
tests/test-graft.t
281

In D6504 is not the message method alright?

taapas1128 marked an inline comment as not done.Jun 17 2019, 7:36 AM
taapas1128 updated this revision to Diff 15537.
taapas1128 updated this revision to Diff 15539.Jun 17 2019, 7:54 AM
taapas1128 updated this revision to Diff 15541.Jun 17 2019, 8:06 AM

Ah, because those were already in both unfinishedstates and in STATES. I assume there are still some small functional changes. For example, hg status -v should now mentioning an unfinished transplant operation (the transplant extension added an entry to unfinishedstates, but it wasn't in STATES).

I have updated the tests for transplant.

martinvonz added inline comments.Jun 17 2019, 12:40 PM
mercurial/state.py
135–143

@pulkit, can you explain why bisect needs to be last?

tests/test-graft.t
281

We generally (always?) use a short lowercase message for the hint. I'm not sure we want to change that. OTOH, I also don't want to make the morestatus (hg status -v) output inconsistent like this patch does. I suppose we could have a discussion about changing the style we use in our hints. You could send a message to mercurial-devel@ if you want to do that. It might be easier for you to avoid changing the style and let each operation have a message ("X in progress"), a hint ("use X to continue ..."), and another message for morestatus ("To continue: ...").

tests/test-strip.t
276 ↗(On Diff #15529)

I really don't want to have an intermediate patch affect test cases like this. So I guess I'm suggesting that you fix the existing code first then, although I don't know what cases those are.

taapas1128 updated this revision to Diff 15553.Jun 17 2019, 2:08 PM
taapas1128 marked an inline comment as done.Jun 17 2019, 2:10 PM
taapas1128 added inline comments.Jun 17 2019, 2:21 PM
mercurial/state.py
213

because the hard-coded values were removed. And regarding bisect as you can see nothing has been changed it is not handled by checkunfinished() and getrpostate remains unchanged too.

taapas1128 updated this revision to Diff 15555.Jun 17 2019, 3:17 PM
pulkit added inline comments.Jun 17 2019, 4:11 PM
mercurial/state.py
135–143

Because we can have a unresolved bisect state and unresolved rebase/histedit/evolve/graft or even merge state at the same time.

tests/test-rebase-pull.t
119 ↗(On Diff #15516)

we should keep this message, can you see how we can prevent this change?

taapas1128 marked an inline comment as done.Jun 17 2019, 5:29 PM
taapas1128 updated this revision to Diff 15556.

we should keep this message, can you see how we can prevent this change?

amended that.

taapas1128 added inline comments.Jun 17 2019, 5:32 PM
tests/test-graft.t
281

updates the messages to original ones in the next patch for now. Have a look.

taapas1128 updated this revision to Diff 15565.Jun 18 2019, 3:46 AM
martinvonz added inline comments.Jun 18 2019, 11:41 AM
hgext/strip.py
49–61 ↗(On Diff #15565)

I moved this code very recently, so this needs to be rebased.

60 ↗(On Diff #15565)

Move comma one step left

mercurial/state.py
135–143

But what fails if we don't have it here? No tests fail.

213

I can see that getrepostate() is unchanged, but I cannot see that checkunfinished() is unchanged (line 187 includes 'bisect' in the version I'm looking at). clearunfinished() also seems to handle bisect specially. I still think my suggested reportonly flag is the best option we have.

tests/test-graft.t
281

This should use the multi-line style

tests/test-histedit-fold.t
309

This should use the multi-line style

tests/test-rebase-conflicts.t
83

This should use the multi-line style

tests/test-shelve.t
378

This should use the multi-line style

1156–1157

This should use the single-line style like our other hints

tests/test-strip.t
278–279 ↗(On Diff #15565)

This should use the single-line style

tests/test-transplant.t
43–44 ↗(On Diff #15565)

This should use the single-line style

492 ↗(On Diff #15565)

This should use the multi-line style

taapas1128 marked 3 inline comments as done.Jun 18 2019, 12:42 PM
taapas1128 added inline comments.
mercurial/state.py
213

the special handling is given so that checkunfinished() and clearunfinished() bypass bisect completely.(see line 93 D6501 the comment above unfinishedstates) And reportonly flag is not required because for morestatus uses getrepostate() which is itself a report only feature.

tests/test-graft.t
281

that is done in D6504. Do you want me to fold that here?

martinvonz added inline comments.Jun 18 2019, 12:48 PM
mercurial/state.py
213

Note that I didn't suggest changing getrepostate() -- I said (or tried to say) that I would like checkunfinished() and clearunfinished() to not have handle bisect specially.

tests/test-graft.t
281

I thought we had agreed (at least Pulkit seemed to agree with me) to add a third argument for the multi-line message for verbose status. That should mean that no tests would need to change.

taapas1128 marked an inline comment as done.Jun 18 2019, 12:48 PM
taapas1128 updated this revision to Diff 15581.
taapas1128 updated this revision to Diff 15584.Jun 18 2019, 1:06 PM
taapas1128 marked an inline comment as done.Jun 18 2019, 1:07 PM
taapas1128 marked an inline comment as done.Jun 18 2019, 2:42 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15587.
taapas1128 marked 3 inline comments as done.Jun 18 2019, 2:46 PM

I forgot to add tests for hg status -v case of interrupted update. That is added now.

mercurial/state.py
213

added reportonly flag

taapas1128 marked an inline comment as done.Jun 18 2019, 3:39 PM
martinvonz added inline comments.Jun 18 2019, 4:47 PM
mercurial/state.py
125

This function shares no functionality between the status=False and status=True cases and the callers all seem to know which version they want to call (no dynamic switching between the two), so it would be better to have two separate functions.

176

I think we should pass cmdmsg='outstanding uncommitted merge' here so we affect existing tests as little as possible.

tests/test-graft.t
282

What caused this change?

tests/test-merge1.t
52 ↗(On Diff #15587)

This should use the multi-line style

taapas1128 added inline comments.Jun 18 2019, 4:55 PM
tests/test-graft.t
282

the stopflag. Since graft supports --stop. It should be displayed in hg status --verbose and similar should happen with any extension in future which supports --stop.

tests/test-merge1.t
52 ↗(On Diff #15587)

there is no hg update --abort so I am afraid I cant add a multi line comment here.

taapas1128 updated this revision to Diff 15588.Jun 18 2019, 5:31 PM
taapas1128 marked 3 inline comments as done.Jun 18 2019, 5:32 PM
pulkit added inline comments.Jun 19 2019, 10:22 AM
mercurial/state.py
135–143

I see we got a reportonly flag. So it's not needed anymore. Sorry about that.

taapas1128 marked 2 inline comments as done.Jun 19 2019, 11:48 AM
taapas1128 added inline comments.
mercurial/state.py
137

removed bisect from here . @martinvonz have a look.

martinvonz added inline comments.Jun 19 2019, 2:37 PM
tests/test-graft.t
282

Oh, I had not noticed the stopflag argument (or I had forgotten it). Rebase also supports --stop, so shouldn't that also pass stopflag=True?

martinvonz added inline comments.Jun 19 2019, 2:43 PM
mercurial/state.py
180

nit: s/mergeskip/skipmerge/ perhaps?

tests/test-shelve.t
1155

Don't forget to add patch that cleans up shelve.py to not handle the merge case explicitly (unless it's still needed there for some reason). The same applies to mq.

taapas1128 added inline comments.Jun 19 2019, 2:46 PM
tests/test-shelve.t
1155

as a seperate patch after this right ?

martinvonz added inline comments.Jun 19 2019, 2:52 PM
tests/test-shelve.t
1155

Yes, separate patch (or two, probably, since both mq and shelve seem possible to clean up) after this one.

taapas1128 updated this revision to Diff 15597.Jun 19 2019, 3:03 PM
taapas1128 marked 2 inline comments as done.Jun 19 2019, 3:05 PM

okay I will send seperate patches for them right away . The amends in this are done.

taapas1128 marked an inline comment as done.Jun 19 2019, 3:06 PM
martinvonz accepted this revision.Jun 19 2019, 6:33 PM
martinvonz added inline comments.
hgext/rebase.py
1954–1955 ↗(On Diff #15597)

It seems like this should also be unified with the new tracking of unfinished operations. Will you have time to look into that after the current series?

mercurial/state.py
100–102

We should probably put all the boolean options together and the message overrides together (as they already are). Maybe move stopflag and reportonly before cmdmsg?

This revision is now accepted and ready to land.Jun 19 2019, 6:33 PM
taapas1128 marked an inline comment as done.Jun 20 2019, 2:14 AM
taapas1128 added inline comments.
hgext/rebase.py
1954–1955 ↗(On Diff #15597)

Yes sure have a look at D6551 , D6552.

taapas1128 updated this revision to Diff 15614.Jun 20 2019, 4:14 AM
martinvonz added inline comments.Jun 23 2019, 5:46 PM
mercurial/state.py
100–102

Could you move stopflag too?

taapas1128 updated this revision to Diff 15641.Jun 24 2019, 5:33 AM
taapas1128 marked an inline comment as done.Jun 24 2019, 5:35 AM
This revision was automatically updated to reflect the committed changes.