The devguide (http://docs.python.org/devguide/patch.html) recommends the use of the mercurial “mq” feature to work with patches and that works IMHO very well. It also states that before sending the patch a sanity check should be done ('devguide: Preparation and Generation'). At this stage, if one has the patch as tip (hg qapplied), the advice to run “pathcheck” doesn't help as no changes are noticed.

The message is:
-----------------------------------------------
Modules/Setup.dist is newer than Modules/Setup;
check to make sure you have all the updates you
need in your Modules/Setup file.
Usually, copying Modules/Setup.dist to Modules/Setup will work.
-----------------------------------------------
./python ./Tools/scripts/patchcheck.py
Getting the list of files that have been added/changed ... 0 files
Fixing whitespace ... 0 files
Fixing C file whitespace ... 0 files
Fixing docs whitespace ... 0 files
Docs modified ... NO
Misc/ACKS updated ... NO
Misc/NEWS updated ... NO
The tool should check if some mq patches are applied (from “normal” tip to “mqtip” and make it's checks there.

Thanks in advance !

Francis

Here is a patch that works for me. Please check and review it.

Thanks in advance!

Francis

Thanks for taking this up; it's something that's been bothering me for a
while now.

A couple of comments:

- The mq_changed_files() function will break if the user has specified
  git-format diffs in their ~/.hgrc file. In this case, the diff command
  in the output will begin with "diff --git" instead of "diff -r".

- What happens if a file has been deleted by the patch? The regular
  "hg status" command is set up to only return the added/modified files.
  You might want to check whether the file currently exists before adding
  it to the list of filenames to check.

- Have you considered the possibility of there being multiple patches?
  In this case, I think it makes sense to check the files in every patch,
  not just the topmost.


  An alternative approach that solves all three of these problems is to
  check whether we have any patches applied (using "hg qapplied"), and if
  this is the case, then add "--rev qbase" to the "hg status" command
  line. This will list all files added/modified by patches as well as by
  uncommitted changes.


- Using an mq command (e.g. qdiff or qapplied) will fail if the user does
  not have the mq extension enabled. In this case, mq_changed_files()
  should not allow Mercurial's error message to be printed. Ideally, it
  should distinguish between this and other errors by checking the
  subprocess's stderr, so that if a different error occurs, we can still
  print out the error message.

- In changed_files(), I don't think it makes sense to create an empty
  list ("files = []") and then append to it immediately. It would be
  better to just initialize "files" directly from the list comprehension.

Actually I would change the devguide/patch.html page to focus less on mq (but this is a bit orthogonal).
The easiest thing to do is to make some changes and use "hg diff > patch.diff" and the devguide should advertise this IMHO.  This also works fine with `make patchcheck`.

Nice feedback !

One question :

>
>    An alternative approach that solves all three of these problems is to
>    check whether we have any patches applied (using "hg qapplied"), and if
>    this is the case, then add "--rev qbase" to the "hg status" command
>    line. This will list all files added/modified by patches as well as by
>    uncommitted changes.
>

Shouldn't be --rev qparent ? with --rev qbase the first MQ patch applied 
changes are not listed ... (one wants the changes between
qparent and qtip)

> Shouldn't be --rev qparent ?

Yes, that's right. I seem to confuse qbase and qparent often...

The patch is updated. Notice about:
[...]Ideally, it
  should distinguish between this and other errors by checking the
  subprocess's stderr, so that if a different error occurs, we can still
  print out the error message.
[…]
that should be improved as now stderr is ignored in 'mq_changed_files'
(user case: 'no mq extension enabled'). Suggestions are welcome!

> [..] but this is a bit orthogonal.
IMHO the MQ feature works very well for pydev newbies (as I'm) and it's
a good recommendation (it's a taste thing ...)

> The easiest thing to do is to make some changes and use
> "hg diff>  patch.diff" and the devguide should advertise this IMHO.  This also works fine with `make patchcheck`.

There is some small differences between diff, qdiff and status but if one uses MQ then diff isn't enough. One really wants to know the changes from qparent to qtip and using 'status' as proposed I thing that it's the best solution.

Cheers,
francis

Hmm... it looks like mq_changed_files() duplicates a chunk of logic from
the existing changed_files() code. You can get rid of this redundancy by
replacing mq_changed_files() with a function that checks for applied MQ
patches; let's call it mq_patches_applied(). Then you can say:

    if mq_patches_applied():
        cmd += ' --rev qparent'

before the call to Popen in changed_files(), and leave the rest of the
function unmodified. The basic logic that the code will follow is this:

    if <MQ patches applied>:
        return files listed by 'hg status --added --modified --no-status --rev qparent'
    else:
        return files listed by 'hg status --added --modified --no-status'


The mq_patches_applied() function can be implemented by running
'hg qapplied' and checking whether it (a) prints at least one patch name,
(b) prints out nothing, or (c) gives an error.

Updated.

Interesting: I saw that repetition but due “[…] Ideally, it
should distinguish between this and other errors by checking the
subprocess's stderr, so that if a different error occurs, we can still
print out the error message. […]” I just wanted to keep cmd's separated first :-)

Let me know what can be done …

Cheers,
francis

Patch looks good; I've just got one more small suggestion.

The variable 'some_applied' in mq_patches_applied isn't really necessary
- it would be clearer if you just said:

    return st.returncode == 0 and bstdout

after the call to st.communicate().


> Interesting: I saw that repetition but due “[…] Ideally, it
> should distinguish between this and other errors by checking the
> subprocess's stderr, so that if a different error occurs, we can still
> print out the error message. […]” I just wanted to keep cmd's separated > first :-)

When I said that, I was referring to the 'hg qapplied' command (not the
'hg status' one), but I can see how it might have been confusing ;)

Ok! I haven't added NEWS as I thing is easier for the person that applies the patch to simply write the line directly there instead of merging. Is that ok? let me know if something has to be improved. 

Thanks again for your mentoring this!

New changeset 179bc7557484 by Nadeem Vawda in branch '2.7':
Issue #14053: Fix "make patchcheck" to work with MQ.
http://hg.python.org/cpython/rev/179bc7557484

New changeset fc5de19c66e2 by Nadeem Vawda in branch '3.2':
Issue #14053: Fix "make patchcheck" to work with MQ.
http://hg.python.org/cpython/rev/fc5de19c66e2

New changeset 1bdca2bcba6d by Nadeem Vawda in branch 'default':
Merge: #14053: Fix "make patchcheck" to work with MQ.
http://hg.python.org/cpython/rev/1bdca2bcba6d

Committed. Thanks for the patch!

> I haven't added NEWS as I thing is easier for the person that applies the patch to simply write the line directly there instead of merging. Is that ok?

Yes, that's fine; usually the NEWS entry is more or less the same as the
commit message, so it makes sense to leave it to the committer. It also
makes backporting easier - a diff against 3.3's NEWS file often won't
apply cleanly to 3.2 or 2.7.