GNOME Bugzilla – Bug 707063
ostree: Support for using EDITOR to fill commit subject/body
Last modified: 2013-08-29 19:11:04 UTC
I know ostree is mainly used in automated scripts and stuff, but when working manually with it it's nice to be able to bring up an EDITOR when making a commit.
Created attachment 253519 [details] [review] ostree: Support for using EDITOR to fill commit subject/body Behave similar to git when 'ostree commit' is run without a --subject or --body. Bring up an editor. The first line becomes the subject and following lines become the --body after an optional blank line. Use similar logic to git in determining EDITOR
Review of attachment 253519 [details] [review]: A few mechanical/style comments...but at a high level, I think we should just drop the hard requirement for subject/body (as a separate patch). ::: src/ostree/ot-builtin-commit.c @@ +189,3 @@ + bodybuf = g_string_new (lines[i]); + else + g_string_append_printf (bodybuf, "\n%s", lines[i]); While it's irrelevant here, if this were code where performance was any kind of concern you'd want to do: g_string_append_c (bodybuf, '\n'); g_string_append (bodybuf, lines[i]); to avoid an intermediate strdup. @@ +207,3 @@ + g_string_free (bodybuf, TRUE); + g_free (input); + g_free (output); You could use gs_free for input and output. ::: src/ostree/ot-editor.c @@ +81,3 @@ + tmp = g_file_get_child (ostree_repo_get_path (repo), "tmp"); + if (!gs_file_open_in_tmpdir (tmp, 0600, &file, &output, cancellable, error)) + goto out; The reason the repo has a special tmpdir is so that we can know we can just rename() files into place (/tmp might be tmpfs). But for this, why not just use g_file_new_tmp()? @@ +99,3 @@ + goto out; + + if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) These bits are more concisely written as: gs_subprocess_wait_sync_check() @@ +117,3 @@ + g_clear_object (&output); + g_clear_object (&proc); + g_clear_object (&ctx); These could all be gs_unref_object. Also in order to please static code analyzers I tend to cast to (void), so this would all be: if (file) (void) g_file_delete (file, NULL, NULL);
Created attachment 253525 [details] [review] ostree: Support for using EDITOR to fill commit subject/body Behave similar to git when 'ostree commit' is run without a --subject or --body. Bring up an editor. The first line becomes the subject and following lines become the --body after an optional blank line. Use similar logic to git in determining EDITOR
(In reply to comment #2) > Review of attachment 253519 [details] [review]: > > A few mechanical/style comments...but at a high level, I think we should just > drop the hard requirement for subject/body (as a separate patch). Would this be a backwards incompatible change? They're stored separately in the commit tuples right now. Or would we just make one be a unused value?
Created attachment 253528 [details] [review] ostree: Support for using EDITOR to fill commit subject/body Behave similar to git when 'ostree commit' is run without a --subject or --body. Bring up an editor. The first line becomes the subject and following lines become the --body after an optional blank line. Use similar logic to git in determining EDITOR
Review of attachment 253528 [details] [review]: Looks good, thanks!
Attachment 253528 [details] pushed as a4c3c4a - ostree: Support for using EDITOR to fill commit subject/body