Re: [PATCH 06/10] fast-export: when using paths, avoid corrupt stream with non-existent mark

[Jeff King <peff@peff.net>]

On Sat, Nov 10, 2018 at 10:23:08PM -0800, Elijah Newren wrote:

> If file paths are specified to fast-export and multiple refs point to a
> commit that does not touch any of the relevant file paths, then
> fast-export can hit problems.  fast-export has a list of additional refs
> that it needs to explicitly set after exporting all blobs and commits,
> and when it tries to get_object_mark() on the relevant commit, it can
> get a mark of 0, i.e. "not found", because the commit in question did
> not touch the relevant paths and thus was not exported.  Trying to
> import a stream with a mark corresponding to an unexported object will
> cause fast-import to crash.
> 
> Avoid this problem by taking the commit the ref points to and finding an
> ancestor of it that was exported, and make the ref point to that commit
> instead.

As with the earlier tag commit, I wonder if this might depend on the
context in which you're using fast-export. I suppose that if you did not
feed the ref on the command line that we would not be dealing with it at
all (and maybe that is the answer to my question about the tag thing,
too).

It does seem funny that the behavior for the earlier case (bounded
commits) and this case (skipping some commits) are different. Would you
ever want to keep walking backwards to find an ancestor in the earlier
case? Or vice versa, would you ever want to simply delete a tag in a
case like this one?

I'm not sure sure, but I suspect you may have thought about it a lot
harder than I have. :)

> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index a3c044b0af..5648a8ce9c 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -900,7 +900,18 @@ static void handle_tags_and_duplicates(void)
>  			if (anonymize)
>  				name = anonymize_refname(name);
>  			/* create refs pointing to already seen commits */
> -			commit = (struct commit *)object;
> +			commit = rewrite_commit((struct commit *)object);
> +			if (!commit) {
> +				/*
> +				 * Neither this object nor any of its
> +				 * ancestors touch any relevant paths, so
> +				 * it has been filtered to nothing.  Delete
> +				 * it.
> +				 */
> +				printf("reset %s\nfrom %s\n\n",
> +				       name, sha1_to_hex(null_sha1));
> +				continue;
> +			}

This hunk makes sense.

> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -386,6 +386,30 @@ test_expect_success 'path limiting with import-marks does not lose unmodified fi
>  	grep file0 actual
>  '
>  
> +test_expect_success 'avoid corrupt stream with non-existent mark' '
> +	test_create_repo avoid_non_existent_mark &&
> +	(
> +		cd avoid_non_existent_mark &&
> +
> +		touch important-path &&
> +		git add important-path &&
> +		test_commit initial &&
> +
> +		touch ignored &&
> +		git add ignored &&
> +		test_commit whatever &&
> +
> +		git branch A &&
> +		git branch B &&
> +
> +		echo foo >>important-path &&
> +		git add important-path &&
> +		test_commit more changes &&
> +
> +		git fast-export --all -- important-path | git fast-import --force
> +	)
> +'

Similar comments apply about "touch" and "test_commit" to what I wrote
for the earlier patch.

-Peff