This is an archive of the discontinued Mercurial Phabricator instance.

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

taapas1128 created this revision.Jun 8 2019, 4:58 PM
taapas1128 updated this revision to Diff 15405.Jun 9 2019, 8:53 AM
taapas1128 updated this revision to Diff 15408.Jun 9 2019, 10:48 AM
pulkit added a subscriber: pulkit.Jun 10 2019, 9:44 AM
pulkit added inline comments.
mercurial/state.py
135–136

This change should be a part of patch where isunfinished was introduced.

147–154

This adding of space should also be a part of patch where this line was introduced.

157

merge can be cleared using hg update and hence clearable should be True

taapas1128 marked 3 inline comments as done.Jun 10 2019, 10:58 AM
taapas1128 added inline comments.
mercurial/state.py
135–136

mergecheck is introduced in this patch now so now this change must be okay I suppose.

taapas1128 marked an inline comment as done.Jun 10 2019, 1:54 PM
taapas1128 updated this revision to Diff 15426.
taapas1128 updated this revision to Diff 15428.Jun 10 2019, 2:32 PM
pulkit added inline comments.Jun 10 2019, 7:03 PM
mercurial/state.py
186

the previous patch moves all this code, and this patch just removes it mostly. I think we can prevent the code-movement then.

taapas1128 added inline comments.
mercurial/state.py
186

