Re: [PATCH 02/10] sequencer.c: split up sequencer_remove_state()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 30/12/2022 17:35, René Scharfe wrote:
Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.

The only functional changes here are:

  * Changing the "int" to a "size_t", which is the correct type, as
    "xopts_nr" is a "size_t".

Good, and you declare it in the for statement, which we can do now!


  * Calling the free() before the "if (is_rebase_i(opts) && ...)",
    which is OK, and makes a subsequent change smaller.

It's true that is_rebase_i() can be called after all that free()ing;
here is its definition:

	static inline int is_rebase_i(const struct replay_opts *opts)
	{
		return opts->action == REPLAY_INTERACTIVE_REBASE;
	}

But why?  Making a subsequent change smaller is just a trivial fact if
you do a part if it earlier, but that in itself is not a valid reason
for the reordering.

Yes I'd prefer we did not reorder here either

Best Wishes

Phillip

And I can't find that patch -- sequencer_remove_state() is not touched
again in this series.


Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---
  sequencer.c | 25 +++++++++++++++----------
  1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..655ae9f1a72 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
  	return buf.buf;
  }

+static void replay_opts_release(struct replay_opts *opts)
+{
+	free(opts->gpg_sign);
+	free(opts->reflog_action);
+	free(opts->default_strategy);
+	free(opts->strategy);
+	for (size_t i = 0; i < opts->xopts_nr; i++)
+		free(opts->xopts[i]);
+	free(opts->xopts);
+	strbuf_release(&opts->current_fixups);
+}
+
  int sequencer_remove_state(struct replay_opts *opts)
  {
  	struct strbuf buf = STRBUF_INIT;
-	int i, ret = 0;
+	int ret = 0;
+
+	replay_opts_release(opts);

  	if (is_rebase_i(opts) &&
  	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
  		}
  	}

-	free(opts->gpg_sign);
-	free(opts->reflog_action);
-	free(opts->default_strategy);
-	free(opts->strategy);
-	for (i = 0; i < opts->xopts_nr; i++)
-		free(opts->xopts[i]);
-	free(opts->xopts);
-	strbuf_release(&opts->current_fixups);
-
  	strbuf_reset(&buf);
  	strbuf_addstr(&buf, get_dir(opts));
  	if (remove_dir_recursively(&buf, 0))



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux