git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Samuel Yvon <samuelyvon9@gmail.com>
To: avarab@gmail.com
Cc: git@vger.kernel.org, gitgitgadget@gmail.com, samuelyvon9@gmail.com
Subject: Re: [PATCH] builtin-commit: re-read file index before launching editor
Date: Tue,  9 Nov 2021 10:22:19 -0500	[thread overview]
Message-ID: <20211109152219.11037-1-samuelyvon9@gmail.com> (raw)
In-Reply-To: <211109.868rxxvgdi.gmgdl@evledraar.gmail.com>

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> *nod*, the implicit suggestion here being: Let's put more of that
> summary into the commit message. It helps when looking up/discovering
> older behavior.

Sorry! I will prepare an amended patch later. Exploring the linked commits
has raised more questions on my end.

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The comment was first added in 2888605c649 (builtin-commit: fix
>         partial-commit support, 2007-11-18), and quite suspicuous in timing is
> f5bbc3225c4 (Port git commit to C., 2007-11-08) where we moved from
> git-commit.sh.

Exploring these commits, I fear I might have been confused; the editor was
always launched after the reset (still today). The problem, as noted earlier,
is the fact that the run_status is ran before the cache reset, which
creates the outdated diff. I can't speculate to the intent of the
author, but exploring the code makes this comment:

> +    /*
> +     * Re-read the index as pre-commit hook could have updated it,
> +     * and write it out as a tree.  We must do this before we invoke
> +     * the editor and after we invoke run_status above.
> +     */

more confusing, because there is no point of invoking the editor after the reset
if the status is written before, since the editor won't show anything different.

On one hand, flushing then showing the editor seems to indicate that we want the
editor to be up-to-date, but because the status is prepared before the flushing,
maybe not?

While it seems the current behaviour has been the behaviour since the start,
I am inclined to continue pushing for this change. Unless I am missing something,
the comment is contradictory and we should be coherent with the idea of
accepting changes made within the pre-commit hook, as noted in 
https://lore.kernel.org/git/xmqqk0yripca.fsf@gitster.c.googlers.com/t/#u:

>> Junio C Hamano <gitster@pobox.com> writes:
>> Even before ec84bd00 (git-commit: Refactor creation of log message.,
>> 2008-02-05), the code anticipated that pre-commit may touch the index
>> and tried to cope with it.

  reply	other threads:[~2021-11-09 15:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-09  2:06 [PATCH] builtin-commit: re-read file index before launching editor Samuel Yvon via GitGitGadget
2021-11-09  2:32 ` Ævar Arnfjörð Bjarmason
2021-11-09  3:08   ` samuelyvon9
2021-11-09  9:11     ` Ævar Arnfjörð Bjarmason
2021-11-09 15:22       ` Samuel Yvon [this message]
2021-11-09 18:36         ` Junio C Hamano
2021-11-09 20:01           ` Samuel Yvon
2021-11-11 22:09             ` Junio C Hamano
2021-11-09 16:41 ` Description of github.com/git/git, was " Johannes Schindelin
2021-11-09 17:01   ` Samuel Yvon
2021-11-09 19:03   ` Junio C Hamano
2021-11-09 19:23     ` Taylor Blau
2021-11-09 19:27     ` Samuel Yvon
2021-11-10 12:22       ` Johannes Schindelin
2021-11-11 17:55 ` [PATCH v2] builtin-commit: re-read file index before run_status Samuel Yvon via GitGitGadget
2021-11-12 23:23   ` Junio C Hamano
2021-11-17 16:48     ` Samuel Yvon
2021-11-18 23:51       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211109152219.11037-1-samuelyvon9@gmail.com \
    --to=samuelyvon9@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).