All of lore.kernel.org
 help / color / mirror / Atom feed
* git interpret-trailers with multiple keys
@ 2016-04-06 16:12 Michael S. Tsirkin
  2016-04-06 16:58 ` Matthieu Moy
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-06 16:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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?

-- 
MST

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-06 16:12 git interpret-trailers with multiple keys Michael S. Tsirkin
@ 2016-04-06 16:58 ` Matthieu Moy
  2016-04-06 17:16   ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2016-04-06 16:58 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: git, Junio C Hamano

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

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.

(BTW, I think you wanted a closing > at the end)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-06 16:58 ` Matthieu Moy
@ 2016-04-06 17:16   ` Michael S. Tsirkin
  2016-04-06 17:42     ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-06 17:16 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, Junio C Hamano

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.
> 
> 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.
> 
> (BTW, I think you wanted a closing > at the end)

Is this worth fixing? It doesn't look like a behaviour anyone
would want...

> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-06 17:16   ` Michael S. Tsirkin
@ 2016-04-06 17:42     ` Junio C Hamano
  2016-04-06 19:30       ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2016-04-06 17:42 UTC (permalink / raw)
  To: Christian Couder; +Cc: Michael S. Tsirkin, Matthieu Moy, git

"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.
>> 
>> 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.
>> 
>> (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.  Perhaps next to trailer.<token>.command, we
would need trailer.<token>.value?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-06 17:42     ` Junio C Hamano
@ 2016-04-06 19:30       ` Michael S. Tsirkin
  2016-04-07  2:28         ` Christian Couder
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-06 19:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, Matthieu Moy, git

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


-- 
MST

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-06 19:30       ` Michael S. Tsirkin
@ 2016-04-07  2:28         ` Christian Couder
  2016-04-10 15:32           ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Christian Couder @ 2016-04-07  2:28 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Junio C Hamano, Matthieu Moy, git

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.

If people really want some configured trailers to be used only on
request, it is possible to add a config option for that.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-07  2:28         ` Christian Couder
@ 2016-04-10 15:32           ` Michael S. Tsirkin
  2016-04-10 16:57             ` Christian Couder
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-10 15:32 UTC (permalink / raw)
  To: Christian Couder; +Cc: Junio C Hamano, Matthieu Moy, git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-10 15:32           ` Michael S. Tsirkin
@ 2016-04-10 16:57             ` Christian Couder
  2016-04-10 17:06               ` Michael S. Tsirkin
  2016-04-10 17:43               ` Michael S. Tsirkin
  0 siblings, 2 replies; 12+ messages in thread
From: Christian Couder @ 2016-04-10 16:57 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Junio C Hamano, Matthieu Moy, git

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-10 16:57             ` Christian Couder
@ 2016-04-10 17:06               ` Michael S. Tsirkin
  2016-04-10 17:43               ` Michael S. Tsirkin
  1 sibling, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-10 17:06 UTC (permalink / raw)
  To: Christian Couder; +Cc: Junio C Hamano, Matthieu Moy, git

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-10 16:57             ` Christian Couder
  2016-04-10 17:06               ` Michael S. Tsirkin
@ 2016-04-10 17:43               ` Michael S. Tsirkin
  2016-04-11  7:09                 ` Matthieu Moy
  1 sibling, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-10 17:43 UTC (permalink / raw)
  To: Christian Couder; +Cc: Junio C Hamano, Matthieu Moy, git

On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote:
> 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 don't think it has to do with commands.
For example, if we add "value" it should behave the same.

So I think a better name is "ifnotlisted", with values "add"
and "donothing".

Thoughts?

-- 
MST

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-10 17:43               ` Michael S. Tsirkin
@ 2016-04-11  7:09                 ` Matthieu Moy
  2016-04-11  7:24                   ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2016-04-11  7:09 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Christian Couder, Junio C Hamano, git

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote:
>> 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 don't think it has to do with commands.
> For example, if we add "value" it should behave the same.
>
> So I think a better name is "ifnotlisted", with values "add"
> and "donothing".

Having a negation in the variable name feels wrong. When the token is
listed on the command-line and ifnotlisted=donothing, I have to apply a
double-negation to guess what would happen (=> "if listed then do
something").

I agree that having such variable would be a good thing. It would solve
your issue (i.e. "How to I configure a token for quick use from the
command-line without applying it unconditionally"), and allow full
backward compatibility.

I'd call the option "apply" or perhaps "run", with values 1/true/always
= default = current behavior, or "auto" = "apply when asked from the
command-line". I'm wondering whether other values could make sense (not
to implement it right now, but to keep the design open to further
extensions): perhaps apply=ifauthor could mean "apply this trailer to
patches I'm the author of" for example.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: git interpret-trailers with multiple keys
  2016-04-11  7:09                 ` Matthieu Moy
@ 2016-04-11  7:24                   ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2016-04-11  7:24 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Christian Couder, Junio C Hamano, git

On Mon, Apr 11, 2016 at 09:09:48AM +0200, Matthieu Moy wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Apr 10, 2016 at 06:57:53PM +0200, Christian Couder wrote:
> >> 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 don't think it has to do with commands.
> > For example, if we add "value" it should behave the same.
> >
> > So I think a better name is "ifnotlisted", with values "add"
> > and "donothing".
> 
> Having a negation in the variable name feels wrong. When the token is
> listed on the command-line and ifnotlisted=donothing, I have to apply a
> double-negation to guess what would happen (=> "if listed then do
> something").

Isn't this similar to ifmissing?

> I agree that having such variable would be a good thing. It would solve
> your issue (i.e. "How to I configure a token for quick use from the
> command-line without applying it unconditionally"), and allow full
> backward compatibility.
> 
> I'd call the option "apply" or perhaps "run", with values 1/true/always
> = default = current behavior, or "auto" = "apply when asked from the
> command-line". I'm wondering whether other values could make sense (not
> to implement it right now, but to keep the design open to further
> extensions): perhaps apply=ifauthor could mean "apply this trailer to
> patches I'm the author of" for example.
> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-04-11  7:25 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-06 16:12 git interpret-trailers with multiple keys Michael S. Tsirkin
2016-04-06 16:58 ` Matthieu Moy
2016-04-06 17:16   ` Michael S. Tsirkin
2016-04-06 17:42     ` Junio C Hamano
2016-04-06 19:30       ` Michael S. Tsirkin
2016-04-07  2:28         ` Christian Couder
2016-04-10 15:32           ` Michael S. Tsirkin
2016-04-10 16:57             ` Christian Couder
2016-04-10 17:06               ` Michael S. Tsirkin
2016-04-10 17:43               ` Michael S. Tsirkin
2016-04-11  7:09                 ` Matthieu Moy
2016-04-11  7:24                   ` Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.