Drush issue queue commands

I like.

We should probably have it use `git am` though when the patch is created using `git format-patch`.

The big difference is that the patch has the patcher's info it in so it shows them as the author, you as the committer like so:

http://drupal.org/commitlog/commit/2320/88c22bd06c91e7a151309834d2c7b4bb...

You don't really have to do everything in those instructions to make a patch like that though. All you need to do is make a commit after you do your changes and then use git format-patch. Patches that use git format-patch can then be applied by us with git am and the metadata stays in tact, as if we cherry-pick'd it from their branch.

My choice would be to only support patches made via this method.

I think we could solve the repetitive execution and the existing branch name problem by checking if the branch exists first, and deleting it if so. Seems pretty safe if we prefix it something like:

git checkout -b drushiq-[description]-[issue]

We could check for it with:

git show-ref --verify refs/heads/drushiq-[description]-[issue]

The `git remote show origin` is confusing because I think the patch instructions are a bit confusing. They work as long as you checked out your branch how it tells you to at the beginning, but say you do this:

git clone http://git.drupal.org/project/drush.git
cd drush
git checkout -b description_blah-1078108

Make changes…

git add -A
git commit  -m "blah blah commit"
git fetch origin
git rebase origin/7.x-4.x

That will fail because the branch was tracking master instead of 7.x-4.x, because I didn't use --branch 7.x-4.x in the original clone. This scenario seems likely to me since I'm probably not going to create a new clone for every patch I make. If we're creating or applying patches, we need to make sure we're doing it from local branches that are tracking the correct remote branches.

An easy way to avoid this is to specify the remote branch while creating the local one:

git checkout -b description_blah-1078108 origin/7.x-4.x

If you checkout the branch that way, `git remote show origin` will show you what remote branch the local branch is tracking and it should be safe to rebase before making a patch.

Tracking. This is awesome.

i really like this too. not sure if it belongs here or in drush_extras but i think it will be very popular. lets keep developing it here ... please keep asking questions as needed. i see that mark replied to the last one so i'm not sure how to help move this along right now.

subscribing

Its worth mentioning that there's been some talk in #drupal-gitsupport about the current format-patch/am workflow, and not everyone is crazy about it. Some are suggesting just using git diff for now until we have per-issue repos, so it may be worth holding off on this until things settle down a bit.

Awesome. Looking forward to looking at this!

Yeah, this looks really slick!

The way the test bot handles p0 vs. p1 is to just try applying at p1 first, and if that fails, try again at p0. Not sure if that's feasible here or not, but I wanted to mention it.

Also, it'd be nice to someday have an API to get issue metadata in json or something instead of having to scrape the HTML. See http://groups.drupal.org/node/126529 and http://3281d.com/projects/remote-issue-queues for more on this sort of approach.

Subscribing, and looking forward to use this.

+1

Greg, I think the issue with per issue branches is a sort of middle point between how Drupal.org has traditionally worked and github. On github, you fork projects, which creates a copy of the main repo associated with you, and then you do a pull request where the maintainer adds your repository as a remote, and then cherry-picks or merges your changes into the main repo/branch. The only difference with issue branches is that its per issue rather than per user. Those repos will still have a feature branch where the work will be done.

That said, I still haven't watched the video from drupalcon where this was explained better. I'm doing a bit of guessing on how the issue branches will work.

Discussion of per-issue repos and how they'll work is totally off topic for this thread. ;) It's partly explained via our DCC talk:
http://groups.drupal.org/node/133289
Don't know where the video is posted, but that at least has links to our slides.

The gory details are at these two posts:
http://groups.drupal.org/node/52953
http://groups.drupal.org/node/50438

But please, let's stop discussing those here, okay? ;)

Thanks,
-Derek

I tried this out and it works pretty well at first glance. The repetitive execution thing is a bit hairy but I bet we can find a solution. I'll thinker a bit.

I really do hope someone takes on the relatively easy issue of making a page which shows issue metadata (including most recent attachment) in JSON.

+++ b/commands/pm/iq.drush.inc
@@ -0,0 +1,627 @@
+      ¶

You have a lot of trailing whitespace, like here.

Powered by Dreditor.

