Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Jacob Keller]

On Tue, Nov 17, 2015 at 4:26 AM, SZEDER Gábor  wrote:
>
> Quoting Eric Sunshine :
>
>> On Tue, Nov 17, 2015 at 12:10:35AM +, Keller, Jacob E wrote:
>>>
>>> On Mon, 2015-11-16 at 18:50 -0500, Eric Sunshine wrote:
>>> > It should be possible to extract the alias within the shell itself
>>> > without a separate process. For instance:
>>> >
>>> > read alias rest
>>> >
>>> > will leave the first token in $alias and the remainder of the line in
>>> > $rest, and it's all done within the shell process.
>
>
> Actually, putting this read in a while loop and feeding 'git send-email's
> output into that does fork() a subshell:
>

Yea, I realized this. One extra subshell isn't a big deal but if we
can eliminate that it is good.

>   $ echo "outside: $BASH_SUBSHELL" |while read line ; do echo "$line
> inside: $BASH_SUBSHELL" ; done
>   outside: 0  inside: 1
>
>>> I'll look into this :)
>>
>>
>> My reason for asking is concern about scripts possibly breaking if
>> someone comes along and wants to "fix" --dump-aliases to also dump
>> the alias expansions. One possibility is just to punt today and say
>> that when that feature is needed in the future, then that someone can
>> add a --verbose option to complement --dump-aliases which would emit
>> the alias expansions as well. One nice thing about punting at this
>> point is that we don't (today) have to define a format for the output
>> of the expansions.
>
>
> I think we should cross the bridge when we get to it.

Agreed.

>
> However, we could still be nice to that brave soul who might want to cross
> it in the future, and since at this point we are interested in listing only
> alias names, perhaps we should not appropriate the broader '--dump-alias'
> option, but go with the more specific '--dump-alias-names' instead.
>

Disagree. I think the best approach for expanding --dump-aliases
output is turning it into a keyword-option argument such as:

--dump-aliases=format

and we can say "historic reasons the format is always names only",
then we can extend the support that way. I don't really think that
"--dump-alias-names" is nice since then we'd need separate option if
this ever does change, and it's rather long if it never changes.

I suspect it won't be something people need either.

Regards,
Jake

>
> Gábor
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[SZEDER Gábor]

Quoting Eric Sunshine :


On Tue, Nov 17, 2015 at 12:10:35AM +, Keller, Jacob E wrote:

On Mon, 2015-11-16 at 18:50 -0500, Eric Sunshine wrote:
> It should be possible to extract the alias within the shell itself
> without a separate process. For instance:
>
> read alias rest
>
> will leave the first token in $alias and the remainder of the line in
> $rest, and it's all done within the shell process.


Actually, putting this read in a while loop and feeding 'git  
send-email's output into that does fork() a subshell:


  $ echo "outside: $BASH_SUBSHELL" |while read line ; do echo "$line   
inside: $BASH_SUBSHELL" ; done

  outside: 0  inside: 1


I'll look into this :)


My reason for asking is concern about scripts possibly breaking if
someone comes along and wants to "fix" --dump-aliases to also dump
the alias expansions. One possibility is just to punt today and say
that when that feature is needed in the future, then that someone can
add a --verbose option to complement --dump-aliases which would emit
the alias expansions as well. One nice thing about punting at this
point is that we don't (today) have to define a format for the output
of the expansions.


I think we should cross the bridge when we get to it.

However, we could still be nice to that brave soul who might want to  
cross it in the future, and since at this point we are interested in  
listing only alias names, perhaps we should not appropriate the  
broader '--dump-alias' option, but go with the more specific  
'--dump-alias-names' instead.



Gábor

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Jacob Keller]

