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

From: Thomas Rast <trast@inf.ethz.ch>
Date: May 3, 2013, 17:56 UTC
Message-ID: <87y5bw3q1s.fsf@hexa.v.cablecom.net>
In-Reply-To: <1367555502-4706-5-git-send-email-felipe.contreras@gmail.com>

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

Show 5 quoted lines

> 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.

[...]

Show 10 quoted lines

> -			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