[PATCH 1/2] completion: create variable for untracked file modes

[PATCH 1/2] completion: create variable for untracked file modes

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..57a0acc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 +__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$(__git_untracked_file_modes)" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)
-- 
2.8.3.windows.1

Re: [PATCH 1/2] completion: create variable for untracked file modes

[Jeff King]

On Wed, Jun 01, 2016 at 01:42:33AM +0200, Thomas Braun wrote:

> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
>  contrib/completion/git-completion.bash | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 3402475..57a0acc 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1098,6 +1098,8 @@ _git_clone ()
>  	esac
>  }
>  +__git_untracked_file_modes="all no normal"
> +
>  _git_commit ()
>  {
>  	case "$prev" in

There's something funny about the formatting of your patch. The first
"+" line is indented, which it shouldn't be. As it is, it looks like
context (but it's not actually part of the preimage). But if it's not
context, then you are missing a context line.

It kind of looks like you put a literal "+__git_untracked..." line in
the file and then committed, then added the next line, and committed
that. Or you edited the patch by hand.

Anyway, I couldn't actually apply it.

> @@ -1119,7 +1121,7 @@ _git_commit ()
>  		return
>  		;;
>  	--untracked-files=*)
> -		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
> +		__gitcomp "$(__git_untracked_file_modes)" "" "${cur##--untracked-files=}"

Your __git_untracked_file_modes is a variable, but "$()" will run it as
a command. You want just "$__git_untracked_file_modes", or to make it
into a function.

-Peff

Re: [PATCH 1/2] completion: create variable for untracked file modes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 3402475..57a0acc 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1098,6 +1098,8 @@ _git_clone ()
>>  	esac
>>  }
>>  +__git_untracked_file_modes="all no normal"
>> +
>>  _git_commit ()
>>  {
>>  	case "$prev" in
>
> There's something funny about the formatting of your patch. The first
> "+" line is indented, which it shouldn't be. As it is, it looks like
> context (but it's not actually part of the preimage). But if it's not
> context, then you are missing a context line.

Not just that.  Count the context lines and notice that this appears
to have only 2 lines of precontext.

I think the MUA is somehow eating a blank line context (i.e. a
single SP on a line by itself) immediately after the closing brace
of the function before _git_commit and the next new line that began
with '+' in the original and made them into a single line.  I've
seen this exact breakage before, I think.

Re: [PATCH 1/2] completion: create variable for untracked file modes

[Thomas Braun]

> Junio C Hamano <gitster@pobox.com> hat am 1. Juni 2016 um 09:02 geschrieben:
> 
> 
> Jeff King <peff@peff.net> writes:
> 
> >> diff --git a/contrib/completion/git-completion.bash
> >> b/contrib/completion/git-completion.bash
> >> index 3402475..57a0acc 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -1098,6 +1098,8 @@ _git_clone ()
> >>  	esac
> >>  }
> >>  +__git_untracked_file_modes="all no normal"
> >> +
> >>  _git_commit ()
> >>  {
> >>  	case "$prev" in
> >
> > There's something funny about the formatting of your patch. The first
> > "+" line is indented, which it shouldn't be. As it is, it looks like
> > context (but it's not actually part of the preimage). But if it's not
> > context, then you are missing a context line.
> 
> Not just that.  Count the context lines and notice that this appears
> to have only 2 lines of precontext.
> 
> I think the MUA is somehow eating a blank line context (i.e. a
> single SP on a line by itself) immediately after the closing brace
> of the function before _git_commit and the next new line that began
> with '+' in the original and made them into a single line.  I've
> seen this exact breakage before, I think.

Thanks both for noticing and sorry for the mess.

I'm using Thunderbird 45.1.1 on Windows and the "Toggle Word Wrap" addon
for well trying to avoid messing up whitespace. The emails are created
with `git format-patch ... | git imap-send` and sent to my drafts
folder. There they look good *until* I choose "Edit as new" which seems
to be required to actually send the email.  So I'll
stick to other solutions for now.

Reroll follows.

[PATCH v2 1/2] completion: create variable for untracked file modes

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..addea89 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 
+__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)

[PATCH v2 2/2] completion: add git status

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index addea89..77343da 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1782,6 +1782,35 @@ _git_stage ()
 	_git_add
 }
 
+_git_status ()
+{
+	case "$cur" in
+	--ignore-submodules=*)
+		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
+		return
+		;;
+	--untracked-files=*)
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
+		return
+		;;
+	--column=*)
+		__gitcomp "
+			always never auto column row plain dense nodense
+			" "" "${cur##--column=}"
+		return
+		;;
+	--*)
+		__gitcomp "
+			--short --branch --porcelain --long --verbose
+			--untracked-files= --ignore-submodules= --ignored
+			--column= --no-column
+			"
+		return
+		;;
+	esac
+	__git_complete_file
+}
+
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword

Re: [PATCH v2 1/2] completion: create variable for untracked file modes

[SZEDER Gábor]

This subject would perhaps read better:

   completion: factor out untracked file modes into a variable


Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:

> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
>  contrib/completion/git-completion.bash | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash  
> b/contrib/completion/git-completion.bash
> index 3402475..addea89 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1098,6 +1098,8 @@ _git_clone ()
>  	esac
>  }
>
> +__git_untracked_file_modes="all no normal"
> +
>  _git_commit ()
>  {
>  	case "$prev" in
> @@ -1119,7 +1121,7 @@ _git_commit ()
>  		return
>  		;;
>  	--untracked-files=*)
> -		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
> +		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
>  		return
>  		;;
>  	--*)

Re: [PATCH v2 2/2] completion: add git status

[SZEDER Gábor]

Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:

> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
>  contrib/completion/git-completion.bash | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash  
> b/contrib/completion/git-completion.bash
> index addea89..77343da 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1782,6 +1782,35 @@ _git_stage ()
>  	_git_add
>  }
>
> +_git_status ()
> +{
> +	case "$cur" in
> +	--ignore-submodules=*)
> +		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
> +		return
> +		;;
> +	--untracked-files=*)
> +		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
> +		return
> +		;;
> +	--column=*)
> +		__gitcomp "
> +			always never auto column row plain dense nodense
> +			" "" "${cur##--column=}"
> +		return
> +		;;
> +	--*)
> +		__gitcomp "
> +			--short --branch --porcelain --long --verbose
> +			--untracked-files= --ignore-submodules= --ignored
> +			--column= --no-column
> +			"
> +		return
> +		;;
> +	esac
> +	__git_complete_file

__git_complete_file()'s job is to complete the '<rev>:<path>' notation,
e.g. 'master:Mak<TAB>',  which is not what we want here, because this
notation doesn't make sense for 'git status' and because 'git status
<TAB>' would then offer refs instead of files.

I think there are two choices what to do instead:

   - Don't do anything :)  Bash will then fall back to filename
     completion, which is quite close to what we want here (and in this
     case the return statements from the other case arms can go away as
     well).  The drawback is that all ignored files in the current
     working directory will show up after 'git status <TAB>'.

   - use __git_complete_index_file() with appropriate options, perhaps
     '--cached --others', but I didn't think this through.  For bonus
     points pass additional options when certain 'git status' options are
     already present on the command line, e.g. pass '--ignored', too, if
     it is present.

> +}
> +
>  __git_config_get_set_variables ()
>  {
>  	local prevword word config_file= c=$cword

Re: [PATCH v2 1/2] completion: create variable for untracked file modes

[Thomas Braun]

Am 01.06.2016 um 13:59 schrieb SZEDER Gábor:
> 
> This subject would perhaps read better:
> 
>   completion: factor out untracked file modes into a variable

Yes, definitly. Will be included in reroll.

> Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:
> 
>> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
>> ---
>>  contrib/completion/git-completion.bash | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/completion/git-completion.bash
>> b/contrib/completion/git-completion.bash
>> index 3402475..addea89 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1098,6 +1098,8 @@ _git_clone ()
>>      esac
>>  }
>>
>> +__git_untracked_file_modes="all no normal"
>> +
>>  _git_commit ()
>>  {
>>      case "$prev" in
>> @@ -1119,7 +1121,7 @@ _git_commit ()
>>          return
>>          ;;
>>      --untracked-files=*)
>> -        __gitcomp "all no normal" "" "${cur##--untracked-files=}"
>> +        __gitcomp "$__git_untracked_file_modes" ""
>> "${cur##--untracked-files=}"
>>          return
>>          ;;
>>      --*)
> 
> 
> 

Re: [PATCH v2 2/2] completion: add git status

[Thomas Braun]

Am 01.06.2016 um 14:15 schrieb SZEDER Gábor:
> 
> Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:
> 
>> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
>> ---
>>  contrib/completion/git-completion.bash | 29
>> +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/contrib/completion/git-completion.bash
>> b/contrib/completion/git-completion.bash
>> index addea89..77343da 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1782,6 +1782,35 @@ _git_stage ()
>>      _git_add
>>  }
>>
>> +_git_status ()
>> +{
>> +    case "$cur" in
>> +    --ignore-submodules=*)
>> +        __gitcomp "none untracked dirty all" ""
>> "${cur##--ignore-submodules=}"
>> +        return
>> +        ;;
>> +    --untracked-files=*)
>> +        __gitcomp "$__git_untracked_file_modes" ""
>> "${cur##--untracked-files=}"
>> +        return
>> +        ;;
>> +    --column=*)
>> +        __gitcomp "
>> +            always never auto column row plain dense nodense
>> +            " "" "${cur##--column=}"
>> +        return
>> +        ;;
>> +    --*)
>> +        __gitcomp "
>> +            --short --branch --porcelain --long --verbose
>> +            --untracked-files= --ignore-submodules= --ignored
>> +            --column= --no-column
>> +            "
>> +        return
>> +        ;;
>> +    esac
>> +    __git_complete_file
> 
> __git_complete_file()'s job is to complete the '<rev>:<path>' notation,
> e.g. 'master:Mak<TAB>',  which is not what we want here, because this
> notation doesn't make sense for 'git status' and because 'git status
> <TAB>' would then offer refs instead of files.

Correct. I might have been mislead by the name ;)

> I think there are two choices what to do instead:
> 
>   - Don't do anything :)  Bash will then fall back to filename
>     completion, which is quite close to what we want here (and in this
>     case the return statements from the other case arms can go away as
>     well).  The drawback is that all ignored files in the current
>     working directory will show up after 'git status <TAB>'.
> 
>   - use __git_complete_index_file() with appropriate options, perhaps
>     '--cached --others', but I didn't think this through.  For bonus
>     points pass additional options when certain 'git status' options are
>     already present on the command line, e.g. pass '--ignored', too, if
>     it is present.

I went for the bonus points way. If that is too involved I can also go
back to "Don't do anything".

[PATCH v3 2/2] completion: add git status

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 55 ++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index addea89..fa7a03a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1782,6 +1782,61 @@ _git_stage ()
 	_git_add
 }
 