Yes but this is according to the splitting @martinvonz suggested. The same thing happens between D6484 and D6501. Actually this is done for the benefit of the reviewer so he/she can see how the code changed (here _getrepostate() function.

taapas1128 updated this revision to Diff 15440.Jun 11 2019, 9:48 AM

Results of tests are shown.

Are there other functional changes that are not shown? For example, was it previously allowed to do some operations (update?) during an interrupted rebase that are no longer allowed?

mercurial/state.py
135–136

Yes, I agree with doing it here. It wasn't used before, so it wasn't clear then why it would be needed or how it would be used.

But actually, it still doesn't seem to be used. Should it be removed?

149

Did you mean None instead of 'None'?

149

There was a comment somewhere saying that merge has to come last. It seems merge is no longer last and you've solved it by checking specifically for 'merge' in many places. I think it would be better to check for unfinished merges last like we used to. That can be achieved by using a helper that returns the states in the desired order, or it can be done by hiding the registration behind a function that knows to insert new states before 'merge'. Or perhaps it can be generalized to some "priority" option for the states where the lowest priority is checked last (and rebase, histedit and similar would all have the same priority)

173

Why is merge not considered? The caller is expected to call bailifchanged() right after. I think that will report the unfinished merge state. What happens if we report it here? Would it result in a different message? Or would it fail when it shouldn't when bailifchanged() is called with merge=True (or False?)?

214

Hard-coded values like this goes against the idea of a generic check for unfinished operations. What is it that we really want to check here? The same applies in lots of places above, of course. This really needs to be fixed.

It seems bisect state is quite different from the others. We want to allow any (?) operation with an unfinished bisect state. We just want to report it as an unfinished operation in morestatus. So maybe we need another flag like reportonly=True?

taapas1128 added inline comments.Jun 11 2019, 1:19 PM
mercurial/state.py
149

In getrepostate() function merge must come last that is why I have filtered off update and merge and then checked them at the very end. In checkunfinished() there is no such thing.

173

Actually in older code too there is no support for merge and bisect in checkunfinished() or clearunfinished() and is handled seperately. So I have bypassed merge and bisect over here too to comply with it. As you can see later in clearunfinished() has util.unlink() and as fname is not present for merge it cant be passed here.

214

These are hard-coded as we need to check update and merge last. Earlier they were hard coded in form of a list. here I have bypassed them earlier and then checked for them last .

taapas1128 marked an inline comment as not done.Jun 11 2019, 1:25 PM

@martinvonz I haven't changed anything regarding rebase and update on interrupted states hence not added any tests.

mercurial/state.py
149

I will correct that None. Maybe an empty string "" would serve better.

taapas1128 marked an inline comment as not done.Jun 12 2019, 1:55 PM

@martinvonz I haven't changed anything regarding rebase and update on interrupted states hence not added any tests.

Do those commands call checkunfinished?

mercurial/state.py
149

I think None is better. That more clearly says "there is no associated file", while empty string sounds more like "the associated file is named ''".

149

I know that's what you did regarding merge (as I said). I suggested a different solution (or three, actually). Are you saying that won't work? Can you explain why in that case?

173

So what happens if we check merge state too here? I know we didn't do it before, but would it make sense to do it? It would probably belong in a separate patch if we did it.

214

I know. That's what I tried to explain. I suggested different solutions. Can you explain why they won't work or why they're worse?

taapas1128 marked an inline comment as done.Jun 12 2019, 2:53 PM

yes, they do . as they did earlier no extra case has been generated.

mercurial/state.py
149

Yeah, I did work something out with suggestion of @pulkit . here (https://bitbucket.org/taapasX28/hg/commits/9bf3ef88f2912e5f87a9de7119f9137916bf8426). Have a look. This would allow a user to append to a list whilst keeping merge and bisect at last.

149

okay I will update that with None then.

173

if I pass merge through it (even while keeping its priority at bottom too) things get messed up and abort: merge in progress at places it shouldn't be. Like on every commit , update , graft (i tested on test-graft.t). I am going through the code searching why this is happening however that wont be easy and take a seperate patch for sure .

taapas1128 added inline comments.Jun 12 2019, 4:10 PM
mercurial/state.py
173

I went through the code this is happening due to the checkunfinished() which is called just before the commit in _docommit() at line 1666

taapas1128 edited the summary of this revision. (Show Details)Jun 13 2019, 9:19 AM
taapas1128 updated this revision to Diff 15480.
taapas1128 added inline comments.Jun 13 2019, 9:22 AM
mercurial/cmdutil.py
622

@martinvonz have a look this is making sure bisect and merge stays the last.

mercurial/state.py
173

I guess the problem was with wrong value of allowcommit flag for merge. It is not skipped now and it works alright.

taapas1128 updated this revision to Diff 15497.Jun 13 2019, 4:44 PM
taapas1128 updated this revision to Diff 15499.Jun 13 2019, 4:51 PM
taapas1128 updated this revision to Diff 15501.Jun 13 2019, 5:41 PM
taapas1128 updated this revision to Diff 15503.Jun 13 2019, 5:45 PM

@martinvonz @pulkit passing merge through checkunfinished() with the correct allowcommit flag has led to change in the tests most of which are positive and the merge in progress which were detected by separate methods earlier can be detected by checkunfinished() alone. The only test that has given negative results in test-strip.t. I will try to improve that too.

mercurial/state.py
135–136

removed that

147

@martinvonz have a look this is making sure bisect and merge stays the last.

187

merge fname set to None .

taapas1128 updated this revision to Diff 15506.Jun 14 2019, 4:33 AM

This patch does multiple things,

  1. improve how states are registered
  2. move STATES to cmdutil

it will be nice if we take 1) out and amend that into the initial patch which introduced how states are registered.

taapas1128 updated this revision to Diff 15516.Jun 14 2019, 3:34 PM
taapas1128 edited the summary of this revision. (Show Details)Jun 14 2019, 3:48 PM
taapas1128 edited the summary of this revision. (Show Details)Jun 15 2019, 6:40 AM
taapas1128 updated this revision to Diff 15529.

@martinvonz I haven't changed anything regarding rebase and update on interrupted states hence not added any tests.

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).

martinvonz added inline comments.Jun 17 2019, 12:44 AM
mercurial/state.py
136–144

Why does bisect have to be treated differently?

214

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
136–144

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
136–144

@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
214

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
136–144

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
136–144

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

214

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
214

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
214

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
214

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
126

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.

177

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
136–144

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
138

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
181

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.