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 <mst@xxxxxxxxxx> wrote: > > On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote: > >> "Michael S. Tsirkin" <mst@xxxxxxxxxx> writes: > >> > >> > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote: > >> >> "Michael S. Tsirkin" <mst@xxxxxxxxxx> writes: > >> >> > >> >> > I have this in .git/config > >> >> > > >> >> > [trailer "r"] > >> >> > key = Reviewed-by > >> >> > command = "echo \"Michael S. Tsirkin <mst@xxxxxxxxxx\"" > >> >> > [trailer "s"] > >> >> > key = Signed-off-by > >> >> > command = "echo \"Michael S. Tsirkin <mst@xxxxxxxxxx\"" > >> >> > > >> >> > whenever I run git interpret-trailers -t r I see these lines added: > >> >> > > >> >> > Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx > >> >> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx > >> >> > Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx > >> >> > > >> >> > Why is Reviewed-by repeated? Bug or feature? > >> >> > >> >> The first two lines are added unconditionally: > >> >> > >> >> $ echo | git interpret-trailers > >> >> > >> >> Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx > >> >> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx > >> >> > >> >> 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 > <mst@xxxxxxxxxx>' --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 <token>=<value> arguments are also passed on the command line, when a trailer.<token>.command is configured, the command will also be executed for each of these arguments. And the <value> 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.<token>.command, we > >> would need trailer.<token>.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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html