Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL

[Torsten Bögershausen]

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.





Signed-off-by: Mike Fisher 
---
 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;
 }
 }

Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL

[brian m. carlson]

On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
> 
> 

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


signature.asc
Description: PGP signature

[PATCH] Remove dependency on deprecated Net::SMTP::SSL

[Mike Fisher]

Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL:



Signed-off-by: Mike Fisher 
---
 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)

Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Stephan Beyer]

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.

Re: Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?

[Jeff King]

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

Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C

[Stephan Beyer]

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", ,
>N_("verify the next bisection state then find the next 
> bisection state"), BISECT_AUTO_NEXT),
> + OPT_CMDMODE(0, "bisect-autostart", ,
> +  N_("start the bisection if BISECT_START empty or 
> missing"), BISECT_AUTOSTART),

The word "is" is missing.

~Stephan

Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Stephan Beyer]

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=", )) {
> + 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(, 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

Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

[Stephan Beyer]

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=", )) {
>   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

Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms

[Jakub Narębski]

W dniu 20.11.2016 o 18:32, Junio C Hamano pisze:
> Karthik Nayak  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=[,]',
where if length is omitted it means to the end; perhaps with special
casing (as in Perl) of negative  and/or negative .

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

Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms

[Junio C Hamano]

Karthik Nayak  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.

Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms

[Karthik Nayak]

On Sun, Nov 20, 2016 at 8:46 PM, Karthik Nayak  wrote:
> On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano  wrote:
>> Jacob Keller  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(,
>> "%%(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

Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output

[Karthik Nayak]

cc'in Matthieu since he wrote the patch.

On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski  wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak 
>>
>> 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 
>> Mentored-by: Christian Couder 
>> Mentored-by: Matthieu Moy 
>> Signed-off-by: Karthik Nayak 
>> ---
>>  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

Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms

[Karthik Nayak]

On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano  wrote:
> Jacob Keller  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(,
> "%%(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  wrote:
> W dniu 15.11.2016 o 18:42, Junio C Hamano pisze:
>> Jacob Keller  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

[PATCH] i18n: Fixed unmatched single quote in error message

[Jiang Xin]

Fixed unmatched single quote introduced by commit:

 * f56fffef9a sequencer: teach write_message() to append an optional LF

Signed-off-by: Jiang Xin 
---
 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(_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(_file) < 0) {
rollback_lock_file(_file);
-- 
2.11.0.rc0.11.g127c283