Re: [PATCH v3] sequencer.c: use reverse_commit_list() helper

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2022, 19:44 UTC
Message-ID: <xmqqee31lm1w.fsf@gitster.g>
In-Reply-To: <pull.1177.v3.git.1647429623830.gitgitgadget@gmail.com>

"Jayati Shrivastava via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Jayati Shrivastava <gaurijove@gmail.com>
>
> Instead of creating a new allocation, reverse the
> original list in-place by calling the
> reverse_commit_list() helper.

OK. I recall that v2 wrapped with longer lines that was easier to read.

Show 9 quoted lines

> The original code
> discards the list "bases" after storing its
> reverse copy in a newly created list "reversed".
> If the code that followed from here used both
> "bases" and "reversed", the modification would not
> have worked, but since the original list "bases"
> gets discarded, we can simply reverse "bases"
> in-place with the reverse_commit_list() helper and
> reuse the same variable in the code that follows.

I am 30% surprised to see this in the log message ;-). There is nothing incorrect in the description per-se, but it is something I would expect in a review that reads a patch and follows along thinking aloud to see if the code makes sense, or below "---", but it does explain why the patch chose to lose the extra variable, so probably it is OK.

> builtin/merge.c has been left unmodified, since in its case, the
> original list is needed separately from its reverse copy by the
> code.

Good.

The patch text looks good, too.

Thanks.