git completion do sed on binary file

BUG In review Nguyen Thai Ngoc Duy 7 participants 16 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Nguyen Thai Ngoc Duy

author

2016-06-15 22:45 UTC #0 lore raw

Probably missed since git-merge builtin effort:

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

Signature · 1 line

Duy

Nguyen Thai Ngoc Duy

author

2016-06-15 22:45 UTC #1 lore raw

PATCH 1/1 bash-completion: fix getting strategy list

Bash completion needs to know what strategies git supports. Maybe other similar tools have the same demand. So add
"git merge--show-strategies"

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---

Quoted 16 lines · Nguyen Thai Ngoc Duy

Probably missed since git-merge builtin effort:

__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"
}

Quoted 1 line

It takes several seconds to finish that function.

Maybe something like this?

 Documentation/git-merge.txt            |    4 ++++
 builtin-merge.c                        |    7 +++++++
 contrib/completion/git-completion.bash |    7 +------
 3 files changed, 12 insertions(+), 6 deletions(-)

Documentation/git-merge.txt +4 -0

index 17a15ac..f3fe1c9 100644
@@ -12,6 +12,7 @@ SYNOPSIS
 'git merge' [-n] [--stat] [--no-commit] [--squash] [-s <strategy>]...
 	[-m <msg>] <remote> <remote>...
 'git merge' <msg> HEAD <remote>...
+'git merge' --show-strategies
 
 DESCRIPTION
 -----------

@@ -37,6 +38,9 @@ include::merge-options.txt[]
 	least one <remote>.  Specifying more than one <remote>
 	obviously means you are trying an Octopus.
 
+--show-strategies::
+	Show all available strategies. For internal use only.
+
 include::merge-strategies.txt[]

builtin-merge.c +7 -0

index de025ac..613c96a 100644
@@ -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.");

contrib/completion/git-completion.bash +1 -6

index 158b912..1eea49a 100755
@@ -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)"

Signature · 1 line

1.6.0.96.g2fad1.dirty

Shawn O. Pearce

2016-06-15 22:45 UTC #2 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 14 lines · Nguyen Thai Ngoc Duy

Quoted 14 lines

__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"
}

Quoted 1 line

Quoted 1 line
It takes several seconds to finish that function.

Youch.

Quoted 1 line

Maybe something like this?

Yea, this looks reasonable. ACK from me, though the bigger change is to builtin-merge and not the completion. Miklos? Dscho?

Quoted 36 lines

builtin-merge.c +7 -0

index de025ac..613c96a 100644
@@ -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.");

contrib/completion/git-completion.bash +1 -6

index 158b912..1eea49a 100755
@@ -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)"

Signature · 1 line

Shawn.

Johannes Sixt

2016-06-15 22:45 UTC #3 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

In reply to Nguyen Thai Ngoc Duy

Documentation/git-merge.txt include::merge-options.txt[] · line 43

Quoted 3 lines · Nguyen Thai Ngoc Duy

+--show-strategies::
Documentation/git-merge.txt +2 -0
+	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...

builtin-merge.c int cmd_merge(int argc, const char **argv, const char *prefix) · line 805

Quoted 1 line

builtin-merge.c +1 -0

+	/* needed for git bash completion and similar tools */

... which would make this comment slightly odd.

builtin-merge.c int cmd_merge(int argc, const char **argv, const char *prefix) · line 809

Quoted 4 lines

builtin-merge.c +4 -0

+	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");

contrib/completion/git-completion.bash __git_merge_strategies () · line 274

Quoted 1 line

contrib/completion/git-completion.bash +1 -0
+	$(git --exec-path)/git-merge --show-strategies

+ git merge --show-strategies

-- Hannes

Johannes Sixt

2016-06-15 22:45 UTC #4 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Review

Quoted 9 lines · Johannes Sixt