+_git_status ()
+{
+	local complete_opt
+	local untracked_state
+
+	case "$cur" in
+	--ignore-submodules=*)
+		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
+		return
+		;;
+	--untracked-files=*)
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
+		return
+		;;
+	--column=*)
+		__gitcomp "
+			always never auto column row plain dense nodense
+			" "" "${cur##--column=}"
+		return
+		;;
+	--*)
+		__gitcomp "
+			--short --branch --porcelain --long --verbose
+			--untracked-files= --ignore-submodules= --ignored
+			--column= --no-column
+			"
+		return
+		;;
+	esac
+
+	untracked_state="$(__git_find_on_cmdline "--untracked-files=no\
+		--untracked-files=normal --untracked-files=all")"
+	untracked_state=${untracked_state##--untracked-files=}
+
+	if [ -z "$untracked_state" ]; then
+		untracked_state="$(git --git-dir="$(__gitdir)" config "status.showUntrackedFiles")"
+	fi
+
+	case "$untracked_state" in
+		no)
+			# --ignored option does not matter
+			complete_opt=
+			;;
+		all|normal|*)
+			complete_opt="--cached --directory --no-empty-directory --others"
+
+			if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
+				complete_opt="$complete_opt --ignored --exclude=*"
+			fi
+			;;
+	esac
+
+	__git_complete_index_file "$complete_opt"
+}
+
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-- 
2.8.3.windows.1

[PATCH v3 1/2] completion: factor out untracked file modes into a variable

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..addea89 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 
+__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)

Re: [PATCH v3 2/2] completion: add git status

[Junio C Hamano]

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:

> +	untracked_state="$(__git_find_on_cmdline "--untracked-files=no\
> +		--untracked-files=normal --untracked-files=all")"

Just wondering but does this help my use of the command like

	$ git status -uno <TAB>

or do I now have to spell it out like

	$ git status --untracked-files=no <TAB>

to take advantage of it?

> +	untracked_state=${untracked_state##--untracked-files=}
> +
> +	if [ -z "$untracked_state" ]; then
> +		untracked_state="$(git --git-dir="$(__gitdir)" config "status.showUntrackedFiles")"
> +	fi
> +
> +	case "$untracked_state" in
> +		no)
> +			# --ignored option does not matter

Style.  I see existing case/esac statements that use this style, but
our preference is not to indent case arms like this; rather:

	case "$untracked_state" in
        no)
        	# --ignored ...

which saves the indentation one level overall.

> +			complete_opt=
> +			;;
> +		all|normal|*)
> +			complete_opt="--cached --directory --no-empty-directory --others"
> +
> +			if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then

Same question as the "--untracked-files=no vs -uno" applies here.

> +				complete_opt="$complete_opt --ignored --exclude=*"
> +			fi
> +			;;
> +	esac
> +
> +	__git_complete_index_file "$complete_opt"
> +}
> +
>  __git_config_get_set_variables ()
>  {
>  	local prevword word config_file= c=$cword

Re: [PATCH v3 2/2] completion: add git status

[Thomas Braun]

Am 02.06.2016 um 20:14 schrieb Junio C Hamano:
> Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> 
>> +	untracked_state="$(__git_find_on_cmdline "--untracked-files=no\
>> +		--untracked-files=normal --untracked-files=all")"
> 
> Just wondering but does this help my use of the command like
> 
> 	$ git status -uno <TAB>
> 
> or do I now have to spell it out like
> 
> 	$ git status --untracked-files=no <TAB>
> 
> to take advantage of it?

I was unsure if I should support the short option (-u) as well. On
thinking about it again there is little use of only doing it for the
long option.
Will be handled in a reroll.

>> +	untracked_state=${untracked_state##--untracked-files=}
>> +
>> +	if [ -z "$untracked_state" ]; then
>> +		untracked_state="$(git --git-dir="$(__gitdir)" config "status.showUntrackedFiles")"
>> +	fi
>> +
>> +	case "$untracked_state" in
>> +		no)
>> +			# --ignored option does not matter
> 
> Style.  I see existing case/esac statements that use this style, but
> our preference is not to indent case arms like this; rather:
> 
> 	case "$untracked_state" in
>         no)
>         	# --ignored ...
> 
> which saves the indentation one level overall.

thanks, will be fixed.

>> +			complete_opt=
>> +			;;
>> +		all|normal|*)
>> +			complete_opt="--cached --directory --no-empty-directory --others"
>> +
>> +			if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
> 
> Same question as the "--untracked-files=no vs -uno" applies here.

Is there a short version of --ignored? I could not find one in the help,
and from a look into cmd_status in commit.c I would say there is none.

Re: [PATCH v3 2/2] completion: add git status

[Junio C Hamano]

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:

>>> +			if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
>> 
>> Same question as the "--untracked-files=no vs -uno" applies here.
>
> Is there a short version of --ignored? I could not find one in the help,
> and from a look into cmd_status in commit.c I would say there is none.

I was primarily wondering about the effect of parse-options have.
It lets you truncate a long option to its unique prefix (e.g.
"--untracked-files=all" can be spelled as "--unt=all").  It seems
that "--ignored" must be spelled in full, which means the use of
find-on-cmdline we see above is OK, but the reason why it is so is a
bit subtle.  It may deserve a comment there, perhaps.


[Footnote]

*1* The reason is because "--ignored" happens to be the shortest
truncation of "--ignored" in order to disambiguate it from
"--ignore-submodules".

Re: [PATCH v3 2/2] completion: add git status

[Jeff King]

On Fri, Jun 03, 2016 at 09:34:00AM -0700, Junio C Hamano wrote:

> Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> 
> >>> +			if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
> >> 
> >> Same question as the "--untracked-files=no vs -uno" applies here.
> >
> > Is there a short version of --ignored? I could not find one in the help,
> > and from a look into cmd_status in commit.c I would say there is none.
> 
> I was primarily wondering about the effect of parse-options have.
> It lets you truncate a long option to its unique prefix (e.g.
> "--untracked-files=all" can be spelled as "--unt=all").  It seems
> that "--ignored" must be spelled in full, which means the use of
> find-on-cmdline we see above is OK, but the reason why it is so is a
> bit subtle.  It may deserve a comment there, perhaps.

I don't think we handle arguments to unique-prefix options throughout
the completion. There's lots of:

  case "${words[c]}" in
  --foo) ...
  --bar) ...
  --etc) ...

I suspect trying to support them everywhere would be a moderate pain,
and I doubt it is all that useful. We already know the person is using
tab-completion, so the natural thing to do after typing "--unt" is to
hit "<Tab>" rather than "=". That gives you the same effect, with the
added feedback that you're using a recognized action.

I know not everybody will the "natural thing" I claim, and if it were
easy to support everywhere, I don't mind doing it. But I suspect
(without thinking very hard on it) that it would make those case
statements a bit harder to read and maintain.

-Peff

Re: [PATCH v3 2/2] completion: add git status

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I know not everybody will the "natural thing" I claim, and if it were
> easy to support everywhere, I don't mind doing it. But I suspect
> (without thinking very hard on it) that it would make those case
> statements a bit harder to read and maintain.

Oh, I agree with that 100%.  I didn't mean to suggest (let alone to
demand) to support the possible truncations.

I simply was hoping that Thomas would respond with your "For a user
who uses tab completion, it is natural to use --unt<TAB> so by the
time we use find-on-command-line, we can expect the fully-spelled
form" when I asked about "--unt=no"; that would give us a warm and
fuzzy confirmation that the patch author has thought things through
when designing the new feature.

Re: [PATCH v3 2/2] completion: add git status

[Thomas Braun]

Am 03.06.2016 um 19:54 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
> 
>> I know not everybody will the "natural thing" I claim, and if it were
>> easy to support everywhere, I don't mind doing it. But I suspect
>> (without thinking very hard on it) that it would make those case
>> statements a bit harder to read and maintain.
> 
> Oh, I agree with that 100%.  I didn't mean to suggest (let alone to
> demand) to support the possible truncations.
> 
> I simply was hoping that Thomas would respond with your "For a user
> who uses tab completion, it is natural to use --unt<TAB> so by the
> time we use find-on-command-line, we can expect the fully-spelled
> form" when I asked about "--unt=no"; that would give us a warm and
> fuzzy confirmation that the patch author has thought things through
> when designing the new feature.

I understood from Junio's comment [1]

> $ git status -uno <TAB>

that the question was about the short option version not about how
possible truncations are handled.

On the other side I must confess I did not think about the possibility
that the user truncates a long option as in --unt=all. Looking through
the completion file I have not found a place where the truncated
versions are supported.

[1]: http://article.gmane.org/gmane.comp.version-control.git/296220

[PATCH v4 0/3] support completion for git status

[Thomas Braun]

changes since v3:
* support short version -u of --untracked-files option
* introduce __git_get_option_value for general usage
* fix style issues
* support order dependent statements like
  git status -uno --untracked-files=all
  properly

Thomas Braun (3):
  completion: factor out untracked file modes into a variable
  completion: add __git_get_option_value helper
  completion: add git status

 contrib/completion/git-completion.bash | 98 +++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

-- 
2.8.3.windows.1

[PATCH v4 1/3] completion: factor out untracked file modes into a variable

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..addea89 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 
+__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)
-- 
2.8.3.windows.1

[PATCH v4 2/3] completion: add __git_get_option_value helper

[Thomas Braun]

This function allows to search the commmand line and config
files for an option, long and short, with mandatory value.

The function would return e.g. for the command line
"git status -uno --untracked-files=all" the result
"all" regardless of the config option.

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index addea89..4bd17aa 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -803,6 +803,50 @@ __git_find_on_cmdline ()
 	done
 }
 
+# Echo the value of an option set on the command line or config
+#
+# $1: short option name
+# $2: long option name including =
+# $3: list of possible values
+# $4: config string (optional)
+#
+# example:
+# result="$(__git_get_option_value "-d" "--do-something="\
+#     "yes no" "core.doSomething")"
+#
+# result is then either empty (no option set) or "yes" or "no"
+#
+# __git_get_option_value requires 3 arguments
+__git_get_option_value ()
+{
+	local c short_opt long_opt val
+	local result= values config_key word
+
+	short_opt="$1"
+	long_opt="$2"
+	values="$3"
+	config_key="$4"
+
+	((c = $cword - 1))
+	while [ $c -ge 0 ]; do
+		word="${words[c]}"
+		for val in $values; do
+			if [ "$short_opt$val" = "$word" ]
+			|| [ "$long_opt$val"  = "$word" ]; then
+				result="$val"
+				break 2
+			fi
+		done
+		((c--))
+	done
+
+	if [ -n "$config_key" ] && [ -z "$result" ]; then
+		result="$(git --git-dir="$(__gitdir)" config "$config_key")"
+	fi
+
+	echo "$result"
+}
+
 __git_has_doubledash ()
 {
 	local c=1
-- 
2.8.3.windows.1

[PATCH v4 3/3] completion: add git status

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4bd17aa..9eff33c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1826,6 +1826,56 @@ _git_stage ()
 	_git_add
 }
 
