[PATCH] rebase -i: respect core.abbrev for real

[PATCH] rebase -i: respect core.abbrev for real

[Kirill A. Shutemov]

I have tried to fix this before: see 568950388be2, but it doesn't
really work.

I don't know how it happend, but that commit makes interactive rebase to
respect core.abbrev only during --edit-todo, but not the initial todo
list edit.

For this time I've included a test-case to avoid this frustration again.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 git-rebase--interactive.sh    | 4 ++--
 t/t3404-rebase-interactive.sh | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6a4629cbc2b..1855e12f1ada 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -962,7 +962,7 @@ else
 	shortrevisions=$shorthead
 fi
 git rev-list $merges_option --pretty=oneline --abbrev-commit \
-	--abbrev=7 --reverse --left-right --topo-order \
+	--reverse --left-right --topo-order \
 	$revisions ${restrict_revision+^$restrict_revision} | \
 	sed -n "s/^>//p" |
 while read -r shortsha1 rest
@@ -1020,7 +1020,7 @@ then
 			# just the history of its first-parent for others that will
 			# be rebasing on top of it
 			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
-			short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
+			short=$(git rev-list -1 --abbrev-commit $rev)
 			sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
 			rm "$rewritten"/$rev
 		fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8197ed29a9ec..a8ffc24ce46b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'respect core.abbrev' '
+	git config core.abbrev 12 &&
+	set_cat_todo_editor &&
+	test_must_fail git rebase -i HEAD~4 >todo-list 2>&1
+	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
+'
+
 test_done
-- 
2.1.4

Re: [PATCH] rebase -i: respect core.abbrev for real

[Eric Sunshine]

On Mon, Jan 19, 2015 at 9:20 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> I have tried to fix this before: see 568950388be2, but it doesn't
> really work.
>
> I don't know how it happend, but that commit makes interactive rebase to
> respect core.abbrev only during --edit-todo, but not the initial todo
> list edit.
>
> For this time I've included a test-case to avoid this frustration again.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed29a9ec..a8ffc24ce46b 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
>         )
>  '
>
> +test_expect_success 'respect core.abbrev' '
> +       git config core.abbrev 12 &&
> +       set_cat_todo_editor &&
> +       test_must_fail git rebase -i HEAD~4 >todo-list 2>&1

Broken &&-chain.

> +       test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
> +'
> +
>  test_done
> --
> 2.1.4

[PATCHv2] rebase -i: respect core.abbrev for real

[Kirill A. Shutemov]

I have tried to fix this before: see 568950388be2, but it doesn't
really work.

I don't know how it happend, but that commit makes interactive rebase to
respect core.abbrev only during --edit-todo, but not the initial todo
list edit.

For this time I've included a test-case to avoid this frustration again.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 v2: fix &&-chain in the test-case
---
 git-rebase--interactive.sh    | 4 ++--
 t/t3404-rebase-interactive.sh | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6a4629cbc2b..1855e12f1ada 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -962,7 +962,7 @@ else
 	shortrevisions=$shorthead
 fi
 git rev-list $merges_option --pretty=oneline --abbrev-commit \
-	--abbrev=7 --reverse --left-right --topo-order \
+	--reverse --left-right --topo-order \
 	$revisions ${restrict_revision+^$restrict_revision} | \
 	sed -n "s/^>//p" |
 while read -r shortsha1 rest
@@ -1020,7 +1020,7 @@ then
 			# just the history of its first-parent for others that will
 			# be rebasing on top of it
 			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
-			short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
+			short=$(git rev-list -1 --abbrev-commit $rev)
 			sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
 			rm "$rewritten"/$rev
 		fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8197ed29a9ec..a31f7e0430e1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'respect core.abbrev' '
+	git config core.abbrev 12 &&
+	set_cat_todo_editor &&
+	test_must_fail git rebase -i HEAD~4 >todo-list &&
+	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
+'
+
 test_done
-- 
2.1.4

Re: [PATCHv2] rebase -i: respect core.abbrev for real

[Junio C Hamano]

"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> I have tried to fix this before: see 568950388be2, but it doesn't
> really work.
>
> I don't know how it happend, but that commit makes interactive rebase to
> respect core.abbrev only during --edit-todo, but not the initial todo
> list edit.
>
> For this time I've included a test-case to avoid this frustration again.

