prepare-commit-msg hook during rebase

prepare-commit-msg hook during rebase

[Michael J Gruber]

Hi there

For a while now, I thought I was using prepare-commit-msg hook wrong
but finally took the time to analyse this further. Per the doc, the
hook receives the source of the commit message as $2
(message/template/merge/squash/commit or empty).

I notice the following with `rebase -i`:
- When rb applies a patch to be edited/reworded, the hook is called
with `commit` as the message source.
- When rb applies a patch merely to be picked, the hook is called with
`message` as the message source.

The latter also happens when non-interactive rebase applies commits.

I find this confusing for two reasons:
- Whether edit/reword or pick, there is always a commit being applied,
and it's the source of the message. So why not `commit` in both cases?
- The doc says that `message` is for inputs from `-m` or `-F`. So,
certainly this should not apply when the message comes from a picked
commit.

Also, I'm not sure whether the claim about `-m` is true, but that's
another issue; we even might want to distinguish between `-m` and `-F`
here.

Does the source `message` during rb pick occur due to an
implementation detail, maybe since the rewrite to sequencer?

Cheers
Michael

Re: prepare-commit-msg hook during rebase

[Phillip Wood]

Hi Michael

On 15/04/2024 11:36, Michael J Gruber wrote:
> Hi there
> 
> For a while now, I thought I was using prepare-commit-msg hook wrong
> but finally took the time to analyse this further. Per the doc, the
> hook receives the source of the commit message as $2
> (message/template/merge/squash/commit or empty).
> 
> I notice the following with `rebase -i`:
> - When rb applies a patch to be edited/reworded, the hook is called
> with `commit` as the message source.
> - When rb applies a patch merely to be picked, the hook is called with
> `message` as the message source.
> 
> The latter also happens when non-interactive rebase applies commits.
> 
> I find this confusing for two reasons:
> - Whether edit/reword or pick, there is always a commit being applied,
> and it's the source of the message. So why not `commit` in both cases?
> - The doc says that `message` is for inputs from `-m` or `-F`. So,
> certainly this should not apply when the message comes from a picked
> commit.
> 
> Also, I'm not sure whether the claim about `-m` is true, but that's
> another issue; we even might want to distinguish between `-m` and `-F`
> here.
> 
> Does the source `message` during rb pick occur due to an
> implementation detail, maybe since the rewrite to sequencer?

All of the hook behavior in rebase is an accident of the implementation 
and stems from the scripted implementation. I'm not sure if the source 
passed to the hook has changed over time but it is quite possible that 
it has. I agree "commit" would be a more logical source. Personally I'm 
not sure it makes sense to run this hook for ordinary picks, though 
perhaps it makes sense to run it when the message is to be edited. Do 
you have a use for this hook while rebasing or are you just confused by 
the source argument?

Best Wishes

Phillip

> Cheers
> Michael
> 

Re: prepare-commit-msg hook during rebase

[Michael J Gruber]

Phillip Wood venit, vidit, dixit 2024-04-15 16:01:31:
> Hi Michael
> 
> On 15/04/2024 11:36, Michael J Gruber wrote:
> > Hi there
> > 
> > For a while now, I thought I was using prepare-commit-msg hook wrong
> > but finally took the time to analyse this further. Per the doc, the
> > hook receives the source of the commit message as $2
> > (message/template/merge/squash/commit or empty).
> > 
> > I notice the following with `rebase -i`:
> > - When rb applies a patch to be edited/reworded, the hook is called
> > with `commit` as the message source.
> > - When rb applies a patch merely to be picked, the hook is called with
> > `message` as the message source.
> > 
> > The latter also happens when non-interactive rebase applies commits.
> > 
> > I find this confusing for two reasons:
> > - Whether edit/reword or pick, there is always a commit being applied,
> > and it's the source of the message. So why not `commit` in both cases?
> > - The doc says that `message` is for inputs from `-m` or `-F`. So,
> > certainly this should not apply when the message comes from a picked
> > commit.
> > 
> > Also, I'm not sure whether the claim about `-m` is true, but that's
> > another issue; we even might want to distinguish between `-m` and `-F`
> > here.
> > 
> > Does the source `message` during rb pick occur due to an
> > implementation detail, maybe since the rewrite to sequencer?
> 
> All of the hook behavior in rebase is an accident of the implementation 
> and stems from the scripted implementation. I'm not sure if the source 
> passed to the hook has changed over time but it is quite possible that 
> it has. I agree "commit" would be a more logical source. Personally I'm 
> not sure it makes sense to run this hook for ordinary picks, though 
> perhaps it makes sense to run it when the message is to be edited. Do 
> you have a use for this hook while rebasing or are you just confused by 
> the source argument?

It is run during a rebase, and I don't have a use for it there, in
particular not for picked commits which I do not amend. That is what
triggered my investigation (unintended runs).

In fact, my hook messes up commits during a rebase if I run rebase from
a subdirectory, and I am sure that it didn't (quite) some time in the
past. But this can have several reasons:

- prepare-commit-msg was not run for rebase picks at all in the past
- prepare-commit-msg was run with source "commit" in the past
- prepare-commit-msg was run from the topdir (not cwd) in the past

I can avoid the mess by detecting the rebase state in the hook (or
cd'ing to topdir before the rebase), but I'm wondering what changed and
what is the right behaviour (to which I would adjust, of course).

Also, looking at sequencer.c from line 6545, sequencer_determine_whence()
looks funny: FROM_CHERRY_PICK_MULTI from the first if will always be
overwritten by the second if/else. I'm not saying this is strictly
related, but we do have untested code paths in that area. whence is part
of what makes commit decide on hooks being run and arguments passed to
them.

Michael