From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: git interpret-trailers with multiple keys Date: Sun, 10 Apr 2016 18:32:49 +0300 Message-ID: <20160410182750-mutt-send-email-mst@redhat.com> References: <20160406191054-mutt-send-email-mst@redhat.com> <20160406201509-mutt-send-email-mst@redhat.com> <20160406212940-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Junio C Hamano , Matthieu Moy , git To: Christian Couder X-From: git-owner@vger.kernel.org Sun Apr 10 17:32:59 2016 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1apHMB-0000Q1-1V for gcvg-git-2@plane.gmane.org; Sun, 10 Apr 2016 17:32:59 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753557AbcDJPcx (ORCPT ); Sun, 10 Apr 2016 11:32:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38775 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753068AbcDJPcw (ORCPT ); Sun, 10 Apr 2016 11:32:52 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 09F0078224; Sun, 10 Apr 2016 15:32:52 +0000 (UTC) Received: from redhat.com (vpn1-5-25.ams2.redhat.com [10.36.5.25]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id u3AFWnB4015377; Sun, 10 Apr 2016 11:32:50 -0400 Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Wed, Apr 06, 2016 at 10:28:21PM -0400, Christian Couder wrote: > On Wed, Apr 6, 2016 at 3:30 PM, Michael S. Tsirkin wrote: > > On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote: > >> >> "Michael S. Tsirkin" writes: > >> >> > >> >> > I have this in .git/config > >> >> > > >> >> > [trailer "r"] > >> >> > key = Reviewed-by > >> >> > command = "echo \"Michael S. Tsirkin >> >> > [trailer "s"] > >> >> > key = Signed-off-by > >> >> > command = "echo \"Michael S. Tsirkin >> >> > > >> >> > whenever I run git interpret-trailers -t r I see these lines added: > >> >> > > >> >> > Reviewed-by: Michael S. Tsirkin >> >> > Signed-off-by: Michael S. Tsirkin >> >> > Reviewed-by: Michael S. Tsirkin >> >> > > >> >> > Why is Reviewed-by repeated? Bug or feature? > >> >> > >> >> The first two lines are added unconditionally: > >> >> > >> >> $ echo | git interpret-trailers > >> >> > >> >> Reviewed-by: Michael S. Tsirkin >> >> Signed-off-by: Michael S. Tsirkin >> >> > >> >> The last line is added because you've asked for it with --trailer r. > > Yes, and because the default is to add the trailer at the end. > > >> >> I don't think it's currently possible to get the behavior you seem to > >> >> expect, ie. to define trailer tokens fully (key and value) in your > >> >> config file but use them only on request. > > Yes, because you could define for example a function like this: > > reviewed() { > git interpret-trailers --trailer 'Reviewed-by: Michael S. Tsirkin > ' --in-place "$@" > } > > So it is kind of easy already to make things requestable. Not if any commands are configured. interpret-trailers will insist on running them in any case. > If people really want some configured trailers to be used only on > request, it is possible to add a config option for that. this is not what the documentation says though: If some = arguments are also passed on the command line, when a trailer..command is configured, the command will also be executed for each of these arguments. And the part of these arguments, if any, will be used to replace the $ARG string in the command. so it says command *for a given token* is run. I would say that if people really want to run all trailers while also passing some on command line, *that* should be a config option. Current default violates the principle of least surprise. > >> >> (BTW, I think you wanted a closing > at the end) > >> > > >> > Is this worth fixing? It doesn't look like a behaviour anyone > >> > would want... > >> > >> CC'ing Christian who's done the "trailers" thing. > >> > >> Personally, I do not think adding any configured trailers without > >> being asked is a sensible behaviour, but it is likely that people > >> already depend on it, as we seem to see "How do I configure to > >> always add this and that trailer?" from time to time. I do not > >> think it is unreasonable to disable the "automatically add > >> everything that is configured" when the command line arguments ask > >> for some specific trailer, but I haven't thought deeply about it. > >> > >> An additional (uninformed) observation is that the 'echo' looks like > >> an ugly workaround for the lack of "always use this string as the > >> value" configuration. > > > > Or at least a default. > > > >> Perhaps next to trailer..command, we > >> would need trailer..value? > > Yeah, that is possible too. > It could be bit redundant if we already have a config option to say if > the trailer has to be requested. Seems unrelated - if one just wants a string, using echo as a command is inefficient and inconvenient. -- MST