I suspect that the work that introduced expand/collapse could have
gone one step further so that all of the short object names you are
touching in this patch can stay in the full 40-hex name to avoid
ambiguity.  That is, keep the internal representation always use the
full object name, and then always call these three:

    collapse_todo_ids 
    git_sequence_editor
    expand_todo_ids

in this order.

Wouldn't that make the end result a lot cleaner, I wonder?


>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  v2: fix &&-chain in the test-case
> ---
>  git-rebase--interactive.sh    | 4 ++--
>  t/t3404-rebase-interactive.sh | 7 +++++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index c6a4629cbc2b..1855e12f1ada 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -962,7 +962,7 @@ else
>  	shortrevisions=$shorthead
>  fi
>  git rev-list $merges_option --pretty=oneline --abbrev-commit \
> -	--abbrev=7 --reverse --left-right --topo-order \
> +	--reverse --left-right --topo-order \
>  	$revisions ${restrict_revision+^$restrict_revision} | \
>  	sed -n "s/^>//p" |
>  while read -r shortsha1 rest
> @@ -1020,7 +1020,7 @@ then
>  			# just the history of its first-parent for others that will
>  			# be rebasing on top of it
>  			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
> -			short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
> +			short=$(git rev-list -1 --abbrev-commit $rev)
>  			sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
>  			rm "$rewritten"/$rev
>  		fi
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed29a9ec..a31f7e0430e1 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
>  	)
>  '
>  
> +test_expect_success 'respect core.abbrev' '
> +	git config core.abbrev 12 &&
> +	set_cat_todo_editor &&
> +	test_must_fail git rebase -i HEAD~4 >todo-list &&
> +	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
> +'
> +
>  test_done

[PATCHv3] rebase -i: respect core.abbrev for real

[Kirill A. Shutemov]

I have tried to fix this before: see 568950388be2, but it doesn't
really work.

I don't know how it happend, but that commit makes interactive rebase to
respect core.abbrev only during --edit-todo, but not the initial todo
list edit.

For this time I've included a test-case to avoid this frustration again.

The patch change code to use full 40-hex revision ids for todo actions
everywhere and collapse them only to show to user.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 v3:
    - use full 40-hex revision ids for todo actions everywhere and collapse them
      only to show to user;
 v2:
    - fix &&-chain in the test-case
---
 git-rebase--interactive.sh    | 17 ++++++++---------
 t/t3404-rebase-interactive.sh |  7 +++++++
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6a4629cbc2b..c96b9847e9fc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -961,14 +961,13 @@ else
 	revisions=$onto...$orig_head
 	shortrevisions=$shorthead
 fi
-git rev-list $merges_option --pretty=oneline --abbrev-commit \
-	--abbrev=7 --reverse --left-right --topo-order \
+git rev-list $merges_option --pretty=oneline --reverse --left-right --topo-order \
 	$revisions ${restrict_revision+^$restrict_revision} | \
 	sed -n "s/^>//p" |
-while read -r shortsha1 rest
+while read -r sha1 rest
 do
 
-	if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
+	if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
 	then
 		comment_out="$comment_char "
 	else
@@ -977,9 +976,8 @@ do
 
 	if test t != "$preserve_merges"
 	then
-		printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+		printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
 	else
-		sha1=$(git rev-parse $shortsha1)
 		if test -z "$rebase_root"
 		then
 			preserve=t
@@ -996,7 +994,7 @@ do
 		if test f = "$preserve"
 		then
 			touch "$rewritten"/$sha1
-			printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+			printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
 		fi
 	fi
 done
@@ -1020,8 +1018,8 @@ then
 			# just the history of its first-parent for others that will
 			# be rebasing on top of it
 			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
-			short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
-			sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+			sha1=$(git rev-list -1 $rev)
+			sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
 			rm "$rewritten"/$rev
 		fi
 	done
@@ -1054,6 +1052,7 @@ has_action "$todo" ||
 	return 2
 
 cp "$todo" "$todo".backup
+collapse_todo_ids
 git_sequence_editor "$todo" ||
 	die_abort "Could not execute editor"
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8197ed29a9ec..a31f7e0430e1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'respect core.abbrev' '
+	git config core.abbrev 12 &&
+	set_cat_todo_editor &&
+	test_must_fail git rebase -i HEAD~4 >todo-list &&
+	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
+'
+
 test_done
-- 
2.1.4

Re: [PATCHv3] rebase -i: respect core.abbrev for real

[Junio C Hamano]

