Nicolas Pitre [off-list ref] writes:
On Mon, 9 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
quoted
nr_objects in the next patch is used to reflect the number of actual
objects in the stream, which may be smaller than the number recorded
in pack header.
This highlights an issue that has been nagging me for a while.
We decided to send the final number of objects in the thin pack header
for two reasons:
1) it allows to properly size the SHA1 table upfront which already
contains entries for the omitted objects;
2) the whole pack doesn't have to be re-summed again after being
completed on the receiving end since we don't alter the header.
However this means that the progress meter will now be wrong and that's
terrible ! Users *will* complain that the meter doesn't reach 100% and
they'll protest for being denied the remaining objects during the
transfer !
Joking aside, we should think about doing something about it. I was
wondering if some kind of prefix to the pack stream could be inserted
onto the wire when sending a pack v4. Something like:
'T', 'H', 'I', 'N', <actual_number_of_sent_objects_in_network_order>
This 8-byte prefix would simply be discarded by index-pack after being
parsed.
What do you think?
I do not think it is _too_ bad if the meter jumped from 92% to 100%
when we finish reading from the other end ;-), as long as we can
reliably tell that we read the right thing.
Which brings me to a tangent. Do we have a means to make sure that
the data received over the wire is bit-for-bit correct as a whole
when it is a thin pack stream? When it is a non-thin pack stream,
we have the checksum at the end added by sha1close() which
index-pack.c::parse_pack_objects() can (and does) verify.
On Mon, 9 Sep 2013, Junio C Hamano wrote:
Nicolas Pitre [off-list ref] writes:
quoted
On Mon, 9 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
quoted
nr_objects in the next patch is used to reflect the number of actual
objects in the stream, which may be smaller than the number recorded
in pack header.
This highlights an issue that has been nagging me for a while.
We decided to send the final number of objects in the thin pack header
for two reasons:
1) it allows to properly size the SHA1 table upfront which already
contains entries for the omitted objects;
2) the whole pack doesn't have to be re-summed again after being
completed on the receiving end since we don't alter the header.
However this means that the progress meter will now be wrong and that's
terrible ! Users *will* complain that the meter doesn't reach 100% and
they'll protest for being denied the remaining objects during the
transfer !
Joking aside, we should think about doing something about it. I was
wondering if some kind of prefix to the pack stream could be inserted
onto the wire when sending a pack v4. Something like:
'T', 'H', 'I', 'N', <actual_number_of_sent_objects_in_network_order>
This 8-byte prefix would simply be discarded by index-pack after being
parsed.
What do you think?
I do not think it is _too_ bad if the meter jumped from 92% to 100%
when we finish reading from the other end ;-), as long as we can
reliably tell that we read the right thing.
Sure. but eventually people will complain about this. So while we're
about to introduce a new pack format anyway, better think of this little
cosmetic detail now when it can be included in the pack v4 capability
negociation.
Which brings me to a tangent. Do we have a means to make sure that
the data received over the wire is bit-for-bit correct as a whole
when it is a thin pack stream? When it is a non-thin pack stream,
we have the checksum at the end added by sha1close() which
index-pack.c::parse_pack_objects() can (and does) verify.
The trailing checksum is still there. Nothing has changed in that
regard.
Nicolas