On Mon, Nov 16, 2015 at 11:20 PM, Eric Sunshine  wrote:
> Since git-send-email.perl already configures GetOpt::Long with the
> 'pass_through' option, one possibility would be to invoke
> GetOptions() once for --list-aliases (or --dump-aliases), and then
> again for the normal options. Doing so may be a bit ugly; on the
> other hand, it does indicate pretty clearly that --list-aliases is a
> distinct "mode" of operation. On top of your patch, it might look
> something like this:
>
> --- 8< ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index ee14894..cada5ea 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -296,6 +296,12 @@ $SIG{INT}  = \_handler;
>
>  my $help;
>  my $rc = GetOptions("h" => \$help,
> +   "list-aliases" => \$list_aliases);
> +usage() unless $rc;
> +die "--list-aliases incompatible with other options\n"
> +   if !$help and $list_aliases and @ARGV;
> +
> +$rc = GetOptions(
> "sender|from=s" => \$sender,
>  "in-reply-to=s" => \$initial_reply_to,
> "subject=s" => \$initial_subject,
> @@ -349,7 +355,6 @@ my $rc = GetOptions("h" => \$help,
> "force" => \$force,
> "xmailer!" => \$use_xmailer,
> "no-xmailer" => sub {$use_xmailer = 0},
> -"list-aliases" => \$list_aliases,
>  );
>
>  usage() if $help;
> --- 8< ---
>
> Though, it may be overkill for this minor use-case.

I actually really like this approach. I will squash it in.

Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:56 -0500, Eric Sunshine wrote:
> On Mon, Nov 16, 2015 at 6:50 PM, Eric Sunshine  om> wrote:
> > Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> > exclusive with many of the other options? New tests would check
> > such
> > exclusivity as well.
> 
> In fact, while I agree with Szeder that it makes sense to re-use
> send-email's aliases parsing functionality (and was going to suggest
> the same, but he beat me to it), this new option is awfully
> orthogonal
> to the overall purpose of send-email, thus, doesn't really fit in
> well
> and almost cries out for a command of its own which would be used by
> send-email and bash completion (though I'm not convinced that it's
> worth going that route for this one minor use-case).

I don't think it's worth it at this point, because we'd have to extract
out the alias parsing logic from send-email, which is not easy.

The option is pretty orthogonal to git-send-email, but until/unless
git-send-email is re-implemented in C, i don't really see value in
trying to separate the logic out... That is a lot more effort for very
little gain.

Regards,
JakeN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Eric Sunshine]

On Mon, Nov 16, 2015 at 6:40 PM, Keller, Jacob E
 wrote:
> On Mon, 2015-11-16 at 18:30 -0500, Eric Sunshine wrote:
>> Also, is it possible that some consumer down the road might want
>> richer output which includes the expansion of each alias? For
>> instance, it could emit the alias name as the first token on each
>> line
>> and the expansion as the remainder. Consumers interested in only the
>> alias name would grab the first token on the line and ignore
>> everything else.
>
> Maybe? The problem with printing the full address is that it may not be
> quoted or similar, and it makes the bash completion require an extra
> parameter.. I am not sure how valuable the alias expansion would be for
> use? The main concern I have is we'd need to use another process on top
> to extract only alias names.

It should be possible to extract the alias within the shell itself
without a separate process. For instance:

read alias rest

will leave the first token in $alias and the remainder of the line in
$rest, and it's all done within the shell process.

>> New test(s) seem to be missing.
>
> I had removed the tests from the old version because they weren't
> necessary anymore. New ones wouldn't hurt here either, though.. I'll
> work on that.

I'm not sure which tests you mean, but I was referring to tests to
make sure that git-send-email recognizes --list-aliases (or
--dump-aliases if you switch to that) and that it produces the
expected output in the expected format.

Also, shouldn't --list-aliases (or --dump-aliases) be mutually
exclusive with many of the other options? New tests would check such
exclusivity as well.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:50 -0500, Eric Sunshine wrote:
> It should be possible to extract the alias within the shell itself
> without a separate process. For instance:
> 
> read alias rest
> 
> will leave the first token in $alias and the remainder of the line in
> $rest, and it's all done within the shell process.
> 

I'll look into this :)

> > > New test(s) seem to be missing.
> > 
> > I had removed the tests from the old version because they weren't
> > necessary anymore. New ones wouldn't hurt here either, though..
> > I'll
> > work on that.
> 
> I'm not sure which tests you mean, but I was referring to tests to
> make sure that git-send-email recognizes --list-aliases (or
> --dump-aliases if you switch to that) and that it produces the
> expected output in the expected format.
> 

Yep, I added some in my respin.

> Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> exclusive with many of the other options? New tests would check such
> exclusivity as well.

I am at a loss for how to do that correctly in the perl. Help would be
appreciated here.

Regards,
JakN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:30 -0500, Eric Sunshine wrote:
> On Sun, Nov 15, 2015 at 3:22 PM, Jacob Keller  om> wrote:
> > Add an option "list-aliases" which changes the default behavior of
> > git-send-email. This mode will simply read the alias files
> > configured by
> > sendemail.aliasesfile and sendemail.aliasfiletype and print a list
> > of
> > all known aliases. The intended usecase for this option is the
> > bash-completion script which will use it to autocomplete aliases on
> > the
> > options which take addresses.
> 
> As this is primarily a plumbing option, I wonder if --dump-aliases
> might be a more suitable name.
> 

Sure that would be reasonable.

> Also, is it possible that some consumer down the road might want
> richer output which includes the expansion of each alias? For
> instance, it could emit the alias name as the first token on each
> line
> and the expansion as the remainder. Consumers interested in only the
> alias name would grab the first token on the line and ignore
> everything else.
> 

Maybe? The problem with printing the full address is that it may not be
quoted or similar, and it makes the bash completion require an extra
parameter.. I am not sure how valuable the alias expansion would be for
use? The main concern I have is we'd need to use another process on top
to extract only alias names.

> > Signed-off-by: Jacob Keller 
> > ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > @@ -101,6 +102,9 @@ git send-email [options]  > rev-list options >
> >   `git format-patch` ones.
> >  --force* Send even if safety checks
> > would prevent it.
> > 
> > +  Information:
> > +--list-aliases * read the aliases from
> > configured alias files
> 
> This description is odd. It seems to imply that aliases will be
> loaded
> (and used) only if this option is given, and says nothing about its
> actual purpose of dumping the aliases.
> 

I can re-word this.

> Also, with one exception, all the other option descriptions are
> capitalized. This probably ought to follow suit.
> 
> > +if ($list_aliases) {
> > +print $_,"\n" for (keys %aliases);
> > +exit(0);
> > +}
> 
> New test(s) seem to be missing.
> 

I had removed the tests from the old version because they weren't
necessary anymore. New ones wouldn't hurt here either, though.. I'll
work on that.

Regards,
Jake

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Eric Sunshine]

On Sun, Nov 15, 2015 at 3:22 PM, Jacob Keller  wrote:
> Add an option "list-aliases" which changes the default behavior of
> git-send-email. This mode will simply read the alias files configured by
> sendemail.aliasesfile and sendemail.aliasfiletype and print a list of
> all known aliases. The intended usecase for this option is the
> bash-completion script which will use it to autocomplete aliases on the
> options which take addresses.

As this is primarily a plumbing option, I wonder if --dump-aliases
might be a more suitable name.

Also, is it possible that some consumer down the road might want
richer output which includes the expansion of each alias? For
instance, it could emit the alias name as the first token on each line
and the expansion as the remainder. Consumers interested in only the
alias name would grab the first token on the line and ignore
everything else.

> Signed-off-by: Jacob Keller 
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> @@ -101,6 +102,9 @@ git send-email [options]  options >
>   `git format-patch` ones.
>  --force* Send even if safety checks would 
> prevent it.
>
> +  Information:
> +--list-aliases * read the aliases from configured alias 
> files

This description is odd. It seems to imply that aliases will be loaded
(and used) only if this option is given, and says nothing about its
actual purpose of dumping the aliases.

Also, with one exception, all the other option descriptions are
capitalized. This probably ought to follow suit.

> +if ($list_aliases) {
> +print $_,"\n" for (keys %aliases);
> +exit(0);
> +}

New test(s) seem to be missing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Eric Sunshine]

On Mon, Nov 16, 2015 at 6:50 PM, Eric Sunshine  wrote:
> Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> exclusive with many of the other options? New tests would check such
> exclusivity as well.

In fact, while I agree with Szeder that it makes sense to re-use
send-email's aliases parsing functionality (and was going to suggest
the same, but he beat me to it), this new option is awfully orthogonal
to the overall purpose of send-email, thus, doesn't really fit in well
and almost cries out for a command of its own which would be used by
send-email and bash completion (though I'm not convinced that it's
worth going that route for this one minor use-case).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Eric Sunshine]

On Tue, Nov 17, 2015 at 12:10:35AM +, Keller, Jacob E wrote:
> On Mon, 2015-11-16 at 18:50 -0500, Eric Sunshine wrote:
> > It should be possible to extract the alias within the shell itself
> > without a separate process. For instance:
> > 
> > read alias rest
> > 
> > will leave the first token in $alias and the remainder of the line in
> > $rest, and it's all done within the shell process.
> 
> I'll look into this :)

My reason for asking is concern about scripts possibly breaking if
someone comes along and wants to "fix" --dump-aliases to also dump
the alias expansions. One possibility is just to punt today and say
that when that feature is needed in the future, then that someone can
add a --verbose option to complement --dump-aliases which would emit
the alias expansions as well. One nice thing about punting at this
point is that we don't (today) have to define a format for the output
of the expansions. If we did want to think about it, one verbose,
non-ambiguous format would be to show the alias name on a line by
itself, and each expansion value on a line by itself indented by a
tab. For instance:

managers
bob
fred
devs
jane
john

> > Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> > exclusive with many of the other options? New tests would check such
> > exclusivity as well.
> 
> I am at a loss for how to do that correctly in the perl. Help would be
> appreciated here.

Since git-send-email.perl already configures GetOpt::Long with the
'pass_through' option, one possibility would be to invoke
GetOptions() once for --list-aliases (or --dump-aliases), and then
again for the normal options. Doing so may be a bit ugly; on the
other hand, it does indicate pretty clearly that --list-aliases is a
distinct "mode" of operation. On top of your patch, it might look
something like this:

--- 8< ---
diff --git a/git-send-email.perl b/git-send-email.perl
index ee14894..cada5ea 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -296,6 +296,12 @@ $SIG{INT}  = \_handler;
 
 my $help;
 my $rc = GetOptions("h" => \$help,
+   "list-aliases" => \$list_aliases);
+usage() unless $rc;
+die "--list-aliases incompatible with other options\n"
+   if !$help and $list_aliases and @ARGV;
+
+$rc = GetOptions(
"sender|from=s" => \$sender,
 "in-reply-to=s" => \$initial_reply_to,
"subject=s" => \$initial_subject,
@@ -349,7 +355,6 @@ my $rc = GetOptions("h" => \$help,
"force" => \$force,
"xmailer!" => \$use_xmailer,
"no-xmailer" => sub {$use_xmailer = 0},
-"list-aliases" => \$list_aliases,
 );
 
 usage() if $help;
--- 8< ---

Though, it may be overkill for this minor use-case.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Jacob Keller]

From: Jacob Keller 

Add an option "list-aliases" which changes the default behavior of
git-send-email. This mode will simply read the alias files configured by
sendemail.aliasesfile and sendemail.aliasfiletype and print a list of
all known aliases. The intended usecase for this option is the
bash-completion script which will use it to autocomplete aliases on the
options which take addresses.

Signed-off-by: Jacob Keller 
---
 Documentation/git-send-email.txt | 10 ++
 git-send-email.perl  | 11 +++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index b9134d234f53..481770d72e13 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 
 [verse]
 'git send-email' [options] ...
+'git send-email' --list-aliases
 
 
 DESCRIPTION
@@ -387,6 +388,15 @@ default to '--validate'.
Send emails even if safety checks would prevent it.
 
 
+Information
+~~~
+
+--list-aliases::
+   Instead of the standard operation, list all aliases found in the
+   configured alias file(s), and then exit. See 'sendemail.aliasesfile'
+   for more information about aliases.
+
+
 CONFIGURATION
 -
 
diff --git a/git-send-email.perl b/git-send-email.perl
index e907e0eacf31..ee14894b172b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -46,6 +46,7 @@ package main;
 sub usage {
print < \$help,
"force" => \$force,
"xmailer!" => \$use_xmailer,
"no-xmailer" => sub {$use_xmailer = 0},
+"list-aliases" => \$list_aliases,
 );
 
 usage() if $help;
@@ -551,6 +557,11 @@ if (@alias_files and $aliasfiletype and defined 
$parse_alias{$aliasfiletype}) {
}
 }
 
+if ($list_aliases) {
+print $_,"\n" for (keys %aliases);
+exit(0);
+}
+
 # is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if
 # $f is a revision list specification to be passed to format-patch.
 sub is_format_patch_arg {
-- 
2.6.3.491.g3e3f6ce

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html