Re: [PATCH v2] builtin-commit: re-read file index before run_status

From: Samuel Yvon <samuelyvon9@gmail.com>
Date: Nov 17, 2021, 16:48 UTC
Message-ID: <20211117164842.36381-1-samuelyvon9@gmail.com>
In-Reply-To: <xmqqpmr5nekx.fsf@gitster.g>

Apologies for the time I took to reply,

Junio C Hamano <gitster@pobox.com> writes:

Show 7 quoted lines

> 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 <gitster@pobox.com> writes:

Show 8 quoted lines

> Samuel Yvon <samuelyvon9@gmail.com> writes:
> > 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.