threads / patch / 46436

patchgit-contacts: Add recognition of Reported-by

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

## tl;dr

8 messages between Jul 21, 2017 and Jul 27, 2017. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Eric Blake· Jul 21, 2017, 14:15 UTC · lore

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 <eblake@redhat.com>
---
 contrib/contacts/git-contacts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/contacts/git-contacts +1 −1
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 {

-- 
2.13.3
Junio C Hamano· Jul 21, 2017, 14:37 UTC · re: Eric Blake · lore

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

Eric Blake <eblake@redhat.com> writes:
Show 6 quoted lines
> 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 <eblake@redhat.com>
> ---

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.
Show 16 quoted lines
>  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· Jul 21, 2017, 15:27 UTC · re: Junio C Hamano · lore

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

On 07/21/2017 09:37 AM, Junio C Hamano wrote:
Show 15 quoted lines
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>> ---
> 
> 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).

Show 20 quoted lines
> 
> 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
Junio C Hamano· Jul 21, 2017, 16:03 UTC · re: Eric Blake · lore

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

Eric Blake <eblake@redhat.com> writes:
Show 7 quoted lines
> 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.
Jeff King· Jul 24, 2017, 18:31 UTC · re: Junio C Hamano · lore

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

On Fri, Jul 21, 2017 at 09:03:16AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> Eric Blake <eblake@redhat.com> 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
Junio C Hamano· Jul 24, 2017, 19:29 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
>> 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 <key, value> pairs so that the user of the tool can further process the value differently depending on the key, or something?

Jeff King· Jul 24, 2017, 19:36 UTC · re: Junio C Hamano · lore

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

On Mon, Jul 24, 2017 at 12:29:04PM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> > 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
> <key, value> 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
Junio C Hamano· Jul 27, 2017, 16:45 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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.

← back to recent threads