GNOME Bugzilla – Bug 657558
git bz apply improvements
Last modified: 2012-03-01 20:43:55 UTC
I've been working with this for a while now and it seems to work well.
Created attachment 194962 [details] [review] apply: pass "-3" to git-am Using 3-way merge makes it much more likely that the patch will apply correctly when the local tree is newer than the tree that the patch was generated from, and if the patch doesn't apply cleanly, git will leave a file with conflict markers rather than forcing you to reapply the patch manually.
Created attachment 194963 [details] [review] apply: add --interactive option Add a -i/--interactive flag to apply (which works similarly to interactive rebasing) and use that rather than prompting whether or not to apply each patch. In addition to choosing which patches to apply, this also allows reordering the patches before applying, and choosing to apply committed or rejected patches.
Created attachment 194964 [details] [review] apply: add --continue/--skip/--abort Rather than failing completely when git-am fails to apply a patch, write out our current state to git-am's temporary directory, and then tell the user to use "git bz apply --continue/--skip/--abort" after handling the merge failure. When the user uses one of those options, read back our state, pass the flag on to git-am, and (assuming git-am succeeds), continue with the next patch.
Created attachment 194965 [details] [review] apply: attempt to handle plain diffs If a bug has a plain diff (as opposed to git-format-patch output), prepend some minimal headers to it to get it into a format git-am will accept. But output a warning so the user knows the commit message will need to be edited (and use an author email address format that git.gnome.org at least will reject).
*** Bug 663653 has been marked as a duplicate of this bug. ***
Review of attachment 194965 [details] [review]: Definitely better than bailing out ::: git-bz @@ +1516,2 @@ for patch in patches: + if patch.data[:5] != "From " and patch.data.find("\nFrom ") == -1: The different handling of initial and subsequent matches made this unclear to me, would find: re.search(r'^From ', patch.data, re.MULTILINE) is not None more readable. (Or re.search(r'(^|\n)From ', patch.data) is not None to avoid the reader having to know what re.MULTILINE does) @@ +1518,3 @@ + # Plain diff... rewrite it into something git-am will accept + headers = """From xxx +From: git-bz <%s@(none)> isn't the attacher typically an email address (not sure if bugzilla user ids are always emails, but typically are)? It seemed to be when I tested for one patch. joe@example.com@(none) is a weird default. You can also get more information like: users = bug.server.get_xmlrpc_proxy().User.get({ 'names': [attacher] })['users'] name = users[0]['real_name'] email = users[0]['email'] (You probably don't want to do that upfront for every attachment, rather lazily when you need it)
Review of attachment 194962 [details] [review]: At some point I had some plans for trying both -3 and --reject, since --reject gives better results when -3 just gives up because the ID's aren't known - it leaves .reject files that you can work from. But then again, cleaning up .reject files is no fun, and rather confusing. Better to have -3 than just the default. I'll land this with the rest of the series.
Review of attachment 194963 [details] [review]: I like the idea and the code. The one thing I'm not so sure about is making the behavior when nothing is presented to just go ahead and try to apply everything blindly. It's not too uncommmon for me to paste the wrong bug url into a git bz apply - sometimes even from an entirely wrong module. And 'git bz apply --abort' is little tedious to type in that case. What if the default behavior (maybe the only behavior?) was to show the attachments that it would apply, followed by a; Apply? [(y)es, (n)o, (i)nteractive] prompt. I agree that the one-by-one isn't useful, but I think the double-check for sanity is.
Review of attachment 194964 [details] [review]: Just some tiny things, otherwise looks plausible (I don't say "right" because I haven't dug through the details of git am to confirm what you are doing, but I assume you've tested :-) ::: git-bz @@ +1431,3 @@ + +def do_apply(*args): + git_dir = git.rev_parse("--git-dir") more "idiomatic" according to conventions here as git.rev_parse(git_dir=True) @@ +1452,3 @@ + f.close() + except: + die("Not inside a git bz apply operation") I'd put quotes round 'git bz apply' here @@ +1455,3 @@ + + try: + process = git.am(arg, "--resolvemsg", resolvemsg, _interactive=True) resolvemsg=resolvemsg
(In reply to comment #6) > joe@example.com@(none) > > is a weird default. as mentioned in the commit message (but not the code I guess), it's designed to force git.gnome.org to reject the commit, so you don't accidentally push it without filling in a commit message body first. (And also because in the current version it didn't have the author name, but you pointed out how to fix that.) I thought about maybe having git-bz-apply force the user to write a commit message when applying the patch, but that would be annoying in the case where you're just testing out the patch and know you won't be committing it. (In particular, if you're planning to tell the submitter to resubmit it in git-format-patch format after you test it out anyway.) Although I guess you could just use a dummy commit message in that case and that wouldn't take too much time... so maybe that is the right solution. (In reply to comment #8) > What if the default behavior (maybe the only behavior?) was to show the > attachments that it would apply, followed by a; > > Apply? [(y)es, (n)o, (i)nteractive] > > prompt. I agree that the one-by-one isn't useful, but I think the double-check > for sanity is. Well, if you want to sanity check, you can just pass -i; if the patch list looks good, then just quit the editor without changing anything, and if not, either edit it or delete everything before quitting. (I've found that I do that a lot. But it's also nice to have the apply-right-away behavior in the case where, eg, you've just been looking at the bug in bugzilla and just copied the URL out of the browser and so you know exactly what you're going to get.) If you do want a more sanity-check-y default though, it could still just be to default to the current interactive mode, rather than having a separate prompt...
(In reply to comment #9) > + process = git.am(arg, "--resolvemsg", resolvemsg, > _interactive=True) > > resolvemsg=resolvemsg ok, but what about process = git.am("-3", "--resolvemsg", resolvemsg, filename, _interactive=True) ?
(In reply to comment #11) > (In reply to comment #9) > > + process = git.am(arg, "--resolvemsg", resolvemsg, > > _interactive=True) > > > > resolvemsg=resolvemsg > > ok, but what about > > process = git.am("-3", "--resolvemsg", resolvemsg, filename, > _interactive=True) > > ? "-3" has to stay as -3, since 3=True isn't valid Python. :-) If there aren't command line ordering problems, then: git.am("-3", filename, resolvemsg=resolvemsg, _interactive=True) is how I'd write it. - Owen
(In reply to comment #10) > (In reply to comment #6) > > joe@example.com@(none) > > > > is a weird default. > > as mentioned in the commit message (but not the code I guess), it's designed to > force git.gnome.org to reject the commit, so you don't accidentally push it > without filling in a commit message body first. (And also because in the > current version it didn't have the author name, but you pointed out how to fix > that.) I should read more carefully... I can change the hooks on git.gnome.org to reject something else in addition, maybe: FIXME: autogenerated-commit message (or just FIXME: ? rejecting FIXME without a colon is too strict, looking at GTK+ for a big sample size but followed by a colon it doesn't seem to occur organically in commit messages) > I thought about maybe having git-bz-apply force the user to write a commit > message when applying the patch, but that would be annoying in the case where > you're just testing out the patch and know you won't be committing it. (In > particular, if you're planning to tell the submitter to resubmit it in > git-format-patch format after you test it out anyway.) Although I guess you > could just use a dummy commit message in that case and that wouldn't take too > much time... so maybe that is the right solution. Bringing up the editor with the autogenerated commit information might be useful, though you do usually want to test before spending a lot of time on a commit message. Scraping the attachment comment out of the long_descs on the bug could also be useful, as a basis for the commit message, but in the end, we really want to encourage people _not_ to submit raw patches, so that is likely too much work. > (In reply to comment #8) > > What if the default behavior (maybe the only behavior?) was to show the > > attachments that it would apply, followed by a; > > > > Apply? [(y)es, (n)o, (i)nteractive] > > > > prompt. I agree that the one-by-one isn't useful, but I think the double-check > > for sanity is. > > Well, if you want to sanity check, you can just pass -i; if the patch list > looks good, then just quit the editor without changing anything, and if not, > either edit it or delete everything before quitting. (I've found that I do that > a lot. But it's also nice to have the apply-right-away behavior in the case > where, eg, you've just been looking at the bug in bugzilla and just copied the > URL out of the browser and so you know exactly what you're going to get.) git bz is always walking a line between behaving like git and behaving in a forgiving manner. Do we really want to replicate the process that a new user has to know 'git am -3' and 'git pull --rebase' not 'git am' and 'git pull' (and so forth). > If you do want a more sanity-check-y default though, it could still just be to > default to the current interactive mode, rather than having a separate > prompt... I thought about that, but "delete everything to abort" is not the easiest form of cancelling. I might have to try a prompting approach to see how it worked, but my feeling is that is a balance between speed and leaving the user in an awkward situation too frequently.
Created attachment 207292 [details] [review] plain "git diff" patch against glib master, for testing
Created attachment 208270 [details] [review] apply: pass "-3" to git-am Using 3-way merge makes it much more likely that the patch will apply correctly when the local tree is newer than the tree that the patch was generated from, and if the patch doesn't apply cleanly, git will leave a file with conflict markers rather than forcing you to reapply the patch manually.
Created attachment 208271 [details] [review] apply: add interactive option Rather than asking whether or not to apply each patch individually, list all of the patches at once and allow the user to either apply all of them, apply none of them, or interactively choose which ones to apply (a la interactive rebasing). In addition to choosing which patches to apply, the interactive option also allows reordering the patches before applying, and choosing to apply committed or rejected patches.
Created attachment 208272 [details] [review] apply: add --continue/--skip/--abort Rather than failing completely when git-am fails to apply a patch, write out our current state to git-am's temporary directory, and then tell the user to use "git bz apply --continue/--skip/--abort" after handling the merge failure. When the user uses one of those options, read back our state, pass the flag on to git-am, and (assuming git-am succeeds), continue with the next patch.
Created attachment 208273 [details] [review] apply: attempt to handle plain diffs If a bug has a plain diff (as opposed to git-format-patch output), prepend some minimal headers to it to get it into a format git-am will accept, and then make the user write a commit message for it before proceeding.
(In reply to comment #8) > What if the default behavior (maybe the only behavior?) was to show the > attachments that it would apply, followed by a; > > Apply? [(y)es, (n)o, (i)nteractive] done. (It's the only behavior. --interactive is gone now.) (In reply to comment #10) > I thought about maybe having git-bz-apply force the user to write a commit > message when applying the patch... Although I guess you > could just use a dummy commit message in that case and that wouldn't take too > much time... so maybe that is the right solution. also done also, updated the documentation, and reworked it to only add-url at the end rather than doing it for each patch in series
Review of attachment 208271 [details] [review]: Just one character here I don't like, I can just fix it up before pushing. ::: git-bz @@ +1553,3 @@ for patch in bug.patches: if patch.status == 'committed' or patch.status == 'rejected': + print "%d (skipping, %s) - %s" % (patch.attach_id, patch.status, patch.description) Not completely sure that the attachment database ID is relevant information to the user, but sure, doesn't hurt. @@ +1570,3 @@ + patches_by_id[patch.attach_id] = patch + if patch.status == 'committed' or patch.status == 'rejected': + template.write("# %d - %s (%s)\n" % (patch.attach_id, patch.description, patch.status)) Since presumably the point of this is to allow the user to uncomment, this shouldn't have a space before the %d - since we require the number to be the first thing on the line, having the leading space invites the user to screw this up - compare edit_bug() (The other thing that could be done is to permit leading spaces - edit_bug() is a little strict in matching because I wanted to avoid lines in the bug comment being mistaken for attachments, but there's no point here. Still, I think if we're inviting the user to uncomment, we should avoid the possibility of them thinking they need to delete the space.)
Review of attachment 208272 [details] [review]: Needs the [verse] hackaround I did for 'git bz edit and line breaks, otherwise, seems fine.
Review of attachment 208273 [details] [review]: Looks plausible
Attachment 208270 [details] pushed as 483d5e2 - apply: pass "-3" to git-am Attachment 208271 [details] pushed as e422eea - apply: add interactive option Attachment 208272 [details] pushed as ce11237 - apply: add --continue/--skip/--abort Attachment 208273 [details] pushed as f69a952 - apply: attempt to handle plain diffs
Created attachment 208814 [details] [review] plain diff against mutter head Patch to test commit hook