Child command context is not updated when global context changed

[libozh]

In command.go
func (c *Command) ExecuteC() (cmd *Command, err error) {
...
// We have to pass global context to children command
// if context is present on the parent command.
if cmd.ctx == nil {
cmd.ctx = c.ctx
}
...
}

The code doesn't match the intention of the comment. c.ctx hold the global context. Should the code be:

// We have to pass global context to children command
// if context is present on the parent command.
if c.ctx != nil {
	cmd.ctx = c.ctx
}

Without this change, if global context changed, the child command still hold the old context.

[marckhouzam]

Thanks @libozh

cobra/command.go

Lines 1109 to 1113 in 199b7ab

	// We have to pass global context to children command
	// if context is present on the parent command.
	if cmd.ctx == nil {
	cmd.ctx = c.ctx
	}

In the above code c is the root command while cmd is the sub-command being called.
I interpret the line if cmd.ctx == nil { to say that if the sub-command does not already have a context, it gets set to the root context.

I haven't used contexts with Cobra, but my guess is that you can set a different context for a sub-command than for the root and therefore you don't want the root context to overwrite the sub-command context. So it seems reasonable to me.

Was your observation based on looking at the code or did you hit a problem with this logic?

Without this change, if global context changed, the child command still hold the old context.

I'm not sure when a context could change. I think it is set once for one program execution.

[libozh]

Thank you Marc for the feedback.
In my specific use case, the root command context carries cancellation signals, it will pass it to sub-command. When root command update its context, but sub-command still keeps the old context, the cancellation would fail. I think based on the "context" usage, the sub-command should stay consistent with root command. An example when root command updates its context is: there is an update for the command configuration, the command re-run automatically with a new context.
Actually when I look into the pull requests today, I find a similar pull request as above: #1875
I think it is trying to cover similar use cases.