Bug 171085 - commit-log-editor should respect the git editor if one is set
Summary: commit-log-editor should respect the git editor if one is set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Conrad Shultz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-20 15:56 PDT by Conrad Shultz
Modified: 2017-04-21 10:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.36 KB, patch)
2017-04-20 16:01 PDT, Conrad Shultz
no flags Details | Formatted Diff | Diff
Patch (1.49 KB, patch)
2017-04-20 17:01 PDT, Conrad Shultz
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Shultz 2017-04-20 15:56:54 PDT
commit-log-editor currently will consider SVN and CVS editor preferences. It should also respect the git editor if one is set through GIT_EDITOR or the global git config.
Comment 1 Conrad Shultz 2017-04-20 16:01:08 PDT
Created attachment 307656 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-04-20 16:02:51 PDT
<rdar://problem/31745506>
Comment 3 Kocsen Chung 2017-04-20 16:16:30 PDT
Changes look good. 

For git/git-svn users, have we considered using a `prepare-commit-msg` hook (https://git-scm.com/docs/githooks#_prepare_commit_msg).
And allow git to deal with the "editor setting" logic?
Comment 4 Conrad Shultz 2017-04-20 16:17:54 PDT
(In reply to Kocsen Chung from comment #3)
> Changes look good. 
> 
> For git/git-svn users, have we considered using a `prepare-commit-msg` hook
> (https://git-scm.com/docs/githooks#_prepare_commit_msg).
> And allow git to deal with the "editor setting" logic?

I haven't looked into that; I tried, for now, to keep the logic as similar as possible to what we're doing for other SCMs.
Comment 5 Daniel Bates 2017-04-20 16:45:05 PDT
Comment on attachment 307656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307656&action=review

> Tools/Scripts/commit-log-editor:97
> +if (isGit()) {
> +    $editor = $ENV{GIT_EDITOR};
> +    $editor = `git config --global --get core.editor` if !$editor;
> +}

Take GIT_EDITOR := vi and "git config core.editor" := Tools/Scripts/commit-log-editor. Make a change to some file and run "git commit -a". Then Git will invoke vi directly instead of commit-log-editor by definition of GIT_EDITOR in <https://git-scm.com/docs/git-var#_variables>. That is, we never run this code.

We should take a similar approach as for SVN_LOG_EDITOR and CVS_LOG_EDITOR and query for an environment variable that does not coincide with an environment variable used by Git, say GIT_LOG_EDITOR.
Comment 6 Conrad Shultz 2017-04-20 17:01:04 PDT
Created attachment 307665 [details]
Patch
Comment 7 Conrad Shultz 2017-04-20 17:01:24 PDT
(In reply to Daniel Bates from comment #5)
> Comment on attachment 307656 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307656&action=review
> 
> > Tools/Scripts/commit-log-editor:97
> > +if (isGit()) {
> > +    $editor = $ENV{GIT_EDITOR};
> > +    $editor = `git config --global --get core.editor` if !$editor;
> > +}
> 
> Take GIT_EDITOR := vi and "git config core.editor" :=
> Tools/Scripts/commit-log-editor. Make a change to some file and run "git
> commit -a". Then Git will invoke vi directly instead of commit-log-editor by
> definition of GIT_EDITOR in <https://git-scm.com/docs/git-var#_variables>.
> That is, we never run this code.
> 
> We should take a similar approach as for SVN_LOG_EDITOR and CVS_LOG_EDITOR
> and query for an environment variable that does not coincide with an
> environment variable used by Git, say GIT_LOG_EDITOR.

Agreed; posted a new version using GIT_LOG_EDITOR.
Comment 8 Daniel Bates 2017-04-20 17:23:42 PDT
Comment on attachment 307665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307665&action=review

> Tools/ChangeLog:10
> +        If git is available, consider GIT_LOG_EDITOR and any global git editor preference when deciding which editor to present.
> +        We examine the global editor preference since that may be set automatically by installers or third-party tools.

Nit: git => Git
(You mention Git twice in line 9)

It is an unwritten convention that we tend to wrap ChangeLog lines that exceed ~100 characters.

> Tools/Scripts/commit-log-editor:97
> +if (isGit()) {
> +    $editor = $ENV{GIT_LOG_EDITOR};
> +    $editor = `git config --global --get core.editor` if !$editor;
> +}

The behavior of this code to query the global git core editor may surprise a person that never defines GIT_LOG_EDITOR,  SVN_LOG_EDITOR, or CVS_LOG_EDITOR and uses commit-log-editor located in a Git checkout of WebKit in projects that use other version control systems because isGit() will return true (since the WebKit checkout is Git-based). Though a similar surprise may arise if a person just set GIT_LOG_EDITOR when using commit-log-editor in such a scenario.
Comment 9 Daniel Bates 2017-04-20 17:24:33 PDT
Comment on attachment 307665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307665&action=review

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=171085

Please add the radar bug URL, <rdar://problem/31745506>, under this line.
Comment 10 Conrad Shultz 2017-04-21 10:22:40 PDT
(In reply to Daniel Bates from comment #8)
> Comment on attachment 307665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307665&action=review
> 
> > Tools/ChangeLog:10
> > +        If git is available, consider GIT_LOG_EDITOR and any global git editor preference when deciding which editor to present.
> > +        We examine the global editor preference since that may be set automatically by installers or third-party tools.
> 
> Nit: git => Git
> (You mention Git twice in line 9)

Fixed in both places.
> 
> It is an unwritten convention that we tend to wrap ChangeLog lines that
> exceed ~100 characters.

Fixed.

> 
> > Tools/Scripts/commit-log-editor:97
> > +if (isGit()) {
> > +    $editor = $ENV{GIT_LOG_EDITOR};
> > +    $editor = `git config --global --get core.editor` if !$editor;
> > +}
> 
> The behavior of this code to query the global git core editor may surprise a
> person that never defines GIT_LOG_EDITOR,  SVN_LOG_EDITOR, or CVS_LOG_EDITOR
> and uses commit-log-editor located in a Git checkout of WebKit in projects
> that use other version control systems because isGit() will return true
> (since the WebKit checkout is Git-based). Though a similar surprise may
> arise if a person just set GIT_LOG_EDITOR when using commit-log-editor in
> such a scenario.

Agreed.

Also added the radar URL. Thanks for the review!
Comment 11 Conrad Shultz 2017-04-21 10:27:27 PDT
Committed r215615 = 3415f0bae58df85cfdfa29a9c84b402c73238ab1
http://svn.webkit.org/repository/webkit/trunk@215615