"Samuel Yvon via GitGitGadget" [off-list ref] writes:
The comment suggesting that the cache must be reset after run_status
and before the editor being launched was added in ec84bd00,
(git-commit: Refactor creation of log message., 2008-02-05). It is
unclear why the run_status must be called *after* the cache reset.
An older thread cited earlier suspected that it is to reflect the
changes given to "git commit" proper, excluding whatever pre-commit
did, to the return value of run_status(), which becomes the value of
committable variable to answer "do we have anything to commit?".
And moving the call would affect both the contents of the status
buffer (i.e. the list of paths got changed starts including what
pre-commit did) and the "committable" bit by counting such a change
as a true change, avoiding the "no empty commit by default" check,
in a consistent way, hopefully. I wonder if we have test to
demonstrate that, and if there isn't perhaps we would want to add
one.
However, calling run_status after the cache reset does not update
the status line to state of the current index in the case a
pre-commit hook is ran and changes files in the staging area.
And if this change also affects the "committable" assignment in a
consistent way, it should probably want to be mentioned in this
paragraph, too.
I am not convinced by the claim that there is no need for careful
transition plans (yet), but I personally agree with the end state
(with the above suggested tweaks, that is).
Thanks for working on the topic.
Apologies for the time I took to reply,
Junio C Hamano [off-list ref] writes:
And moving the call would affect both the contents of the status
buffer (i.e. the list of paths got changed starts including what
pre-commit did) and the "committable" bit by counting such a change
as a true change, avoiding the "no empty commit by default" check,
in a consistent way, hopefully. I wonder if we have test to
demonstrate that, and if there isn't perhaps we would want to add
one.
So, just to make sure I understand well, the concern is that an empty commit
would trigger the commit routine, run the pre-commit hook, which may add files
(thus making it an non-empty commit) and then push 100% automatic changes to a
repo. I agree that this would be invalid behaviour and very odd, I will
make sure no empty commit is allowed.
Junio C Hamano [off-list ref] writes:
Samuel Yvon [off-list ref] writes:
quoted
However, calling run_status after the cache reset does not update
the status line to state of the current index in the case a
pre-commit hook is ran and changes files in the staging area.
And if this change also affects the "committable" assignment in a
consistent way, it should probably want to be mentioned in this
paragraph, too.
What do you mean by "commitable assignment"?
I am not convinced by the claim that there is no need for careful
transition plans (yet), but I personally agree with the end state
(with the above suggested tweaks, that is).
With the last message, I agree the safest option is probably to leave this
configurable for now and off by default.
So here's the next steps that I intend to take to get this merged in:
- Add a test for empty commit (if non-existent) and ensure the behaviour is the same
- Add a config option (or maybe a switch?) for migration purposes, with the default
being the current behaviour.
Samuel Yvon [off-list ref] writes:
quoted
And if this change also affects the "committable" assignment in a
consistent way, it should probably want to be mentioned in this
paragraph, too.
What do you mean by "commitable assignment"?
Assignment to the committable variable in the codepath you are
looking at, was what I meant.