Re: [PATCH v2 32/34] sequencer (rebase -i): show the progress

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

 



Johannes Schindelin <johannes.schindelin@xxxxxx> writes:

> The interactive rebase keeps the user informed about its progress.
> If the sequencer wants to do the grunt work of the interactive
> rebase, it also needs to show that progress.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
>  sequencer.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

Sensible.  This further adds two comparisons with TODO_COMMENT and
makes need for is_counted() or something like that felt more.

    $ git grep TODO_COMMENT pu sequencer.c

shows that some places "x < TODO_COMMENT" is used while some other
places "y != TODO_COMMENT" is used, both for the same purpose, and
"z >= TODO_COMMENT" is also seen for the negation of the same.

I think all of them except for one can become is_counted() or
!is_counted() to convey what they want to check better while the one
used in the loop control:

	for (i = 0; i < TODO_COMMENT; i++)

should probably become:

	for (i = 0; i < ARRAY_SIZE(todo_command_info); i++)

as the parsing loop wants to check the input against all known
commands and there is no strong reason for that layer to know how
the insns are numbered.  is_xxx() implementation can take advantage
of the way the insns are numbered, but there is no point spreading
the knowledge to higher layer in the callchain.

Other than that, all 34 patches looked sensible steps explained as a
coherent story that unfolds bit by bit, which was mostly a pleasant
read.

Thanks.



[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]