commit-log-editor should respect the git editor if one is set

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Conrad Shultz]

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.

[Conrad Shultz]

Created attachment 307656 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31745506>

[Kocsen Chung]

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?

[Conrad Shultz]

(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.

[Daniel Bates]

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.

[Conrad Shultz]

Created attachment 307665 [details]
Patch

[Conrad Shultz]

(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.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Conrad Shultz]

(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!

[Conrad Shultz]

Committed r215615 = 3415f0bae58df85cfdfa29a9c84b402c73238ab1
http://svn.webkit.org/repository/webkit/trunk@215615