171085 2017-04-20 15:56:54 -0700 commit-log-editor should respect the git editor if one is set 2017-04-21 10:27:27 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 conrad_shultz conrad_shultz conrad_shultz dbates kocsen_chung lforschler webkit-bug-importer oldest_to_newest 1299354 0 conrad_shultz 2017-04-20 15:56:54 -0700 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. 1299359 1 307656 conrad_shultz 2017-04-20 16:01:08 -0700 Created attachment 307656 Patch 1299362 2 webkit-bug-importer 2017-04-20 16:02:51 -0700 <rdar://problem/31745506> 1299371 3 kocsen_chung 2017-04-20 16:16:30 -0700 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? 1299374 4 conrad_shultz 2017-04-20 16:17:54 -0700 (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. 1299386 5 307656 dbates 2017-04-20 16:45:05 -0700 Comment on attachment 307656 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. 1299399 6 307665 conrad_shultz 2017-04-20 17:01:04 -0700 Created attachment 307665 Patch 1299401 7 conrad_shultz 2017-04-20 17:01:24 -0700 (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. 1299417 8 307665 dbates 2017-04-20 17:23:42 -0700 Comment on attachment 307665 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. 1299418 9 307665 dbates 2017-04-20 17:24:33 -0700 Comment on attachment 307665 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. 1299669 10 conrad_shultz 2017-04-21 10:22:40 -0700 (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! 1299673 11 conrad_shultz 2017-04-21 10:27:27 -0700 Committed r215615 = 3415f0bae58df85cfdfa29a9c84b402c73238ab1 http://svn.webkit.org/repository/webkit/trunk@215615 307656 2017-04-20 16:01:08 -0700 2017-04-20 17:01:03 -0700 Patch bug-171085-20170420160108.patch text/plain 1393 conrad_shultz U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1NTY1CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzBmZjIzNDExMDRmZWM0ZTgxOTMwOTUxNWVkZjZiNTNl MGFiZDFiOC4uNjk4NzRhYWMyMWNmYWM0ZDcxM2JjODdhMmRjNjAyODQxN2YxZDU0OSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE3LTA0LTIwICBDb25yYWQgU2h1bHR6ICA8Y29ucmFkX3NodWx0ekBhcHBsZS5jb20+ CisKKyAgICAgICAgY29tbWl0LWxvZy1lZGl0b3Igc2hvdWxkIHJlc3BlY3QgdGhlIGdpdCBlZGl0 b3IgaWYgb25lIGlzIHNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTcxMDg1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBTY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9yOgorICAgICAgICBJZiBnaXQgaXMg YXZhaWxhYmxlLCBjb25zaWRlciBHSVRfRURJVE9SIGFuZCBhbnkgZ2xvYmFsIGdpdCBlZGl0b3Ig cHJlZmVyZW5jZSB3aGVuIGRlY2lkaW5nIHdoaWNoIGVkaXRvciB0byBwcmVzZW50LgorCiAyMDE3 LTA0LTIwICBYYW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KIAogICAgICAgICBbR1RLXVtq aGJ1aWxkXSBVcGRhdGUgZ2xpYiBhbmQgZ2xpYi1uZXR3b3JraW5nIHRvIHRoZSBsYXRlc3Qgc3Rh YmxlIHZlcnNpb25zCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9y IGIvVG9vbHMvU2NyaXB0cy9jb21taXQtbG9nLWVkaXRvcgppbmRleCAyZmYwZmEzMzJmZmY3OWZh M2RjZjJhNzNjZDU2NDNkNTA3Zjk2M2IzLi4xNzY3MmE3OTM4NDY4ZWQ2MjM4ZmRhMGVmNWJkYzg1 YmIwMzllYmQ5IDEwMDc1NQotLS0gYS9Ub29scy9TY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9yCisr KyBiL1Rvb2xzL1NjcmlwdHMvY29tbWl0LWxvZy1lZGl0b3IKQEAgLTkwLDcgKzkwLDEzIEBAIGlm ICghJGxvZykgewogCiBteSAkYmFzZURpciA9IGJhc2VQcm9kdWN0RGlyKCk7CiAKLW15ICRlZGl0 b3IgPSAkRU5We1NWTl9MT0dfRURJVE9SfTsKK215ICRlZGl0b3I7CitpZiAoaXNHaXQoKSkgewor ICAgICRlZGl0b3IgPSAkRU5We0dJVF9FRElUT1J9OworICAgICRlZGl0b3IgPSBgZ2l0IGNvbmZp ZyAtLWdsb2JhbCAtLWdldCBjb3JlLmVkaXRvcmAgaWYgISRlZGl0b3I7Cit9CisKKyRlZGl0b3Ig PSAkRU5We1NWTl9MT0dfRURJVE9SfSBpZiAhJGVkaXRvcjsKICRlZGl0b3IgPSAkRU5We0NWU19M T0dfRURJVE9SfSBpZiAhJGVkaXRvcjsKICRlZGl0b3IgPSAiIiBpZiAkZWRpdG9yICYmIGlzQ29t bWl0TG9nRWRpdG9yKCRlZGl0b3IpOwogCg== 307665 2017-04-20 17:01:04 -0700 2017-04-20 17:23:42 -0700 Patch bug-171085-20170420170104.patch text/plain 1522 conrad_shultz U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1NTY1CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzBmZjIzNDExMDRmZWM0ZTgxOTMwOTUxNWVkZjZiNTNl MGFiZDFiOC4uNzZjZjJmZDU3YmVjNmM4N2RhMDgyOTYyNmU1YTczNGY2M2YyYzNlMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0 IEBACisyMDE3LTA0LTIwICBDb25yYWQgU2h1bHR6ICA8Y29ucmFkX3NodWx0ekBhcHBsZS5jb20+ CisKKyAgICAgICAgY29tbWl0LWxvZy1lZGl0b3Igc2hvdWxkIHJlc3BlY3QgdGhlIGdpdCBlZGl0 b3IgaWYgb25lIGlzIHNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTcxMDg1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBTY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9yOgorICAgICAgICBJZiBnaXQgaXMg YXZhaWxhYmxlLCBjb25zaWRlciBHSVRfTE9HX0VESVRPUiBhbmQgYW55IGdsb2JhbCBnaXQgZWRp dG9yIHByZWZlcmVuY2Ugd2hlbiBkZWNpZGluZyB3aGljaCBlZGl0b3IgdG8gcHJlc2VudC4KKyAg ICAgICAgV2UgZXhhbWluZSB0aGUgZ2xvYmFsIGVkaXRvciBwcmVmZXJlbmNlIHNpbmNlIHRoYXQg bWF5IGJlIHNldCBhdXRvbWF0aWNhbGx5IGJ5IGluc3RhbGxlcnMgb3IgdGhpcmQtcGFydHkgdG9v bHMuCisKIDIwMTctMDQtMjAgIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgogCiAgICAg ICAgIFtHVEtdW2poYnVpbGRdIFVwZGF0ZSBnbGliIGFuZCBnbGliLW5ldHdvcmtpbmcgdG8gdGhl IGxhdGVzdCBzdGFibGUgdmVyc2lvbnMKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvY29tbWl0 LWxvZy1lZGl0b3IgYi9Ub29scy9TY3JpcHRzL2NvbW1pdC1sb2ctZWRpdG9yCmluZGV4IDJmZjBm YTMzMmZmZjc5ZmEzZGNmMmE3M2NkNTY0M2Q1MDdmOTYzYjMuLjgyM2FkZGUwNTE4MDc3NDdmNTg0 ZWI5NmI0Njg5NzJiYjhlOTY3NjQgMTAwNzU1Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvY29tbWl0LWxv Zy1lZGl0b3IKKysrIGIvVG9vbHMvU2NyaXB0cy9jb21taXQtbG9nLWVkaXRvcgpAQCAtOTAsNyAr OTAsMTMgQEAgaWYgKCEkbG9nKSB7CiAKIG15ICRiYXNlRGlyID0gYmFzZVByb2R1Y3REaXIoKTsK IAotbXkgJGVkaXRvciA9ICRFTlZ7U1ZOX0xPR19FRElUT1J9OworbXkgJGVkaXRvcjsKK2lmIChp c0dpdCgpKSB7CisgICAgJGVkaXRvciA9ICRFTlZ7R0lUX0xPR19FRElUT1J9OworICAgICRlZGl0 b3IgPSBgZ2l0IGNvbmZpZyAtLWdsb2JhbCAtLWdldCBjb3JlLmVkaXRvcmAgaWYgISRlZGl0b3I7 Cit9CisKKyRlZGl0b3IgPSAkRU5We1NWTl9MT0dfRURJVE9SfSBpZiAhJGVkaXRvcjsKICRlZGl0 b3IgPSAkRU5We0NWU19MT0dfRURJVE9SfSBpZiAhJGVkaXRvcjsKICRlZGl0b3IgPSAiIiBpZiAk ZWRpdG9yICYmIGlzQ29tbWl0TG9nRWRpdG9yKCRlZGl0b3IpOwogCg==