644917 - Imported patch names are incorrect

Status: RESOLVED FIXED

(Developer Services :: Mercurial: qimportbz, defect)

Description

[Steve Fink [:sfink] [:s:]]

% hg qimport bz://638178
Fetching... done
Parsing... done
1: Mochitest for getExecutableLines
timeless: review+
2: Fetch batch of executable lines at a time from jsdIScript
Which patches do you want to import? [Default is '1'] 2
adding 638178 to series file
abort: patch . not in series
% hg series -v
0 U 638178

So it imports the patch successfully, but fails to rename it. Importing with the full URL to an attachment works. The problem is that it ends up calling normpath(basename("bz://638178/")), which is normpath(""), which is ".". Notice that I passed in a bz url without a trailing slash, but it normalized it.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix filenames for bz://NNNNN imports

Comment 2

[Steve Fink [:sfink] [:s:]]

Just pushed with r=robarnold via email.

http://hg.mozilla.org/users/robarnold_cmu.edu/qimportbz/rev/12e11daf441e

Comment 3

[timeless]

this seems to have broken things for me. I'm going to file some bugs with my other patches and then provide a counter patch for this.

Comment 4

[timeless]

Attachment: changes which work for me

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Another way to skin the cat

Oh, sorry. You're right; my "fix" ended up breaking one way and fixing the other. I completely forgot to send out the newer fix I've been running with.

There are 3 cases to handle: bz://12345/98765 vs bz://12345 vs auto_choose_all. (with multiple patches.) Your patch looks like it'd work for the 1st two, and maybe for the 3rd, but what do you think of this approach? (We really ought to be fixing mq itself to allow things like this better, though...)

Comment 6

[timeless]

Comment on attachment 525425 [details] [diff] [review]
Another way to skin the cat

       for (patch, path) in list(bzhandler.imported_patches):

afaict (from having done similar junk and being lectured for it) this will always be true:
+        if path is None:
+          # Find the initial patch where qimport would have inserted it
+          oldpatchname = q.full_series[insert]

so you should just drop this:
+        else:
+          # Mimic qimport's procedure for constructing patch names
+          oldpatchname = util.normpath(os.path.basename(path))

because the only place that is added to the list is here:
     if patch:
-      imported_patches.append((patch, req.get_full_url()))
+      imported_patches.append((patch, None))

Thus, what you should do is change to:
       for patch in list(bzhandler.imported_patches):
and
       imported_patches.append(patch)

but otherwise, yeah, i like this approach.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Use insertion position of imported patch

