[BUG] git completion do sed on binary file

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>  > Bash completion needs to know what strategies git supports. Maybe
>  > other similar tools have the same demand. So add
>  > "git merge--show-strategies"
>
>             ^^
>
>  Silly detail, but a space is missing here.

>   > __git_merge_strategies ()
>   > {
>   >         if [ -n "$__git_merge_strategylist" ]; then
>   >                 echo "$__git_merge_strategylist"
>   >                 return
>   >         fi
>   >         sed -n "/^all_strategies='/{
>   >                 s/^all_strategies='//
>   >                 s/'//
>   >                 p
>   >                 q
>   >                 }" "$(git --exec-path)/git-merge"
>   > }
>   > 
>   > It takes several seconds to finish that function.

> diff --git a/builtin-merge.c b/builtin-merge.c
> index de025ac..613c96a 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -802,6 +802,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	const char *best_strategy = NULL, *wt_strategy = NULL;
>  	struct commit_list **remotes = &remoteheads;
>  
> +	/* needed for git bash completion and similar tools */
> +	if (argc == 2 && !strcmp(argv[1], "--show-strategies")) {
> +		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
> +			printf("%s\n", all_strategy[i].name);
> +		return 0;
> +	}
> +
>  	setup_work_tree();
>  	if (unmerged_cache())
>  		die("You are in the middle of a conflicted merge.");
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 158b912..1eea49a 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -271,12 +271,7 @@ __git_merge_strategies ()
>  		echo "$__git_merge_strategylist"
>  		return
>  	fi
> -	sed -n "/^all_strategies='/{
> -		s/^all_strategies='//
> -		s/'//
> -		p
> -		q
> -		}" "$(git --exec-path)/git-merge"
> +	$(git --exec-path)/git-merge --show-strategies
>  }
>  __git_merge_strategylist=
>  __git_merge_strategylist="$(__git_merge_strategies 2>/dev/null)"

