Gentle ping for review.

Gentle ping for review.

In D5940#88529, @martinvonz wrote:

Can you fix the first line of the commit message to say "don't allow dirty working copy with PATH" (the "don't" was missing). It was allowed before this patch, it is not allowed with this patch (unless the config is set, of course).

Ping for review.

In D5940#88065, @martinvonz wrote:

I don't understand the commit message. experimental.uncommitondirtydir already works with PATH, doesn't it? I think you mean something like "uncommit: allow dirty working copy with PATH". Did I understand that right?

Ping for review.

Thanks for queuing @pulkit. @yuja thanks for the review.

In D5940#87026, @pulkit wrote:

Hi, it will be good if you specify that the patch is authored by someone else, mention their name and also provide the link to the original PR of this patch.

nit-pick is resolved. getintrange() will throw a ParseError on setting size to one of min:, :max or :. In ideal case, on calling from expectsize() it shouldn't fail.

@yuja I've updated the revision with the suggested changes.

rHG66399f2e92aa solves this issue, many thanks @spectral. Closing this.

@spectral @yuja Can I close this?

Can you share your failed attempt?

Maybe you can get how revset works by testing expression with
hg debugrevspec -v.

First, I think the word expect is too general. Perhaps, this should be called
expectsize() or expectlen().

@pulkit Thanks for the quick review! I've updated the revision with the suggested changes. @durin42 It would be great if you let us know your opinion on one(<set>) as an alias for expect(<set>, 1). I did it as it was present in WeShouldDoThat page.

@durin42 Thanks for the quick review.
@mharbison72 Yup. I should not be making changes to the ui as this can be fixed simply within the call itself. My bad. I've updated the revision as per your suggestion, many thanks!

Sorry for bumping this up. I worked on a similar issue on D5490. My initial revision was similar to this one. @spectral Don't you think it'll affect record extension which was written with zero test coverage? Please don't feel obligated. I am just being curious.

One nit: better to not use .split() since a filename may include whitespace.

Thanks for queuing this!

_performrevert() would be in the same boat, but it was explicitly flagged on
at f37a69ec3f47. This implies that the diff.ignorews option would be used
in practice to exclude whitespace changes while interactive commit/revert.
So disabling any whitespace options would break someone's workflow.

Sorry for late, but doesn't it break hg record --ignore-all-space, etc.?

@durin42 I've added test for the current revision. Unfortunately, I'm unable to write a test for the wrong behaviour. I'll try writing it again. Any hints from your side?

Could you add a test?

(Bonus points if you add a test that shows the wrong behavior and then fix it in this change.)

In D5494#81855, @pulkit wrote:

Sorry for bumping on this now, can you add tests for this as follow-up?

This is logically incorrect. The problem is that we're doing variable
expansion at too lower layer. vfs(expand(user_specified_path)) makes
some sense, but vfs(expand(getcwd())) is clearly wrong. And the vfs class
can't know where the base comes from.