"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> I have tried to fix this before: see 568950388be2, but it doesn't
> really work.
>
> I don't know how it happend, but that commit makes interactive rebase to
> respect core.abbrev only during --edit-todo, but not the initial todo
> list edit.
>
> For this time I've included a test-case to avoid this frustration again.
>
> The patch change code to use full 40-hex revision ids for todo actions
> everywhere and collapse them only to show to user.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  v3:
>     - use full 40-hex revision ids for todo actions everywhere and collapse them
>       only to show to user;

> @@ -1054,6 +1052,7 @@ has_action "$todo" ||
>  	return 2
>  
>  cp "$todo" "$todo".backup
> +collapse_todo_ids
>  git_sequence_editor "$todo" ||
>  	die_abort "Could not execute editor"
>  

OK, the matching expand_todo_ids is just beyond the horizon of this
patch context.

I was hoping that with this change we internally operate with the
full object names throughout the program, only to show shortened
ones in the editor, but I still see a handful of "rev-parse --short"
outside collapse_todo_ids.  Upon closer inspection, it turns out
that they are only about formatting "# Rebase a..b onto c", which is
never rewritten in transform/collapse/expand_todo_ids, so I think
all is well.

Thanks.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed29a9ec..a31f7e0430e1 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
>  	)
>  '
>  
> +test_expect_success 'respect core.abbrev' '
> +	git config core.abbrev 12 &&
> +	set_cat_todo_editor &&
> +	test_must_fail git rebase -i HEAD~4 >todo-list &&
> +	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
> +'
> +
>  test_done

RFC "grep '...\{m,n\}"?

[Junio C Hamano]

"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed29a9ec..a31f7e0430e1 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
>  	)
>  '
>  
> +test_expect_success 'respect core.abbrev' '
> +	git config core.abbrev 12 &&
> +	set_cat_todo_editor &&
> +	test_must_fail git rebase -i HEAD~4 >todo-list &&
> +	test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
> +'

Documentation/CodingGuidelines says

 - As to use of grep, stick to a subset of BRE (namely, no \{m,n\},
   [::], [==], or [..]) for portability.

   - We do not use \{m,n\};

   - We do not use -E;

   - We do not use ? or + (which are \{0,1\} and \{1,\}
     respectively in BRE) but that goes without saying as these
     are ERE elements not BRE (note that \? and \+ are not even part
     of BRE -- making them accessible from BRE is a GNU extension).

but I see we have multiple hits from "git grep 'grep .*\\{'" (all in
the t/ directory).  I wonder

 - if everybody's system is now OK with \{m,n\} these days, or

 - there are people who are grateful that we stayed away from using
   \{m,n\} but they are not running the tests because their system
   is too exotic to pass other parts of the test suite.

If the former, we may want to drop the \{m,n\} from the forbidden
list.

Re: RFC "grep '...\{m,n\}"?

[Duy Nguyen]

On Fri, Jan 23, 2015 at 3:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 8197ed29a9ec..a31f7e0430e1 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1039,4 +1039,11 @@ test_expect_success 'short SHA-1 collide' '
>>       )
>>  '
>>
>> +test_expect_success 'respect core.abbrev' '
>> +     git config core.abbrev 12 &&
>> +     set_cat_todo_editor &&
>> +     test_must_fail git rebase -i HEAD~4 >todo-list &&
>> +     test 4 = $(grep -c "pick [0-9a-f]\{12,\}" todo-list)
>> +'
>
> Documentation/CodingGuidelines says
>
>  - As to use of grep, stick to a subset of BRE (namely, no \{m,n\},
>    [::], [==], or [..]) for portability.
>
>    - We do not use \{m,n\};
>
>    - We do not use -E;
>
>    - We do not use ? or + (which are \{0,1\} and \{1,\}
>      respectively in BRE) but that goes without saying as these
>      are ERE elements not BRE (note that \? and \+ are not even part
>      of BRE -- making them accessible from BRE is a GNU extension).
>
> but I see we have multiple hits from "git grep 'grep .*\\{'" (all in
> the t/ directory).  I wonder
>
>  - if everybody's system is now OK with \{m,n\} these days, or
>
>  - there are people who are grateful that we stayed away from using
>    \{m,n\} but they are not running the tests because their system
>    is too exotic to pass other parts of the test suite.

Can we switch to using git-grep in the test suite and avoid these
platform issues? Or maybe switch to test-grep.c that is a wrapper of
compat regex to make sure the same feature set is used across
platforms.
-- 
Duy