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

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

[Alex Riesen]

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(+)

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

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

[Alex Riesen]

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(+)

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

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

[Johannes Schindelin]

Hi Alex,

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?

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

Ciao,
Dscho

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

[Jakub Narębski]

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

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

[Jakub Narębski]

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.

> ---
>  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} {
> 

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

[Junio C Hamano]

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.

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

[Jakub Narębski]

On 27 June 2016 at 19:53, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Alex Riesen]

Jakub Narębski, Mon, Jun 27, 2016 19:32:25 +0200:
> 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

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

[Alex Riesen]

Johannes Schindelin, Mon, Jun 27, 2016 17:49:30 +0200:
> 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

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

[Alex Riesen]

Jakub Narębski, Mon, Jun 27, 2016 19:28:07 +0200:
> 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.

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