+_git_status ()
+{
+	local complete_opt
+	local untracked_state
+
+	case "$cur" in
+	--ignore-submodules=*)
+		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
+		return
+		;;
+	--untracked-files=*)
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
+		return
+		;;
+	--column=*)
+		__gitcomp "
+			always never auto column row plain dense nodense
+			" "" "${cur##--column=}"
+		return
+		;;
+	--*)
+		__gitcomp "
+			--short --branch --porcelain --long --verbose
+			--untracked-files= --ignore-submodules= --ignored
+			--column= --no-column
+			"
+		return
+		;;
+	esac
+
+	untracked_state="$(__git_get_option_value "-u" "--untracked-files="\
+		"$__git_untracked_file_modes" "status.showUntrackedFiles")"
+
+	case "$untracked_state" in
+	no)
+		# --ignored option does not matter
+		complete_opt=
+		;;
+	all|normal|*)
+		complete_opt="--cached --directory --no-empty-directory --others"
+
+		if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
+			complete_opt="$complete_opt --ignored --exclude=*"
+		fi
+		;;
+	esac
+
+	__git_complete_index_file "$complete_opt"
+}
+
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-- 
2.8.3.windows.1

Re: [PATCH v4 3/3] completion: add git status

[Junio C Hamano]

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:

> +	case "$cur" in
> +	--ignore-submodules=*)
> +		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
> +		return
> +		;;
> +	--untracked-files=*)
> +		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
> +		return
> +		;;
> +	--column=*)
> +		__gitcomp "
> +			always never auto column row plain dense nodense
> +			" "" "${cur##--column=}"
> +		return
> +		;;
> +	--*)
> +		__gitcomp "
> +			--short --branch --porcelain --long --verbose
> +			--untracked-files= --ignore-submodules= --ignored
> +			--column= --no-column
> +			"
> +		return
> +		;;
> +	esac
> +
> +	untracked_state="$(__git_get_option_value "-u" "--untracked-files="\

If you have a SP before that backslash, you can avoid getting
misunderstood that you are attempting to extend that string
"--untracked-files=".  The backslash is telling the shell that there
are more arguments to come, and it is misleading to rely on the fast
that the next line happens to begin with a whitespace.

> +		"$__git_untracked_file_modes" "status.showUntrackedFiles")"


> +	case "$untracked_state" in
> +	no)
> +		# --ignored option does not matter
> +		complete_opt=
> +		;;
> +	all|normal|*)
> +		complete_opt="--cached --directory --no-empty-directory --others"
> +
> +		if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
> +			complete_opt="$complete_opt --ignored --exclude=*"
> +		fi
> +		;;
> +	esac
> +
> +	__git_complete_index_file "$complete_opt"
> +}
> +
>  __git_config_get_set_variables ()
>  {
>  	local prevword word config_file= c=$cword

Re: [PATCH v4 0/3] support completion for git status

[Junio C Hamano]

Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:

> changes since v3:
> * support short version -u of --untracked-files option
> * introduce __git_get_option_value for general usage
> * fix style issues
> * support order dependent statements like
>   git status -uno --untracked-files=all
>   properly
>
> Thomas Braun (3):
>   completion: factor out untracked file modes into a variable
>   completion: add __git_get_option_value helper
>   completion: add git status
>
>  contrib/completion/git-completion.bash | 98 +++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 1 deletion(-)

Thanks.

An ack, suggestions to update, or a veto from SZEDER would carry
more weight than what I'd say here, but the three patches looked
alright.

Re: [PATCH v4 3/3] completion: add git status

[Thomas Braun]

Am 06.06.2016 um 19:57 schrieb Junio C Hamano:
> Thomas Braun <thomas.braun@virtuell-zuhause.de> writes:
> 
>> +	case "$cur" in
>> +	--ignore-submodules=*)
>> +		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
>> +		return
>> +		;;
>> +	--untracked-files=*)
>> +		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
>> +		return
>> +		;;
>> +	--column=*)
>> +		__gitcomp "
>> +			always never auto column row plain dense nodense
>> +			" "" "${cur##--column=}"
>> +		return
>> +		;;
>> +	--*)
>> +		__gitcomp "
>> +			--short --branch --porcelain --long --verbose
>> +			--untracked-files= --ignore-submodules= --ignored
>> +			--column= --no-column
>> +			"
>> +		return
>> +		;;
>> +	esac
>> +
>> +	untracked_state="$(__git_get_option_value "-u" "--untracked-files="\
> 
> If you have a SP before that backslash, you can avoid getting
> misunderstood that you are attempting to extend that string
> "--untracked-files=".  The backslash is telling the shell that there
> are more arguments to come, and it is misleading to rely on the fast
> that the next line happens to begin with a whitespace.

Thanks, will be fixed in the reroll.

>> +		"$__git_untracked_file_modes" "status.showUntrackedFiles")"
> 
> 
>> +	case "$untracked_state" in
>> +	no)
>> +		# --ignored option does not matter
>> +		complete_opt=
>> +		;;
>> +	all|normal|*)
>> +		complete_opt="--cached --directory --no-empty-directory --others"
>> +
>> +		if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
>> +			complete_opt="$complete_opt --ignored --exclude=*"
>> +		fi
>> +		;;
>> +	esac
>> +
>> +	__git_complete_index_file "$complete_opt"
>> +}
>> +
>>  __git_config_get_set_variables ()
>>  {
>>  	local prevword word config_file= c=$cword
> 

[PATCH v5 0/3] completion: add git status

[Thomas Braun]

Changes since v4:
- Add SP before backslash at EOL
- Fix line continuation issue in __git_get_option_value,
  now t9902 passes again

Thomas Braun (3):
  completion: factor out untracked file modes into a variable
  completion: add __git_get_option_value helper
  completion: add git status

 contrib/completion/git-completion.bash | 98 +++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)

-- 
2.8.4.windows.1

