threads / patch / 42723

patch, 2 partsSupport for $FILENAMES in tool definitions

Subject: [PATCH 1/2] Support for $FILENAMES in tool definitions

## tl;dr

10 messages between Jun 27, 2016 and Jun 28, 2016. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Alex Riesen· Jun 27, 2016, 13:21 UTC · lore

This adds a FILENAMES environment variable, which contains the repository pathnames of all selected files the list. The variable contains the names separated by spaces.

Similar to the FILENAME it is broken yet, if the names contain spaces.

Note that the file marked and diffed immediately after starting the GUI up, is not actually selected. One must click on it once to really select it.

Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
---

One day the FILENAME and FILENAMES will have to be fixed to properly pass the file names with spaces. Sorry, I couldn't find how to do it this time around.

 lib/tools.tcl | 3 +++
 1 file changed, 3 insertions(+)

Show changes to lib/tools.tcl +3 −0

diff --git a/lib/tools.tcl b/lib/tools.tcl
index 6ec9411..14d556f 100644
--- a/lib/tools.tcl
+++ b/lib/tools.tcl
@@ -69,6 +69,7 @@ proc tools_populate_one {fullname} {
 proc tools_exec {fullname} {
 	global repo_config env current_diff_path
 	global current_branch is_detached
+	global selected_paths
 
 	if {[is_config_true "guitool.$fullname.needsfile"]} {
 		if {$current_diff_path eq {}} {
@@ -100,6 +101,7 @@ proc tools_exec {fullname} {
 
 	set env(GIT_GUITOOL) $fullname
 	set env(FILENAME) $current_diff_path
+	set env(FILENAMES) [array names selected_paths]
 	if {$is_detached} {
 		set env(CUR_BRANCH) ""
 	} else {
@@ -121,6 +123,7 @@ proc tools_exec {fullname} {
 
 	unset env(GIT_GUITOOL)
 	unset env(FILENAME)
+	unset env(FILENAMES)
 	unset env(CUR_BRANCH)
 	catch { unset env(ARGS) }
 	catch { unset env(REVISION) }

-- 
2.9.0.45.g28c608e.dirty


---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Alex Riesen· Jun 27, 2016, 13:23 UTC · re: Alex Riesen · lore

[PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files

It is very confusing that the file, diff of which is displayed and which is marked as selected in the file list, is not, in fact, selected. I.e. the array of selected files does not contain an entry for it.

Fixing this also improves the use of $FILENAMES in custom defined tools: one
does not have to click the file in the list to make it selected.
---
 lib/diff.tcl | 3 +++
 1 file changed, 3 insertions(+)

Show changes to lib/diff.tcl +3 −0

diff --git a/lib/diff.tcl b/lib/diff.tcl
index 0d56986..30bdd69 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -127,6 +127,9 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {
 	} else {
 		start_show_diff $cont_info
 	}
+
+	global current_diff_path selected_paths
+	set selected_paths($current_diff_path) 1
 }
 
 proc show_unmerged_diff {cont_info} {

-- 
2.9.0.45.g28c608e.dirty

To: 
Cc: 
Bcc: 
Subject: 
Reply-To: Alex Riesen <alexander.riesen@cetitec.com>


---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Jakub Narębski· Jun 27, 2016, 17:32 UTC · re: Alex Riesen · lore

Re: [PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files

W dniu 2016-06-27 o 15:23, Alex Riesen pisze:

Show 6 quoted lines

> It is very confusing that the file, diff of which is displayed and which is
> marked as selected in the file list, is not, in fact, selected. I.e. the array
> of selected files does not contain an entry for it.
> 
> Fixing this also improves the use of $FILENAMES in custom defined tools: one
> does not have to click the file in the list to make it selected.

Could you improve the readability of the commit message, please? Perhaps something like the following:

  It is very confusing that the file which diff is displayed is marked as
  selected, but it is not in fact selected (that means the array of selected
  files does not include the file in question).

...

This patch lacks sign-off.

Show 19 quoted lines

> ---
>  lib/diff.tcl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/diff.tcl b/lib/diff.tcl
> index 0d56986..30bdd69 100644
> --- a/lib/diff.tcl
> +++ b/lib/diff.tcl
> @@ -127,6 +127,9 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {
>  	} else {
>  		start_show_diff $cont_info
>  	}
> +
> +	global current_diff_path selected_paths
> +	set selected_paths($current_diff_path) 1
>  }
>  
>  proc show_unmerged_diff {cont_info} {
>

Alex Riesen· Jun 28, 2016, 08:03 UTC · re: Jakub Narębski · lore

Re: [PATCH 2/2] Ensure the file in the diff pane is always in the list of selected files

Jakub Narębski, Mon, Jun 27, 2016 19:32:25 +0200:

Show 19 quoted lines

> W dniu 2016-06-27 o 15:23, Alex Riesen pisze:
> > It is very confusing that the file, diff of which is displayed and which is
> > marked as selected in the file list, is not, in fact, selected. I.e. the array
> > of selected files does not contain an entry for it.
> > 
> > Fixing this also improves the use of $FILENAMES in custom defined tools: one
> > does not have to click the file in the list to make it selected.
> 
> Could you improve the readability of the commit message, please? Perhaps
> something like the following:
> 
>   It is very confusing that the file which diff is displayed is marked as
>   selected, but it is not in fact selected (that means the array of selected
>   files does not include the file in question).
> 
>   ...
> 
> This patch lacks sign-off.
>

Done. Also the prefix.

--- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus

Johannes Schindelin· Jun 27, 2016, 15:49 UTC · re: Alex Riesen · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

Hi Alex,

On Mon, 27 Jun 2016, Alex Riesen wrote:

Show 10 quoted lines

> This adds a FILENAMES environment variable, which contains the repository
> pathnames of all selected files the list.
> The variable contains the names separated by spaces.
> 
> Similar to the FILENAME it is broken yet, if the names contain spaces.
> 
> Note that the file marked and diffed immediately after starting the GUI up,
> is not actually selected. One must click on it once to really select it.
> 
> Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>

Maybe prefix the subject with `git-gui: ` to indicate that this is for the git-gui.git repository?

Also, please muster some patience, Pat seems to be super busy these days.

Ciao, Dscho

Alex Riesen· Jun 28, 2016, 08:04 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

Johannes Schindelin, Mon, Jun 27, 2016 17:49:30 +0200:

Show 15 quoted lines

> On Mon, 27 Jun 2016, Alex Riesen wrote:
> 
> > This adds a FILENAMES environment variable, which contains the repository
> > pathnames of all selected files the list.
> > The variable contains the names separated by spaces.
> > 
> > Similar to the FILENAME it is broken yet, if the names contain spaces.
> > 
> > Note that the file marked and diffed immediately after starting the GUI up,
> > is not actually selected. One must click on it once to really select it.
> > 
> > Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
> 
> Maybe prefix the subject with `git-gui: ` to indicate that this is for the
> git-gui.git repository?

Done.

> Also, please muster some patience, Pat seems to be super busy these days.

It's alright. Nice to know he's ok, though.

Regards, Alex

--- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus

Jakub Narębski· Jun 27, 2016, 17:28 UTC · re: Alex Riesen · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

On 2016-06-27, Alex Riesen wrote:

> This adds a FILENAMES environment variable, which contains the repository
> pathnames of all selected files the list.
> The variable contains the names separated by spaces.

Why not separate filenames with end-of-line character (LF)? It would still be broken for some filenames, but only for unportable ones. Filenames with internal space (common on MS Windows) would work then.

  http://www.dwheeler.com/essays/filenames-in-shell.html
 
If Tcl allows it, you could separate filenames in FILENAMES environment
variable with NUL ("\0") character...

> Similar to the FILENAME it is broken yet, if the names contain spaces.

Could you clarify? Did you meant that FILENAMES environment variable is similar to existing FILENAME variable, but broken for filenames which contain spaces, or did you mean that both FILENAME (how?) and FILENAMES are broken for filenames with spaces in them?

> 
> Note that the file marked and diffed immediately after starting the GUI up,
> is not actually selected. One must click on it once to really select it.

I'm not that familiar with git-gui / gitk; what do you mean by this sentence? Could you summarize how FILENAME and FILENAMES work, please?

> 
> Signed-off-by: Alex Riesen <alexander.riesen@cetitec.com>
> ---

Junio C Hamano· Jun 27, 2016, 17:53 UTC · re: Jakub Narębski · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

Jakub Narębski <jnareb@gmail.com> writes:

Show 14 quoted lines

> On 2016-06-27, Alex Riesen wrote:
>
>> This adds a FILENAMES environment variable, which contains the repository
>> pathnames of all selected files the list.
>> The variable contains the names separated by spaces.
>
> Why not separate filenames with end-of-line character (LF)? It would still
> be broken for some filenames, but only for unportable ones.  Filenames with
> internal space (common on MS Windows) would work then.
>
>   http://www.dwheeler.com/essays/filenames-in-shell.html
>  
> If Tcl allows it, you could separate filenames in FILENAMES environment
> variable with NUL ("\0") character...

Tcl may or may not handle a string with an embedded NUL, but I think it is hard to have an embedded NUL in an environment variable.

Use of LF is a good suggestion regardless.

Jakub Narębski· Jun 27, 2016, 18:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

On 27 June 2016 at 19:53, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Jakub Narębski <jnareb@gmail.com> writes:
>
>> On 2016-06-27, Alex Riesen wrote:
>>
>>> This adds a FILENAMES environment variable, which contains the repository
>>> pathnames of all selected files the list.
>>> The variable contains the names separated by spaces.
>>
>> Why not separate filenames with end-of-line character (LF)? It would still
>> be broken for some filenames, but only for unportable ones.  Filenames with
>> internal space (common on MS Windows) would work then.
>>
>>   http://www.dwheeler.com/essays/filenames-in-shell.html
>>
>> If Tcl allows it, you could separate filenames in FILENAMES environment
>> variable with NUL ("\0") character...
>
> Tcl may or may not handle a string with an embedded NUL, but I think
> it is hard to have an embedded NUL in an environment variable.
>
> Use of LF is a good suggestion regardless.

Or be a good citizen, and use $IFS... though then the user would need to set it to something sane-ish.

-- 
Jakub Narebski

Alex Riesen· Jun 28, 2016, 08:54 UTC · re: Jakub Narębski · lore

Re: [PATCH 1/2] Support for $FILENAMES in tool definitions

Jakub Narębski, Mon, Jun 27, 2016 19:28:07 +0200:

Show 12 quoted lines

> On 2016-06-27, Alex Riesen wrote:
> 
> > This adds a FILENAMES environment variable, which contains the repository
> > pathnames of all selected files the list.
> > The variable contains the names separated by spaces.
> 
> Why not separate filenames with end-of-line character (LF)? It would still
> be broken for some filenames, but only for unportable ones.  Filenames with
> internal space (common on MS Windows) would work then.
> 
>   http://www.dwheeler.com/essays/filenames-in-shell.html
>

Done. Thanks for the reference, BTW.

> If Tcl allows it, you could separate filenames in FILENAMES environment
> variable with NUL ("\0") character...

It allows using of NUL to join the file names. The problem is that the shell does not know that the variable $FILENAMES doesn't end at the first NUL.

Looks like the script should do the expansion (and quoting!) on its own for this. Or implement another syntax for custom tools, which will allow for, i.e. passing the list of files on stdin. LF or NUL separated than.

Show 6 quoted lines

> > Similar to the FILENAME it is broken yet, if the names contain spaces.
> 
> Could you clarify? Did you meant that FILENAMES environment variable is
> similar to existing FILENAME variable, but broken for filenames which contain
> spaces, or did you mean that both FILENAME (how?) and FILENAMES are broken
> for filenames with spaces in them?

Passing a filename with spaces plainly $FILENAME into the command (i.e. without quoting) produces multiple arguments instead of one. The new $FILENAMES produces incorrect number of arguments for files with spaces for the same reason.

> > Note that the file marked and diffed immediately after starting the GUI up,
> > is not actually selected. One must click on it once to really select it.
> 
> I'm not that familiar with git-gui / gitk; what do you mean by this sentence?

The git-gui (its completely different from gitk) keeps two variables for current files: a scalar variable for the file diff of which is displayed in the lower right pane, and a Tcl array (more like a string-to-int map, if I understand the code) with the names of the files selected in the file list. The array is populated every time you click on a name in the file list, and can contain multiple entries if you hold "Ctrl" while clicking.

> Could you summarize how FILENAME and FILENAMES work, please?

These are just environment variables set before running the tool command. They are unset immediately after starting the command.

The tool command is unconditionally run using "sh -c <command>", so the references to the environment variables are expanded by "sh".

Regards, Alex

--- Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus