{"thread":{"id":"46436","subject":"[PATCH] git-contacts: Add recognition of Reported-by","startedAt":"2017-07-21T14:15:36Z","lastAt":"2017-07-27T16:46:09Z","messageCount":8,"participants":["Eric Blake","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324859","messageId":"20170721141530.25907-1-eblake@redhat.com","threadId":"46436","inReplyTo":null,"subject":"[PATCH] git-contacts: Add recognition of Reported-by","fromName":"Eric Blake","fromEmail":"eblake@redhat.com","sentAt":"2017-07-21T14:15:30Z","receivedAt":"2017-07-21T14:15:36Z","isPatch":true,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"It's nice to cc someone that reported a bug, in order to let\nthem know that a fix is being considered, and possibly even\nget their help in reviewing/testing the patch.\n\nSigned-off-by: Eric Blake \n---\n contrib/contacts/git-contacts | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts\nindex dbe2abf27..85ad732fc 100755\n--- a/contrib/contacts/git-contacts\n+++ b/contrib/contacts/git-contacts\n@@ -11,7 +11,7 @@ use IPC::Open2;\n\n my $since = '5-years-ago';\n my $min_percent = 10;\n-my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc/i;\n+my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc|Reported-by/i;\n my %seen;\n\n sub format_contact {\n-- \n2.13.3\n\n"},{"id":"324862","messageId":"xmqqbmodj1pa.fsf@gitster.mtv.corp.google.com","threadId":"46436","inReplyTo":"20170721141530.25907-1-eblake@redhat.com","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-21T14:37:21Z","receivedAt":"2017-07-21T14:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Blake writes:\n\n> It's nice to cc someone that reported a bug, in order to let\n> them know that a fix is being considered, and possibly even\n> get their help in reviewing/testing the patch.\n>\n> Signed-off-by: Eric Blake \n> ---\n\nI don't know if this new one deserves to be part of the hardcoded\ndefaults; it would be different between the projects and depends on\ntheir convention. I notice that there is no way to configure this\nscript and I suspect that it would be a more generally useful update\nto have it read a configuration variable that lists what kind of sob\nlike things to take addresses from.\n\nThanks.\n\n> contrib/contacts/git-contacts | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts\n> index dbe2abf27..85ad732fc 100755\n> --- a/contrib/contacts/git-contacts\n> +++ b/contrib/contacts/git-contacts\n> @@ -11,7 +11,7 @@ use IPC::Open2;\n>\n> my $since = '5-years-ago';\n> my $min_percent = 10;\n> -my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc/i;\n> +my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc|Reported-by/i;\n> my %seen;\n>\n> sub format_contact {\n"},{"id":"324867","messageId":"a8b47a45-0100-dbef-0bff-fdfdb9cbccb4@redhat.com","threadId":"46436","inReplyTo":"xmqqbmodj1pa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Eric Blake","fromEmail":"eblake@redhat.com","sentAt":"2017-07-21T15:27:01Z","receivedAt":"2017-07-21T15:28:14Z","isPatch":true,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"On 07/21/2017 09:37 AM, Junio C Hamano wrote:\n> Eric Blake writes:\n> \n>> It's nice to cc someone that reported a bug, in order to let\n>> them know that a fix is being considered, and possibly even\n>> get their help in reviewing/testing the patch.\n>>\n>> Signed-off-by: Eric Blake \n>> ---\n> \n> I don't know if this new one deserves to be part of the hardcoded\n> defaults; it would be different between the projects and depends on\n> their convention. I notice that there is no way to configure this\n> script and I suspect that it would be a more generally useful update\n> to have it read a configuration variable that lists what kind of sob\n> like things to take addresses from.\n\nYou mean, something like\n\ngit config --add contacts.autocc Reported-by\ngit config --add contacts.autocc Suggested-by\n\nwhere contacts.autocc would be a new multi-valued config option\nspecifying additional Tag: patterns to scrape out of the commit message?\n\nThe idea seems reasonable, except that I have less experience with\nwriting patches that interact with git config than I do for my one-liner\nattempt, so I would welcome any help from someone with more familiarity\nwith the code base.\n\nAlso, putting it in 'git config' still means that it is a per-developer\nresponsibility to choose which patterns to add to their list. Is there\nany easy way to make a particular repository supply the same list for\nall developers who check it out, without them having to munge things?\n\nAlso, I'm worried about sendemail.cccmd - that's a script that could\nusefully call 'git contacts' under the hood, but that argues that there\nshould be a command-line override (and not just 'git config') for\nchoosing an alternative list of autocc tag patterns on a per-invocation\nbasis. Again, it requires a per-developer setup to wire in a cccmd, but\ntelling developers a one-liner config to set up the command is easier\nthan telling them multiple lines for contacts.autocc.\n\nAnd while we're on the topic of per-project useful defaults, it would be\nnice if diff.orderFile could easily be set to a per-project default,\nrather than requiring per-developer efforts to set that up.\n\nBut yes, I _definitely_ want to be able for a given project to easily\nautocc the tags that it finds appropriate. Your point that different\nprojects have different tags makes total sense (I'm hoping to use\nSuggested-by as one of the tags in qemu, but agree that it is not as\neasy to argue that Suggested-by should be in the hardcoded defaults,\nwhich is why my initial submission only added Reported-by).\n\n> \n> Thanks.\n> \n>> contrib/contacts/git-contacts | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts\n>> index dbe2abf27..85ad732fc 100755\n>> --- a/contrib/contacts/git-contacts\n>> +++ b/contrib/contacts/git-contacts\n>> @@ -11,7 +11,7 @@ use IPC::Open2;\n>>\n>> my $since = '5-years-ago';\n>> my $min_percent = 10;\n>> -my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc/i;\n>> +my $labels_rx = qr/Signed-off-by|Reviewed-by|Acked-by|Cc|Reported-by/i;\n>> my %seen;\n>>\n>> sub format_contact {\n> \n\n-- \nEric Blake, Principal Software Engineer\nRed Hat, Inc. +1-919-301-3266\nVirtualization: qemu.org | libvirt.org\n\n"},{"id":"324868","messageId":"xmqqwp71hj5n.fsf@gitster.mtv.corp.google.com","threadId":"46436","inReplyTo":"a8b47a45-0100-dbef-0bff-fdfdb9cbccb4@redhat.com","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-21T16:03:16Z","receivedAt":"2017-07-21T16:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Blake writes:\n\n> You mean, something like\n>\n> git config --add contacts.autocc Reported-by\n> git config --add contacts.autocc Suggested-by\n>\n> where contacts.autocc would be a new multi-valued config option\n> specifying additional Tag: patterns to scrape out of the commit message?\n\nYes, something along that line, and you are correct to point out\nthat I should have mentioned the need for command-line override.\n\nIn fact, if you anticipate that the primary use of this contributed\nscript is as \"send-email --cccmd\", then we probably are better off\ndoing this without any configuration variables, but just add the\nmechanism for command-line override of the hardcoded default.\n\nI also should have mentioned the need for a way to say \"remove all\nhardcoded default and start from scratch\".\n\n> Also, putting it in 'git config' still means that it is a per-developer\n> responsibility to choose which patterns to add to their list. Is there\n> any easy way to make a particular repository supply the same list for\n> all developers who check it out, without them having to munge things?\n\nThat is a good point, but we should be very careful. \"Let's add\nwhatever configuration the project supplies to the user's repository\nupon cloning\" is an absolute no-no, as a malicious project can ship\nsomething like [alias] \"co\" = \"!rm -rf .\" and unsuspecting victim to\nblindly add it to the configuration. \n\nA standard practice we encourage is to ship a file that records the\nsuggested set of configuration variables as part of the source tree\nand mention how to add these to their repository in README (which\nyou are already using to talk about how to contribute to the\nproject, etc.).\n\nThat would give them a chance to inspect what potential damage the\nproject suggestion will make to their environment (hopefully, there\nis none, but the user must be given a chance to ensure that).\n\nThe extra lines you may need in your README may become something like\n\n Run this in your copy of the project:\n\n $ git config sendemail.cccmd \"git contact --cc Suggested-by\"\n\nwith such a scheme.\n"},{"id":"324967","messageId":"20170724183103.b4vbr5xkijj7s7z3@sigill.intra.peff.net","threadId":"46436","inReplyTo":"xmqqwp71hj5n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-24T18:31:03Z","receivedAt":"2017-07-24T18:31:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 21, 2017 at 09:03:16AM -0700, Junio C Hamano wrote:\n\n> Eric Blake writes:\n> \n> > You mean, something like\n> >\n> > git config --add contacts.autocc Reported-by\n> > git config --add contacts.autocc Suggested-by\n> >\n> > where contacts.autocc would be a new multi-valued config option\n> > specifying additional Tag: patterns to scrape out of the commit message?\n> \n> Yes, something along that line, and you are correct to point out\n> that I should have mentioned the need for command-line override.\n> \n> In fact, if you anticipate that the primary use of this contributed\n> script is as \"send-email --cccmd\", then we probably are better off\n> doing this without any configuration variables, but just add the\n> mechanism for command-line override of the hardcoded default.\n> \n> I also should have mentioned the need for a way to say \"remove all\n> hardcoded default and start from scratch\".\n\nThere's already some prior art around trailers in the trailer.* config.\nI wonder if it would make sense to claim a new key there, like:\n\n git config trailer.Reported-by.autocc true\n\nIf \"Reported-by\" is a trailer that your project uses, then there may be\nsome benefit to setting up other config related to it, and this would\nmesh nicely. And then potentially other programs besides git-contacts\nwould want to respect that flag (perhaps send-email would even want to\ndo it itself; I think it already respects cc and s-o-b headers).\n\n-Peff\n"},{"id":"324976","messageId":"xmqqzibtd473.fsf@gitster.mtv.corp.google.com","threadId":"46436","inReplyTo":"20170724183103.b4vbr5xkijj7s7z3@sigill.intra.peff.net","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-24T19:29:04Z","receivedAt":"2017-07-24T19:29:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I also should have mentioned the need for a way to say \"remove all\n>> hardcoded default and start from scratch\".\n>\n> There's already some prior art around trailers in the trailer.* config.\n> I wonder if it would make sense to claim a new key there, like:\n>\n> git config trailer.Reported-by.autocc true\n>\n> If \"Reported-by\" is a trailer that your project uses, then there may be\n> some benefit to setting up other config related to it, and this would\n> mesh nicely. And then potentially other programs besides git-contacts\n> would want to respect that flag (perhaps send-email would even want to\n> do it itself; I think it already respects cc and s-o-b headers).\n\nSounds like a good suggestion. But...\n\nIf I understand your proposal, the trailer stuff would still not\ncare what value .autocc is set to while doing its own thing, but the\nprograms that read the text file that the trailer can work on would\npay attention to it, and they individually have to do so? Perhaps\nthere is a need for another mode \"interpret-trailers\" is told to run\nin, where it is given a text file with trailers in it and is told to\nshow only the value that has .autocc bit on? Alternatively, yield\n pairs so that the user of the tool can further process\nthe value differently depending on the key, or something?\n"},{"id":"324977","messageId":"20170724193651.ocup4b6cs5hqmstx@sigill.intra.peff.net","threadId":"46436","inReplyTo":"xmqqzibtd473.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-24T19:36:51Z","receivedAt":"2017-07-24T19:36:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 24, 2017 at 12:29:04PM -0700, Junio C Hamano wrote:\n\n> > There's already some prior art around trailers in the trailer.* config.\n> > I wonder if it would make sense to claim a new key there, like:\n> >\n> > git config trailer.Reported-by.autocc true\n> >\n> > If \"Reported-by\" is a trailer that your project uses, then there may be\n> > some benefit to setting up other config related to it, and this would\n> > mesh nicely. And then potentially other programs besides git-contacts\n> > would want to respect that flag (perhaps send-email would even want to\n> > do it itself; I think it already respects cc and s-o-b headers).\n> \n> Sounds like a good suggestion. But...\n> \n> If I understand your proposal, the trailer stuff would still not\n> care what value .autocc is set to while doing its own thing, but the\n> programs that read the text file that the trailer can work on would\n> pay attention to it, and they individually have to do so? Perhaps\n> there is a need for another mode \"interpret-trailers\" is told to run\n> in, where it is given a text file with trailers in it and is told to\n> show only the value that has .autocc bit on? Alternatively, yield\n> pairs so that the user of the tool can further process\n> the value differently depending on the key, or something?\n\nYeah, I don't think interpret-trailers is currently a useful tool for\nlooking at an extra config key like that. I'd expect it would need to be\nextended, or a new tool added, or perhaps existing tools would need to\nlearn more about how trailers work (e.g., it would be nice if git-log\ncould do matching or even print them via %(trailer:reported-by)\nplaceholders or something). I think there's a lot of potential work in\nthat area.\n\nOf course git-contacts (or send-email) _can_ just look look at all of\nthe trailer.*.autocc config and try to match those manually. But the\npoint of having trailer config, I think, is that we should stop doing\nad-hoc parsing and have a tool for manipulating and querying trailers.\nIf interpret-trailers isn't up to the task yet, I'd rather see work go\nthere.\n\nBut that (manual parsing) is basically how the current cc and s-o-b\ntrailers implemented inside of git-send-email, so I don't think it would\nbe the end of the world as a quick hack that could later be expanded to\nuse the trailer infrastructure.\n\n-Peff\n"},{"id":"325192","messageId":"xmqqh8xxomka.fsf@gitster.mtv.corp.google.com","threadId":"46436","inReplyTo":"20170724193651.ocup4b6cs5hqmstx@sigill.intra.peff.net","subject":"Re: [PATCH] git-contacts: Add recognition of Reported-by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-27T16:45:57Z","receivedAt":"2017-07-27T16:46:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I don't think interpret-trailers is currently a useful tool for\n> looking at an extra config key like that. I'd expect it would need to be\n> extended, or a new tool added, or perhaps existing tools would need to\n> learn more about how trailers work (e.g., it would be nice if git-log\n> could do matching or even print them via %(trailer:reported-by)\n> placeholders or something). I think there's a lot of potential work in\n> that area.\n>\n> Of course git-contacts (or send-email) _can_ just look look at all of\n> the trailer.*.autocc config and try to match those manually. But the\n> point of having trailer config, I think, is that we should stop doing\n> ad-hoc parsing and have a tool for manipulating and querying trailers.\n> If interpret-trailers isn't up to the task yet, I'd rather see work go\n> there.\n>\n> But that (manual parsing) is basically how the current cc and s-o-b\n> trailers implemented inside of git-send-email, so I don't think it would\n> be the end of the world as a quick hack that could later be expanded to\n> use the trailer infrastructure.\n\nOK.\n\nIn any case, I think not CC'ing the reporter would be a bug, and an\nupdate to the \"contacts\" script (in contrib/) to use an improved\nconvention and interpret-trailers tool can be built on top of the\nversion Eric posted, so let's take the patch anyway for now.\n\nThanks.\n"}]}