{"thread":{"id":"42723","subject":"[PATCH 1/2] Support for $FILENAMES in tool definitions","startedAt":"2016-06-27T13:22:03Z","lastAt":"2016-06-28T08:55:16Z","messageCount":10,"participants":["Alex Riesen","Johannes Schindelin","Jakub Narębski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"290231","messageId":"20160627132137.GC4194@pflmari","threadId":"42723","inReplyTo":null,"subject":"[PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2016-06-27T13:21:37Z","receivedAt":"2016-06-27T13:22:03Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"This adds a FILENAMES environment variable, which contains the repository\npathnames of all selected files the list.\nThe variable contains the names separated by spaces.\n\nSimilar to the FILENAME it is broken yet, if the names contain spaces.\n\nNote that the file marked and diffed immediately after starting the GUI up,\nis not actually selected. One must click on it once to really select it.\n\nSigned-off-by: Alex Riesen \n---\n\nOne day the FILENAME and FILENAMES will have to be fixed to properly pass the\nfile names with spaces. Sorry, I couldn't find how to do it this time around.\n\n lib/tools.tcl | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/lib/tools.tcl b/lib/tools.tcl\nindex 6ec9411..14d556f 100644\n--- a/lib/tools.tcl\n+++ b/lib/tools.tcl\n@@ -69,6 +69,7 @@ proc tools_populate_one {fullname} {\n proc tools_exec {fullname} {\n \tglobal repo_config env current_diff_path\n \tglobal current_branch is_detached\n+\tglobal selected_paths\n \n \tif {[is_config_true \"guitool.$fullname.needsfile\"]} {\n \t\tif {$current_diff_path eq {}} {\n@@ -100,6 +101,7 @@ proc tools_exec {fullname} {\n \n \tset env(GIT_GUITOOL) $fullname\n \tset env(FILENAME) $current_diff_path\n+\tset env(FILENAMES) [array names selected_paths]\n \tif {$is_detached} {\n \t\tset env(CUR_BRANCH) \"\"\n \t} else {\n@@ -121,6 +123,7 @@ proc tools_exec {fullname} {\n \n \tunset env(GIT_GUITOOL)\n \tunset env(FILENAME)\n+\tunset env(FILENAMES)\n \tunset env(CUR_BRANCH)\n \tcatch { unset env(ARGS) }\n \tcatch { unset env(REVISION) }\n-- \n2.9.0.45.g28c608e.dirty\n\n\n---\nDiese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.\nhttps://www.avast.com/antivirus\n\n"},{"id":"290232","messageId":"20160627132308.GD4194@pflmari","threadId":"42723","inReplyTo":"20160627132137.GC4194@pflmari","subject":"[PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2016-06-27T13:23:08Z","receivedAt":"2016-06-27T13:23:33Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"It is very confusing that the file, diff of which is displayed and which is\nmarked as selected in the file list, is not, in fact, selected. I.e. the array\nof selected files does not contain an entry for it.\n\nFixing this also improves the use of $FILENAMES in custom defined tools: one\ndoes not have to click the file in the list to make it selected.\n---\n lib/diff.tcl | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex 0d56986..30bdd69 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -127,6 +127,9 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n \t} else {\n \t\tstart_show_diff $cont_info\n \t}\n+\n+\tglobal current_diff_path selected_paths\n+\tset selected_paths($current_diff_path) 1\n }\n \n proc show_unmerged_diff {cont_info} {\n-- \n2.9.0.45.g28c608e.dirty\n\nTo: \nCc: \nBcc: \nSubject: \nReply-To: Alex Riesen \n\n\n---\nDiese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.\nhttps://www.avast.com/antivirus\n\n"},{"id":"290244","messageId":"alpine.DEB.2.20.1606271748250.12947@virtualbox","threadId":"42723","inReplyTo":"20160627132137.GC4194@pflmari","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-27T15:49:30Z","receivedAt":"2016-06-27T15:59:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alex,\n\nOn Mon, 27 Jun 2016, Alex Riesen wrote:\n\n> This adds a FILENAMES environment variable, which contains the repository\n> pathnames of all selected files the list.\n> The variable contains the names separated by spaces.\n> \n> Similar to the FILENAME it is broken yet, if the names contain spaces.\n> \n> Note that the file marked and diffed immediately after starting the GUI up,\n> is not actually selected. One must click on it once to really select it.\n> \n> Signed-off-by: Alex Riesen \n\nMaybe prefix the subject with `git-gui: ` to indicate that this is for the\ngit-gui.git repository?\n\nAlso, please muster some patience, Pat seems to be super busy these days.\n\nCiao,\nDscho\n"},{"id":"290257","messageId":"57716227.1030104@gmail.com","threadId":"42723","inReplyTo":"20160627132137.GC4194@pflmari","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-06-27T17:28:07Z","receivedAt":"2016-06-27T17:28:51Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 2016-06-27, Alex Riesen wrote:\n\n> This adds a FILENAMES environment variable, which contains the repository\n> pathnames of all selected files the list.\n> The variable contains the names separated by spaces.\n\nWhy not separate filenames with end-of-line character (LF)? It would still\nbe broken for some filenames, but only for unportable ones. Filenames with\ninternal space (common on MS Windows) would work then.\n\n http://www.dwheeler.com/essays/filenames-in-shell.html\n \nIf Tcl allows it, you could separate filenames in FILENAMES environment\nvariable with NUL (\"\\0\") character...\n\n> Similar to the FILENAME it is broken yet, if the names contain spaces.\n\nCould you clarify? Did you meant that FILENAMES environment variable is\nsimilar to existing FILENAME variable, but broken for filenames which contain\nspaces, or did you mean that both FILENAME (how?) and FILENAMES are broken\nfor filenames with spaces in them?\n\n> \n> Note that the file marked and diffed immediately after starting the GUI up,\n> is not actually selected. One must click on it once to really select it.\n\nI'm not that familiar with git-gui / gitk; what do you mean by this sentence?\nCould you summarize how FILENAME and FILENAMES work, please?\n\n> \n> Signed-off-by: Alex Riesen \n> ---\n\n"},{"id":"290258","messageId":"57716329.8000809@gmail.com","threadId":"42723","inReplyTo":"20160627132308.GD4194@pflmari","subject":"Re: [PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-06-27T17:32:25Z","receivedAt":"2016-06-27T17:32:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2016-06-27 o 15:23, Alex Riesen pisze:\n> It is very confusing that the file, diff of which is displayed and which is\n> marked as selected in the file list, is not, in fact, selected. I.e. the array\n> of selected files does not contain an entry for it.\n> \n> Fixing this also improves the use of $FILENAMES in custom defined tools: one\n> does not have to click the file in the list to make it selected.\n\nCould you improve the readability of the commit message, please? Perhaps\nsomething like the following:\n\n It is very confusing that the file which diff is displayed is marked as\n selected, but it is not in fact selected (that means the array of selected\n files does not include the file in question).\n\n ...\n\nThis patch lacks sign-off.\n\n> ---\n> lib/diff.tcl | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/lib/diff.tcl b/lib/diff.tcl\n> index 0d56986..30bdd69 100644\n> --- a/lib/diff.tcl\n> +++ b/lib/diff.tcl\n> @@ -127,6 +127,9 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n> \t} else {\n> \t\tstart_show_diff $cont_info\n> \t}\n> +\n> +\tglobal current_diff_path selected_paths\n> +\tset selected_paths($current_diff_path) 1\n> }\n> \n> proc show_unmerged_diff {cont_info} {\n> \n\n"},{"id":"290266","messageId":"xmqq7fdabjsb.fsf@gitster.mtv.corp.google.com","threadId":"42723","inReplyTo":"57716227.1030104@gmail.com","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-27T17:53:24Z","receivedAt":"2016-06-27T17:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> On 2016-06-27, Alex Riesen wrote:\n>\n>> This adds a FILENAMES environment variable, which contains the repository\n>> pathnames of all selected files the list.\n>> The variable contains the names separated by spaces.\n>\n> Why not separate filenames with end-of-line character (LF)? It would still\n> be broken for some filenames, but only for unportable ones. Filenames with\n> internal space (common on MS Windows) would work then.\n>\n> http://www.dwheeler.com/essays/filenames-in-shell.html\n> \n> If Tcl allows it, you could separate filenames in FILENAMES environment\n> variable with NUL (\"\\0\") character...\n\nTcl may or may not handle a string with an embedded NUL, but I think\nit is hard to have an embedded NUL in an environment variable.\n\nUse of LF is a good suggestion regardless.\n\n"},{"id":"290267","messageId":"CANQwDwcq=O-ZVW2=+EnZnmCZpTjP=soQ_-cQP_n0p-TJ6DFQ-w@mail.gmail.com","threadId":"42723","inReplyTo":"xmqq7fdabjsb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-06-27T18:09:47Z","receivedAt":"2016-06-27T18:10:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 27 June 2016 at 19:53, Junio C Hamano wrote:\n> Jakub Narębski writes:\n>\n>> On 2016-06-27, Alex Riesen wrote:\n>>\n>>> This adds a FILENAMES environment variable, which contains the repository\n>>> pathnames of all selected files the list.\n>>> The variable contains the names separated by spaces.\n>>\n>> Why not separate filenames with end-of-line character (LF)? It would still\n>> be broken for some filenames, but only for unportable ones. Filenames with\n>> internal space (common on MS Windows) would work then.\n>>\n>> http://www.dwheeler.com/essays/filenames-in-shell.html\n>>\n>> If Tcl allows it, you could separate filenames in FILENAMES environment\n>> variable with NUL (\"\\0\") character...\n>\n> Tcl may or may not handle a string with an embedded NUL, but I think\n> it is hard to have an embedded NUL in an environment variable.\n>\n> Use of LF is a good suggestion regardless.\n\nOr be a good citizen, and use $IFS... though then the user would\nneed to set it to something sane-ish.\n\n-- \nJakub Narebski\n"},{"id":"290345","messageId":"20160628080340.GA3710@pflmari","threadId":"42723","inReplyTo":"57716329.8000809@gmail.com","subject":"Re: [PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2016-06-28T08:03:40Z","receivedAt":"2016-06-28T08:04:29Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jakub Narębski, Mon, Jun 27, 2016 19:32:25 +0200:\n> W dniu 2016-06-27 o 15:23, Alex Riesen pisze:\n> > It is very confusing that the file, diff of which is displayed and which is\n> > marked as selected in the file list, is not, in fact, selected. I.e. the array\n> > of selected files does not contain an entry for it.\n> > \n> > Fixing this also improves the use of $FILENAMES in custom defined tools: one\n> > does not have to click the file in the list to make it selected.\n> \n> Could you improve the readability of the commit message, please? Perhaps\n> something like the following:\n> \n> It is very confusing that the file which diff is displayed is marked as\n> selected, but it is not in fact selected (that means the array of selected\n> files does not include the file in question).\n> \n> ...\n> \n> This patch lacks sign-off.\n> \n\nDone. Also the prefix.\n\n\n---\nDiese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.\nhttps://www.avast.com/antivirus\n\n"},{"id":"290346","messageId":"20160628080426.GB3710@pflmari","threadId":"42723","inReplyTo":"alpine.DEB.2.20.1606271748250.12947@virtualbox","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2016-06-28T08:04:26Z","receivedAt":"2016-06-28T08:05:47Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Johannes Schindelin, Mon, Jun 27, 2016 17:49:30 +0200:\n> On Mon, 27 Jun 2016, Alex Riesen wrote:\n> \n> > This adds a FILENAMES environment variable, which contains the repository\n> > pathnames of all selected files the list.\n> > The variable contains the names separated by spaces.\n> > \n> > Similar to the FILENAME it is broken yet, if the names contain spaces.\n> > \n> > Note that the file marked and diffed immediately after starting the GUI up,\n> > is not actually selected. One must click on it once to really select it.\n> > \n> > Signed-off-by: Alex Riesen \n> \n> Maybe prefix the subject with `git-gui: ` to indicate that this is for the\n> git-gui.git repository?\n\nDone.\n\n> Also, please muster some patience, Pat seems to be super busy these days.\n\nIt's alright. Nice to know he's ok, though.\n\nRegards,\nAlex\n\n\n---\nDiese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.\nhttps://www.avast.com/antivirus\n\n"},{"id":"290352","messageId":"20160628085421.GC3710@pflmari","threadId":"42723","inReplyTo":"57716227.1030104@gmail.com","subject":"Re: [PATCH 1/2] Support for $FILENAMES in tool definitions","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2016-06-28T08:54:21Z","receivedAt":"2016-06-28T08:55:16Z","isPatch":true,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Jakub Narębski, Mon, Jun 27, 2016 19:28:07 +0200:\n> On 2016-06-27, Alex Riesen wrote:\n> \n> > This adds a FILENAMES environment variable, which contains the repository\n> > pathnames of all selected files the list.\n> > The variable contains the names separated by spaces.\n> \n> Why not separate filenames with end-of-line character (LF)? It would still\n> be broken for some filenames, but only for unportable ones. Filenames with\n> internal space (common on MS Windows) would work then.\n> \n> http://www.dwheeler.com/essays/filenames-in-shell.html\n> \n\nDone. Thanks for the reference, BTW.\n\n> If Tcl allows it, you could separate filenames in FILENAMES environment\n> variable with NUL (\"\\0\") character...\n\nIt allows using of NUL to join the file names. The problem is that the shell\ndoes not know that the variable $FILENAMES doesn't end at the first NUL.\n\nLooks like the script should do the expansion (and quoting!) on its own for\nthis. Or implement another syntax for custom tools, which will allow for, i.e.\npassing the list of files on stdin. LF or NUL separated than.\n\n> > Similar to the FILENAME it is broken yet, if the names contain spaces.\n> \n> Could you clarify? Did you meant that FILENAMES environment variable is\n> similar to existing FILENAME variable, but broken for filenames which contain\n> spaces, or did you mean that both FILENAME (how?) and FILENAMES are broken\n> for filenames with spaces in them?\n\nPassing a filename with spaces plainly $FILENAME into the command (i.e.\nwithout quoting) produces multiple arguments instead of one.\nThe new $FILENAMES produces incorrect number of arguments for files with\nspaces for the same reason.\n\n> > Note that the file marked and diffed immediately after starting the GUI up,\n> > is not actually selected. One must click on it once to really select it.\n> \n> I'm not that familiar with git-gui / gitk; what do you mean by this sentence?\n\nThe git-gui (its completely different from gitk) keeps two variables for\ncurrent files: a scalar variable for the file diff of which is displayed in\nthe lower right pane, and a Tcl array (more like a string-to-int map, if I\nunderstand the code) with the names of the files selected in the file list.\nThe array is populated every time you click on a name in the file list, and\ncan contain multiple entries if you hold \"Ctrl\" while clicking.\n\n> Could you summarize how FILENAME and FILENAMES work, please?\n\nThese are just environment variables set before running the tool command. They\nare unset immediately after starting the command.\n\nThe tool command is unconditionally run using \"sh -c \", so the\nreferences to the environment variables are expanded by \"sh\".\n\nRegards,\nAlex\n\n---\nDiese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.\nhttps://www.avast.com/antivirus\n\n"}]}