Re: abidiff: Added/Deleted/Changed markers

Re: abidiff: Added/Deleted/Changed markers

[Matthias Männich via libabigail]

Hi!

Thanks for looking into this!

On Tue, Mar 03, 2020 at 11:53:56AM +0000, Giuliano Procida wrote:
>Added/Deleted/Changed markers appear in abidiff output under various
>circumstances.
>
>In --leaf-changes-only mode, "[A]", "[D]", "[C]" appear
>unconditionally to prefix each of the changes (and notably, this is
>what we see when doing kernel ABI diffs). In normal mode, they only
>appear if there are more than 100 changes in the given section.
>
>"[C]" is also not always followed by a space character.
>
>The markers are redundant in that they can be determined from their
>context. They would be most useful if doing something like
>line-by-line processing of abidiff output, but they are not always
>emitted so that's not happening.
>
>I'd like to do one of two things to make abidiff output more
>consistent and predictable. Either option will require the scripted
>adjustment of test cases. It's straightforward (I've already done it).
>
>1. Emit them unconditionally, always followed by a space character.
>2. Remove them altogether.

I feel like option 1) would preserve the current behaviour and would
make it consistent. Dodji, any particular reason for the 100 threshold?
If not, I would suggest the 'emit unconditionally' way of addressing
this.

Cheers,
Matthias

>
>What option would be best? I don't have strong opinions here.
>
>Giuliano.

abidiff: Added/Deleted/Changed markers

[Giuliano Procida via libabigail]

Added/Deleted/Changed markers appear in abidiff output under various
circumstances.

In --leaf-changes-only mode, "[A]", "[D]", "[C]" appear
unconditionally to prefix each of the changes (and notably, this is
what we see when doing kernel ABI diffs). In normal mode, they only
appear if there are more than 100 changes in the given section.

"[C]" is also not always followed by a space character.

The markers are redundant in that they can be determined from their
context. They would be most useful if doing something like
line-by-line processing of abidiff output, but they are not always
emitted so that's not happening.

I'd like to do one of two things to make abidiff output more
consistent and predictable. Either option will require the scripted
adjustment of test cases. It's straightforward (I've already done it).

1. Emit them unconditionally, always followed by a space character.
2. Remove them altogether.

What option would be best? I don't have strong opinions here.

Giuliano.

Re: abidiff: Added/Deleted/Changed markers

[Dodji Seketeli]

Hello,

Matthias Männich <maennich@google.com> a écrit:

> Hi!
>
> Thanks for looking into this!
>
> On Tue, Mar 03, 2020 at 11:53:56AM +0000, Giuliano Procida wrote:
>>Added/Deleted/Changed markers appear in abidiff output under various
>>circumstances.
>>
>>In --leaf-changes-only mode, "[A]", "[D]", "[C]" appear
>>unconditionally to prefix each of the changes (and notably, this is
>>what we see when doing kernel ABI diffs). In normal mode, they only
>>appear if there are more than 100 changes in the given section.
>>
>>"[C]" is also not always followed by a space character.
>>
>>The markers are redundant in that they can be determined from their
>>context. They would be most useful if doing something like
>>line-by-line processing of abidiff output, but they are not always
>>emitted so that's not happening.
>>
>>I'd like to do one of two things to make abidiff output more
>>consistent and predictable. Either option will require the scripted
>>adjustment of test cases. It's straightforward (I've already done it).
>>
>>1. Emit them unconditionally, always followed by a space character.
>>2. Remove them altogether.
>
> I feel like option 1) would preserve the current behaviour and would
> make it consistent. Dodji, any particular reason for the 100 threshold?
> If not, I would suggest the 'emit unconditionally' way of addressing
> this.

It's true that the markers are redundant in that they can be determined
from their context.  The problem though is that when there are many
changes (many lines) on the screen, the user cannot see the context
without scrolling.

Some users reported this little annoyance to me many years ago, so
that's why we decided to just add the markers when they were "many"
changes.

Now I guess that users who want to grep the output for various reasons,
would want more predictability (consistency) here.

I don't have a strong opinion, so I'll agree with whatever you guys find
more useful. I would tend to go for the "emit unconditionally" as well
and say that redundancy is not a real issue in this case.  But that's
just me.

Cheers,

-- 
		Dodji

Re: abidiff: Added/Deleted/Changed markers

[Giuliano Procida via libabigail]

Hi.

On Fri, 6 Mar 2020 at 10:18, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello,
>
> Matthias Männich <maennich@google.com> a écrit:
>
> > Hi!
> >
> > Thanks for looking into this!
> >
> > On Tue, Mar 03, 2020 at 11:53:56AM +0000, Giuliano Procida wrote:
> >>Added/Deleted/Changed markers appear in abidiff output under various
> >>circumstances.
> >>
> >>In --leaf-changes-only mode, "[A]", "[D]", "[C]" appear
> >>unconditionally to prefix each of the changes (and notably, this is
> >>what we see when doing kernel ABI diffs). In normal mode, they only
> >>appear if there are more than 100 changes in the given section.
> >>
> >>"[C]" is also not always followed by a space character.
> >>
> >>The markers are redundant in that they can be determined from their
> >>context. They would be most useful if doing something like
> >>line-by-line processing of abidiff output, but they are not always
> >>emitted so that's not happening.
> >>
> >>I'd like to do one of two things to make abidiff output more
> >>consistent and predictable. Either option will require the scripted
> >>adjustment of test cases. It's straightforward (I've already done it).
> >>
> >>1. Emit them unconditionally, always followed by a space character.
> >>2. Remove them altogether.
> >
> > I feel like option 1) would preserve the current behaviour and would
> > make it consistent. Dodji, any particular reason for the 100 threshold?
> > If not, I would suggest the 'emit unconditionally' way of addressing
> > this.
>
> It's true that the markers are redundant in that they can be determined
> from their context.  The problem though is that when there are many
> changes (many lines) on the screen, the user cannot see the context
> without scrolling.
>
> Some users reported this little annoyance to me many years ago, so
> that's why we decided to just add the markers when they were "many"
> changes.

I suppose it could be put under flag control, but I'd be really
reluctant to add more logic (and so more things to go wrong, more
tests etc.).

To get an idea of how annoying the tags are in practice you can take a
look at the test files in the commits I sent yesterday.

> Now I guess that users who want to grep the output for various reasons,
> would want more predictability (consistency) here.
>
> I don't have a strong opinion, so I'll agree with whatever you guys find
> more useful. I would tend to go for the "emit unconditionally" as well
> and say that redundancy is not a real issue in this case.  But that's
> just me.

We are currently using abidiff in leaf-changes-only mode (which
already does emit the tags unconditionally). So, please discount our
opinions when it comes to the normal mode of operation.

> Cheers,
>
> --
>                 Dodji

Regards,
Giuliano.