Re: [GSoC PATCH v2 2/4] pack-write: add helper to fill promisor file after repack

[Junio C Hamano <gitster@pobox.com>]

LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:

> Create a `copy_all_promisor_files()` helper function used to copy the
> contents of all ".promisor" files in a `repository` inside another
> ".promisor" file.
>
> This function can be used to preserve the contents of all ".promisor"
> files inside a new ".promisor" file, for example when a repack happens.
>
> This function is written in such a way so that it will read all the
> ".promisor" files inside the given `repository` line by line, and copy
> only the lines that are not already present in the destination file. This
> is done to avoid copying the same lines multiple times that may come from
> multiple (redundant) packfiles. There might be another better/cleaner way
> to achieve this.

In the previous step, we extablished that these "back then their ref
X used to point at object Y" records are there so that we can
identify which refs were fetched at the time the packfile was
downloaded to help debugging.  When repacking, losing these records
certainly would lose information.

But would concatenating all into a single file help preserve the
useful information?  Don't we need do better than that?

A NEEDSWORK comment, as was discussed in another thread or two in
the recent past, is not necessarily a well thought out fully
finished specification of an additional piece of work.  "We know
this has a problem, we may need to do something about it, like
concatenating to save the contents, perhaps?  We do not know the
answer, and we do not bother thinking it through right at this
moment.  It is left to the future developers to figure it out" is
what a NEEDSWORK comment is about.

Your first response to such a comment may be "yeah, I agree that it
is bad to lose information we added to help debugging", but the
second one should be to wonder if the "like concatenating..." is the
best approach going forward.

In other words, we should take a NEEDSWORK comment as a mere
starting point, and what NEEDS your work begins at thinking what
needs to be done about the problem raised there.

By mixing them up all into a single list, you no longer can tell
when their ref X was observed to be pointing at object Y anymore.
You may have two packs originally, with a record for "ref X pointing
at object Y" in each of them, but by deduping them, you lose the
information that you cloned at one time, and made an additional
fetch on another day, and the fact the ref X was pointing at the
same value at both times.  I am not sure if it is a good
implementation if the objective of this topic is to preserve
information that is useful for debugging.

I wonder if it helps to append to each line the file timestamp of
the .promisor file we took the record originally?  For the sake of
completeness, we could consider adding the filename as well, but we
can quickly dismiss it as not so useful ;-)

If repacking already repacked promisor packfile, the records would
already contain such a timestamp at the end, so the code to copy
existing records must be prepared to see if the records are the
<ref, oid> tuple, or <ref, oid, timestamp> tuple, and act
accordingly.

I am *not* saying that without such a "preserve timestamp" column in
the record, copying existing records to a new .promisor file is
useless.  But we do not see any explanation why the author thinks
that it is sufficient to copy existing records while silently
deduping.  We can implement only one choice backed by series of
decisions like "timestamp might help" and "original filenames would
probably not help", and the design should describe what was
considered and rejected (as opposed to "we didn't think things
through---we just did what the original NEEDSWORK comment suggested
doing").

Thanks.