actually I merge is not detected w.r.t statefile but with the help of a predicate but since it needs be part of the same class I named the statefile as None for it.

telling that here would be fine I suppose so I should add it here.

I might have a better solution to it what if I leave this as it is and add two more variables to the constructor specialmsg=None and specialhint=None. In case the extension uses any special message or hint in place of standard ones these two values can be given accordingly and standard messages can be overidden.

yeah it should be part of the next patch but having it here will make isfinishedcomplete

I have implemented that have a look. For core commands I have retained the if cases for hgext I have replaced that

removed that

Not: we usually don't include "special" in the names of this type of argument. A default of None often implies that it's calculated in some way if it's not provided.

Nit: The . got lost here. Without it, hg update -C might also update to another commit.

Should move the graft and update cases to where they're registered (around line 152)

Nit: return here and in the case below for consistency (or assign to msg also on line 145) .

Nit: mergecheck seems unused and it seems no callers pass in a value for it, so it could probably be removed (at least for now).

nit: no need of this else

nit: space before and after the operator

the code around these statements doing append is looking a bit complex. for making it cleaner and more readable, let's do either one of thing:

1. initialize an object before and then append
2. improve formatting of this

This applies to all the append which are there in the patch.

this comment on top of unfinished state is lost. We should reword and keep it.

that will to deal -> that deals

prepend all the class names with an _, like self._cmdname, self._fname etc. only the variables

That's a code style to say they are internal to readers and class users.

needs space between = operator. I guess this is caught by test-check*, make sure you run tests.

oh I will correct that. Strange it wasn't caught in test-check-code.t. I always try to run that.

I tried again it was not detected. Should I file this as a bug ?

add a new line after this.

we can accept cmddata here as dict, no need for **. They can be removed from callers of it's function too. Only use when we pass that to statecheck.init()

mark this "private" by prepending name with _?

Perhaps this should be opname instead? It's not always a command.

I'm not sure the cmd prefix in cmdmsg and cmdhint adds anything

nit: I think you mean s/required/desired/

This can also be "private", I think

I think it would be simpler to make addunfinished() take four arguments instead of a dict. That's what we usually do. @pulkit, what do you think? Is there are reason to pass a single dict here?

It's more important to document what it does than how it does it. The callers don't need to know about the stuff that's currently documented here.

there are not just 4 arguments but arguments for personazed messages too. Also this method has been made simin=lar to the registration method of registrar which also takes a dict.

I did that to keep a different name as compared to default messages.

there are not just 4 arguments but arguments for personazed messages too.

merge.update() has 11 arguments :P That doesn't mean it's good, though, and that example is actually pretty bad. I think 6-7 arguments isn't bad, and using a dict is especially bad because it loses type safety (you can do data = {'claerable': True} and not notice that you misspelled it). It might be okay to create a simple type for this, but I think it's going to be simpler to just use separate function arguments.

Also this method has been made simin=lar to the registration method of registrar which also takes a dict.

Where is that? I looked a bit in registrar.py but I couldn't find it.

I think it would be simpler to make addunfinished() take four arguments instead of a dict. That's what we usually do. @pulkit, what do you
think? Is there are reason to pass a single dict here?

I agree that passing four arguments will be better than a dict. I didn't realize that before reading your latest comment.

It's funny how Pulkit's comment is now at this line and looks like asking to add spaces around = here. But in this case = is not an operator, it's used to indicate keyword arguments. We don't want spaces around it (similarly to PEP8).

Bad:

addunfinished(
    'graft',
    fname = 'graftstate',
    clearable = True,
    allowcommit = False,
    cmdhint = _("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

Good:

addunfinished(
    'graft',
    fname='graftstate',
    clearable=True,
    allowcommit=False,
    cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

Even better (as long as it fits into 79 characters per line):

addunfinished(
    'graft', 'graftstate', clearable=True, allowcommit=False,
    cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

There's a lot of places where this needs to be fixed in this series, not just here.

A good patch to send right after this patch is one that removes this line and lets shelve use the default message. I understand why they picked "already" (because it makes sense when you run hg shelve/unshelve with an ongoing unshelve), but it doesn't make sense when e.g. running hg commit. There is already a test showing the bad example (and the good one) in test-shelve.t.

I'm not sure if we should have a default for clearable. Not having a default would force callers to think about it, which may be good. OTOH, the default of False is pretty safe, so it's probably fine to have a default. But if we do have a default, I think you should update the callers to not pass clearable when they want the default.

Don't specify allowcommit when it's the same as the default (the same applies in other places)

Thank you I will take care from now on :)

unrelated change?

need a space after ,.

unrelated change?

s/of/or (unfinished condition or not)

need to amend it D6484. sorry.

Add space before fname