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

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

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

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

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

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.

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?

Did you mean None instead of 'None'?

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)

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

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?

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.

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.

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 .

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

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 ''".

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?

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.

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?

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.

okay I will update that with None then.

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 .

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

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

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

removed that

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

merge fname set to None .

Why does bisect have to be treated differently?

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

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

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

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

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.

In D6504 is not the message method alright?

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

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

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.

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.

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

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

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

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

Move comma one step left

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

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.

This should use the multi-line style

This should use the multi-line style

This should use the multi-line style

This should use the multi-line style

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

This should use the single-line style

This should use the single-line style

This should use the multi-line style

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.

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

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.

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.

added reportonly flag

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.

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

What caused this change?

This should use the multi-line style

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.

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

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

removed bisect from here . @martinvonz have a look.

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?

nit: s/mergeskip/skipmerge/ perhaps?

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.

as a seperate patch after this right ?

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

Yes sure have a look at D6551 , D6552.

Could you move stopflag too?