* [PATCH] rebase -i: respect core.abbrev for real
@ 2015-01-19 14:20 Kirill A. Shutemov
2015-01-19 22:43 ` Eric Sunshine
0 siblings, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-01-19 14:20 UTC (permalink / raw)
To: gitster, git; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] rebase -i: respect core.abbrev for real
2015-01-19 14:20 [PATCH] rebase -i: respect core.abbrev for real Kirill A. Shutemov
@ 2015-01-19 22:43 ` Eric Sunshine
2015-01-20 10:42 ` [PATCHv2] " Kirill A. Shutemov
2015-01-22 11:50 ` [PATCHv3] " Kirill A. Shutemov
0 siblings, 2 replies; 8+ messages in thread
From: Eric Sunshine @ 2015-01-19 22:43 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Junio C Hamano, Git List
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] rebase -i: respect core.abbrev for real
2015-01-19 22:43 ` Eric Sunshine
@ 2015-01-20 10:42 ` Kirill A. Shutemov
2015-01-22 6:56 ` Junio C Hamano
2015-01-22 11:50 ` [PATCHv3] " Kirill A. Shutemov
1 sibling, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-01-20 10:42 UTC (permalink / raw)
To: Eric Sunshine, gitster, git; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv2] rebase -i: respect core.abbrev for real
2015-01-20 10:42 ` [PATCHv2] " Kirill A. Shutemov
@ 2015-01-22 6:56 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-01-22 6:56 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Eric Sunshine, git
"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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv3] rebase -i: respect core.abbrev for real
2015-01-19 22:43 ` Eric Sunshine
2015-01-20 10:42 ` [PATCHv2] " Kirill A. Shutemov
@ 2015-01-22 11:50 ` Kirill A. Shutemov
2015-01-22 20:10 ` Junio C Hamano
2015-01-22 20:19 ` RFC "grep '...\{m,n\}"? Junio C Hamano
1 sibling, 2 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2015-01-22 11:50 UTC (permalink / raw)
To: Eric Sunshine, gitster, git; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv3] rebase -i: respect core.abbrev for real
2015-01-22 11:50 ` [PATCHv3] " Kirill A. Shutemov
@ 2015-01-22 20:10 ` Junio C Hamano
2015-01-22 20:19 ` RFC "grep '...\{m,n\}"? Junio C Hamano
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-01-22 20:10 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Eric Sunshine, git
"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
^ permalink raw reply [flat|nested] 8+ messages in thread
* RFC "grep '...\{m,n\}"?
2015-01-22 11:50 ` [PATCHv3] " Kirill A. Shutemov
2015-01-22 20:10 ` Junio C Hamano
@ 2015-01-22 20:19 ` Junio C Hamano
2015-01-23 0:39 ` Duy Nguyen
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2015-01-22 20:19 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Kirill A. Shutemov
"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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: RFC "grep '...\{m,n\}"?
2015-01-22 20:19 ` RFC "grep '...\{m,n\}"? Junio C Hamano
@ 2015-01-23 0:39 ` Duy Nguyen
0 siblings, 0 replies; 8+ messages in thread
From: Duy Nguyen @ 2015-01-23 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Eric Sunshine, Kirill A. Shutemov
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-01-23 0:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-19 14:20 [PATCH] rebase -i: respect core.abbrev for real Kirill A. Shutemov
2015-01-19 22:43 ` Eric Sunshine
2015-01-20 10:42 ` [PATCHv2] " Kirill A. Shutemov
2015-01-22 6:56 ` Junio C Hamano
2015-01-22 11:50 ` [PATCHv3] " Kirill A. Shutemov
2015-01-22 20:10 ` Junio C Hamano
2015-01-22 20:19 ` RFC "grep '...\{m,n\}"? Junio C Hamano
2015-01-23 0:39 ` Duy Nguyen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.