Re: [Quilt-dev] [PATCH] Enhanced decoration for "series -v" command

[Jean Delvare]

Hi all,

[Jean Delvare]
> First, why was the top patch treated any differently in the first
> place? I don't think it adds any value. The top patch is always the
> last applied one in the series (in other words, the last one
> prefixed with a "+" is we weren't treating it differently).

[Peter Williams]
> I asked myself the same question.

I think I found the answer. "quilt patches" will list all patches
modifying a given file. The list is basically a subset of what "quilt
series" outputs. One difference is that the top patch isn't necessarily
part of this list, which means that, in verbose mode, the last patch
with a "+" may or may not be the top patch. I guess this is a valid
reason to identify the top patch with a different prefix, and if we do
it in "quilt patches", we better do it in "quilt series" as well.

[Jean Delvare]
> Second, I noticed that "quilt patches" has an output similar to that
> of "quilt series". Wouldn't be better to refactor the printing logic
> and move it to patchfns? This would ensure that both outputs always
> use the same convention (which would be great IMHO), and may also
> save some code.

[Peter Williams]
> It would be nice if this output were the same as that from "quilt 
> series" as it would simplify adding this feature (currently not high
> on  my list) to gquilt.

I attempted to refactor this functionality, moving the common code from
"patches" and "series" to patchfns. The advantage in doing so is that we
are then guaranteed that all outputs will always use the same
convention, and there is a single place to change when adding
decorations or otherwise changing the output. My proposed patch is
attached, comments welcome.

 quilt/patches.in    |   29 ++++++++++-------------------
 quilt/series.in     |   25 ++++++-------------------
 scripts/patchfns.in |   20 ++++++++++++++++++++
 3 files changed, 36 insertions(+), 38 deletions(-)

It admittedly doesn't save that many lines of code, but I think it makes
the code cleaner and it improves maintainability. The performance drop
is only about 2%, sounds acceptable.

I first had print_patches() find the status of each patch by itself, but
I noticed a huge performance drop (quilt series was almost 3 times
slower than before). It happens that both callers already knew the
status of the patches, while print_patches() would have to test each
patch individually (using is_applied() and is_applied_last()). So I
changed the calling parameters to let the caller pass the information to
print_patches(), and the performance drop mostly disappeared.

Thanks,
-- 
Jean Delvare

quilt-CVS-refactor-print-patches.diff
Description: Text document