[PATCH v5 1/3] completion: factor out untracked file modes into a variable

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..addea89 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 
+__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)
-- 
2.8.4.windows.1

[PATCH v5 2/3] completion: add __git_get_option_value helper

[Thomas Braun]

This function allows to search the commmand line and config
files for an option, long and short, with mandatory value.

The function would return e.g. for the command line
"git status -uno --untracked-files=all" the result
"all" regardless of the config option.

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index addea89..0bf67c9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -803,6 +803,50 @@ __git_find_on_cmdline ()
 	done
 }
 
+# Echo the value of an option set on the command line or config
+#
+# $1: short option name
+# $2: long option name including =
+# $3: list of possible values
+# $4: config string (optional)
+#
+# example:
+# result="$(__git_get_option_value "-d" "--do-something=" \
+#     "yes no" "core.doSomething")"
+#
+# result is then either empty (no option set) or "yes" or "no"
+#
+# __git_get_option_value requires 3 arguments
+__git_get_option_value ()
+{
+	local c short_opt long_opt val
+	local result= values config_key word
+
+	short_opt="$1"
+	long_opt="$2"
+	values="$3"
+	config_key="$4"
+
+	((c = $cword - 1))
+	while [ $c -ge 0 ]; do
+		word="${words[c]}"
+		for val in $values; do
+			if [ "$short_opt$val" = "$word" ] ||
+			   [ "$long_opt$val"  = "$word" ]; then
+				result="$val"
+				break 2
+			fi
+		done
+		((c--))
+	done
+
+	if [ -n "$config_key" ] && [ -z "$result" ]; then
+		result="$(git --git-dir="$(__gitdir)" config "$config_key")"
+	fi
+
+	echo "$result"
+}
+
 __git_has_doubledash ()
 {
 	local c=1
-- 
2.8.4.windows.1

[PATCH v5 3/3] completion: add git status

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0bf67c9..ddda5e5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1826,6 +1826,56 @@ _git_stage ()
 	_git_add
 }
 
+_git_status ()
+{
+	local complete_opt
+	local untracked_state
+
+	case "$cur" in
+	--ignore-submodules=*)
+		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
+		return
+		;;
+	--untracked-files=*)
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
+		return
+		;;
+	--column=*)
+		__gitcomp "
+			always never auto column row plain dense nodense
+			" "" "${cur##--column=}"
+		return
+		;;
+	--*)
+		__gitcomp "
+			--short --branch --porcelain --long --verbose
+			--untracked-files= --ignore-submodules= --ignored
+			--column= --no-column
+			"
+		return
+		;;
+	esac
+
+	untracked_state="$(__git_get_option_value "-u" "--untracked-files=" \
+		"$__git_untracked_file_modes" "status.showUntrackedFiles")"
+
+	case "$untracked_state" in
+	no)
+		# --ignored option does not matter
+		complete_opt=
+		;;
+	all|normal|*)
+		complete_opt="--cached --directory --no-empty-directory --others"
+
+		if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
+			complete_opt="$complete_opt --ignored --exclude=*"
+		fi
+		;;
+	esac
+
+	__git_complete_index_file "$complete_opt"
+}
+
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-- 
2.8.4.windows.1

[PATCH v5 1/3] completion: factor out untracked file modes into a variable

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..addea89 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1098,6 +1098,8 @@ _git_clone ()
 	esac
 }
 
+__git_untracked_file_modes="all no normal"
+
 _git_commit ()
 {
 	case "$prev" in
@@ -1119,7 +1121,7 @@ _git_commit ()
 		return
 		;;
 	--untracked-files=*)
-		__gitcomp "all no normal" "" "${cur##--untracked-files=}"
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
 		return
 		;;
 	--*)
-- 
2.8.4.windows.1

[PATCH v5 3/3] completion: add git status

[Thomas Braun]

Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
---
 contrib/completion/git-completion.bash | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0bf67c9..ddda5e5 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1826,6 +1826,56 @@ _git_stage ()
 	_git_add
 }
 
+_git_status ()
+{
+	local complete_opt
+	local untracked_state
+
+	case "$cur" in
+	--ignore-submodules=*)
+		__gitcomp "none untracked dirty all" "" "${cur##--ignore-submodules=}"
+		return
+		;;
+	--untracked-files=*)
+		__gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
+		return
+		;;
+	--column=*)
+		__gitcomp "
+			always never auto column row plain dense nodense
+			" "" "${cur##--column=}"
+		return
+		;;
+	--*)
+		__gitcomp "
+			--short --branch --porcelain --long --verbose
+			--untracked-files= --ignore-submodules= --ignored
+			--column= --no-column
+			"
+		return
+		;;
+	esac
+
+	untracked_state="$(__git_get_option_value "-u" "--untracked-files=" \
+		"$__git_untracked_file_modes" "status.showUntrackedFiles")"
+
+	case "$untracked_state" in
+	no)
+		# --ignored option does not matter
+		complete_opt=
+		;;
+	all|normal|*)
+		complete_opt="--cached --directory --no-empty-directory --others"
+
+		if [ -n "$(__git_find_on_cmdline "--ignored")" ]; then
+			complete_opt="$complete_opt --ignored --exclude=*"
+		fi
+		;;
+	esac
+
+	__git_complete_index_file "$complete_opt"
+}
+
 __git_config_get_set_variables ()
 {
 	local prevword word config_file= c=$cword
-- 
2.8.4.windows.1

Re: [PATCH v4 2/3] completion: add __git_get_option_value helper

[SZEDER Gábor]

Hallo Thomas,

I saw v5 hit my mailbox while writing this.  I glanced it over and it
seems my comments here apply to that version as well.

Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:

> This function allows to search the commmand line and config
> files for an option, long and short, with mandatory value.
>
> The function would return e.g. for the command line
> "git status -uno --untracked-files=all" the result
> "all" regardless of the config option.

Wow, regarding my earlier remark about bonus points: I didn't realize
that there were so many bonus point to give away :)

> Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> ---
> contrib/completion/git-completion.bash | 44  
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash  
> b/contrib/completion/git-completion.bash
> index addea89..4bd17aa 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -803,6 +803,50 @@ __git_find_on_cmdline ()
> 	done
> }
>
> +# Echo the value of an option set on the command line or config
> +#
> +# $1: short option name
> +# $2: long option name including =

I'm not sure about requiring the '=', the function could just append
it as necessary.  More on this below.

> +# $3: list of possible values
> +# $4: config string (optional)

I don't understand why the list of possible values is necessary.

This function will be called when the caller wants to take different
actions based on different values, so the caller will process the
function's output with a case statement or an if-else chain, both of
which would be perfectly capable to ignore whatever invalid value the
user might have specified.  Therefore, I think this function doesn't
need the list of possible values, it should just return whatever value
it found after the option.

> +# example:
> +# result="$(__git_get_option_value "-d" "--do-something="\
> +#     "yes no" "core.doSomething")"
> +#
> +# result is then either empty (no option set) or "yes" or "no"
> +#
> +# __git_get_option_value requires 3 arguments
> +__git_get_option_value ()
> +{
> +	local c short_opt long_opt val
> +	local result= values config_key word
> +
> +	short_opt="$1"
> +	long_opt="$2"
> +	values="$3"
> +	config_key="$4"

These can be assigned when the variables are declared, saving a couple
of lines.

> +	((c = $cword - 1))
> +	while [ $c -ge 0 ]; do

Searching from the end of the command line, so even if someone were to
do a 'git status -uall -unormal -uno <TAB>', this would still do the
right thing.  Good!

However ;)
Just for fun imagine following:

       $ >-uno
       $ git status -- -uno <TAB>

'git status' treats that '-uno' after the doubledash as a filename,
but this function interprets it as an option, and on the subsequent
TAB the completion script won't list untracked files.

I'm tempted to say that this is such a pathological corner case that
it doesn't worth worrying about.

> +		word="${words[c]}"
> +		for val in $values; do

Without the possible values argument this inner loop could go away.

> +			if [ "$short_opt$val" = "$word" ]
> +			|| [ "$long_opt$val"  = "$word" ]; then
> +				result="$val"
> +				break 2

