* Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar
From: Junio C Hamano @ 2016-11-21 20:25 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <20161121190514.18574-1-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The interactive rebase does not currently play well with
> core.commentchar. Let's add some tests to highlight those problems
> that will be fixed in the remainder of the series.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Sorry, I should have commented here after --- line what changes were
proposed by this set of amends.
> t/t0030-stripspace.sh | 9 +++++++++
> t/t3404-rebase-interactive.sh | 11 +++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d861c..c1f6411eb2 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
> test_cmp expect actual
> '
>
> +test_expect_failure '-c with comment char defined in .git/config' '
> + test_config core.commentchar = &&
> + printf "= foo\n" >expect &&
> + printf "foo" | (
> + mkdir sub && cd sub && git stripspace -c
> + ) >actual &&
> + test_cmp expect actual
> +'
I noticed that the lack of "\n" at the end was merely copied from the
previous existing test (i.e. "-c with with changed comment char"),
which was already wrong the same way, so I kept that part as-is.
Running "stripspace" in a subdirectory is to avoid the ".git/config
was read without repository discovery as long as the command runs at
the top-level of the working tree" accident so that this highlights
the breakage with or without Peff's b9605bc4f2 ("config: only read
.git/config from configured repos", 2016-09-12).
> +
> test_expect_success 'avoid SP-HT sequence in commented line' '
> printf "#\tone\n#\n# two\n" >expect &&
> printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d6d65a3a94..d941f0a69f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,6 +983,17 @@ 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_config core.commentchar auto &&
> + write_script copy-edit-script.sh <<-\EOF &&
> + cp "$1" edit-script
> + EOF
> + test_set_editor "$(pwd)/copy-edit-script.sh" &&
> + test_when_finished "git rebase --abort || :" &&
> + git rebase -i HEAD^ &&
> + test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
> +'
> +
Removed from here is a leftover debugging bit (grep "^#"
edit-script).
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Johannes Sixt @ 2016-11-21 19:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqqy40circz.fsf@gitster.mtv.corp.google.com>
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
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Junio C Hamano @ 2016-11-21 19:12 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <20161121184931.36nuhg2h7u6mjsld@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:
>
>> > + test_cmp expect actual
>> > +'
>> > +
>>
>> Is this a recent regression? When applied on top of 'maint' or
>> older, it seems to pass just fine.
>>
>> ... Goes and looks ...
>>
>> Interesting. Peff's b9605bc4f2 ("config: only read .git/config from
>> configured repos", 2016-09-12) is where this starts failing, which
>> is understandable given the code change to builtin/stripspace.c in
>> [2/3].
>>
>> The analysis of the log message in [2/3] is wrong and needs
>> updating, though. In the old world order it worked by accident to
>> call git_config() without calling setup_git_directory(); after
>> b9605bc4f2, that no longer is valid and is exposed as a bug.
>
> Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> this case, but called out the fact that "cd subdir && git stripspace"
> would never have worked. So one step back, 2 steps forward, and Dscho's
> patch is the right step forward.
Yes, absolutely.
I sent out a set of proposed amends, and the one for this step 1/3
runs the command inside a subdirectory to force it not to find the
.git/config file relative to its pwd, which can reveal the existing
breakage without help by b9605bc4f2 ;-) hence can be forked for
older maintenance tracks.
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Junio C Hamano @ 2016-11-21 19:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <8b783f3d-8fda-b5cb-239f-895b10a22a73@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> 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.
^ permalink raw reply
* [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Junio C Hamano @ 2016-11-21 19:05 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <20161121190514.18574-1-gitster@pobox.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
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, and in turn forgot to let rebase -i handle that
new value gracefully.
Reported by Taufiq Hoven.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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 655ebaa471..c167bc36b3 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="#"
+ ;;
+?)
+ ;;
+*)
+ comment_char=$(echo "$comment_char" | cut -c1)
+ ;;
+esac
warn () {
printf '%s\n' "$*" >&2
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
--
2.11.0-rc2-154-g95ba452916
^ permalink raw reply related
* [PATCH 2/3] stripspace: respect repository config
From: Junio C Hamano @ 2016-11-21 19:05 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <20161121190514.18574-1-gitster@pobox.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The way "git stripspace" reads the configuration was not quite
correct, in that it forgot to probe for a possibly existing
repository (note: stripspace is designed to be usable outside the
repository as well) before doing so. Due to this, .git/config was
read only when the command was run from the top-level of the working
tree.
A recent change b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) stopped reading the repository-local
configuration file ".git/config" unless the repository discovery
process is done, and ".git/config" is no longer read even when run
from the top-level, which exposed the bug even more.
When rebasing interactively with a commentChar defined in the
current repository's config, the help text at the bottom of the edit
script potentially used an incorrect comment character. This was not
only funny-looking, but also resulted in tons of warnings like this
one:
Warning: the command isn't recognized in the following line
- #
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/stripspace.c | 4 +++-
t/t0030-stripspace.sh | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716ef43..1e62a008cb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (argc)
usage_with_options(stripspace_usage, options);
- if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+ if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+ setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+ }
if (strbuf_read(&buf, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index c1f6411eb2..bbf3e39e3d 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
'
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | (
--
2.11.0-rc2-154-g95ba452916
^ permalink raw reply related
* [PATCH 1/3] rebase -i: highlight problems with core.commentchar
From: Junio C Hamano @ 2016-11-21 19:05 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <xmqq7f7wk7x9.fsf@gitster.mtv.corp.google.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to highlight those problems
that will be fixed in the remainder of the series.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t0030-stripspace.sh | 9 +++++++++
t/t3404-rebase-interactive.sh | 11 +++++++++++
2 files changed, 20 insertions(+)
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d861c..c1f6411eb2 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
'
+test_expect_failure '-c with comment char defined in .git/config' '
+ test_config core.commentchar = &&
+ printf "= foo\n" >expect &&
+ printf "foo" | (
+ mkdir sub && cd sub && git stripspace -c
+ ) >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'avoid SP-HT sequence in commented line' '
printf "#\tone\n#\n# two\n" >expect &&
printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d6d65a3a94..d941f0a69f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -983,6 +983,17 @@ 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_config core.commentchar auto &&
+ write_script copy-edit-script.sh <<-\EOF &&
+ cp "$1" edit-script
+ EOF
+ test_set_editor "$(pwd)/copy-edit-script.sh" &&
+ test_when_finished "git rebase --abort || :" &&
+ git rebase -i HEAD^ &&
+ test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
test_when_finished "git branch -D torebase" &&
git checkout -b torebase branch1 &&
--
2.11.0-rc2-154-g95ba452916
^ permalink raw reply related
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Johannes Sixt @ 2016-11-21 18:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqq37ikk762.fsf@gitster.mtv.corp.google.com>
Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> 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
^ permalink raw reply
* Re: Feature request: Improve diff algorithm
From: Jacob Keller @ 2016-11-21 18:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: KES, git
In-Reply-To: <CAGZ79kZbGPneUXVEqJYhQAn+dfYve7qCjhO7QFaV1JBs3HD1aA@mail.gmail.com>
On Mon, Nov 21, 2016 at 10:17 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Nov 21, 2016 at 8:56 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Mon, Nov 21, 2016 at 12:11 AM, KES <kes-kes@yandex.ru> wrote:
>>> Hi.
>>>
>>
>> Hi,
>>
>>> I have some question about how diff works then give proposal:
>>>
>>> it will be very useful for each "symbol" store additional meta info as source line length. So in this case when git counter two equal sequence of commands it will do further comparison: Adds 23 chars deletes none VS adds 75 chars deletes 46
>>>
>>> Actually I got this:
>>>
>>> @@ -129,8 +132,9 @@ sub _preprocess_message {
>>> sub _process_message {
>>> my ($self, $message) = @_;
>>>
>>> - my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>> + my $time = [ gettimeofday ];
>>>
>>> + my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>> return $self->send_error(ERROR_REQUEST_INVALID)
>>> unless defined($method);
>>>
>>> Instead of expected:
>>> @@ -129,6 +132,8 @@ sub _preprocess_message {
>>> sub _process_message {
>>> my ($self, $message) = @_;
>>>
>>> + my $time = [ gettimeofday ];
>>> +
>>> my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>> -
>>> return $self->send_error(ERROR_REQUEST_INVALID)
>>>
>>
>> Have you tried the various options for git to search for smaller
>> diffs? Or using the other diff algorithms such as histogram instead of
>> patience?
>>
>
> The newest version of Git comes with a flag to move around the diff
> better, based on the work at https://github.com/mhagger/diff-slider-tools
Unfortunately in this case, I'm not convinced that it will improve the
diff. It's worth a try as well though.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Jeff King @ 2016-11-21 18:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:
> > + test_cmp expect actual
> > +'
> > +
>
> Is this a recent regression? When applied on top of 'maint' or
> older, it seems to pass just fine.
>
> ... Goes and looks ...
>
> Interesting. Peff's b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) is where this starts failing, which
> is understandable given the code change to builtin/stripspace.c in
> [2/3].
>
> The analysis of the log message in [2/3] is wrong and needs
> updating, though. In the old world order it worked by accident to
> call git_config() without calling setup_git_directory(); after
> b9605bc4f2, that no longer is valid and is exposed as a bug.
Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
this case, but called out the fact that "cd subdir && git stripspace"
would never have worked. So one step back, 2 steps forward, and Dscho's
patch is the right step forward.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Junio C Hamano @ 2016-11-21 18:40 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <975c0002-8afe-efa6-d967-72f2a0d21169@kdbg.org>
Johannes Sixt <j6t@kdbg.org> 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?
^ permalink raw reply
* Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Johannes Sixt @ 2016-11-21 18:26 UTC (permalink / raw)
To: Johannes Schindelin, git
Cc: Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <9ef529a4fbb60990a91d7bbfdd49c6d20d49e442.1479737858.git.johannes.schindelin@gmx.de>
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
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Junio C Hamano @ 2016-11-21 18:24 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <xmqqbmx8k8c0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> +test_expect_failure '-c with comment char defined in .git/config' '
>> + test_config core.commentchar = &&
>> + printf "= foo\n" >expect &&
>> + printf "foo" | git stripspace -c >actual &&
>
> We'd want "\n" on this printf to match the one before as well, as
> this test is not about "does stripspace complete an incomplete
> line?", I think.
>
> I could amend it while queuing, but I need to know if I am missing a
> reason why this must be an incomplete line before doing so.
>
>> + test_cmp expect actual
>> +'
>> +
>
> Is this a recent regression? When applied on top of 'maint' or
> older, it seems to pass just fine.
I think we can force failure by running this test somewhere other
than the top level of the working tree. A set of proposed amends
incoming ...
^ permalink raw reply
* Re: Feature request: Improve diff algorithm
From: Stefan Beller @ 2016-11-21 18:17 UTC (permalink / raw)
To: Jacob Keller; +Cc: KES, git
In-Reply-To: <CA+P7+xrKfqOb-3FhKxXdnF6g9PktNWNzY+ZnHJS=yuVo1YdXzg@mail.gmail.com>
On Mon, Nov 21, 2016 at 8:56 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Mon, Nov 21, 2016 at 12:11 AM, KES <kes-kes@yandex.ru> wrote:
>> Hi.
>>
>
> Hi,
>
>> I have some question about how diff works then give proposal:
>>
>> it will be very useful for each "symbol" store additional meta info as source line length. So in this case when git counter two equal sequence of commands it will do further comparison: Adds 23 chars deletes none VS adds 75 chars deletes 46
>>
>> Actually I got this:
>>
>> @@ -129,8 +132,9 @@ sub _preprocess_message {
>> sub _process_message {
>> my ($self, $message) = @_;
>>
>> - my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>> + my $time = [ gettimeofday ];
>>
>> + my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>> return $self->send_error(ERROR_REQUEST_INVALID)
>> unless defined($method);
>>
>> Instead of expected:
>> @@ -129,6 +132,8 @@ sub _preprocess_message {
>> sub _process_message {
>> my ($self, $message) = @_;
>>
>> + my $time = [ gettimeofday ];
>> +
>> my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>> -
>> return $self->send_error(ERROR_REQUEST_INVALID)
>>
>
> Have you tried the various options for git to search for smaller
> diffs? Or using the other diff algorithms such as histogram instead of
> patience?
>
The newest version of Git comes with a flag to move around the diff
better, based on the work at https://github.com/mhagger/diff-slider-tools
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Junio C Hamano @ 2016-11-21 18:15 UTC (permalink / raw)
To: Johannes Schindelin, Jeff King
Cc: git, Ralf Thielow, Nguyễn Thái Ngọc Duy,
Taufiq Hoven
In-Reply-To: <f47dce15719954d6d4d8a550856757366871143e.1479737858.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d8..202ac07 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
> test_cmp expect actual
> '
>
> +test_expect_failure '-c with comment char defined in .git/config' '
> + test_config core.commentchar = &&
> + printf "= foo\n" >expect &&
> + printf "foo" | git stripspace -c >actual &&
We'd want "\n" on this printf to match the one before as well, as
this test is not about "does stripspace complete an incomplete
line?", I think.
I could amend it while queuing, but I need to know if I am missing a
reason why this must be an incomplete line before doing so.
> + test_cmp expect actual
> +'
> +
Is this a recent regression? When applied on top of 'maint' or
older, it seems to pass just fine.
... Goes and looks ...
Interesting. Peff's b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) is where this starts failing, which
is understandable given the code change to builtin/stripspace.c in
[2/3].
The analysis of the log message in [2/3] is wrong and needs
updating, though. In the old world order it worked by accident to
call git_config() without calling setup_git_directory(); after
b9605bc4f2, that no longer is valid and is exposed as a bug.
Your [2/3] is a good fix for that change.
In any case, well spotted.
> test_expect_success 'avoid SP-HT sequence in commented line' '
> printf "#\tone\n#\n# two\n" >expect &&
> printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e38e296..e080399 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -976,6 +976,18 @@ 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_config core.commentchar auto &&
> + write_script copy-edit-script.sh <<-\EOF &&
> + cp "$1" edit-script
> + EOF
> + test_set_editor "$(pwd)/copy-edit-script.sh" &&
> + test_when_finished "git rebase --abort || :" &&
> + git rebase -i HEAD^ &&
> + grep "^#" edit-script &&
This was added for debugging that was forgotten?
> + test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
This says "There shouldn't be any line left once we remove
'#'-commented lines, empty lines and pick insns.". OK.
The correction in [3/3] seems good.
> +'
> +
> test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
> test_when_finished "git branch -D torebase" &&
> git checkout -b torebase branch1 &&
^ permalink raw reply
* Re: [PATCH v4 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-11-21 18:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, jonathantanmy
In-Reply-To: <20161118225220.GA76630@google.com>
On 11/18, Brandon Williams wrote:
> Also, in order to use the tree_entry_interesting code it looks like I'll
> either have to pipe through a flag saying 'yes i want to match against
> submodules' like I did for the other pathspec codepath. Either that or
> add functionality to perform wildmatching against partial matches (ie
> directories and submodules) since currently the tree_entry_interesting
> code path just punts and says 'well say it matches for now and check
> again later' whenever it runs into a directory (I can't really make it
> do that for submodules without a flag of somesort as tests could break).
> Or maybe both?
Looks like my initial assumption was incorrect, I just needed to be
smarter than punting when running into a submodule. Should be able to
just ensure that the entry matches up to at least the first wildcard
character before punting and all should be good.
--
Brandon Williams
^ permalink raw reply
* RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: David Turner @ 2016-11-21 18:12 UTC (permalink / raw)
To: 'Stefan Beller'
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <CAGZ79kb-TKgQm1_p3NyJS7mso2FFWMNHa7F8vwzocjSJfyjXJQ@mail.gmail.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> >> + if (submodule_is_interesting(old->name,
> null_sha1)
> >> + && ok_to_remove_submodule(old->name))
> >> + return 0;
> >> + }
> >
> > Do we need a return 1 in here somewhere? Because otherwise, we fall
> through and return 0 later.
>
> Otherwise we would fall through and run
>
> if (errno == ENOENT)
> return 0;
> return o->gently ? -1 :
> add_rejected_path(o, error_type, ce->name);
>
> which produces different results than 0?
Oh, I see. I was misreading that errno check.
^ permalink raw reply
* Re: [PATCH] i18n: Fixed unmatched single quote in error message
From: Junio C Hamano @ 2016-11-21 17:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jiang Xin, Git List
In-Reply-To: <alpine.DEB.2.20.1611211522280.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi,
>
> On Sun, 20 Nov 2016, Jiang Xin wrote:
>
>> Fixed unmatched single quote introduced by commit:
>>
>> * f56fffef9a sequencer: teach write_message() to append an optional LF
>>
>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>
> ACK!
>
> Thank you,
> Dscho
Thanks, both.
^ permalink raw reply
* Re: [PATCH 0/2] add format specifiers to display trailers
From: Junio C Hamano @ 2016-11-21 17:23 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jacob Keller, Jonathan Tan, Git mailing list
In-Reply-To: <CA+P7+xrQEBYQQhqJQQCpLrs+4WOJOvH1X27w5Ou=2VPT=FegGQ@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> We have %s and %b so that we can reconstruct the whole thing by
>> using both. It is unclear how %bT fits in this picture. I wonder
>> if we also need another placeholder that expands to the body of the
>> message without the trailer---otherwise the whole set would become
>> incoherent, no?
>
> I'm not entirely sure what to do here. I just wanted a way to easily
> format "just the trailers" of a message. We could add something that
> formats just the non-trailers, that's not too difficult. Not really
> sure what I'd call it though.
I was wondering if %(log:<name of a part>) was a better way to go.
%(log:title) and %(log:body) would be equivalents of traditional %s
and %b, and %(log:body) in turn would be a shorter way to write
%(log:description)%+(log:trailer), i.e. show the message body, and
if there is a trailer block, add it after adding a blank line.
Or something like that?
^ permalink raw reply
* Re: [PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined
From: Jacob Keller @ 2016-11-21 16:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git mailing list, Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <cover.1479737858.git.johannes.schindelin@gmx.de>
On Mon, Nov 21, 2016 at 6:18 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> The Git for Windows project recently got a bug report that detailed how
> `git rebase -i` fails when core.commentchar=auto:
>
> https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q
>
> This patch series fixes rebase -i's handling of core.commentchar.
>
>
> Johannes Schindelin (3):
> rebase -i: identify problems with core.commentchar
> stripspace: respect repository config
> rebase -i: handle core.commentChar=auto
>
> builtin/stripspace.c | 4 +++-
> git-rebase--interactive.sh | 13 +++++++++++--
> t/t0030-stripspace.sh | 7 +++++++
> t/t3404-rebase-interactive.sh | 12 ++++++++++++
> 4 files changed, 33 insertions(+), 3 deletions(-)
>
>
> base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d
> Published-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1
>
> --
> 2.10.1.583.g721a9e0
>
The series makes sense to me.
Thanks,
Jake
^ permalink raw reply
* Re: Feature request: Improve diff algorithm
From: Jacob Keller @ 2016-11-21 16:56 UTC (permalink / raw)
To: KES; +Cc: git
In-Reply-To: <14657461479715884@web21h.yandex.ru>
On Mon, Nov 21, 2016 at 12:11 AM, KES <kes-kes@yandex.ru> wrote:
> Hi.
>
Hi,
> I have some question about how diff works then give proposal:
>
> it will be very useful for each "symbol" store additional meta info as source line length. So in this case when git counter two equal sequence of commands it will do further comparison: Adds 23 chars deletes none VS adds 75 chars deletes 46
>
> Actually I got this:
>
> @@ -129,8 +132,9 @@ sub _preprocess_message {
> sub _process_message {
> my ($self, $message) = @_;
>
> - my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
> + my $time = [ gettimeofday ];
>
> + my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
> return $self->send_error(ERROR_REQUEST_INVALID)
> unless defined($method);
>
> Instead of expected:
> @@ -129,6 +132,8 @@ sub _preprocess_message {
> sub _process_message {
> my ($self, $message) = @_;
>
> + my $time = [ gettimeofday ];
> +
> my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
> -
> return $self->send_error(ERROR_REQUEST_INVALID)
>
Have you tried the various options for git to search for smaller
diffs? Or using the other diff algorithms such as histogram instead of
patience?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] i18n: Fixed unmatched single quote in error message
From: Johannes Schindelin @ 2016-11-21 14:22 UTC (permalink / raw)
To: Jiang Xin; +Cc: Junio C Hamano, Git List
In-Reply-To: <721cdca4d000627fa163da15c515a50fd9529a62.1479644251.git.worldhello.net@gmail.com>
Hi,
On Sun, 20 Nov 2016, Jiang Xin wrote:
> Fixed unmatched single quote introduced by commit:
>
> * f56fffef9a sequencer: teach write_message() to append an optional LF
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
ACK!
Thank you,
Dscho
^ permalink raw reply
* [PATCH 3/3] rebase -i: handle core.commentChar=auto
From: Johannes Schindelin @ 2016-11-21 14:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <cover.1479737858.git.johannes.schindelin@gmx.de>
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, and in turn forgot to let rebase -i handle that
new value gracefully.
Reported by Taufiq Hoven.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
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=#
+ ;;
+?)
+ ;;
+*)
+ comment_char=$(comment_char | cut -c1)
+ ;;
+esac
warn () {
printf '%s\n' "$*" >&2
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e080399..0f3d177 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,7 +976,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
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH 1/3] rebase -i: identify problems with core.commentchar
From: Johannes Schindelin @ 2016-11-21 14:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <cover.1479737858.git.johannes.schindelin@gmx.de>
The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to identify those problems.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0030-stripspace.sh | 7 +++++++
t/t3404-rebase-interactive.sh | 12 ++++++++++++
2 files changed, 19 insertions(+)
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..202ac07 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
'
+test_expect_failure '-c with comment char defined in .git/config' '
+ test_config core.commentchar = &&
+ printf "= foo\n" >expect &&
+ printf "foo" | git stripspace -c >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'avoid SP-HT sequence in commented line' '
printf "#\tone\n#\n# two\n" >expect &&
printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e38e296..e080399 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,6 +976,18 @@ 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_config core.commentchar auto &&
+ write_script copy-edit-script.sh <<-\EOF &&
+ cp "$1" edit-script
+ EOF
+ test_set_editor "$(pwd)/copy-edit-script.sh" &&
+ test_when_finished "git rebase --abort || :" &&
+ git rebase -i HEAD^ &&
+ grep "^#" edit-script &&
+ test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
test_when_finished "git branch -D torebase" &&
git checkout -b torebase branch1 &&
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH 2/3] stripspace: respect repository config
From: Johannes Schindelin @ 2016-11-21 14:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Ralf Thielow,
Nguyễn Thái Ngọc Duy, Taufiq Hoven
In-Reply-To: <cover.1479737858.git.johannes.schindelin@gmx.de>
When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
`stripspace` command to respect the config setting `core.commentChar`,
it forgot that this variable may be defined in .git/config.
So when rebasing interactively with a commentChar defined in the current
repository's config, the help text at the bottom of the edit script
potentially used an incorrect comment character. This was not only
funny-looking, but also resulted in tons of warnings like this one:
Warning: the command isn't recognized in the following line
- #
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/stripspace.c | 4 +++-
t/t0030-stripspace.sh | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716e..1e62a00 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (argc)
usage_with_options(stripspace_usage, options);
- if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+ if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+ setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+ }
if (strbuf_read(&buf, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 202ac07..67f77df 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
'
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | git stripspace -c >actual &&
--
2.10.1.583.g721a9e0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox