Re: [PATCH] git-contacts: Add recognition of Reported-by

[Junio C Hamano]

Jeff King  writes:

> Yeah, I don't think interpret-trailers is currently a useful tool for
> looking at an extra config key like that. I'd expect it would need to be
> extended, or a new tool added, or perhaps existing tools would need to
> learn more about how trailers work (e.g., it would be nice if git-log
> could do matching or even print them via %(trailer:reported-by)
> placeholders or something). I think there's a lot of potential work in
> that area.
>
> Of course git-contacts (or send-email) _can_ just look look at all of
> the trailer.*.autocc config and try to match those manually. But the
> point of having trailer config, I think, is that we should stop doing
> ad-hoc parsing and have a tool for manipulating and querying trailers.
> If interpret-trailers isn't up to the task yet, I'd rather see work go
> there.
>
> But that (manual parsing) is basically how the current cc and s-o-b
> trailers implemented inside of git-send-email, so I don't think it would
> be the end of the world as a quick hack that could later be expanded to
> use the trailer infrastructure.

OK.

In any case, I think not CC'ing the reporter would be a bug, and an
update to the "contacts" script (in contrib/) to use an improved
convention and interpret-trailers tool can be built on top of the
version Eric posted, so let's take the patch anyway for now.

Thanks.

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Jeff King]

On Mon, Jul 24, 2017 at 12:29:04PM -0700, Junio C Hamano wrote:

> > There's already some prior art around trailers in the trailer.* config.
> > I wonder if it would make sense to claim a new key there, like:
> >
> >   git config trailer.Reported-by.autocc true
> >
> > If "Reported-by" is a trailer that your project uses, then there may be
> > some benefit to setting up other config related to it, and this would
> > mesh nicely. And then potentially other programs besides git-contacts
> > would want to respect that flag (perhaps send-email would even want to
> > do it itself; I think it already respects cc and s-o-b headers).
> 
> Sounds like a good suggestion.  But...
> 
> If I understand your proposal, the trailer stuff would still not
> care what value .autocc is set to while doing its own thing, but the
> programs that read the text file that the trailer can work on would
> pay attention to it, and they individually have to do so?  Perhaps
> there is a need for another mode "interpret-trailers" is told to run
> in, where it is given a text file with trailers in it and is told to
> show only the value that has .autocc bit on?  Alternatively, yield
>  pairs so that the user of the tool can further process
> the value differently depending on the key, or something?

Yeah, I don't think interpret-trailers is currently a useful tool for
looking at an extra config key like that. I'd expect it would need to be
extended, or a new tool added, or perhaps existing tools would need to
learn more about how trailers work (e.g., it would be nice if git-log
could do matching or even print them via %(trailer:reported-by)
placeholders or something). I think there's a lot of potential work in
that area.

Of course git-contacts (or send-email) _can_ just look look at all of
the trailer.*.autocc config and try to match those manually. But the
point of having trailer config, I think, is that we should stop doing
ad-hoc parsing and have a tool for manipulating and querying trailers.
If interpret-trailers isn't up to the task yet, I'd rather see work go
there.

But that (manual parsing) is basically how the current cc and s-o-b
trailers implemented inside of git-send-email, so I don't think it would
be the end of the world as a quick hack that could later be expanded to
use the trailer infrastructure.

-Peff

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Junio C Hamano]

Jeff King  writes:

>> I also should have mentioned the need for a way to say "remove all
>> hardcoded default and start from scratch".
>
> There's already some prior art around trailers in the trailer.* config.
> I wonder if it would make sense to claim a new key there, like:
>
>   git config trailer.Reported-by.autocc true
>
> If "Reported-by" is a trailer that your project uses, then there may be
> some benefit to setting up other config related to it, and this would
> mesh nicely. And then potentially other programs besides git-contacts
> would want to respect that flag (perhaps send-email would even want to
> do it itself; I think it already respects cc and s-o-b headers).

Sounds like a good suggestion.  But...

If I understand your proposal, the trailer stuff would still not
care what value .autocc is set to while doing its own thing, but the
programs that read the text file that the trailer can work on would
pay attention to it, and they individually have to do so?  Perhaps
there is a need for another mode "interpret-trailers" is told to run
in, where it is given a text file with trailers in it and is told to
show only the value that has .autocc bit on?  Alternatively, yield
 pairs so that the user of the tool can further process
the value differently depending on the key, or something?

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Jeff King]

On Fri, Jul 21, 2017 at 09:03:16AM -0700, Junio C Hamano wrote:

> Eric Blake  writes:
> 
> > You mean, something like
> >
> > git config --add contacts.autocc Reported-by
> > git config --add contacts.autocc Suggested-by
> >
> > where contacts.autocc would be a new multi-valued config option
> > specifying additional Tag: patterns to scrape out of the commit message?
> 
> Yes, something along that line, and you are correct to point out
> that I should have mentioned the need for command-line override.
> 
> In fact, if you anticipate that the primary use of this contributed
> script is as "send-email --cccmd", then we probably are better off
> doing this without any configuration variables, but just add the
> mechanism for command-line override of the hardcoded default.
> 
> I also should have mentioned the need for a way to say "remove all
> hardcoded default and start from scratch".

There's already some prior art around trailers in the trailer.* config.
I wonder if it would make sense to claim a new key there, like:

  git config trailer.Reported-by.autocc true

If "Reported-by" is a trailer that your project uses, then there may be
some benefit to setting up other config related to it, and this would
mesh nicely. And then potentially other programs besides git-contacts
would want to respect that flag (perhaps send-email would even want to
do it itself; I think it already respects cc and s-o-b headers).

-Peff

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Junio C Hamano]

Eric Blake  writes:

> You mean, something like
>
> git config --add contacts.autocc Reported-by
> git config --add contacts.autocc Suggested-by
>
> where contacts.autocc would be a new multi-valued config option
> specifying additional Tag: patterns to scrape out of the commit message?

Yes, something along that line, and you are correct to point out
that I should have mentioned the need for command-line override.

In fact, if you anticipate that the primary use of this contributed
script is as "send-email --cccmd", then we probably are better off
doing this without any configuration variables, but just add the
mechanism for command-line override of the hardcoded default.

I also should have mentioned the need for a way to say "remove all
hardcoded default and start from scratch".

> Also, putting it in 'git config' still means that it is a per-developer
> responsibility to choose which patterns to add to their list.  Is there
> any easy way to make a particular repository supply the same list for
> all developers who check it out, without them having to munge things?

That is a good point, but we should be very careful.  "Let's add
whatever configuration the project supplies to the user's repository
upon cloning" is an absolute no-no, as a malicious project can ship
something like [alias] "co" = "!rm -rf ." and unsuspecting victim to
blindly add it to the configuration.  

A standard practice we encourage is to ship a file that records the
suggested set of configuration variables as part of the source tree
and mention how to add these to their repository in README (which
you are already using to talk about how to contribute to the
project, etc.).

That would give them a chance to inspect what potential damage the
project suggestion will make to their environment (hopefully, there
is none, but the user must be given a chance to ensure that).

The extra lines you may need in your README may become something like

Run this in your copy of the project:

$ git config sendemail.cccmd "git contact --cc Suggested-by"

with such a scheme.

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Eric Blake]

On 07/21/2017 09:37 AM, Junio C Hamano wrote:
> Eric Blake  writes:
> 
>> It's nice to cc someone that reported a bug, in order to let
>> them know that a fix is being considered, and possibly even
>> get their help in reviewing/testing the patch.
>>
>> Signed-off-by: Eric Blake 
>> ---
> 
> I don't know if this new one deserves to be part of the hardcoded
> defaults; it would be different between the projects and depends on
> their convention.  I notice that there is no way to configure this
> script and I suspect that it would be a more generally useful update
> to have it read a configuration variable that lists what kind of sob
> like things to take addresses from.

You mean, something like

git config --add contacts.autocc Reported-by
git config --add contacts.autocc Suggested-by

where contacts.autocc would be a new multi-valued config option
specifying additional Tag: patterns to scrape out of the commit message?

The idea seems reasonable, except that I have less experience with
writing patches that interact with git config than I do for my one-liner
attempt, so I would welcome any help from someone with more familiarity
with the code base.

Also, putting it in 'git config' still means that it is a per-developer
responsibility to choose which patterns to add to their list.  Is there
any easy way to make a particular repository supply the same list for
all developers who check it out, without them having to munge things?

Also, I'm worried about sendemail.cccmd - that's a script that could
usefully call 'git contacts' under the hood, but that argues that there
should be a command-line override (and not just 'git config') for
choosing an alternative list of autocc tag patterns on a per-invocation
basis.  Again, it requires a per-developer setup to wire in a cccmd, but
telling developers a one-liner config to set up the command is easier
than telling them multiple lines for contacts.autocc.

And while we're on the topic of per-project useful defaults, it would be
nice if diff.orderFile could easily be set to a per-project default,
rather than requiring per-developer efforts to set that up.

But yes, I _definitely_ want to be able for a given project to easily
autocc the tags that it finds appropriate.  Your point that different
projects have different tags makes total sense (I'm hoping to use
Suggested-by as one of the tags in qemu, but agree that it is not as
easy to argue that Suggested-by should be in the hardcoded defaults,
which is why my initial submission only added Reported-by).

> 
> Thanks.
> 
>>  contrib/contacts/git-contacts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts
>> index dbe2abf27..85ad732fc 100755
>> --- a/contrib/contacts/git-contacts
>> +++ b/contrib/contacts/git-contacts
>> @@ -11,7 +11,7 @@ use IPC::Open2;
>>
>>  my $since = '5-years-ago';
>>  my $min_percent = 10;
>> -my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc/i;
>> +my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc|Reported-by/i;
>>  my %seen;
>>
>>  sub format_contact {
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] git-contacts: Add recognition of Reported-by

[Junio C Hamano]

Eric Blake  writes:

> It's nice to cc someone that reported a bug, in order to let
> them know that a fix is being considered, and possibly even
> get their help in reviewing/testing the patch.
>
> Signed-off-by: Eric Blake 
> ---

I don't know if this new one deserves to be part of the hardcoded
defaults; it would be different between the projects and depends on
their convention.  I notice that there is no way to configure this
script and I suspect that it would be a more generally useful update
to have it read a configuration variable that lists what kind of sob
like things to take addresses from.

Thanks.

>  contrib/contacts/git-contacts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts
> index dbe2abf27..85ad732fc 100755
> --- a/contrib/contacts/git-contacts
> +++ b/contrib/contacts/git-contacts
> @@ -11,7 +11,7 @@ use IPC::Open2;
>
>  my $since = '5-years-ago';
>  my $min_percent = 10;
> -my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc/i;
> +my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc|Reported-by/i;
>  my %seen;
>
>  sub format_contact {