Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:
-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+''|auto)
+ comment_char=#
+ ;;
+?)
+ ;;
+*)
+ comment_char=$(comment_char | cut -c1)
comment_char is a command? Did you mean
comment_char=$(echo "$comment_char" | cut -c1)
It could be written without forking a process:
comment_char=${comment_char%${comment_char#?}}
(aka "remove from the end what remains after removing the first character")
-- Hannes
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Johannes Sixt writes:
> comment_char is a command? Did you mean
>
> comment_char=$(echo "$comment_char" | cut -c1)
;-)
> It could be written without forking a process:
>
> comment_char=${comment_char%${comment_char#?}}
>
> (aka "remove from the end what remains after removing the first character")
Hopefully nobody would include any glob metacharacters in there,
e.g. "core.commentchar='=*'", which would break that?
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
Johannes Sixt writes:
It could be written without forking a process:
comment_char=${comment_char%${comment_char#?}}
(aka "remove from the end what remains after removing the first character")
Hopefully nobody would include any glob metacharacters in there,
e.g. "core.commentchar='=*'", which would break that?
Heh. I tested a few variations, but not this one. Make it
comment_char=${comment_char%"${comment_char#?}"}
;)
-- Hannes
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Johannes Sixt writes:
> Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
>> Johannes Sixt writes:
>>> It could be written without forking a process:
>>>
>>> comment_char=${comment_char%${comment_char#?}}
>>>
>>> (aka "remove from the end what remains after removing the first character")
>>
>> Hopefully nobody would include any glob metacharacters in there,
>> e.g. "core.commentchar='=*'", which would break that?
>
> Heh. I tested a few variations, but not this one. Make it
>
> comment_char=${comment_char%"${comment_char#?}"}
I tried a few implementations of shells, and decided that this is
not worth the portability hassle.
Setting comment-char to multi-letter sequence is not supported at
all, and this code is protecting itself from that nonsense---it does
not need to be fast, and Dscho already gives a fast path for a
single letter case in his patch.
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Am 21.11.2016 um 20:07 schrieb Junio C Hamano: Setting comment-char to multi-letter sequence is not supported at all, and this code is protecting itself from that nonsense---it does not need to be fast, and Dscho already gives a fast path for a single letter case in his patch. Fair enough, no problem. -- Hannes
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Junio C Hamano writes:
> @@ -93,8 +93,17 @@ eval '
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +'' | auto)
> + comment_char="#"
> + ;;
> +?)
> + ;;
> +*)
> + comment_char=$(echo "$comment_char" | cut -c1)
> + ;;
> +esac
Amended in is a fix for a typo the other Johannes noticed.
Thanks.
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d941f0a69f..5d0a7dca9d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar'
> '
> test B = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
>
> -test_expect_failure 'rebase -i respects core.commentchar=auto' '
> +test_expect_success 'rebase -i respects core.commentchar=auto' '
> test_config core.commentchar auto &&
> write_script copy-edit-script.sh <<-\EOF &&
> cp "$1" edit-script
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
wrote:
> When 84c9dc2 (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17) extended the core.commentChar functionality to
> allow for the value 'auto', it forgot that rebase -i was already taught to
> handle core.commentChar,
I think this is 180bad3 (rebase -i: respect core.commentchar -
2013-02-11) and a couple followup fixes. My bad.
> and in turn forgot to let rebase -i handle that
> new value gracefully.
>
> Reported by Taufiq Hoven.
Another report in the same area: a merge conflict always writes the
"Conflicts:" section in the commit message with '#' as comment char
when core.commentChar is auto. I've known this for a while, but it has
not bitten me hard enough to do something about it,
>
> Signed-off-by: Johannes Schindelin
> ---
> git-rebase--interactive.sh| 13 +++--
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ca994c5..6bb740c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -93,8 +93,17 @@ eval '
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +''|auto)
> + comment_char=#
My first thought was "this kinda defeats the purpose of auto, doesn't
it". But 'auto' here is irrelevant because we control the leading
character of all lines in the todo file. 'auto' makes little sense in
this context, falling back to any comment char would do.
So, ack (after you fix the $(comment_char other people have mentioned,
of course).
--
Duy
Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
Hi Hannes,
On Mon, 21 Nov 2016, Johannes Sixt wrote:
> Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:
> > -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> > -: ${comment_char:=#}
> > +comment_char=$(git config --get core.commentchar 2>/dev/null)
> > +case "$comment_char" in
> > +''|auto)
> > + comment_char=#
> > + ;;
> > +?)
> > + ;;
> > +*)
> > + comment_char=$(comment_char | cut -c1)
>
> comment_char is a command? Did you mean
>
> comment_char=$(echo "$comment_char" | cut -c1)
D'oh.
> It could be written without forking a process:
>
> comment_char=${comment_char%${comment_char#?}}
>
> (aka "remove from the end what remains after removing the first character")
I was considering this, actually, but it is rather unreadable. Better
rewrite it in C, to begin with.
Thanks,
Dscho