builtin-merge.c int cmd_merge(int argc, const char **argv, const char *prefix) · line 809
Quoted 4 lines · Nguyen Thai Ngoc Duy
builtin-merge.c +4 -0
+	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:
(quoted diff) +1 -0
+	if (!strcmp(argv[1], "--show-strategies")) {

Oops, not really improved. This still needs to check for argc >= 2.

Review

Quoted 5 lines

(quoted diff) +5 -0

+		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");

-- Hannes

Nguyen Thai Ngoc Duy

author

2016-06-15 22:45 UTC #5 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

In reply to Johannes Sixt

Quoted 9 lines · Johannes Sixt

Review
Quoted 3 lines · Nguyen Thai Ngoc Duy
+--show-strategies::
(quoted diff) +2 -0
+     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. The code in git-completion.bash is a hack and I replace it with another the hack. It won't work for custom merges and git-completion.bash will need to be synced manually anyway, so maybe this patch will do better:

(quoted diff) +0 -0

b/contrib/completion/git-completion.bash
index 158b912..2fed6ac 100755

contrib/completion/git-completion.bash +2 ~~-13~~

@@ -267,19 +267,8 @@ __git_remotes ()
 
 __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"
-}
-__git_merge_strategylist=
-__git_merge_strategylist="$(__git_merge_strategies 2>/dev/null)"
+	echo recursive octopus resolve ours subtree
+}
 
 __git_complete_file ()
 {

Signature · 1 line

Duy

Junio C Hamano

2016-06-15 22:45 UTC #6 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 4 lines · Nguyen Thai Ngoc Duy

On second thought, I don't think the patch's worth it. The code in git-completion.bash is a hack and I replace it with another the hack. It won't work for custom merges and git-completion.bash will need to be synced manually anyway, so maybe this patch will do better:

This would be the right thing to do for the 'maint' track. In the longer term, especially if we are to actually make the "custom" thing official, I think teaching "git-merge" to report what are available is the only sensible approach, though.

Junio C Hamano

2016-06-15 22:45 UTC #7 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

In reply to Nguyen Thai Ngoc Duy

Quoted 4 lines · Nguyen Thai Ngoc Duy

Quoted 3 lines · Johannes Sixt
Quoted 1 line · Nguyen Thai Ngoc Duy
+--show-strategies::

Quoted 8 lines

Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+     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.

---

contrib/completion/git-completion.bash +8 -6

index 158b912..a310040 100755
@@ -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 ()
 {

Miklos Vajna

2016-06-15 22:45 UTC #8 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 22 lines · Junio C Hamano

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:

Quoted 7 lines

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

author

2016-06-15 22:45 UTC #9 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 29 lines · Miklos Vajna

Quoted 28 lines · Junio C Hamano
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

Quoted 5 lines

Quoted 1 line
Make sure you spell strategy names correctly.
I like this, not adding something to builtin-merge when it's already
there. :)

Me too. Did not realize it's already there :P

Signature · 1 line

Duy

Mikael Magnusson

2016-06-15 22:45 UTC #10 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

In reply to Junio C Hamano

2008/8/20 Junio C Hamano <[email protected]>:

Quoted 67 lines

Quoted 4 lines · Nguyen Thai Ngoc Duy
Quoted 3 lines · Johannes Sixt
Quoted 1 line · Nguyen Thai Ngoc Duy
+--show-strategies::
Quoted 8 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+     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.
---
contrib/completion/git-completion.bash +6 -3
index 158b912..a310040 100755
@@ -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

contrib/completion/git-completion.bash __git_merge_strategies () · line 287

Quoted 10 lines

contrib/completion/git-completion.bash +2 -3

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

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:

Could not find merge strategy 'help'.

available strategies in '/usr/libexec/git-core'
-----------------------------------------------
file octopus ours recursive resolve subtree

which i suspect does not work with your regex.

Signature · 1 line

Mikael Magnusson

Junio C Hamano

2016-06-15 22:45 UTC #11 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 2 lines · Mikael Magnusson

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?

Junio C Hamano

2016-06-15 22:45 UTC #12 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 7 lines · Junio C Hamano

Quoted 2 lines · Mikael Magnusson
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(-)

builtin-merge.c +11 -3

index 1f9389b..3e8db0d 100644
@@ -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);
 	}

Miklos Vajna

2016-06-15 22:45 UTC #13 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 2 lines · Junio C Hamano

Quoted 2 lines
Yes, there are some weird patch in 'next'. Miklos, could you fix the output format of that thing?

You were faster. ;-)

Quoted 25 lines

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

builtin-merge.c +3 -3

index 1f9389b..3e8db0d 100644
@@ -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++)

builtin-merge.c static struct strategy *get_strategy(const char *name) · line 125

Quoted 10 lines

builtin-merge.c +8 -0

+			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);
 	}

Given that we don't have 100+ merge strategies like we have 100+ git commands, I think printing them in one line should not be problematic, and this definitely improves script usability, so I like it, thanks.

Matthieu Moy

2016-06-15 22:45 UTC #14 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

In reply to Nguyen Thai Ngoc Duy

Quoted 3 lines · Nguyen Thai Ngoc Duy

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.

Signature · 1 line

Matthieu

Nguyen Thai Ngoc Duy

author

2016-06-15 22:45 UTC #15 lore raw

Re: [PATCH] bash-completion: fix getting strategy list

Quoted 9 lines · Matthieu Moy

Quoted 3 lines · Nguyen Thai Ngoc Duy
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.

Yes, my poor eyes. Junio if you accept the patch, please add one more space. Thanks :)

Signature · 1 line

Duy