> Nguyen Thai Ngoc Duy schrieb:
>> +	if (argc == 2 && !strcmp(argv[1], "--show-strategies")) {
>> +		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
>> +			printf("%s\n", all_strategy[i].name);
>> +		return 0;
> 
> Improved error checking, but quick and dirty:
> 
> +	if (!strcmp(argv[1], "--show-strategies")) {

> +		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
> +			printf("%s\n", all_strategy[i].name);
> +		return argc == 2 ? 0 :
> +			 error("--show-strategies does not take "
> +				"any arguments");

> Nguyen Thai Ngoc Duy schrieb:
>
> > +--show-strategies::
>  > +     Show all available strategies. For internal use only.
>  > +
>
>
> IMO, you don't need to declare this option as internal; offering it for
>  the public is fine...

> On 8/19/08, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Nguyen Thai Ngoc Duy schrieb:
>>
>> > +--show-strategies::
>>  > +     Show all available strategies. For internal use only.
>>  > +
>>
>>
>> IMO, you don't need to declare this option as internal; offering it for
>>  the public is fine...
>
> On second thought, I don't think the patch's worth it.

> completion: find out supported merge strategies correctly
> 
> "git-merge" is a binary executable these days, and looking for assignment
> to $all_strategies variable does not work well.
> 
> When asked for an unknown strategy, pre-1.6.0 and post-1.6.0 "git merge"
> commands respectively say:
> 
>     $ $HOME/git-snap-v1.5.6.5/bin/git merge -s help
>     available strategies are: recur recursive octopus resolve stupid ours subtree 
>     $ $HOME/git-snap-v1.6.0/bin/git merge -s help
>     Could not find merge strategy 'help'.
>     Available strategies are: recursive octopus resolve ours subtree.
> 
> both on its standard error stream.  We can use this to learn what
> strategies are supported.
> 
> The sed script is written in such a way that it catches both old and new
> message styles ("Available" vs "available", and the full stop at the end).
> It also allows future versions of "git merge" to line-wrap the list of
> strategies, and add extra comments, like this:
> 
>     $ $HOME/git-snap-v1.6.1/bin/git merge -s help
>     Could not find merge strategy 'help'.
>     Available strategies are: blame recursive octopus resolve ours
>     subtree.
>     Also you have custom strategies: theirs
> 
>     Make sure you spell strategy names correctly.

> On Wed, Aug 20, 2008 at 02:13:42PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>  > completion: find out supported merge strategies correctly
>  >
>  > "git-merge" is a binary executable these days, and looking for assignment
>  > to $all_strategies variable does not work well.
>  >
>  > When asked for an unknown strategy, pre-1.6.0 and post-1.6.0 "git merge"
>  > commands respectively say:
>  >
>  >     $ $HOME/git-snap-v1.5.6.5/bin/git merge -s help
>  >     available strategies are: recur recursive octopus resolve stupid ours subtree
>  >     $ $HOME/git-snap-v1.6.0/bin/git merge -s help
>  >     Could not find merge strategy 'help'.
>  >     Available strategies are: recursive octopus resolve ours subtree.
>  >
>  > both on its standard error stream.  We can use this to learn what
>  > strategies are supported.
>  >
>  > The sed script is written in such a way that it catches both old and new
>  > message styles ("Available" vs "available", and the full stop at the end).
>  > It also allows future versions of "git merge" to line-wrap the list of
>  > strategies, and add extra comments, like this:
>  >
>  >     $ $HOME/git-snap-v1.6.1/bin/git merge -s help
>  >     Could not find merge strategy 'help'.
>  >     Available strategies are: blame recursive octopus resolve ours
>  >     subtree.
>  >     Also you have custom strategies: theirs
>  >
>  >     Make sure you spell strategy names correctly.
>
>
> I like this, not adding something to builtin-merge when it's already
>  there. :)

> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
>> On 8/19/08, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Nguyen Thai Ngoc Duy schrieb:
>>>
>>> > +--show-strategies::
>>>  > +     Show all available strategies. For internal use only.
>>>  > +
>>>
>>>
>>> IMO, you don't need to declare this option as internal; offering it for
>>>  the public is fine...
>>
>> On second thought, I don't think the patch's worth it.
>
> How about doing this instead?
>
> -- >8 --
> completion: find out supported merge strategies correctly
>
> "git-merge" is a binary executable these days, and looking for assignment
> to $all_strategies variable does not work well.
>
> When asked for an unknown strategy, pre-1.6.0 and post-1.6.0 "git merge"
> commands respectively say:
>
>    $ $HOME/git-snap-v1.5.6.5/bin/git merge -s help
>    available strategies are: recur recursive octopus resolve stupid ours subtree
>    $ $HOME/git-snap-v1.6.0/bin/git merge -s help
>    Could not find merge strategy 'help'.
>    Available strategies are: recursive octopus resolve ours subtree.
>
> both on its standard error stream.  We can use this to learn what
> strategies are supported.
>
> The sed script is written in such a way that it catches both old and new
> message styles ("Available" vs "available", and the full stop at the end).
> It also allows future versions of "git merge" to line-wrap the list of
> strategies, and add extra comments, like this:
>
>    $ $HOME/git-snap-v1.6.1/bin/git merge -s help
>    Could not find merge strategy 'help'.
>    Available strategies are: blame recursive octopus resolve ours
>    subtree.
>    Also you have custom strategies: theirs
>
>    Make sure you spell strategy names correctly.
>
> ---
> diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
> index 158b912..a310040 100755
> --- c/contrib/completion/git-completion.bash
> +++ w/contrib/completion/git-completion.bash
> @@ -271,15 +271,17 @@ __git_merge_strategies ()
>                echo "$__git_merge_strategylist"
>                return
>        fi
> -       sed -n "/^all_strategies='/{
> -               s/^all_strategies='//
> -               s/'//
> +       git merge -s help 2>&1 |
> +       sed -n -e '/[Aa]vailable strategies are: /,/^$/{
> +               s/\.$//
> +               s/.*://
> +               s/^[    ]*//
> +               s/[     ]*$//
>                p
> -               q
> -               }" "$(git --exec-path)/git-merge"
> +       }'
>  }
>  __git_merge_strategylist=
> -__git_merge_strategylist="$(__git_merge_strategies 2>/dev/null)"
> +__git_merge_strategylist=$(__git_merge_strategies 2>/dev/null)
>
>  __git_complete_file ()
>  {

> "Mikael Magnusson" <mikachu@gmail.com> writes:
>
>> I don't know if i somehow have some weird patch merged in, i tried to check
>> and didn't find anything, at least. But when i run git merge -s help i get:
>
> Yes, there are some weird patch in 'next'.  Miklos, could you fix the
> output format of that thing?

> I do not think the code in 'next' that reuses the "help" thing to show
> only the list to stdout while still giving the error message to stderr
> makes any sense.
> 
> Let's do this.
> 
> ---
> 
>  builtin-merge.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git c/builtin-merge.c w/builtin-merge.c
> index 1f9389b..3e8db0d 100644
> --- c/builtin-merge.c
> +++ w/builtin-merge.c
> @@ -110,9 +110,17 @@ static struct strategy *get_strategy(const char *name)
>  		}
>  	}
>  	if (!is_in_cmdlist(&main_cmds, name) && !is_in_cmdlist(&other_cmds, name)) {
> -
> -		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
> -		list_commands("strategies", longest, &main_cmds, &other_cmds);
> +		fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
> +		fprintf(stderr, "Available strategies are:");
> +		for (i = 0; i < main_cmds.cnt; i++)
> +			fprintf(stderr, " %s", main_cmds.names[i]->name);
> +		fprintf(stderr, ".\n");
> +		if (other_cmds.cnt) {
> +			fprintf(stderr, "Available custom strategies are:");
> +			for (i = 0; i < other_cmds.cnt; i++)
> +				fprintf(stderr, " %s", other_cmds.names[i]->name);
> +			fprintf(stderr, ".\n");
> +		}
>  		exit(1);
>  	}