classification
Title: Make patchcheck work with MQ
Type: Stage: resolved
Components: Demos and Tools, Devguide Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, eric.araujo, ezio.melotti, francismb, nadeem.vawda, python-dev
Priority: normal Keywords: patch

Created on 2012-02-19 14:24 by francismb, last changed 2012-02-22 10:03 by nadeem.vawda. This issue is now closed.

Files
File name Uploaded Description Edit
issue14053_336a614f35a3.patch francismb, 2012-02-19 18:22 Adds files that changed due mq applied patches review
issue14053_1f9461ef6312.patch francismb, 2012-02-20 21:11 review
issue14053_3297dcdad196.patch francismb, 2012-02-21 00:04 review
issue14053_2cceb4d3079a.patch francismb, 2012-02-21 21:50 review
Messages (13)
msg153703 - (view) Author: Francis MB (francismb) * Date: 2012-02-19 14:24
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
msg153715 - (view) Author: Francis MB (francismb) * Date: 2012-02-19 18:22
Here is a patch that works for me. Please check and review it.

Thanks in advance!

Francis
msg153723 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-19 20:13
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.
msg153763 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-02-20 05:06
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`.
msg153787 - (view) Author: Francis MB (francismb) * Date: 2012-02-20 18:08
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)
msg153796 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-20 18:51
> Shouldn't be --rev qparent ?

Yes, that's right. I seem to confuse qbase and qparent often...
msg153806 - (view) Author: Francis MB (francismb) * Date: 2012-02-20 21:11
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
msg153813 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-20 22:30
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.
msg153822 - (view) Author: Francis MB (francismb) * Date: 2012-02-21 00:04
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
msg153858 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-21 08:15
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 ;)
msg153903 - (view) Author: Francis MB (francismb) * Date: 2012-02-21 21:50
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!
msg153946 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-22 09:53
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
msg153948 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-22 10:03
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.
History
Date User Action Args
2012-02-22 10:03:27nadeem.vawdasetstatus: open -> closed
resolution: fixed
messages: + msg153948

stage: resolved
2012-02-22 09:53:47python-devsetnosy: + python-dev
messages: + msg153946
2012-02-21 21:50:15francismbsetfiles: + issue14053_2cceb4d3079a.patch

messages: + msg153903
2012-02-21 08:15:53nadeem.vawdasetmessages: + msg153858
2012-02-21 00:04:39francismbsetfiles: + issue14053_3297dcdad196.patch

messages: + msg153822
2012-02-20 23:48:12eric.araujosettitle: Make Tools/scripts/patchcheck.py compatible with mercurial mqueues. -> Make patchcheck work with MQ
2012-02-20 22:30:52nadeem.vawdasetmessages: + msg153813
2012-02-20 21:11:30francismbsetfiles: + issue14053_1f9461ef6312.patch

messages: + msg153806
2012-02-20 18:51:37nadeem.vawdasetmessages: + msg153796
2012-02-20 18:08:20francismbsetmessages: + msg153787
title: Make patchcheck compatible with MQ -> Make Tools/scripts/patchcheck.py compatible with mercurial mqueues.
2012-02-20 05:06:45ezio.melottisetmessages: + msg153763
2012-02-19 22:41:29eric.araujosetnosy: + brett.cannon

title: Make Tools/scripts/patchcheck.py compatible with mercurial mqueues. -> Make patchcheck compatible with MQ
2012-02-19 20:13:15nadeem.vawdasetmessages: + msg153723
2012-02-19 18:22:38francismbsetfiles: + issue14053_336a614f35a3.patch
keywords: + patch
messages: + msg153715
2012-02-19 16:18:47pitrousetnosy: + eric.araujo

versions: + Python 2.7, Python 3.2, Python 3.3
2012-02-19 16:17:18nadeem.vawdasetnosy: + nadeem.vawda
2012-02-19 14:24:30francismbcreate