On Sun, Apr 10, 2016 at 11:32 AM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> 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 one want something requestable instead of automatic, one should
definitely not configure any command.
>> 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:
What I meant is that we could create new options called maybe
trailer.autocommands and trailer.<token>.autocommands that default to
'true' and if 'false' the command would not be run automatically and
the corresponding trailer would not be added.
> 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.
Current default is documented and is the most powerful default.
Yes, it might be surprising though.
>> >> >> (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.
Efficiency is not very high in the list for this kind of things. Also
when these features were developed, many people wanted different
powerful things and many people said they could help develop them
though very few did help. So if you think trailer.<token>.value is
really needed you are welcome to develop it.
Thanks,
Christian.
--
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