Re: git interpret-trailers with multiple keys

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote:
> On Sun, Apr 10, 2016 at 11:32 AM, Michael S. Tsirkin <mst@redhat.com> 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@redhat.com> wrote:
> >> > On Wed, Apr 06, 2016 at 10:42:42AM -0700, Junio C Hamano wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >>
> >> >> > On Wed, Apr 06, 2016 at 06:58:30PM +0200, Matthieu Moy wrote:
> >> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> >>
> >> >> >> > I have this in .git/config
> >> >> >> >
> >> >> >> > [trailer "r"]
> >> >> >> >         key = Reviewed-by
> >> >> >> >         command = "echo \"Michael S. Tsirkin <mst@redhat.com\""
> >> >> >> > [trailer "s"]
> >> >> >> >         key = Signed-off-by
> >> >> >> >         command = "echo \"Michael S. Tsirkin <mst@redhat.com\""
> >> >> >> >
> >> >> >> > whenever I run git interpret-trailers -t r I see these lines added:
> >> >> >> >
> >> >> >> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com
> >> >> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com
> >> >> >> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com
> >> >> >> >
> >> >> >> > 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@redhat.com
> >> >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com
> >> >> >>
> >> >> >> 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@redhat.com>' --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.

Then one can't set any values, only keys.

> >> 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.

I'm not sure what makes you say that. What makes it the most powerful?

> 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.