* [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
* [PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined
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
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
^ permalink raw reply
* --root seems to cancel out --keep-empty in git-rebase
From: Pierre Ossman @ 2016-11-21 13:30 UTC (permalink / raw)
To: git
As the subject says, these two commands give identical behaviour:
git rebase --interactive --keep-empty --root
and:
git rebase --interactive --root
I.e. --keep-empty has no effect when --root is specified.
Ideas for workaround welcome.
Regards
--
Pierre Ossman Software Development
Cendio AB https://cendio.com
Teknikringen 8 https://twitter.com/ThinLinc
583 30 Linköping https://facebook.com/ThinLinc
Phone: +46-13-214600 https://plus.google.com/+CendioThinLinc
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output
From: Matthieu Moy @ 2016-11-21 8:41 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jakub Narębski, Git List, Jacob Keller
In-Reply-To: <CAOLa=ZRU--FyFWNvv7RGH2mho36hvUu2ye=JOapXZ6QoEy=nRw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> cc'in Matthieu since he wrote the patch.
>
> On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>>> the atom %(upstream:track) can be translated if needed. This is needed
>>> as we port branch.c to use ref-filter's printing API's.
>>>
>>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>> ref-filter.c | 28 ++++++++++++++++++++++++----
>>> ref-filter.h | 2 ++
>>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> index b47b900..944671a 100644
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -15,6 +15,26 @@
>>> #include "version.h"
>>> #include "wt-status.h"
>>>
>>> +static struct ref_msg {
>>> + const char *gone;
>>> + const char *ahead;
>>> + const char *behind;
>>> + const char *ahead_behind;
>>> +} msgs = {
>>> + "gone",
>>> + "ahead %d",
>>> + "behind %d",
>>> + "ahead %d, behind %d"
>>> +};
>>> +
>>> +void setup_ref_filter_porcelain_msg(void)
>>> +{
>>> + msgs.gone = _("gone");
>>> + msgs.ahead = _("ahead %d");
>>> + msgs.behind = _("behind %d");
>>> + msgs.ahead_behind = _("ahead %d, behind %d");
>>> +}
>>
>> Do I understand it correctly that this mechanism is here to avoid
>> repeated calls into gettext, as those messages would get repeated
>> over and over; otherwise one would use foo = N_("...") and _(foo),
>> isn't it?
That's not the primary goal. The primary goal is to keep untranslated,
and immutable messages in plumbing commands. We may decide one day that
_("gone") is not the best message for the end user and replace it with,
say, _("vanished"), but the "gone" has to remain the same forever and
regardless of the user's config for scripts using it.
We could have written
in_porcelain ? _("gone") : "gone"
here and there in the code, but having a single place where we set all
the messages seems simpler. Call setup_ref_filter_porcelain_msg() and
get the porcelain messages, don't call it and keep the plumbing
messages.
Note that it's not the first place in the code where we do this, see
setup_unpack_trees_porcelain in unpack-trees.c for another instance.
Karthik: the commit message could be improved, for example:
Introduce setup_ref_filter_porcelain_msg() so that the messages used in
the atom %(upstream:track) can be translated if needed. By default, keep
the messages untranslated, which is the right behavior for plumbing
commands. This is needed as we port branch.c to use ref-filter's
printing API's.
Why not a comment right below "} msgs = {" saying e.g.:
/* Untranslated plumbing messages: */
>> I wonder if there is some way to avoid duplication here, but I don't
>> see anything easy and safe (e.g. against running setup_*() twice).
I think we do need duplication for the reason above.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Feature request: Improve diff algorithm
From: KES @ 2016-11-21 8:11 UTC (permalink / raw)
To: git
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)
Details: http://stackoverflow.com/questions/40550751/unexpected-result-in-git-diff/40552165?noredirect=1#comment68648377_40552165
^ permalink raw reply
* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
From: Torsten Bögershausen @ 2016-11-21 5:37 UTC (permalink / raw)
To: Mike Fisher, git
In-Reply-To: <451E4A46-BA43-41A5-9E68-DE0D89BE676A@csh.rit.edu>
>On 20/11/16 22:18, Mike Fisher wrote:
Thanks for contributing to Git.
One comment on the head line:
>Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL
The word "refactor" may be used in other way: Re-structure the code,
and use the same API.
"Remove dependency on deprecated Net::SMTP::SSL"
> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
Is there a security risk with require Net::SMTP::SSL ?
If yes, the commit message should state this.
If no:
Even if it is deprecated, is it still in use somewhere ?
Does it hurt someone, is there any OS release where the old code doesn't work
anymore ?
Or is it "only" nice to have ?
Since when does Net::SMTP include Net::SMTP::SSL ?
On which system has the change been tested ?
I think the commit message could and should give more information like this.
My comments may be over-critical.
Lets see if other people from the list know more than me.
>
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
>
> Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
> ---
> git-send-email.perl | 54 +++++++++++++++++++++++++----------------------------
> 1 file changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be4..fc166c5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1330,15 +1330,17 @@ Message-Id: $message_id
> print $sm "$header\n$message";
> close $sm or die $!;
> } else {
> -
I can see one refactoring, that is the removal of an empty line.
>
> if (!defined $smtp_server) {
> die "The required SMTP server is not properly defined."
> }
>
> + require Net::SMTP;
> + $smtp_domain ||= maildomain();
> + my $smtp_ssl = 0;
> +
> if ($smtp_encryption eq 'ssl') {
> $smtp_server_port ||= 465; # ssmtp
> - require Net::SMTP::SSL;
> - $smtp_domain ||= maildomain();
> + $smtp_ssl = 1;
> require IO::Socket::SSL;
>
> # Suppress "variable accessed once" warning.
> @@ -1347,37 +1349,31 @@ Message-Id: $message_id
> $IO::Socket::SSL::DEBUG = 1;
> }
>
> - # Net::SMTP::SSL->new() does not forward any SSL options
> IO::Socket::SSL::set_client_defaults(
> ssl_verify_params());
> - $smtp ||= Net::SMTP::SSL->new($smtp_server,
> - Hello => $smtp_domain,
> - Port => $smtp_server_port,
> - Debug => $debug_net_smtp);
> }
> else {
> - require Net::SMTP;
> - $smtp_domain ||= maildomain();
> $smtp_server_port ||= 25;
> - $smtp ||= Net::SMTP->new($smtp_server,
> - Hello => $smtp_domain,
> - Debug => $debug_net_smtp,
> - Port => $smtp_server_port);
> - if ($smtp_encryption eq 'tls' && $smtp) {
> - require Net::SMTP::SSL;
> - $smtp->command('STARTTLS');
> - $smtp->response();
> - if ($smtp->code == 220) {
> - $smtp = Net::SMTP::SSL->start_SSL($smtp,
> - ssl_verify_params())
> - or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> - $smtp_encryption = '';
> - # Send EHLO again to receive fresh
> - # supported commands
> - $smtp->hello($smtp_domain);
> - } else {
> - die "Server does not support STARTTLS! ".$smtp->message;
> - }
> + }
> +
> + $smtp ||= Net::SMTP->new($smtp_server,
> + Hello => $smtp_domain,
> + Port => $smtp_server_port,
> + Debug => $debug_net_smtp,
> + SSL => $smtp_ssl);
> +
> + if ($smtp_encryption eq 'tls' && $smtp) {
> + $smtp->command('STARTTLS');
> + $smtp->response();
> + if ($smtp->code == 220) {
> + $smtp->starttls(ssl_verify_params())
> + or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> + $smtp_encryption = '';
> + # Send EHLO again to receive fresh
> + # supported commands
> + $smtp->hello($smtp_domain);
> + } else {
> + die "Server does not support STARTTLS! ".$smtp->message;
> }
> }
>
^ permalink raw reply
* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
From: brian m. carlson @ 2016-11-20 21:53 UTC (permalink / raw)
To: Mike Fisher; +Cc: git
In-Reply-To: <451E4A46-BA43-41A5-9E68-DE0D89BE676A@csh.rit.edu>
[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]
On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
>
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
As much as I hate to say this, I think this is going to cause
compatibility problems. Net::SMTP is part of core Perl (as of v5.7.3),
but the version you want to rely on (which you did not provide an
explicit dependency on) is from October 2014.
That basically means that no Perl on a Red Hat or CentOS system is going
to provide that support, since RHEL 7 was released in June 2014.
Providing an updated Git on those platforms would require replacing the
system Perl or parts of it, which would be undesirable. This would
affect Debian 7 as well.
We currently support Perl 5.8 [0], so if you want to remove support for
Net::SMTP::SSL, I'd recommend a solution that works with that version.
[0] I personally believe we should drop support for Perl older than
5.10.1 (if not newer), but that's my opinion and it isn't shared by
other list regulars.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* [PATCH] Remove dependency on deprecated Net::SMTP::SSL
From: Mike Fisher @ 2016-11-20 21:18 UTC (permalink / raw)
To: git
Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL:
<http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
---
git-send-email.perl | 54
+++++++++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 29 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..fc166c5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1330,15 +1330,17 @@ Message-Id: $message_id
print $sm "$header\n$message";
close $sm or die $!;
} else {
-
if (!defined $smtp_server) {
die "The required SMTP server is not properly defined."
}
+ require Net::SMTP;
+ $smtp_domain ||= maildomain();
+ my $smtp_ssl = 0;
+
if ($smtp_encryption eq 'ssl') {
$smtp_server_port ||= 465; # ssmtp
- require Net::SMTP::SSL;
- $smtp_domain ||= maildomain();
+ $smtp_ssl = 1;
require IO::Socket::SSL;
# Suppress "variable accessed once" warning.
@@ -1347,37 +1349,31 @@ Message-Id: $message_id
$IO::Socket::SSL::DEBUG = 1;
}
- # Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
- $smtp ||= Net::SMTP::SSL->new($smtp_server,
- Hello => $smtp_domain,
- Port => $smtp_server_port,
- Debug => $debug_net_smtp);
}
else {
- require Net::SMTP;
- $smtp_domain ||= maildomain();
$smtp_server_port ||= 25;
- $smtp ||= Net::SMTP->new($smtp_server,
- Hello => $smtp_domain,
- Debug => $debug_net_smtp,
- Port => $smtp_server_port);
- if ($smtp_encryption eq 'tls' && $smtp) {
- require Net::SMTP::SSL;
- $smtp->command('STARTTLS');
- $smtp->response();
- if ($smtp->code == 220) {
- $smtp = Net::SMTP::SSL->start_SSL($smtp,
- ssl_verify_params())
- or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
- $smtp_encryption = '';
- # Send EHLO again to receive fresh
- # supported commands
- $smtp->hello($smtp_domain);
- } else {
- die "Server does not support STARTTLS! ".$smtp->message;
- }
+ }
+
+ $smtp ||= Net::SMTP->new($smtp_server,
+ Hello => $smtp_domain,
+ Port => $smtp_server_port,
+ Debug => $debug_net_smtp,
+ SSL => $smtp_ssl);
+
+ if ($smtp_encryption eq 'tls' && $smtp) {
+ $smtp->command('STARTTLS');
+ $smtp->response();
+ if ($smtp->code == 220) {
+ $smtp->starttls(ssl_verify_params())
+ or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
+ $smtp_encryption = '';
+ # Send EHLO again to receive fresh
+ # supported commands
+ $smtp->hello($smtp_domain);
+ } else {
+ die "Server does not support STARTTLS! ".$smtp->message;
}
}
--
2.9.3 (Apple Git-75)
^ permalink raw reply related
* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer @ 2016-11-20 20:19 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <52f6241f-e584-d830-ca66-084dc509c7fe@gmx.net>
On 11/20/2016 09:01 PM, Stephan Beyer wrote:
> First, replace the current set_terms() by
>
> static void set_terms(struct bisect_terms *terms, const char *bad,
> const char *good)
> {
> terms->term_good = xstrdup(good);
> terms->term_bad = xstrdup(bad);
> }
>
> ie, without calling write_terms(...).
I did not want to confuse you here but I forgot to mention that there
should also be freeing code, i.e. initialize your terms to NULL in the
beginning of cmd_builtin__helper, and always free them if it is not
null. This freeing code could also be in an extra function free_terms()
and you call it in set_terms() and for cleanup in the end.
^ permalink raw reply
* Re: Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?
From: Jeff King @ 2016-11-20 20:17 UTC (permalink / raw)
To: Matthieu S; +Cc: git
In-Reply-To: <CAEYvigLz3muWD-QFjMZUn=H3RQoxhTYX9EwB6=aiMjWOEN3CBA@mail.gmail.com>
On Fri, Nov 18, 2016 at 03:40:22PM -0800, Matthieu S wrote:
> Why is the speed so different if one uses --word-diff instead of
> --word-diff-regex= ? Is it just because my expression is (slightly)
> more complex than the default one (split on period instead of only
> whitespace) ? Or is it that the default word-diff is implemented
> differently/more efficiently? How can I overcome this speed slowdown?
I think it's probably both.
See diff.c:find_word_boundaries(). If there's no regex, we use a simple
loop over isspace() to find the boundaries. I don't recall anybody
measuring the performance before, but I'm not surprised to hear that
matching a regex is slower.
If I look at the output of "perf", though, it looks like we also spend a
lot more time in xdl_clean_mmatch(). Which isn't surprising. Your regex
treats commas as boundaries, which is going to generate a lot more
matches for this particular data set (though the output is the same, I
think, because of the nature of the change).
I would have expected "--word-diff-regex=[^[:space:]]" to be faster than
your regex, though, and it does not seem to be.
-Peff
^ permalink raw reply
* Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C
From: Stephan Beyer @ 2016-11-20 20:15 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b1a-067117ef-cd0d-469b-ba80-ea1a1169f694-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 502bf18..1767916 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -422,6 +425,7 @@ static int bisect_next(...)
> {
> int res, no_checkout;
>
> + bisect_autostart(terms);
You are not checking for return values here. (The shell code simply
exited if there is no tty, but you don't.)
> @@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> return retval || bisect_auto_next(terms, NULL);
> }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_start())) {
> + const char *yesno;
> + const char *argv[] = {NULL};
> + fprintf(stderr, _("You need to start by \"git bisect "
> + "start\"\n"));
> +
> + if (!isatty(0))
isatty(STDIN_FILENO)?
> + return 1;
> +
> + /*
> + * TRANSLATORS: Make sure to include [Y] and [n] in your
> + * translation. THe program will only accept English input
Typo "THe"
> + * at this point.
> + */
Taking "at this point" into consideration, I think the Y and n can be
easily translated now that it is in C. I guess, by using...
> + yesno = git_prompt(_("Do you want me to do it for you "
> + "[Y/n]? "), PROMPT_ECHO);
> + if (starts_with(yesno, "n") || starts_with(yesno, "N"))
... starts_with(yesno, _("n")) || starts_with(yesno, _("N"))
here (but not sure). However, this would be an extra patch on top of
this series.
> + exit(0);
Shouldn't this also be "return 1;"? Saying "no" is the same outcome as
not having a tty to ask for yes or no.
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("find the next bisection commit"), BISECT_NEXT),
> OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
> + OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> + N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
The word "is" is missing.
~Stephan
^ permalink raw reply
* Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Stephan Beyer @ 2016-11-20 20:01 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1af0-5d688c2e-868d-4d8c-a8fd-9a675f7f01da-000000@eu-west-1.amazonses.com>
Hi Pranit,
this one is hard to review because you do two or three commits in one here.
I think the first commit should be the exit()->return conversion, the
second commit is next and autonext, and the third commit is the pretty
trivial bisect_start commit ;) However, you did it this way and it's
always a hassle to split commit, so I don't really care...
However, I was reviewing this superficially, to be honest. This mail
skips the next and autonext part.
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/bisect.c b/bisect.c
> index 45d598d..7c97e85 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix)
> *
> * If that's not the case, we need to check the merge bases.
> * If a merge base must be tested by the user, its source code will be
> - * checked out to be tested by the user and we will exit.
> + * checked out to be tested by the user and we will return.
> */
> -static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> +static int check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> {
> char *filename = git_pathdup("BISECT_ANCESTORS_OK");
> struct stat st;
> - int fd;
> + int fd, res = 0;
>
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> if (!current_bad_oid)
> - die(_("a %s revision is needed"), term_bad);
> + error(_("a %s revision is needed"), term_bad);
Only error() or return error()?
> @@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> filename);
> else
> close(fd);
> +
> + goto done;
> done:
I never understand why you do this. In case of adding a "fail" label
(and fail code like "res = -1;") between "goto done" and "done:", it's
fine... but without one this is just a nop.
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d3e17f..fcd7574 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> no_checkout = 1;
>
> for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--")) {
> + const char *arg;
> + if (starts_with(argv[i], "'"))
> + arg = sq_dequote(xstrdup(argv[i]));
> + else
> + arg = argv[i];
One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's
an inconsistent leak... I guess it's a bad idea to do it this way ;)
(Also below.)
> @@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> no_checkout = 1;
> } else if (!strcmp(arg, "--term-good") ||
> !strcmp(arg, "--term-old")) {
> + if (starts_with(argv[++i], "'"))
> + terms->term_good = sq_dequote(xstrdup(argv[i]));
> + else
> + terms->term_good = xstrdup(argv[i]);
> must_write_terms = 1;
> - terms->term_good = xstrdup(argv[++i]);
> } else if (skip_prefix(arg, "--term-good=", &arg)) {
> must_write_terms = 1;
> - terms->term_good = xstrdup(arg);
> + terms->term_good = arg;
No ;) (See my other comments (to other patches) for the "terms" leaks.)
[This repeats several times below.]
> diff --git a/git-bisect.sh b/git-bisect.sh
> index f0896b3..d574c44 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -109,6 +88,7 @@ bisect_skip() {
> bisect_state() {
> bisect_autostart
> state=$1
> + get_terms
> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> get_terms
> case "$#,$state" in
I can't say if this change is right or wrong. It looks right, but: How
does this relate to the other changes? Is this the right patch for it?
~Stephan
^ permalink raw reply
* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer @ 2016-11-20 20:01 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1ad3-ea75ed97-2514-427e-8e57-9f10efd4e6e9-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos, retval = 0;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp = NULL;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + }
> +
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + const char *arg = argv[i];
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + } else if (!strcmp(arg, "--no-checkout")) {
> + no_checkout = 1;
> + } else if (!strcmp(arg, "--term-good") ||
> + !strcmp(arg, "--term-old")) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(argv[++i]);
All these xstrdup() for the terms here and below will leak memory.
I recommend to use xstrdup() also at (*) below, and use
free(terms->term_good) above this line (and for every occurrence below,
of course).
> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
[...]
> @@ -497,6 +705,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> die(_("--bisect-terms requires 0 or 1 argument"));
> res = bisect_terms(&terms, argv, argc);
> break;
> + case BISECT_START:
> + terms.term_good = "good";
> + terms.term_bad = "bad";
Here is (*): use xstrdup("good") etc.
And then, as already mentioned for another patch, free(terms.*) below.
I personally am a friend of small functions and would prefer something
like as follows... (This is a comment about several patches of your
series, not only this one.)
First, replace the current set_terms() by
static void set_terms(struct bisect_terms *terms, const char *bad,
const char *good)
{
terms->term_good = xstrdup(good);
terms->term_bad = xstrdup(bad);
}
ie, without calling write_terms(...).
And then replace the *current* set_terms() calls by set_terms(...);
write_terms(...); calls.
Second, add
static void get_default_terms(struct bisect_terms *terms)
{
set_terms(terms, "bad", "good");
}
and use this instead of the two lines quoted above (and all its other
occurrences).
Third, use the new set_terms() everywhere instead of settings terms
members directly (with the exception of get_terms()).
This sounds like a safer variant (with respect to leaks and handling
them) to me than doing it the current way.
~Stephan
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jakub Narębski @ 2016-11-20 18:43 UTC (permalink / raw)
To: Junio C Hamano, Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq4m32kqet.fsf@gitster.mtv.corp.google.com>
W dniu 20.11.2016 o 18:32, Junio C Hamano pisze:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We could have lstrip and rstrip as you suggested and perhaps make
>> it work together too. But I see this going off the scope of this
>> series. Maybe I'll follow up with another series introducing these
>> features. Since we can currently make do with 'strip=2' I'll drop
>> this patch from v8 of this series and pursue this idea after this.
>
> My primary point was that if we know we want to add "rstrip" later
> and still decide not to add it right now, it is OK, but we will
> regret it if we named the one we are going to add right now "strip".
> That will mean that future users, when "rstrip" is introduced, will
> end up having to choose between "strip" and "rstrip" (as opposed to
> "lstrip" and "rstrip"), wondering why left-variant is more important
> and named without left/right prefix.
Another solution would be to implement 'splice=<offset>[,<length>]',
where if length is omitted it means to the end; perhaps with special
casing (as in Perl) of negative <offset> and/or negative <length>.
Or implement POSIX shell expansion:
%(parameter%word) - Remove Smallest Suffix Glob Pattern.
%(parameter%%word) - Remove Largest Suffix Glob Pattern.
%(parameter#word) - Remove Smallest Prefix Pattern.
%(parameter##word) - Remove Largest Prefix Pattern.
Though this one looks like serious overkill...
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-20 17:32 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jakub Narębski, Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZRf+vPOPK=ovP7JmJ52qdgwuqkpGH4UfP=+caQeyu9Ucw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.
My primary point was that if we know we want to add "rstrip" later
and still decide not to add it right now, it is OK, but we will
regret it if we named the one we are going to add right now "strip".
That will mean that future users, when "rstrip" is introduced, will
end up having to choose between "strip" and "rstrip" (as opposed to
"lstrip" and "rstrip"), wondering why left-variant is more important
and named without left/right prefix.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-20 16:52 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Junio C Hamano, Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZRf+vPOPK=ovP7JmJ52qdgwuqkpGH4UfP=+caQeyu9Ucw@mail.gmail.com>
On Sun, Nov 20, 2016 at 8:46 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>>> strip=1,strip=-1, which could be
>>>>>> done but ...
>>>>>
>>>>> ... would be unnecessary if this is the only use case:
>>>>>
>>>>>> strbuf_addf(&fmt,
>>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>>> local.buf, remote.buf);
>>>>>
>>>>> You can "strip to leave only 2 components" and compare the result
>>>>> with refs/remotes instead, no?
>>>>>
>>>>
>>>> Of course, my only objective was that someone would find it useful to
>>>> have these two additional
>>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>>
>>>> --
>>>> Regards,
>>>> Karthik Nayak
>>>
>>> I think having strip and rstrip make sense, (along with support for
>>> negative numbers) I don't think we need to make them work together
>>> unless someone is interested, since we can use strip=-2 to get the
>>> behavior we need today.
>>
>> I am OK with multiple strips Karthik suggests, e.g.
>>
>> %(refname:strip=1,rstrip=-1)
>>
>> if it is cleanly implemented.
>>
>> I have a bit of trouble with these names, though. If we call one
>> strip and the other rstrip, to only those who know about rstrip it
>> would be clear that strip is about stripping from the left. Perhaps
>> we should call it lstrip for symmetry and ease-of-remembering?
>>
>> refs/heads/master:lstrip=-1 => master (strip all but one level
>> from the left)
>>
>> refs/heads/master:rstrip=-2 => refs/heads (strip all but two
>> levels from the right)
>>
>> refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
>> from the left and then strip all but one level from the right)
>>
>> I dunno.
>
> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.
>
I meant 'strip=-2'. I mean I'll add in the negative striping in this
series and follow
up with something that'd introduce lstrip and rstrip.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output
From: Karthik Nayak @ 2016-11-20 15:33 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller, Matthieu Moy
In-Reply-To: <af0b7bdc-2b29-0d04-85f1-aa1d5a2ba549@gmail.com>
cc'in Matthieu since he wrote the patch.
On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>> the atom %(upstream:track) can be translated if needed. This is needed
>> as we port branch.c to use ref-filter's printing API's.
>>
>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> ref-filter.c | 28 ++++++++++++++++++++++++----
>> ref-filter.h | 2 ++
>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index b47b900..944671a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -15,6 +15,26 @@
>> #include "version.h"
>> #include "wt-status.h"
>>
>> +static struct ref_msg {
>> + const char *gone;
>> + const char *ahead;
>> + const char *behind;
>> + const char *ahead_behind;
>> +} msgs = {
>> + "gone",
>> + "ahead %d",
>> + "behind %d",
>> + "ahead %d, behind %d"
>> +};
>> +
>> +void setup_ref_filter_porcelain_msg(void)
>> +{
>> + msgs.gone = _("gone");
>> + msgs.ahead = _("ahead %d");
>> + msgs.behind = _("behind %d");
>> + msgs.ahead_behind = _("ahead %d, behind %d");
>> +}
>
> Do I understand it correctly that this mechanism is here to avoid
> repeated calls into gettext, as those messages would get repeated
> over and over; otherwise one would use foo = N_("...") and _(foo),
> isn't it?
>
> I wonder if there is some way to avoid duplication here, but I don't
> see anything easy and safe (e.g. against running setup_*() twice).
>
That is the intention.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-20 15:16 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Junio C Hamano, Jacob Keller, Git mailing list
In-Reply-To: <20d067ef-9e2c-0d1f-f81a-06c154e95e4f@gmail.com>
On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>> strip=1,strip=-1, which could be
>>>>> done but ...
>>>>
>>>> ... would be unnecessary if this is the only use case:
>>>>
>>>>> strbuf_addf(&fmt,
>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>> local.buf, remote.buf);
>>>>
>>>> You can "strip to leave only 2 components" and compare the result
>>>> with refs/remotes instead, no?
>>>>
>>>
>>> Of course, my only objective was that someone would find it useful to
>>> have these two additional
>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>
>>> --
>>> Regards,
>>> Karthik Nayak
>>
>> I think having strip and rstrip make sense, (along with support for
>> negative numbers) I don't think we need to make them work together
>> unless someone is interested, since we can use strip=-2 to get the
>> behavior we need today.
>
> I am OK with multiple strips Karthik suggests, e.g.
>
> %(refname:strip=1,rstrip=-1)
>
> if it is cleanly implemented.
>
> I have a bit of trouble with these names, though. If we call one
> strip and the other rstrip, to only those who know about rstrip it
> would be clear that strip is about stripping from the left. Perhaps
> we should call it lstrip for symmetry and ease-of-remembering?
>
> refs/heads/master:lstrip=-1 => master (strip all but one level
> from the left)
>
> refs/heads/master:rstrip=-2 => refs/heads (strip all but two
> levels from the right)
>
> refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
> from the left and then strip all but one level from the right)
>
> I dunno.
We could have lstrip and rstrip as you suggested and perhaps make it work
together too. But I see this going off the scope of this series. Maybe
I'll follow up
with another series introducing these features. Since we can currently
make do with
'strip=2' I'll drop this patch from v8 of this series and pursue this
idea after this.
On Sat, Nov 19, 2016 at 3:19 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 15.11.2016 o 18:42, Junio C Hamano pisze:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> dirname makes sense. What about implementing a reverse variant of
>>> strip, which you could perform stripping of right-most components and
>>> instead of stripping by a number, strip "to" a number, ie: keep the
>>> left N most components, and then you could use something like
>>> ...
>>> I think that would be more general purpose than basename, and less confusing?
>>
>> I think you are going in the right direction. I had a similar
>> thought but built around a different axis. I.e. if strip=1 strips
>> one from the left, perhaps we want to have rstrip=1 that strips one
>> from the right, and also strip=-1 to mean strip everything except
>> one from the left and so on?. I think this and your keep (and
>> perhaps you'll have rkeep for completeness) have the same expressive
>> power. I do not offhand have a preference one over the other.
>>
>> Somehow it sounds a bit strange to me to treat 'remotes' as the same
>> class of token as 'heads' and 'tags' (I'd expect 'heads' and
>> 'remotes/origin' would be at the same level in end-user's mind), but
>> that is probably an unrelated tangent. The reason this series wants
>> to introduce :base must be to emulate an existing feature, so that
>> existing feature is a concrete counter-example that argues against
>> my "it sounds a bit strange" reaction.
>
> If it is to implement the feature where we select if to display only
> local branches (refs/heads/**), only remote-tracking branches
> (refs/remotes/**/**), or only tags (refs/tags/**), then perhaps
> ':category' or ':type' would make sense?
>
> As in '%(refname:category)', e.g.
>
> %(if:equals=heads)%(refname:category)%(then)...%(end)
>
This is also a good idea but would bring about the same confusion that Junio
was referring to, i.e.
"Somehow it sounds a bit strange to me to treat 'remotes' as the same
class of token as 'heads' and 'tags' (I'd expect 'heads' and
'remotes/origin' would be at the same level in end-user's mind), but
that is probably an unrelated tangent. The reason this series wants
to introduce :base must be to emulate an existing feature, so that
existing feature is a concrete counter-example that argues against
my "it sounds a bit strange" reaction."
So right now the rstrip/lstrip idea seems to be a good way to go about
this, but I
think that'd be after this series.
--
Regards,
Karthik Nayak
^ permalink raw reply
* [PATCH] i18n: Fixed unmatched single quote in error message
From: Jiang Xin @ 2016-11-20 12:26 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: Git List, Jiang Xin
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>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 6f0ff9e413..30b10ba143 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -248,7 +248,7 @@ static int write_message(const void *buf, size_t len, const char *filename,
}
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(&msg_file);
- return error_errno(_("could not write eol to '%s"), filename);
+ return error_errno(_("could not write eol to '%s'"), filename);
}
if (commit_lock_file(&msg_file) < 0) {
rollback_lock_file(&msg_file);
--
2.11.0.rc0.11.g127c283
^ permalink raw reply related
* Re: [PATCH v7 10/17] ref-filter: introduce refname_atom_parser_internal()
From: Karthik Nayak @ 2016-11-20 7:34 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <5df0a607-4d83-8211-457a-96d7bde46eff@gmail.com>
On Sat, Nov 19, 2016 at 3:06 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Since there are multiple atoms which print refs ('%(refname)',
>> '%(symref)', '%(push)', '%upstream'), it makes sense to have a common
>
> Minor typo; it should be: "%(upstream)"
>
Will fix that.
>> ground for parsing them. This would allow us to share implementations of
>> the atom modifiers between these atoms.
>>
>> Introduce refname_atom_parser_internal() to act as a common parsing
>> function for ref printing atoms. This would eventually be used to
>> introduce refname_atom_parser() and symref_atom_parser() and also be
>> internally used in remote_ref_atom_parser().
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +static void refname_atom_parser_internal(struct refname_atom *atom,
>> + const char *arg, const char *name)
>> +{
>> + if (!arg)
>> + atom->option = R_NORMAL;
>> + else if (!strcmp(arg, "short"))
>> + atom->option = R_SHORT;
>> + else if (skip_prefix(arg, "strip=", &arg)) {
>> + atom->option = R_STRIP;
>> + if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
>> + die(_("positive value expected refname:strip=%s"), arg);
>> + } else
> ^^^^^^
>
> It looks like you have spurious tab here.
>
That would have gone unnoticed, thanks for pointing it out.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 09/17] ref-filter: make "%(symref)" atom work with the ':short' modifier
From: Karthik Nayak @ 2016-11-20 7:31 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <ce2862d5-874b-f244-f9b3-f74e18f7ad42@gmail.com>
On Sat, Nov 19, 2016 at 3:04 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +test_expect_success 'Add symbolic ref for the following tests' '
>> + git symbolic-ref refs/heads/sym refs/heads/master
>> +'
>> +
>> +cat >expected <<EOF
>> +refs/heads/master
>> +EOF
>
> This should be inside the relevant test, not outside. In other
> patches in this series you are putting setup together with the
> rest of test, by using "cat >expected <<-\EOF".
>
Ah! That's because I was just trying to keep it consistent. These tests
are added to t6300, where the `expected` block is usually outside the tests
themselves.
The other tests in the series are added to t6302, where we keep the `expected`
block within the tests themselves.
>> +
>> +test_expect_success 'Verify usage of %(symref) atom' '
>> + git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
>
> This should be spelled " >actual", rather than " > actual"; there
> should be no space between redirection and file name.
>
>> + test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> +heads/master
>> +EOF
>> +
>> +test_expect_success 'Verify usage of %(symref:short) atom' '
>> + git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
>> + test_cmp expected actual
>> +'
>
> Same here.
>
Will remove the space between '>' and 'actual', Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Karthik Nayak @ 2016-11-20 7:23 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <37c2cbf2-7160-49d7-f8f1-3b65d9ecf9ec@gmail.com>
On Sat, Nov 19, 2016 at 1:28 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:11, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Implement %(if:equals=<string>) wherein the if condition is only
>> satisfied if the value obtained between the %(if:...) and %(then) atom
>> is the same as the given '<string>'.
>>
>> Similarly, implement (if:notequals=<string>) wherein the if condition
>> is only satisfied if the value obtained between the %(if:...) and
>> %(then) atom is differnt from the given '<string>'.
> ^^^^^^^^
>
> s/differnt/different/ <-- typo
>
Will change that.
>>
>> Add tests and Documentation for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 3 +++
>> ref-filter.c | 43 +++++++++++++++++++++++++++++++++-----
>> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++
>> 3 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fed8126..b7b8560 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -155,6 +155,9 @@ if::
>> evaluating the string before %(then), this is useful when we
>> use the %(HEAD) atom which prints either "*" or " " and we
>> want to apply the 'if' condition only on the 'HEAD' ref.
>
> So %(if) is actually %(if:notempty) ? Just kidding.
>
It's not a bug, it's a feature ;)
>> + Append ":equals=<string>" or ":notequals=<string>" to compare
>> + the value between the %(if:...) and %(then) atoms with the
>> + given string.
>>
>> In addition to the above, for commit and tag objects, the header
>> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>> };
>>
>> struct if_then_else {
>> + const char *if_equals,
>> + *not_equals;
>
> I guess using anonymous structs from C11 here...
>
>> unsigned int then_atom_seen : 1,
>> else_atom_seen : 1,
>> condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> + struct {
>> + const char *if_equals,
>> + *not_equals;
>> + } if_then_else;
>
> ...to avoid code duplication there is rather out of question?
>
I believe it holds better context without the use of anonymous structs/unions.
But that's my perspective, I wouldn't mind changing it.
>> enum { O_FULL, O_SHORT } objectname;
>> } u;
>> } *used_atom;
>> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>> string_list_clear(¶ms, 0);
>> }
>>
>> +static void if_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> + if (!arg)
>> + return;
>> + else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
>> + ;
>> + else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
>> + ;
>
> Those ';' should be perfectly aligned, isn't it?
>
This should be dropped with the new changes made on this patch.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-11-20 7:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller
In-Reply-To: <xmqqd1hsl5zm.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 19, 2016 at 5:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks, will add it in.
>
> OK, here is a reroll of what I sent earlier in
>
> http://public-inbox.org/git/<xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
>
> but rebased so that it can happen as a preparatory bugfix before
> your series.
>
> The bug dates back to the very original implementation of %(HEAD) in
> 7a48b83219 ("for-each-ref: introduce %(HEAD) asterisk marker",
> 2013-11-18) and was moved to the current location in the v2.6 days
> at c95b758587 ("ref-filter: move code from 'for-each-ref'",
> 2015-06-14).
>
I'll rebase on this patch. Thanks for your efforts.
I assume you'll be merging it in before my series, so I wont be making
it a part of my series.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Prereleases of Git for Windows
From: Johannes Schindelin @ 2016-11-19 15:10 UTC (permalink / raw)
To: git-for-windows; +Cc: git
Hi all,
I debated whether I should clutter the mailing list by announcing the
prereleases I published based on the v2.11.0-rc* releases of upstream Git.
I ended up deciding to announce that I won't announce them on the mailing
list, but only on Twitter [*1*]. After announcing the latest prerelease,
of course:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc2.windows.1
Please give this a good beating and open tickets for issues you encounter
(unless you find that there are already open or closed tickets for the
same bug).
Thank you,
Johannes
Footnote: https://twitter.com/GitForWindows
^ permalink raw reply
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