Good questions. Starting from the top of the workflow ...

1. Lets assume that user starts from a clean branch (i.e no uncommitted code). In fact, iq-apply-patch should error out if this is not the case.
2. yes, i think automatically calling git add * makes sense
3. Don't think we have to handle: "If you make your own branch, "
4. Perhaps use Derek's suggestion from #13: "The way the test bot handles p0 vs. p1 is to just try applying at p1 first, and if that fails, try again at p0. "
5. typo: chunck

@kotnick: It is fine to mention whitespace as part of a substantial review. But it is rude to just mention trivial stuff like whitespace in a patch review. If you can't dedicate more time, or have nothing more to add, then please don't post the review.

subscribe

1. I'm not seeing any id in the issue page called 'attachments'. I tried with #694176: devel_create_greeking can exceed length limit for fields and #1113940: array warnings when generating content. This cripples apply-patch command. This used to work so I have no idea why the ID is seemingly vanished. Thoughts?
2. I will experiment with the tidy command to fix up invalid HTML. Will use it if present (ships with OSX at least).

+++ commands/pm/iq.drush.inc
@@ -0,0 +1,663 @@
+  $items['iq-reset'] = array(
+    'description' => 'Stop working on a patch, and return to the branch point.',
+    'arguments' => array(
+      'delete' => 'Also delete the working branch.',
+    ),

options, not arguments

Powered by Dreditor.

Most of my changes were just removing tabs and fixing indentation. I suggest applying the attached diff. Just ignore any hunks that conflict with yours since there is no substantial change here.

gave this a whirl. one more thought: we do git fetch origin && git rebase origin/master && git format-patch origin/master --stdout. I think that hard coding origin/master is not right. Admin might be reviewing a patch for another branch. We should specify the same branch we used for tracking when we created the current branch.

Also, would it be possible to make an option so that the command uses diff, not format-patch? I prefer to keep me email away from Google.

Side note: isn't git fetch origin && git rebase origin/master the long form of git pull --rebase origin?

drupal.org already solved the email privacy issue - http://drupal.org/node/1022156. we're only going to support the recommended workflow.

not sure if it applies here, but we occasionally use a longer git command in order to be compatible with older git versions.

http://1BeerToKillSubscribeComments.org

(my penance for that subscribe comment, sorry)

subscribing...

Some discussion about patch p0 and p1 at #745224: Apply patches from git diff and git format-patch

Yeah, I'd be happy to see a node/N/json view of all issue and project nodes that includes a JSON array of useful node data. For issues, that'd obviously include all the current metadata, title, etc, and it could also include a list of attachments from all comments, comment count, whatever. There are various discussions on g.d.o about this, but I don't think there's an issue on d.o about it yet.

@dww - i will happily write a project_issue JSON patch if you can review it and a mail.inc patch within 2 weeks.

@moshe: Sorry, I can't make any promises right now. My life is going through some tumultuous times so I've got very spotty availability (and little or no mental focus when I do have any time). Ugh. But, don't let that stop you. There's always mikey_p and hunmonk to review and commit stuff. ;)

Posted patches which enable a JSON menu callback for project_issue nodes. See #112805: JSON menu callback for project issues

looks nice but it indeed needs some work.
I did a few minor fixes, added a branch in my drush_helmo sandbox for this.
http://drupalcode.org/sandbox/helmo/1277350.git/commitdiff_plain/master-...
pretty printed: http://drupalcode.org/sandbox/helmo/1277350.git/commitdiff/master-upstre...

Too bad we don't have iq-upload-patch yet ;)

Why is the code in commands/sql/iq.drush.inc? commands/pm seems more logical.

+++ b/commands/sql/iq.drush.inc
@@ -0,0 +1,713 @@
+  return "#$issue_number$credits: $issue_title";

As per a similar issue in dreditor shouldn't this line include the prefix 'Issue '
#1061758: Make commit message button format something that doesn't start with #

Added a few commits:

* Check bootstrap phase before calling drush_get_extensions(), was also still calling drush_pm_get_extensions()
* Add missing dt() call in drush_log call
* Don't add getcwd() to $filename as it already is a full path
* Fixed placement of iq.drush.inc, back to pm folder

And a new patch for review.

