Michał Kiedrowicz [off-list ref] writes:
+ if (!@$add) {
+ # pure removal
+...
+ } elsif (!@$rem) {
+ # pure addition
+...
+ } else {
+ # assume that it is change
+ print join '',
I know this is not a new problem, but if your patch hunk has both '-' and
'+' lines, what's there to "assume" that it is a change? Isn't it always?
- # empty add/rem block on start context block, or end of chunk
- if ((@rem || @add) && (!$class || $class eq 'ctx')) {
-...
+ ## print from accumulator when have some add/rem lines or end
+ # of chunk (flush context lines)
+ if (((@rem || @add) && $class eq 'ctx') || !$class) {
This seems to change the condition. Earlier, it held true if (there is
anything to show), and (class is unset or equal to ctx). The new code
says something different. Also can $class be undef, and if so, doesn't
it trigger comparison between undef and 'ctx' by having !$class check at
the end of || chain?
Junio C Hamano wrote:
Michał Kiedrowicz [off-list ref] writes:
quoted
+ if (!@$add) {
+ # pure removal
+...
+ } elsif (!@$rem) {
+ # pure addition
+...
+ } else {
+ # assume that it is change
+ print join '',
I know this is not a new problem, but if your patch hunk has both '-' and
'+' lines, what's there to "assume" that it is a change? Isn't it always?
What I meant here when I was writing it that they are lines that changed
between two versions, like '!' in original (not unified) context format.
We can omit this comment.
quoted
- # empty add/rem block on start context block, or end of chunk
- if ((@rem || @add) && (!$class || $class eq 'ctx')) {
-...
+ ## print from accumulator when have some add/rem lines or end
+ # of chunk (flush context lines)
+ if (((@rem || @add) && $class eq 'ctx') || !$class) {
This seems to change the condition. Earlier, it held true if (there is
anything to show), and (class is unset or equal to ctx). The new code
says something different.
Yes it does, as described in the commit message:
[...] It should
not change the gitweb output, but it **slightly changes its behavior**.
Before this commit, context is printed on the class change. Now, it's
printed just before printing added and removed lines, and at the end of
chunk.
The difference is that context lines are also printed accumulated now.
Though why this change is required for refactoring could have been
described in more detail...
Also can $class be undef, and if so, doesn't
it trigger comparison between undef and 'ctx' by having !$class check at
the end of || chain?
Thanks for noticing this (I wonder why testsuite didn't caught it).
It should be
+ ## print from accumulator when have some add/rem lines or end
+ # of chunk (flush context lines)
+ if (!$class || ((@rem || @add) && $class eq 'ctx')) {
--
Jakub Narebski
Poland
Jakub Narebski [off-list ref] wrote:
Junio C Hamano wrote:
quoted
Michał Kiedrowicz [off-list ref] writes:
quoted
+ if (!@$add) {
+ # pure removal
+...
+ } elsif (!@$rem) {
+ # pure addition
+...
+ } else {
+ # assume that it is change
+ print join '',
I know this is not a new problem, but if your patch hunk has both
'-' and '+' lines, what's there to "assume" that it is a change?
Isn't it always?
What I meant here when I was writing it that they are lines that
changed between two versions, like '!' in original (not unified)
context format.
We can omit this comment.
OK.
quoted
quoted
- # empty add/rem block on start context block, or
end of chunk
- if ((@rem || @add) && (!$class || $class eq
'ctx')) { -...
+ ## print from accumulator when have some add/rem
lines or end
+ # of chunk (flush context lines)
+ if (((@rem || @add) && $class eq 'ctx')
|| !$class) {
This seems to change the condition. Earlier, it held true if
(there is anything to show), and (class is unset or equal to ctx).
The new code says something different.
Yes it does, as described in the commit message:
[...] It should
not change the gitweb output, but it **slightly changes its
behavior**. Before this commit, context is printed on the class
change. Now, it's printed just before printing added and removed
lines, and at the end of chunk.
The difference is that context lines are also printed accumulated now.
Though why this change is required for refactoring could have been
described in more detail...
I changed that because I wanted to squash both conditions (the one that
checks if @ctx should be printed and the one that prints @add/@rem
lines) and have just one call to print_sidebyside_diff_lines(). Later,
this function is changed to print_diff_lines() and controls whether
'inline' or 'side-by-side' diff should be printed. Having two
conditions and two calls/functions would make the code redundant. Then
I thought that instead of calling twice print_sidebyside_diff_lines()
(for @ctx and @add/@rem lines, like the code from pre-image prints
these lines separatedly), I can just call it once.
I can revert this change to previous behavior but I think that would
make the condition more complicated.
quoted
Also can $class be undef, and if so,
doesn't it trigger comparison between undef and 'ctx' by
having !$class check at the end of || chain?
Thanks for noticing this (I wonder why testsuite didn't caught it).
It should be
+ ## print from accumulator when have some add/rem
lines or end
+ # of chunk (flush context lines)
+ if (!$class || ((@rem || @add) && $class eq 'ctx'))
{
OK, I'll fix that.
Michal Kiedrowicz wrote:
Jakub Narebski [off-list ref] wrote:
quoted
Junio C Hamano wrote:
quoted
Michał Kiedrowicz [off-list ref] writes:
quoted
quoted
quoted
- # empty add/rem block on start context block, or
end of chunk
- if ((@rem || @add) && (!$class || $class eq
'ctx')) { -...
+ ## print from accumulator when have some add/rem
lines or end
+ # of chunk (flush context lines)
+ if (((@rem || @add) && $class eq 'ctx')
|| !$class) {
This seems to change the condition. Earlier, it held true if
(there is anything to show), and (class is unset or equal to ctx).
The new code says something different.
Yes it does, as described in the commit message:
[...] It should
not change the gitweb output, but it **slightly changes its
behavior**. Before this commit, context is printed on the class
change. Now, it's printed just before printing added and removed
lines, and at the end of chunk.
The difference is that context lines are also printed accumulated now.
Though why this change is required for refactoring could have been
described in more detail...
I changed that because I wanted to squash both conditions (the one that
checks if @ctx should be printed and the one that prints @add/@rem
lines) and have just one call to print_sidebyside_diff_lines(). Later,
this function is changed to print_diff_lines() and controls whether
'inline' or 'side-by-side' diff should be printed. Having two
conditions and two calls/functions would make the code redundant. Then
I thought that instead of calling twice print_sidebyside_diff_lines()
(for @ctx and @add/@rem lines, like the code from pre-image prints
these lines separatedly), I can just call it once.
I can revert this change to previous behavior but I think that would
make the condition more complicated.
No, I think that this change is good idea if it simplifies code flow.
But it really should be described in commit message, not only "what"
(which you did describe), but also "whys".
--
Jakub Narebski
Poland
Jakub Narebski [off-list ref] wrote:
Michal Kiedrowicz wrote:
quoted
Jakub Narebski [off-list ref] wrote:
quoted
Junio C Hamano wrote:
quoted
Michał Kiedrowicz [off-list ref] writes:
quoted
quoted
quoted
quoted
- # empty add/rem block on start context block, or
end of chunk
- if ((@rem || @add) && (!$class || $class eq
'ctx')) { -...
+ ## print from accumulator when have some add/rem
lines or end
+ # of chunk (flush context lines)
+ if (((@rem || @add) && $class eq 'ctx')
|| !$class) {
This seems to change the condition. Earlier, it held true if
(there is anything to show), and (class is unset or equal to ctx).
The new code says something different.
Yes it does, as described in the commit message:
[...] It should
not change the gitweb output, but it **slightly changes its
behavior**. Before this commit, context is printed on the class
change. Now, it's printed just before printing added and removed
lines, and at the end of chunk.
The difference is that context lines are also printed accumulated
now. Though why this change is required for refactoring could have
been described in more detail...
I changed that because I wanted to squash both conditions (the one
that checks if @ctx should be printed and the one that prints
@add/@rem lines) and have just one call to
print_sidebyside_diff_lines(). Later, this function is changed to
print_diff_lines() and controls whether 'inline' or 'side-by-side'
diff should be printed. Having two conditions and two
calls/functions would make the code redundant. Then I thought that
instead of calling twice print_sidebyside_diff_lines() (for @ctx
and @add/@rem lines, like the code from pre-image prints these
lines separatedly), I can just call it once.
I can revert this change to previous behavior but I think that would
make the condition more complicated.
No, I think that this change is good idea if it simplifies code flow.
But it really should be described in commit message, not only "what"
(which you did describe), but also "whys".
Sure, I'll try to put my explanation to the commit message.