(In reply to comment #6)
> Comment on attachment 525425 [details] [diff] [review]
> because the only place that is added to the list is here:
>      if patch:
> -      imported_patches.append((patch, req.get_full_url()))
> +      imported_patches.append((patch, None))
> 
> Thus, what you should do is change to:
>        for patch in list(bzhandler.imported_patches):
> and
>        imported_patches.append(patch)
> 
> but otherwise, yeah, i like this approach.

Oh! That's deceptive. Though I should have caught it. So the fact that imported_patches is a list is deceptive, since there can be only one.

That simplifies the patch. I'll continue pretending like there can be multiple imported_patches for now, since otherwise the code reads funny (a for loop with the same oldpatchname on every iteration.)

Comment 8

[Steve Fink [:sfink] [:s:]]

Ah. Some of the weirdness is from my limited Python understanding. I see now that you used a list instead of a single value because python's module importing copies the value. I made a patch to make it be a single value, using an accessor function for the global variable, but it doesn't really end up any cleaner afaict. And my previous patch will work if you somehow change things to produce multiple imported patches, so I'll stick with that.

Rob, would prefer if we just cross-reviewed our patches (as in, remove you from the review loop)? I know you previously told me that you weren't maintaining this very much anymore, so I just wanted to check.

Comment 9

[timeless]

well, i'm pretty sure you can have multiple imported patches, e.g. for bug 469555 i can import 5 at once.

right now it's a list of tuples a patch object and some patch-name-like-thing.

wrt:
-          if ui.verbose:
-            ui.write("Renamed %s -> %s\n" % (oldpatchname, newpatchname))
+          ui.write("renamed %s -> %s\n" % (oldpatchname, newpatchname))

it actually makes sense to use 'not ui.verbose', because in the verbose case mq will announce the rename itself.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to comment #9)
> well, i'm pretty sure you can have multiple imported patches, e.g. for bug
> 469555 i can import 5 at once.

That's the confusing part -- those are different. They go into the delayed_imports array and are handled separately.

What's going on is that qimportbz hooks itself in using a URL handler. But a URL handler can only do a single patch. So when there are multiple patches to be imported, bzhandler returns the first one as the normal response to URL resolution request, and stores the remaining patches on the side in delayed_imports. But bzhandler has no way to set the name of the single imported patch, so it has to do the renaming dance. For the delayed_imports patches, it has the freedom to do whatever it wants, so it imports them with the correct names in the first place.

This is mostly documented in the comment

    # Do the import as normal. The first patch of any bug is actually imported
    # and the rest are stored in the global delayed_imports. The imported
    # patches have dumb filenames because there's no way to tell mq to pick the
    # patch name *after* download.

The "proper" fix would probably be to modify base mq to allow URL handlers to participate in naming. Even better would be to allow handlers to produce multiple results; then this whole extension could reduce to a set of handlers.

> right now it's a list of tuples a patch object and some patch-name-like-thing.

Yes, but the patch-name-like-thing currently doesn't contain any useful information. It was intended to have just what is necessary to reconstruct the dummy name used to import the single "official" patch, but with my change that is no longer needed.

And it's a list because Python copies imported module globals instead of referencing them, so it's serving as a container to hold an updatable data member.

> wrt:
> -          if ui.verbose:
> -            ui.write("Renamed %s -> %s\n" % (oldpatchname, newpatchname))
> +          ui.write("renamed %s -> %s\n" % (oldpatchname, newpatchname))
> 
> it actually makes sense to use 'not ui.verbose', because in the verbose case mq
> will announce the rename itself.

Heh. That's funny. I'll do that.

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: Use insertion position of imported patch

added some comments too

Comment 12

[Rob Arnold [:robarnold]]

(In reply to comment #10)
> (In reply to comment #9)
> > well, i'm pretty sure you can have multiple imported patches, e.g. for bug
> > 469555 i can import 5 at once.
> 
> That's the confusing part -- those are different. They go into the
> delayed_imports array and are handled separately.
> 
> What's going on is that qimportbz hooks itself in using a URL handler. But a
> URL handler can only do a single patch. So when there are multiple patches to
> be imported, bzhandler returns the first one as the normal response to URL
> resolution request, and stores the remaining patches on the side in
> delayed_imports. But bzhandler has no way to set the name of the single
> imported patch, so it has to do the renaming dance. For the delayed_imports
> patches, it has the freedom to do whatever it wants, so it imports them with
> the correct names in the first place.
> 
> This is mostly documented in the comment
> 
>     # Do the import as normal. The first patch of any bug is actually imported
>     # and the rest are stored in the global delayed_imports. The imported
>     # patches have dumb filenames because there's no way to tell mq to pick the
>     # patch name *after* download.
> 
> The "proper" fix would probably be to modify base mq to allow URL handlers to
> participate in naming. Even better would be to allow handlers to produce
> multiple results; then this whole extension could reduce to a set of handlers.

I discussed this with djc a while back (pre hg 1.5) and he thought it was a good idea. I think it's the right way to approach this (or else we just write to series file directly and bypass the opener logic).

Comment 13

[Steve Fink [:sfink] [:s:]]

Review ping? (Just noticed this on my review list, though I'm not sure why it showed up on *my* list.)

Comment 14

[Rick Reitmaier]

For what its worth; either of the 2 pending patches fixes the issue for me.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 526023 [details] [diff] [review]
Use insertion position of imported patch

This patch fixes the issue for me; unlike Rick in comment 14, I have not tried using the other pending patch.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 526023 [details] [diff] [review]
Use insertion position of imported patch

Review of attachment 526023 [details] [diff] [review]:
-----------------------------------------------------------------

::: __init__.py
@@ +155,5 @@
>      files = map(fixuppath, files)
>  
> +    # Remember where the next patch will be inserted into the series
> +    insert = q.full_series_end()
> +

With Mercurial version 1.9.2+20110831 and Python version 2.7.1, this invocation signals:
AttributeError: 'queue' object has no attribute 'full_series_end'

(So maybe I was premature in claiming in comment 15 that this patch works.)

Comment 17

[Steve Fink [:sfink] [:s:]]

Attachment: Use insertion position of imported patch

(In reply to Felix S Klock II from comment #16)
> With Mercurial version 1.9.2+20110831 and Python version 2.7.1, this
> invocation signals:
> AttributeError: 'queue' object has no attribute 'full_series_end'
> 
> (So maybe I was premature in claiming in comment 15 that this patch works.)

Ugh. full_series_end is gone, replaced by... fullseriesend. Is that progress?

Updated patch.

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: Use insertion position of imported patch

One more renaming. I probably ought to monkeypatch old versions to the new uglier API to eliminate the try/catch garbage.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: imported bz://NNNN patch names are wrong,

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 574344 [details] [diff] [review]
imported bz://NNNN patch names are wrong,

Uh, oops?

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 569175 [details] [diff] [review]
Use insertion position of imported patch

Tag-team r+ via IRC: robarnold says jdm should feel free to take over qimportbz and push directly to the repo. jdm says the patch works for him.

Comment 22

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/users/robarnold_cmu.edu/qimportbz/rev/b86343ae2917