[edit : see next comment]

[edit: removed noise]

fatal: git checkout: updating paths is incompatible with switching branches.
Did you intend to checkout 'origin/' which can not be resolved as commit?

This is a result of the "autosetuprebase = always" option we have in our gitconfig. We'll have to reevaluate this.

The autosetuprebase option breaks the output parsing in _drush_iq_get_branch_merges_with().

It might work to also match for "rebases onto remote" but I think the function needs more work,

[edit: removed noise]

[edit: removed noise]

Marking the following as duplicates:

• #392762: Kitten-o-matic
• #526696: Patching modules with drush
• Drush patch tools (See #1435960: Deprecate in favour of Drush issue queue commands.)

#112805: JSON menu callback for project issues has been committed and deployed. See this issue in JSON format.

Hope folks can restart work on these commands again.

This looks awesome so far.

Looking at the comment in _drush_iq_find_patches() about multiple patches per comment, often in the core queue there will be the patch to apply, the patch with only tests and no fixes to purposefully fail, and an interdiff.txt. There is no set naming convention for distinguishing the two.

Also, patches could be either .diff or .patch.

Will try this out soon. Meanwhile, a few quickies ...

1. I was expecting these commands to have their own section in the help listing. They are mingled with pm commands right now.
2. Use required arguments property as needed in command definition
3. Add Examples to command definitions. I see interesting stuff like 'comment-number' support.
4. Some commands should support --pipe or --format (iqi at least)
5. At some point, we need a Topic or long form help for each command. People will want to know the big picture before they trust drush to start managing their repo. I think laying out the flow if which commands to use when is whats needed.

With respect to "tests only" patches, there is a new standard that is recognized by the test bot:

PATCHNAME-do-not-test.patch
https://twitter.com/#!/randyfay/status/174364180730744834

Hm, that last post is about patches set explicitly to not be tested, not test only patches.
#1092232-21: Bot needs to handle patches named for all core versions -D[678]

In my experience, you want the bot to test both the test-only patch and the patch with both the new tests and the fix. That way, it's obvious that the test is catching a problem in core, and that the rest of the patch fixes the problem in core such that the new (and existing) tests all pass.

I'm not sure that the test bot stores data persistently in the DB about the status of test results for each patch. If so, then it should be relatively easy for pift.module to implement hook_project_issue_json_alter() just like comment_upload.module is doing. Although, I wonder if we're going to run into a weighting problem where pift.module needs to alter after comment_upload.module does so that it can alter the JSON that comment_upload injects. If pift.module doesn't store that data in the DB, I don't really know how this is going to work. Sadly, I know very little about the internal workings of pift.module. It's always been maintained by other folks. I highly suggest finding rfay, jthorston, boombatower, et al and getting their input on this.

Cheers,
-Derek

Right now, the commit messages being generated start with #, causing issues with rebasing.

Following http://drupal.org/node/52287, I switch it to start with 'Issue #'.

Also, I added a check for .diff as I said in #61.

Still cnw.

is anyone available to push this along? See feedback in #60 and #63

I would omit complications like 'module is not under version control' and 'site is already under version control'. Lots of people work on core and modules for their own fun outside of real site. Those folks will have scratch sites not under version control and git clones of modules from git.drupal.org

Tried to apply this patch and use it and it removed my entire git clone. The whole directory. Whoo!

~$ ll
total 0
lrwxrwxrwx 1 webadmin webadmin  8 2012-01-06 14:43 web -> /var/www
~$ drush dl --dev fullcalendar
Project fullcalendar (7.x-2.x-dev) downloaded to /home/webadmin/fullcalendar.                                                                                                                          [success]
Project fullcalendar contains 4 modules: fullcalendar_legend, fullcalendar_colors, fullcalendar_options, fullcalendar.
~$ ll
total 4
drwxr-xr-x 9 webadmin webadmin 4096 2012-08-07 16:45 fullcalendar
lrwxrwxrwx 1 webadmin webadmin    8 2012-01-06 14:43 web -> /var/www
~$ cd fullcalendar/
~/fullcalendar$ drush ap 1457868
Downloading patchfile fullcalendar-1457868-1.patch for project fullcalendar                                                                                                                            [ok]
Missing argument 2 for _drush_download_file(), called in /var/lib/drush/commands/pm/iq.drush.inc on line 264 and defined drush.inc:868                                                                 [warning]
Destination directory  is not writable.                                                                                                                                                                [error]
Switching to branch drush-iq-work-with-entitycache-1457868-4                                                                                                                                           [ok]
fatal: git checkout: updating paths is incompatible with switching branches.
Did you intend to checkout 'fullcalendar/origin/' which can not be resolved as commit?
fatal: git apply: failed to read: Is a directory
git apply failed; falling back to 'patch' tool                                                                                                                                                         [warning]
patch: **** read error : Is a directory
~/fullcalendar$ ll
total 0
~/fullcalendar$ cd ..
~$ ll
total 0
lrwxrwxrwx 1 webadmin webadmin  8 2012-01-06 14:43 web -> /var/www
~$ 

I tested only ap here.

~/Desktop/testdrush$ drush ap --no-git 1711316
Downloading patchfile field-171131-13.patch for project drupal                                                                                                                                         [ok]
chdir(): No such file or directory (errno 2) drush.inc:792                                                                                                                                             [warning]
patch: **** Can't change to directory -i : No such file or directory
patch: **** strip count -d is not a number

This didn't work at all, but didn't kill my repo.

~/Desktop/testdrush$ drush ap 1711316
Downloading patchfile field-171131-13.patch for project drupal                                                                                                                                         [ok]
chdir(): No such file or directory (errno 2) drush.inc:792                                                                                                                                             [warning]
chdir(): No such file or directory (errno 2) drush.inc:792                                                                                                                                             [warning]
chdir(): No such file or directory (errno 2) drush.inc:792                                                                                                                                             [warning]
chdir(): No such file or directory (errno 2) drush.inc:792  

This produced a whole bunch of warnings, but everything worked okay

Why not store the branch name before creating/switching?

git branch --no-color | sed -e '/^[^*]/d' -e 's/* \(.*\)/\1/' will do that for you.

All branches have an upstream. Could we not set the upstream to origin (i.e. to the remote branch?) when we branch? I have not looked at this in any detail so ignore me as needed.

No need to fuss with commits. I think this answer gives what the remote branch which is what we need here - http://stackoverflow.com/a/7251377

Here's a quick review. Will start to use it more and more. Great job!

# drush iq-info 1234
file_get_contents(): Filename cannot be empty iq.drush.inc:667                                                                                                                                          [warning]
Could not fetch issue data from http://drupal.org/node/1234  

Found a warning when the nid is not an issue

# drush iq-info  http://drupal.org/node/12342
 Title       :  Adding images to share gallery and personal galleries 
 ID          :  12342                                                 
 UR          :  http://drupal.org/node/12342                          
 Project     :  Image                                                 
 Project UR  :  http://drupal.org/project/image    

URL label is stripped

# drush iq-apply-patch http://drupal.org/node/357082 --select
Select a patch to apply:
 [0]   :  Cancel                                                                              
 [1]   :  #15     :  http://drupal.org/files/issues/357082.patch                              
 [2]   :  #18     :  http://drupal.org/files/issues/357082_0.patch                            
 [3]   :  #37     :  http://drupal.org/files/issues/357082-pull-filter_value-argument.patch   
 [4]   :  #39     :  http://drupal.org/files/issues/357082-pull-filter_value-argument_0.patch 
 [5]   :  #74     :  http://drupal.org/files/issues/357082-pull-filter_value-argument_1.patch 
 [6]   :  #78     :  http://drupal.org/files/357082-pull-filter_value-argument.patch          
 [7]   :  #95     :  http://drupal.org/files/357082-pull-filter_value-argument_0.patch        
 [8]   :  #102    :  http://drupal.org/files/357082-pull-filter_value-argument-1.patch        
 [9]   :  #104    :  http://drupal.org/files/views-357082-104.patch                           
 [10]  :  #126    :  http://drupal.org/files/views-357082-126.patch                           
 [11]  :  #132    :  http://drupal.org/files/views-357082-132.patch                           
 [12]  :  #149    :  http://drupal.org/files/views-357082-7.x-3.x-dev.patch                   
 [13]  :  #150    :  http://drupal.org/files/views-357082-7.x-3.3_0.patch                     

This is wrong. In that issue there's no patch in #149, two patches in #150, and one in #151 (I was trying to see what happen when a comment has two patches).

# drush iq-apply-patch http://drupal.org/node/1255708
Downloading patchfile quick-drupal-use-existing-9.patch for project drush   

Would be nice to see which comment iq-apply-patch is taking the patch from, in addition to the patch filename.

+++ b/docs/iq-commands.htmlundefined
@@ -0,0 +1,106 @@
+apply the most recent patch, then you can append the commit number to the
+end of the issue id; for example, to take the patch from the seventh commit

You mean "comment" instead of "commit" ?

+++ b/docs/iq-commands.htmlundefined
@@ -0,0 +1,106 @@
+paste it into a terminal window to use wiith iq-apply-patch, all without

typo: wiith

+++ b/docs/iq-commands.htmlundefined
@@ -0,0 +1,106 @@
+needing to re-open the issue to find the most recent patch.

Perhaps re-visiting instead of re-open ?

+++ b/commands/iq/iq.drush.incundefined
@@ -0,0 +1,949 @@
+    'topics' => array('docs-iq-commands'),

This line is missing in some command definitions.

+++ b/commands/iq/iq.drush.incundefined
@@ -0,0 +1,949 @@
+    'description' => 'Create a diff.  Uses `git format patch`, so author information is included.',

"git format-patch"

+++ b/commands/iq/iq.drush.incundefined
@@ -0,0 +1,949 @@
+      'message' => 'Commit message.  Optional; default is to use issue title.',

What issue title? I see the command callback can receive a optional number. It is missing in the command arguments.

+++ b/commands/core/docs.drush.incundefined
@@ -191,6 +191,14 @@ function docs_drush_command() {
+  $items['docs-iq-commands'] = array(
+    'description' => 'Issue queue commands for applying and creating patches from drupal.org.',
+    'hidden' => TRUE,
+    'topic' => TRUE,
+    'bootstrap' => DRUSH_BOOTSTRAP_DRUSH,
+    'callback' => 'drush_print_file',
+    'callback arguments' => array($docs_dir . '/docs/iq-commands.html'),

Since we decided to move iq to Contrib, this should move to iq.drush.inc. The change to command.inc should be committed to drush core.

+++ b/commands/iq/iq.drush.incundefined
@@ -0,0 +1,1005 @@
+    ),
+    'aliases' => array('patch', 'ap'),

I think an alias of 'am' makes sense. Matches git.

1. "'Create a diff between the current branch and the branch it split from.'," might more accurately be "'Create a diff between the current branch and its upstream branch.',
2. "'commit' => 'Commit change to branch. Optional; default is to leave changes unstaged.',". I'm not clear if we mean current branch or that we create a branch. If we mean current branch, we might want to just omit 'to branch' or change to 'to current patch review branch'.
3. "'description' => 'Stop working on a patch, and return to the branch point.',". Lets say 'original branch' here and in descriptions below it.
4. "+ * the issue queue html to elements in the". I think we mean XML, not HTML. Happens again so grep for 'html' and change as needed.
5. Recent versions of dreditor now helpfully build a git commit with --author on it. Presumably it gets that from the patch uploader. Our commits should also specify --author when possible.
6. Ideally we have more complete doxygen on functions like drush_iq_download_info
7. I seem to recall there is a function in the pm engine has does what _drush_iq_project_dir() does. More or less.
8. See develDrushTest.php in devel for instructions on how to do drush unit testing in a contrib module.

Note: I deployed the fix for #1730728: Project issue json needs comment index number included in data structure to the live site...

Re --author: this is what dreditor uses:

https://drupal.org/user/46549/git-attribution
https://drupal.org/user/438598/git-attribution
...

Cheers,
-Derek

Brilliant! Great work, greg.1.anderson!
I am currently working on Patch Manager module: I'm looking for an appropriate solution for patch applying (#1151002: A simpler way to patch). Patch Manager uses the /usr/bin/patch tool - but it is not acceptable for Windows-based users. Your project - what we need.
How can I specify the dependency of your project? (It seems to be impossible - or not?)
Would you mind if we merge the projects?

Patch Manager will give to Drush Issue Queue features as follows:
- All patches are stored in the system as nodes - none is lost.
- Administrative interface of all patches management - everything is under control.

What do you think about it, Greg?

@Staratel: I'm not sure you understand how drush works. It's not a drupal module, and you can run all of these drush commands inside directories that don't actually have a Drupal site installed at all. So you can't have a drupal module depend on a drush command in the same way you can have modules depend on each other. And there's no "system" where patches could be stored as nodes -- there might not be a database setup at all when running these drush commands.

Furthermore, the whole architecture of the patch_manager module is a bit scary from a security standpoint. To apply/revert patches, your webserver would need write access to all your Drupal files, which is generally a big no-no. I just posted #1733958: Document that this module depends on a less-secure website configuration to work about that.

So, I don't think merging projects makes any sense at all. There's possibly some useful code in drush you could harvest for patch_manager, but ultimately, they're fundamentally different architectures for running code. Generally, sites would be better off managing patches with drush (running as the user that owns all the source code files via a shell on their webserver) not via the Drupal admin UI itself. At least in terms of the security of their site configuration. In some cases people don't have direct shell access, and then a tool like patch_manager might make sense. But then a dependency on drush would entirely defeat the purpose.

Hope that helps clarify,
-Derek

Thanks for your response, Derek.

Furthermore, the whole architecture of the patch_manager module is a bit scary from a security standpoint. To apply/revert patches, your webserver would need write access to all your Drupal files, which is generally a big no-no.

Exactly, that is why I'm looking for a way of applying patches in secure manner. I need a clean and OS-independent way of applying and managing patches. Drush Issue Queue is a great project for patch applying and to work with issue queue - but it has no options to manage patches that are already applied. When I say 'are stored in the system' - it means that Drupal installation is used to store patches that were applied to it. I understand that Drush could be used without any Drupal installation - but it could be used with it too. So I am going to implement patching using Drush Issue Queue in Patch Manager module (read below for more information).

Generally, sites would be better off managing patches with drush (running as the user that owns all the source code files via a shell on their webserver) not via the Drupal admin UI itself... In some cases people don't have direct shell access, and then a tool like patch_manager might make sense. But then a dependency on drush would entirely defeat the purpose.

Maybe I am missing something, but wouldn't it better to have a Drupal module that has a setting proposing user to choose which way to apply patch : the first one - using /usr/bin/patch is accessible only for UNIX-users and has warning that it needs insecure server configuration; and the second - using Drush (in fact, Drush with Drush Issue Queue) which has notification that it needs to be installed and a link to appropriate how-to. I think it is good approach to move users from insecure patching to Drush using, isn't it?
To go this way I need one of two things to be done:
1) Either Drush Issue Queue becomes a part of Patch Manager.
2) Or it is included in official Drush release - so it is not necessary to patch it itself before using.
Is my effort clear?

Again, moving this drush command inside patch_manager totally defeats the purpose of having it as a stand-alone drush command that can be used outside of a working Drupal site. So that's not a viable option at all.

Maybe I'm not understanding your proposal, but I don't see how you can have a setting inside patch_manager that will get the user to install and use drush. If I set this to "use drush", I still have to actually log in via the shell (if possible), install drush, and invoke the appropriate commands (which would be outside the control of patch_manager). So, I don't really see what this setting does. However, this is really off topic for this thread. Looks like that's what #1151002: A simpler way to patch is about, so we should talk there, not here.

However, once the dust settles and this set of drush commands here actually works, this is going to be committed as a stand-alone drush extension. It doesn't have to be part of drush core itself. As soon as there's an official release of drush_iq, installing it will be as simple as:

1. Install drush
2. Run drush dl drush_iq

That's it.

Cheers,
-Derek

@Greg - You can give author credit just with uid. Thats what I successfully tried at http://drupalcode.org/project/drush.git/commit/bc1dccf

git push origin :7.x-1.x-dev

I think as long as there are no release nodes pointing to a branch/tag, you can delete it in the normal Git way.

WOW! AWESOME! Can't wait to try this out. :D