Re: [PATCH v2] fast-import: fix lookup for new commits and tags

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

"Marc Becker via GitGitGadget" <gitgitgadget@gmail.com> writes:

The patch text seems to make sense, but ...

> From: Marc Becker <becm@gmx.de>
>
> Queries for commits created during the same 'git fast-import' session
> result in a crash:

Should it say "sometimes results in a crash"?  It is not like that
no queries for commits can ever be made before the session gets
closed, right?

>     < reset refs/heads/newbranch
>     < from <new-commit-oid>
>     > fatal: not a valid commit: <new-commit-oid>
>
> Getting file content for commit has no issue (working implementation):

... what do you mean by "(working implementation)" here?  Do you
mean that without your patch applied, neither commits nor blobs can
be read, and with the patch applied we get a "working
implementation" and blobs can be read?  Or do you mean that objects
other than commits do not have such a problem?

>     < ls <new-commit-oid> file-in-new-commit.txt
>     > {file content}

Or do you mean the current code does not allow reading commits but
everything else it does allow reading?  If so, such a description
should be followed by why commit alone is so special, or put it
differently, why blobs can be accessed just fine.  Otherwise, saying
that getting file content would work fine only confuses readers
without adding much value to them.

> During import, new objects may not (yet) be reachable via the ODB layer.

This "may not (yet)" is very believable; I would be very surprised
if we were holding any and all object data we were fed in core
without flushing to the ODB, but depending on the timing of your
query, necessary objects may or may not have been flushed.  Is that
what you meant by "may not (yet)" here?  If so, that confirms my
"sometimes results in a crash" above, right?

Here is what my assistant produced trying to guess what the commit
wanted to say.  I do not know how close it is to what you wanted to
say, but if the change were explained like this, it may have been
easier to understand.  At least, failures for commit vs success with blobs
you hinted at is explained sufficiently.

Of course, my assistant may be totally off the mark and the following may
not explain what you wanted to do at all.  I dunno.

    fast-import: allow 'from' to reference new commits and tags by oid

    During a 'git fast-import' session, newly created objects are written
    to an in-progress packfile and recorded in fast-import's internal
    object table.  Until a 'checkpoint' command or the end of the session
    finalizes and indexes that packfile, these objects are not visible to
    the repository's object database via odb_read_object_peeled().

    Commands such as 'ls' (via parse_treeish_dataref() and dereference())
    and 'cat-blob' already account for this when given a hex object name:
    they look up the object in fast-import's internal object table with
    find_object() first and read it from the in-progress packfile with
    gfi_unpack_entry() when its '.pack_id' is not MAX_PACK_ID.

    By contrast, when 'from <committish>' (in a 'commit' or 'reset'
    command) is given a hex object name instead of a mark or branch name,
    parse_objectish() parses the hex string into 'b->oid' and calls
    parse_from_existing(), which unconditionally looks up the object via
    odb_read_object_peeled().  If the object is a commit or tag created in
    the current pack cycle, odb_read_object_peeled() returns NULL and
    fast-import dies with "fatal: not a valid commit: <oid>".

    Teach parse_from_existing() to look up 'b->oid' with find_object()
    first, peel any tag objects in the object table using dereference(),
    and unpack the target commit with gfi_unpack_entry() if it resides in
    the in-progress packfile, before falling back to
    odb_read_object_peeled() for objects in the repository's object
    database.

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index fbd919982c..574a03c484 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -292,6 +292,9 @@ static void fast_import_state_init(struct fast_import_state *state,
>  	state->option = option;
>  }
>  
> +static struct object_entry *dereference(struct object_entry *oe,
> +					struct object_id *oid);
> +
>  static void parse_argv(struct fast_import_state *state);
>  static void parse_get_mark(struct fast_import_state *state, const char *p);
>  static void parse_cat_blob(struct fast_import_state *state, const char *p);
> @@ -2658,12 +2661,20 @@ static void parse_from_existing(struct branch *b)
>  		oidclr(&b->branch_tree.versions[1].oid, the_repository->hash_algo);
>  	} else {
>  		unsigned long size;
> -		size_t size_st = 0;
>  		char *buf;
> +		struct object_entry *e = find_object(&b->oid);
>  
> -		buf = odb_read_object_peeled(the_repository->objects, &b->oid,
> -					     OBJ_COMMIT, &size_st, &b->oid);
> -		size = cast_size_t_to_ulong(size_st);
> +		while (e && e->type == OBJ_TAG)
> +			e = dereference(e, &b->oid);

This would mean we would peel down a tag that points at a tree or a
blob to tree or blob.  And that object is not a commit, so we would
continue going down the other "read from odb_read_object_peeled()"
code path.  That is the same behaviour as the original (in happy
case, when the objects are flushed to the odb layer).

> +		if (e && e->type == OBJ_COMMIT && e->pack_id != MAX_PACK_ID) {
> +			buf = gfi_unpack_entry(e, &size);
> +		} else {
> +			size_t size_st = 0;
> +			buf = odb_read_object_peeled(the_repository->objects, &b->oid,
> +						     OBJ_COMMIT, &size_st, &b->oid);
> +			size = cast_size_t_to_ulong(size_st);

And the code inside this "else" clause is the same as the original.

> +		}

So the change itself makes sense.  It is that it is not clear if the
"makes sense" I just said matches what you described in your proposed log
message.

>  		parse_from_commit(b, buf, size);
>  		free(buf);
>  	}