You could just 'echo "$val"' or rather ${word#$short_opt} and return
here ...

> +			fi
> +		done
> +		((c--))
> +	done
> +
> +	if [ -n "$config_key" ] && [ -z "$result" ]; then

... and that would make the second condition unnecessary here ...

> +		result="$(git --git-dir="$(__gitdir)" config "$config_key")"

... and this could just be a simple 'git config' execution, without
command substitution ...

> +	fi
> +
> +	echo "$result"

... and this echo could go away as well.

> +}
> +
> __git_has_doubledash ()
> {
> 	local c=1
> --
> 2.8.3.windows.1


However, I'm not sure we need or want this helper function _at the
moment_.  Yes, in general helper functions are good, and in this case
it makes _git_status() easier to follow, but it has some drawbacks,
too:

   - It has a single callsite: the upcoming _git_status().  No other
     existing case springs to mind where it could be used, i.e. where
     different values of an option would require different actions from
     the completion script.  Maybe we'll have one in the future, maybe
     not.

   - This function works only with the "stuck" form of options, i.e.
     '--opt=val' or '-oval', which is mostly sufficient in this case,
     because 'git status' understands only this form.  However, it
     doesn't work with "unstuck" options, i.e. '--opt val' or '-o val'.
     In many cases git supports only this "unstuck" form, and there are
     many cases where it supports both for a given option.  We can't know
     which form a future callsite might need, but requiring the '=' as
     part of the long option seems to paint us into a corner.

   - I wrote "mostly sufficient" above, because 'git status' does accept
     a valueless '-u|--untracked-files' option, too, e.g.:

       $ git config status.showUntrackedFiles no
       $ git status --untracked-files

     lists untracked files, therefore the completion script should list
     them as well.  Your function can't cope with this case, and I'm not
     sure how it and its caller could differentiate between the presence
     of such a valueless option and no option at all.  Perhaps with an
     additional optional function parameter holding the default value
     that should be echo-ed when a valueless option is encountered.

If this function were not a function but its logic were embedded into
_git_status(), then we wouldn't have to spend any effort _now_ to come
up with a proper calling convention that can cope with stuck vs.
unstuck vs. both forms of options and with valueless options.  We would
deal with all that and the necessary refactorization when (or if ever)
there's a second potential callsite.  Embedding into _git_status()
would give you more freedom to deal with the valueless '-u' option,
too.  If embedded, some of my in-code comments wouldn't apply anymore,
of course.

I'm in favor of crossing the bridge when we get there.


Gábor

Re: [PATCH v4 2/3] completion: add __git_get_option_value helper

[Thomas Braun]

> SZEDER Gábor <szeder@ira.uka.de> hat am 10. Juni 2016 um 15:10 geschrieben:
> 
> Hallo Thomas,
> 
> I saw v5 hit my mailbox while writing this.  I glanced it over and it
> seems my comments here apply to that version as well.

Hi Gábor,

thanks for your comments.
I plan to send a reroll in the near future adressing your remarks.

Bye,
Thomas

> Quoting Thomas Braun <thomas.braun@virtuell-zuhause.de>:
> 
> > This function allows to search the commmand line and config
> > files for an option, long and short, with mandatory value.
> >
> > The function would return e.g. for the command line
> > "git status -uno --untracked-files=all" the result
> > "all" regardless of the config option.
> 
> Wow, regarding my earlier remark about bonus points: I didn't realize
> that there were so many bonus point to give away :)
> 
> > Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de>
> > ---
> > contrib/completion/git-completion.bash | 44  
> > ++++++++++++++++++++++++++++++++++
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash  
> > b/contrib/completion/git-completion.bash
> > index addea89..4bd17aa 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -803,6 +803,50 @@ __git_find_on_cmdline ()
> > 	done
> > }
> >
> > +# Echo the value of an option set on the command line or config
> > +#
> > +# $1: short option name
> > +# $2: long option name including =
> 
> I'm not sure about requiring the '=', the function could just append
> it as necessary.  More on this below.
> 
> > +# $3: list of possible values
> > +# $4: config string (optional)
> 
> I don't understand why the list of possible values is necessary.
> 
> This function will be called when the caller wants to take different
> actions based on different values, so the caller will process the
> function's output with a case statement or an if-else chain, both of
> which would be perfectly capable to ignore whatever invalid value the
> user might have specified.  Therefore, I think this function doesn't
> need the list of possible values, it should just return whatever value
> it found after the option.
> 
> > +# example:
> > +# result="$(__git_get_option_value "-d" "--do-something="\
> > +#     "yes no" "core.doSomething")"
> > +#
> > +# result is then either empty (no option set) or "yes" or "no"
> > +#
> > +# __git_get_option_value requires 3 arguments
> > +__git_get_option_value ()
> > +{
> > +	local c short_opt long_opt val
> > +	local result= values config_key word
> > +
> > +	short_opt="$1"
> > +	long_opt="$2"
> > +	values="$3"
> > +	config_key="$4"
> 
> These can be assigned when the variables are declared, saving a couple
> of lines.
> 
> > +	((c = $cword - 1))
> > +	while [ $c -ge 0 ]; do
> 
> Searching from the end of the command line, so even if someone were to
> do a 'git status -uall -unormal -uno <TAB>', this would still do the
> right thing.  Good!
> 
> However ;)
> Just for fun imagine following:
> 
>        $ >-uno
>        $ git status -- -uno <TAB>
> 
> 'git status' treats that '-uno' after the doubledash as a filename,
> but this function interprets it as an option, and on the subsequent
> TAB the completion script won't list untracked files.
> 
> I'm tempted to say that this is such a pathological corner case that
> it doesn't worth worrying about.
> 
> > +		word="${words[c]}"
> > +		for val in $values; do
> 
> Without the possible values argument this inner loop could go away.
> 
> > +			if [ "$short_opt$val" = "$word" ]
> > +			|| [ "$long_opt$val"  = "$word" ]; then
> > +				result="$val"
> > +				break 2
> 
> You could just 'echo "$val"' or rather ${word#$short_opt} and return
> here ...
> 
> > +			fi
> > +		done
> > +		((c--))
> > +	done
> > +
> > +	if [ -n "$config_key" ] && [ -z "$result" ]; then
> 
> ... and that would make the second condition unnecessary here ...
> 
> > +		result="$(git --git-dir="$(__gitdir)" config "$config_key")"
> 
> ... and this could just be a simple 'git config' execution, without
> command substitution ...
> 
> > +	fi
> > +
> > +	echo "$result"
> 
> ... and this echo could go away as well.
> 
> > +}
> > +
> > __git_has_doubledash ()
> > {
> > 	local c=1
> > --
> > 2.8.3.windows.1
> 
> 
> However, I'm not sure we need or want this helper function _at the
> moment_.  Yes, in general helper functions are good, and in this case
> it makes _git_status() easier to follow, but it has some drawbacks,
> too:
> 
>    - It has a single callsite: the upcoming _git_status().  No other
>      existing case springs to mind where it could be used, i.e. where
>      different values of an option would require different actions from
>      the completion script.  Maybe we'll have one in the future, maybe
>      not.
> 
>    - This function works only with the "stuck" form of options, i.e.
>      '--opt=val' or '-oval', which is mostly sufficient in this case,
>      because 'git status' understands only this form.  However, it
>      doesn't work with "unstuck" options, i.e. '--opt val' or '-o val'.
>      In many cases git supports only this "unstuck" form, and there are
>      many cases where it supports both for a given option.  We can't know
>      which form a future callsite might need, but requiring the '=' as
>      part of the long option seems to paint us into a corner.
> 
>    - I wrote "mostly sufficient" above, because 'git status' does accept
>      a valueless '-u|--untracked-files' option, too, e.g.:
> 
>        $ git config status.showUntrackedFiles no
>        $ git status --untracked-files
> 
>      lists untracked files, therefore the completion script should list
>      them as well.  Your function can't cope with this case, and I'm not
>      sure how it and its caller could differentiate between the presence
>      of such a valueless option and no option at all.  Perhaps with an
>      additional optional function parameter holding the default value
>      that should be echo-ed when a valueless option is encountered.
> 
> If this function were not a function but its logic were embedded into
> _git_status(), then we wouldn't have to spend any effort _now_ to come
> up with a proper calling convention that can cope with stuck vs.
> unstuck vs. both forms of options and with valueless options.  We would
> deal with all that and the necessary refactorization when (or if ever)
> there's a second potential callsite.  Embedding into _git_status()
> would give you more freedom to deal with the valueless '-u' option,
> too.  If embedded, some of my in-code comments wouldn't apply anymore,
> of course.
> 
> I'm in favor of crossing the bridge when we get there.
> 
> 
> Gábor
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>