980986 - qimportbz incorrectly prompts to overwrite patch if filename on bug was equal to the bug number

Status: RESOLVED FIXED

(Developer Services :: Mercurial: qimportbz, defect)

Description

[Ed Morley [:emorley]]

1) hg qdel 973703
2) hg qimportbz bz://973703
Fetching... done
Parsing... done
adding 973703 to series file
A patch file named '973703' already exists in your patch directory. Rename patch 'add Ro and use consistent punctuation' (8387672) (r)/overwrite (o)? o
applying 973703
now at: 973703

-> The file we're being prompted to overwrite did not exist prior; qimportbz just gets in a muddle because the filename on the bug matches the bug number.

Comment 1

[Ed Morley [:emorley]]

Attachment: MozReview Request: bz://980986/edmorley

/r/5311 - qimportbz: Remove support for Mercurial older than v1.9
/r/5313 - qimportbz: Remove support for Python older than v2.6
/r/5315 - qimportbz: pyflakes, pep8, comment & whitespace cleanup
/r/5317 - qimportbz: avoid filename collisions with the temporary patch file (bug 980986)
/r/5319 - qimportbz: don't rename patches to 'y' if no filename entered (bug 980986)
/r/5321 - qimportbz: cleanup the "rename patch" prompt message (bug 980986)

Pull down these commits:

hg pull review -r 9af7b13815ba6893f1f755a465b5362bd94a6d03

Comment 2

[Ed Morley [:emorley]]

https://reviewboard.mozilla.org/r/5317/#review4295

::: hgext/qimportbz/__init__.py
(Diff revision 1)
> +                name = "%s_" % name

I'm happy to change the suffix string, or switch to a prefix if you think more appropriate. I just thought a suffix wouldn't screw with the sort order, if there are multiple patches and for some reason the patch author gave the first one a filename that matched the bug number, and then bug number + a suffix for the later ones.

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5311/#review4375

Good riddance.

Comment 5

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5313/#review4377

Ship It!

Comment 6

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5315/#review4379

Ship It!

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5317/#review4381

Ship It!

::: hgext/qimportbz/__init__.py
(Diff revision 1)
> +                ui.write("Changing patch filename to avoid conflict with temporary file.\n")
> +                name = "%s_" % name

Suffix works here.

Use ui.status() for informative messages. It is also customary to surround status messages with () to diminish their importance.

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5319/#review4383

Ship It!

Comment 9

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5321/#review4385

Ship It!

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8577204 [details]
MozReview Request: bz://980986/edmorley

https://reviewboard.mozilla.org/r/5309/#review4387

Comment 11

[Ed Morley [:emorley]]

https://reviewboard.mozilla.org/r/5317/#review4601

> Suffix works here.
> 
> Use ui.status() for informative messages. It is also customary to surround status messages with () to diminish their importance.

Agree about ui.status(), re the brackets, I can't see a precedance for this either in mq or even in Mercurial core - so I think I'll leave without for now (if only for consistency with the rest of qimportbz).

Comment 12

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #7)
> Use ui.status() for informative messages.

qimportbz now works as expected when using -q:

[~/src/vctools/hgext/bzexport]$ hg qimport bz://973703
Fetching... done
Parsing... done
adding 973703 to series file
Changing patch filename to avoid conflict with temporary file.
renamed 973703 -> 973703_
[~/src/vctools/hgext/bzexport]$ hg qdel 973703_
[~/src/vctools/hgext/bzexport]$ hg qimport -q bz://973703
adding 973703 to series file
[~/src/vctools/hgext/bzexport]$

The "adding 973703 to series file" comes from mq - seems like it should not be using ui.warn() here:
http://selenic.com/hg/file/b7f936f47f2b/hgext/mq.py#l2153

Would you agree? (Can't face jumping through the ridiculous newsgroup patch posting hoops if you think they won't accept it)

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1212e433a461
https://hg.mozilla.org/hgcustom/version-control-tools/rev/39a2e8248e8a
https://hg.mozilla.org/hgcustom/version-control-tools/rev/98eeed7a40ad
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5d7f5b1eb727
https://hg.mozilla.org/hgcustom/version-control-tools/rev/8043dea8a1af
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5c3aa12de69b
https://hg.mozilla.org/hgcustom/version-control-tools/rev/513b0c301a7c

Comment 14

[Gregory Szorc [:gps]]

I'm not sure if upstream will accept that patch. mq is mostly unloved. Come to think of it, I wouldn't be surprised if they accept it with a response like "yeah, sure."

Comment 16

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: Remove support for Python older than v2.6

Comment 17

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: pyflakes, pep8, comment & whitespace cleanup

Comment 18

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: avoid filename collisions with the temporary patch file (bug 980986)

Comment 19

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: don't rename patches to 'y' if no filename entered (bug 980986)

Comment 20

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: cleanup the "rename patch" prompt message (bug 980986)

Comment 21

[Ed Morley [:emorley]]

Attachment: MozReview Request: qimportbz: Remove support for Mercurial older than v1.9