git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Alban Gruin <alban.gruin@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action()
Date: Wed, 02 Oct 2019 11:38:57 +0900	[thread overview]
Message-ID: <xmqq5zl7hnwe.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190925201315.19722-6-alban.gruin@gmail.com> (Alban Gruin's message of "Wed, 25 Sep 2019 22:13:15 +0200")

Alban Gruin <alban.gruin@gmail.com> writes:

> Currently, complete_action() calls sequencer_continue() to do the
> rebase.  Even though the former already has the todo list, the latter
> loads it from the disk and parses it.  Calling directly pick_commits()
> from complete_action() avoids this unnecessary round trip.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  sequencer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

All the earlier steps in this series are necessary because the
inconsistencies caused by not doing things the code updated by the
earlier steps do (e.g. leaving number of commits recorded in
total_nr and done_nr stale) were masked by re-reading the info from
the on-disk file.  And this step exposes these inconsistencies
because the extra reading from the file no longer happens.

That is my reading of the series---did I get it right?

I wonder if that can be stated clearly in the log message of the
earlier steps.

For example, by reading 1/5 or 2/5 alone, it would be natural for
readers to wonder why the original code that did not update done_nr
correctly terminated after going through the todo list (you would
expect that when done reaches total you are finished, so if one of
these variables are not maintained correctly, your termination
condition may be borked), and then by knowing that the current code
more-or-less works correctly by not terminating too early or barfing
to say it ran out of things to do prematurely, the next thing
readers would wonder is if these patches introduce new bugs by
incrementing these variables twice (iow, "does the author of the
patch missed other places where these variables are correctly
updated?")

1/5 for example talks about the variable mostly being used by
prompts, which may be useful to reduce worries by readers about
correctness (iow, "well, if it is only showing wrong number in the
prompts and does not affect correctness otherwise, perhaps that was
why the current code that is buggy did not behave incorrectly").
But I suspect that it is not why these changes in the earlier steps
are acceptable or are right things to do.  So, perhaps replace the
"these are used only in prompts and the numbers being wrong does not
affect correctness" comments from them with:

    By forgetting to update the variable, the original code made it
    not reflect the reality, but this flaw was masked by the code
    calling (sometimes unnecessarily) read_todo_list() again to
    update the variable to its correct value before the next action
    happens.  At the end of this series, the unnecessary call will
    be removed and the inconsistency addressed by this patch would
    start to matter.

or something like that (assuming that the answer to the first
question I asked in this message is "yes", that is), or a more
concise version of it?

Thanks.

  parent reply	other threads:[~2019-10-02  2:43 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190925201315.19722-1-alban.gruin@gmail.com>
2019-09-25 20:13 ` [PATCH v1 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-02  2:10   ` Junio C Hamano
2019-10-02  8:06     ` Johannes Schindelin
2019-10-02  8:59       ` Junio C Hamano
2019-10-02  9:48         ` Johannes Schindelin
2019-10-02 18:03         ` Alban Gruin
2019-09-25 20:13 ` [PATCH v1 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-09-25 20:13 ` [PATCH v1 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-09-25 20:13 ` [PATCH v1 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-09-27 13:30   ` Phillip Wood
2019-10-02  8:16     ` Johannes Schindelin
2019-10-02  9:32       ` Phillip Wood
2019-10-02 12:06         ` Johannes Schindelin
2019-09-25 20:13 ` [PATCH v1 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-09-27 13:26   ` Phillip Wood
2019-10-02  8:20     ` Johannes Schindelin
2019-10-02 18:03       ` Alban Gruin
2019-10-02  2:38   ` Junio C Hamano [this message]
2019-10-02 18:37     ` Alban Gruin
2019-10-26  7:47   ` René Scharfe
2019-10-26 11:27     ` Alban Gruin
2019-10-28  1:39       ` Junio C Hamano
2019-10-28  3:20         ` Junio C Hamano
2019-09-25 20:20 ` [PATCH v1 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-09-27 13:32 ` [PATCH v1 0/5] Use complete_action’s " Phillip Wood
2019-10-07  9:26 ` [PATCH v2 0/5] Use complete_action's " Alban Gruin
2019-10-07  9:26   ` [PATCH v2 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-10-07  9:26   ` [PATCH v2 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-10-07  9:26   ` [PATCH v2 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-10-07  9:26   ` [PATCH v2 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-10-07  9:26   ` [PATCH v2 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-10-08  2:45   ` [PATCH v2 0/5] Use complete_action's todo list to do the rebase Junio C Hamano
2019-10-08 16:18     ` Alban Gruin
2019-10-14 12:49   ` Johannes Schindelin
2019-11-23 14:37   ` [PATCH v3 " Alban Gruin
2019-11-23 14:37     ` [PATCH v3 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-23 14:37     ` [PATCH v3 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-23 14:37     ` [PATCH v3 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-23 14:37     ` [PATCH v3 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-23 14:37     ` [PATCH v3 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-24 17:43     ` [PATCH v4 0/5] Use complete_action's todo list to do the rebase Alban Gruin
2019-11-24 17:43       ` [PATCH v4 1/5] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-11-24 17:43       ` [PATCH v4 2/5] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-11-24 17:43       ` [PATCH v4 3/5] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-11-24 17:43       ` [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() Alban Gruin
2019-11-26 18:27         ` Jonathan Tan
2019-11-24 17:43       ` [PATCH v4 5/5] sequencer: directly call pick_commits() from complete_action() Alban Gruin
2019-11-26 18:41         ` Jonathan Tan
2019-11-27 19:56           ` Alban Gruin
2019-11-27 20:09             ` Alban Gruin

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=xmqq5zl7hnwe.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood@dunelm.org.uk \
    /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).