Re: [PATCH 4/4] fast-import: only store commit objects

[Thomas Rast <trast@inf.ethz.ch>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no point in storing blob, they would increase the time of
> loading the marks, and the vast majority of them will never be used
> again.
>
> This also makes fast-export and fast-import marks compatible.
[...]
> -			if (m->data.marked[k])
> +			if (m->data.marked[k]) {
> +				struct object_entry *e;
> +				e = m->data.marked[k];
> +				if (e->type != OBJ_COMMIT)
> +					continue;
>  				fprintf(f, ":%" PRIuMAX " %s\n", base + k,
> -					sha1_to_hex(m->data.marked[k]->idx.sha1));
> +					sha1_to_hex(e->idx.sha1));
> +			}

IIUC, you are unconditionally storing only marks to commit objects.

Are you allowed to do that at this point?  I notice that
git-fast-export(1) says

   --export-marks=<file>
       Dumps the internal marks table to <file> when complete. Marks are
       written one per line as :markid SHA-1. Only marks for revisions
       are dumped[...]

But git-fast-import(1) says nothing of the sort; I would even claim that

   --export-marks=<file>
       Dumps the internal marks table to <file> when complete.

means that the *full* marks table is dumped.

How do we know that this doesn't break any users of fast-import?  Your
comment isn't very reassuring:

> the vast majority of them will never be used again

So what's with the minority?

In any case, if this does go in, please update the documentation to
match, probably by copying the sentence from git-fast-export(1).

-- 
Thomas Rast
trast@{inf,student}.ethz.ch