* 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 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: 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 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 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 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: 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 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
* [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] 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
* 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
* 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 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
* --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
* [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
* [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 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 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
* 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
* 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 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: [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] 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 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 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
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