GNOME Bugzilla – Bug 702757
Need format-patch feature
Last modified: 2013-09-02 12:54:27 UTC
Require a simple format-patch feature.
Created attachment 250185 [details] [review] Double click on any commit to create patch Basic working version. Please review so I can fix any other issues than pbor has suggested. Next iteration of patch to be submitted will see these improvements: Code and style improvements as suggested by pbor: 1. "...couple of minor coding style issues like the spaces around "+". 2. "...the filename should probably change a bit to have a max length". 3. "...maybe escape dangerous characters". Feature improvements: 1. "Get Patch" button in Diff View for every commit. 1. Pressing "Get Patch" button will give a "Save As" file dialog.
Review of attachment 250185 [details] [review]: Some comments inline... ::: gitg/history/gitg-history.vala @@ +158,3 @@ + private void double_click_to_create_patch(Gtk.TreePath path, Gtk.TreeViewColumn column) + { + Gitg.Commit commit_selected = d_commit_list_model.commit_from_path(path); use var selected_commit = ... @@ +160,3 @@ + Gitg.Commit commit_selected = d_commit_list_model.commit_from_path(path); + + string commit_subject = commit_selected.get_subject(); use replace directly here also no need to prepend commit for all the vars since we are already in that context
Created attachment 252467 [details] [review] Add Get Patch button to Diff View Please review, thank you.
Created attachment 253015 [details] [review] Add Get Patch button to Diff View Final version, all working. So far no bugs. *fingers crossed*.
Review of attachment 253015 [details] [review]: The patch looks fairly good to me, but still needs another iteration (see comments below) Also, I get "Get Patch" label of the button in white... not sure if it is a problem with the patch or if my current theme is buggy ::: libgitg/gitg-diff-view-request-resource.vala @@ -103,2 +103,4 @@ } } + + class DiffViewRequestPatch : DiffViewRequest all this class should go in its own file called gitg-diff-view-request-patch.vala @@ -105,0 +105,10 @@ + + class DiffViewRequestPatch : DiffViewRequest + { ... 7 more ... coding style (missing space and { on wrong line) @@ -105,0 +105,34 @@ + + class DiffViewRequestPatch : DiffViewRequest + { ... 31 more ... coding style: always leave a space after // @@ -105,0 +105,40 @@ + + class DiffViewRequestPatch : DiffViewRequest + { ... 37 more ... coding style @@ -105,0 +105,58 @@ + + class DiffViewRequestPatch : DiffViewRequest + { ... 55 more ... I think we need a more fancy escaping here, not just spaces, e.g. also tabs. Let's see if we find how git itself does it. Anyway let's factor out a subject_to_filename private method so that it is easier to find if we want to improve it incrementally. Also, coding style: space after the comma
Review of attachment 253015 [details] [review]: ::: libgitg/gitg-diff-view-request-resource.vala @@ -105,0 +105,46 @@ + + class DiffViewRequestPatch : DiffViewRequest + { ... 43 more ... Another thing, I think it is better if we create the whole string in memory and then just call http://www.valadoc.org/#!api=glib-2.0/GLib.FileUtils.set_contents instead of using FileOutputStream: this will take care of many detail for us, for instance it will handle overwriting an existing file
Created attachment 253841 [details] [review] Add Get Patch button to Diff View Final patch uploaded on git.gnome.org/gitg Pushed to master in 8bcc579289bd6770685e604ebe56f2072c5500d1 Available at: https://git.gnome.org/browse/gitg/commit/?id=8bcc579289bd6770685e604ebe56f2072c5500d1
Closing bug, thank you for the reviews.