Re: [gpsd-dev] [PATCH] Support UBX NAV-PVT

[Fred Wright]

On Tue, 25 Jul 2017, Clark Li wrote:

> >Previously, I hadn't bothered to try to apply your patch after seeing
> >the issue I reported, but I've now tried it and it (either version)
> >fails to apply.  There seems to be some inconsistency between the state
> >of driver_ubx.c and what the patch expects.  Ordinarily this might be
> >due to the patch's being based on a different revision of the file, but
> >in this case the blob ID in the patch matches the state of the file, so
> >I'm puzzled.  Did you have uncommitted changes to the file at the time
> >you ran git format-patch?  I don't know what the effect of that is, but
> >it's my best guess as to why the patch is bad.
> That last patch was not based on the very latest in master branch. But I
> guess forwarding the output of git-send-email from gmail to outlook
> should also be blamed... I am attaching the original git-format-patch
> output this time, hopefully this one can be applied without conflicts.

It's not essential (though it's desirable) to base the patch on a
reasonably recent master, and that wasn't the problem in this case.  In
fact, as I mentioned, the blob IDs in the diff matched, meaning that the
files you were actually modifying matched, even though changes had been
made elsewhere.

I managed to figure out the main problem, which is that "git am" doesn't
seem to like patches sent as attachments, but screws up in very confusing
ways if you try.  Basically, it seems to go through the right motions, but
gets confused about line numbering.  The solution was to use "git apply"
rather than "git am", but that has the problem that it doesn't create the
commit message automatically, and I had to do that by hand.

What usually works well for emailed patches is to send the patch to the
list directly with "git send-email".  This results in exactly what "git
am" expects.  If you want to email the patch to yourself to test it,
that's fine, but to send it for real, resend it with "git send-email"
rather than forwarding a received version that may have been mangled by an
email client.

The one case I know of where this has trouble is where there are very
long lines, which sometimes happens in regression-test data.  In
principle, binary data could cause trouble as well, but it looks like "git
format-patch" is smart enough to make a binary diff in that case.

> >Except that if you look closely at the diff, you'll see that the only
> >actual change to that file is the introduction of some spurious line
> >breaks.  It makes sense that the output is effectively unchanged, but
> >it doesn't make sense that some spurious newlines appeared.  Did you
> >perhaps obtain the "updated" version via copy-n-paste rather than from
> >"regress-driver -b"?
> The chk file was updated using "regress-driver -b" but again this might be 
> due to the email forwarding...

I suspected something else due to the extra line breaks being in just the
"after" data and not the "before", suggesting that data was getting
garbled on the way *into* the diff rather than in the email.  But whatever
that problem was seems to have gone away now.

> >It's also odd that only that one case was affected by whatever you did.
> >Did you only run the one test case, rather than all of them (typically via 
> >"scons check")?
> scons check was run to check all the regression tests. ublox-aek-4t.log might 
> be the only regression that contains NAV-NAV-SOL.

The reason I suspected that is because only that one case had the
differences, which seemd not email-related as noted above.  But I now see
that there was a real, albeit subtle, difference masked by the newline
issue, namely a one-digit change in one number.  Presumably that's an
expected change.

I want to look at this some more before pushing it, but at least I have a
valid commit now.

Fred Wright