fast-import: fix lookup for new commits - #2252

Hello Marc,

Sorry for the noise, my first mail lost its In-Reply-To header on the
way, so it is not in this thread. Resending the same review here.

I tried this on top of 6de20f609 with DEVELOPER=1 and did not get warnings.

> Queries for commits created during the same 'git fast-import'  session
> result in a crash:
>     < reset refs/heads/newbranch
>     < from <new-commit-oid>
>     > fatal: not a valid commit: <new-commit-oid>

I was able to repro this. With the patch fsck is clean and it works
nicely. Same goes for "commit" with "from <new-oid>", a tag made in
the same session, and a tag of a tag. Existing commits and tags and
"from <blob>" behave like before.

One small nitpick: there is a double space in "'git fast-import'  session".

> to avoid crash and (eventually) improve lookup speed.

Did you measure this? If not, maybe drop it or say it is only a
possible side effect.

> +static struct object_entry *dereference(struct object_entry *oe, struct object_id *oid);

This is over 80 columns, can it be wrapped like the definition?

>  builtin/fast-import.c | 18 ++++++++++++++----

I could not find a test for this. Can you add one to t9300? You can
get the new oid with get-mark and use it in a "reset ... from <oid>"
in the same stream.

Thanks,
Aniket

"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);
>  	}

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

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index d9de2ef0d8..1f52be2c2c 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -3938,4 +3938,72 @@ test_expect_success 'Y: rewrite submodules' '
>  	)
>  '
>  
> +cat >Y-reference-pending-sha256 <<\Y_INPUT_END
> +commit refs/heads/main
> +committer A U Thor <author@example.com> 1712563200 +0000
> +data 14
> +commit message
> +M 644 inline test file
> +data 14
> +dummy content
> +
> +tag newtag
> +from 17e650f097a4db6fe069c96935360d3ccc9b35c4018728c5f311a4217c2ced3d
> +tagger A U Thor <author@example.com> 1712563300 +0000
> +data 11
> +tag message
> +# no emty line after tag data!
> +reset refs/heads/other
> +from 85edc5eac315bc680677b2fc71622bd77476f057eb8dc8ed6a497803a24f1f46
> +
> +commit refs/heads/further
> +committer A U Thor <author@example.com> 1712563400 +0000
> +data 21
> +what about 2nd commit
> +from 17e650f097a4db6fe069c96935360d3ccc9b35c4018728c5f311a4217c2ced3d
> +M 644 inline other test file
> +data 19
> +more dummy content
> +
> +done
> +Y_INPUT_END
> +
> +cat >Y-reference-pending-sha1 <<\Y_INPUT_END
> +commit refs/heads/main
> +committer A U Thor <author@example.com> 1712563200 +0000
> +data 14
> +commit message
> +M 644 inline test file
> +data 14
> +dummy content
> +
> +tag newtag
> +from 85b46dd918355ff17b4109ec738723d42f235a04
> +tagger A U Thor <author@example.com> 1712563300 +0000
> +data 11
> +tag message
> +# no emty line after tag data!
> +reset refs/heads/other
> +from ec6c4db71feb5f66615214dad2324ce030e777a7
> +
> +commit refs/heads/further
> +committer A U Thor <author@example.com> 1712563400 +0000
> +data 21
> +what about 2nd commit
> +from 85b46dd918355ff17b4109ec738723d42f235a04
> +M 644 inline other test file
> +data 19
> +more dummy content
> +
> +done
> +Y_INPUT_END

The above duplicates a rather simple test, and only differences
between them are the embedded object names.  Can't you use the
test_oid_cache & test_oid mechanism to reduce the repetition to
make the tests easier to maintain?  It may look something like this:

test_expect_success 'Y: reference pending tag or commit' '
	test_oid_cache <<-EOF &&
	pending_commit sha1:86b46d....5a04
	pending_commit sha256:17e65...ed3d
	pending_tag sha1:ec6c6d...77a7
	pending_tag sha256:85edc5...1f46
	EOF

	git init reference-pending &&
	(
		cd reference-pending &&
                git fast-import <<-EOF &&
                commit refs/heads/main
                committer A U Thor <author@example.com> 1712563200 +0000
                ...
                dummy content

                tag newtag
                from $(test_oid pending_commit)
                tagger A U Thor <author@example.com> 1712563300 +0000
     		...
		data 19
		more dummy content

		done
		EOF
	)
'

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

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index d9de2ef0d8..9001e2572a 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -3938,4 +3938,47 @@ test_expect_success 'Y: rewrite submodules' '
>  	)
>  '
>  
> +test_expect_success 'Y: reference pending tag or commit' '
> +	test_oid_cache <<-EOF &&
> +		pending_commit sha1:85b46dd918355ff17b4109ec738723d42f235a04
> +		pending_commit sha256:17e650f097a4db6fe069c96935360d3ccc9b35c4018728c5f311a4217c2ced3d
> +		pending_tag sha1:ec6c4db71feb5f66615214dad2324ce030e777a7
> +		pending_tag sha256:85edc5eac315bc680677b2fc71622bd77476f057eb8dc8ed6a497803a24f1f46
> +		EOF

Looking at the output from

    $ git grep -A4 'test_oid_cache <<' t

tells us that here-doc used in the above is indented by one level
too deep.  I will leave checking how the "git fast-import <<-EOF"
should be written as an exercise to the reader ;-)

Thanks.

> +	git init reference-pending &&
> +	(
> +		cd reference-pending &&
> +		git fast-import <<-EOF
> +			commit refs/heads/main
> +			committer A U Thor <author@example.com> 1712563200 +0000
> + ...
> +			done
> +			EOF
> +	)
> +'

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

> From: Marc Becker <becm@gmx.de>
>
> 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.
>
> Signed-off-by: Marc Becker <becm@gmx.de>
> ---
>     fast-import: fix lookup for new commits
>     
>     Changes since v3:
>     
>      * align here-doc indention in test with expected style

Thanks.  This round I have nothing more to add.

Will queue.