From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 3/3] commit -a -m: allow the top-level tree to become empty again
Date: Thu, 29 Jun 2023 12:17:22 -0700 [thread overview]
Message-ID: <xmqq5y769iyl.fsf@gitster.g> (raw)
In-Reply-To: <08c50b64e2a93300eed196505936e58ce8bb639b.1688044991.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Thu, 29 Jun 2023 13:23:10 +0000")
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> That logic was introduced to add a shortcut when committing without
> editing the commit message interactively. A part of that logic was to
> ensure that the index was read into memory:
>
> if (!active_nr && read_cache() < 0)
> die(...)
>
> Translation to English: If the index has not yet been read, read it, and
> if that fails, error out.
Well described. It does make sense to turn !active_nr used here
into a check on the .initialized member.
> And it was natural to do it this way because at the time that condition
> was introduced, the `index_state` structure had no explicit flag to
> indicate that it was initialized: This flag was only introduced in
> 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from
> read_cache(), 2008-08-23), but that commit did not adjust the code path
> where no index file was found and a new, pristine index was initialized.
My mistake, but after 15 years it probably is beyond statute of
limitations ;-)
> Using the `initialized` flag instead, we avoid that mistake, and as a
> bonus we can fix a bug at the same time that was introduced by the
> memory leak fix: When deleting all tracked files and then asking `git
> commit -a -m ...` to commit the result, Git would internally update the
> index, then discard and re-read the index undoing the update, and fail
> to commit anything.
That does sound like the primary bug fixed with this change, not a
bonus, but anyway, the change is very sensible and clearly described
with a good test. Will queue.
Thanks.
prev parent reply other threads:[~2023-06-29 19:20 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-29 13:23 [PATCH 0/3] commit -a -m: allow the top-level tree to become empty again Johannes Schindelin via GitGitGadget
2023-06-29 13:23 ` [PATCH 1/3] do_read_index(): always mark index as initialized unless erroring out Johannes Schindelin via GitGitGadget
2023-06-29 13:23 ` [PATCH 2/3] split-index: accept that a base index can be empty Johannes Schindelin via GitGitGadget
2023-06-29 19:02 ` Junio C Hamano
2023-06-29 13:23 ` [PATCH 3/3] commit -a -m: allow the top-level tree to become empty again Johannes Schindelin via GitGitGadget
2023-06-29 19:17 ` Junio C Hamano [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq5y769iyl.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.