Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Jonathan Nieder <jrnieder@gmail.com>]

Sverre Rabbelier wrote:

> When a revision is specified on the commandline we explicitly output
> a 'reset' command for it if it was not handled already. This allows
> for example the remote-helper protocol to use fast-export to create
> branches that point to a commit that has already been exported.
>
> Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>

Thanks.  I'd suggest squashing in the test from patch 1/3 for easy
reference (since each patch makes the other easier to understand).

> ---
>   The if statement dealing with tag_of_filtered_mode is not as
>   elegant as either me or Dscho would have liked, but we couldn't
>   find a better way to determine if a ref is a tag at this point
>   in the code.
>
>   Additionally, the elem->whence != REV_CMD_RIGHT case should really
>   check if REV_CMD_RIGHT_REF, but as this is not provided by the
>   ref_info structure this is left as is. A result of this is that
>   incorrect input will result in incorrect output, rather than an
>   error message. That is: `git fast-export a..<sha1>` will
>   incorrectly generate a `reset <sha1>` statement in the fast-export
>   stream.
>
>   The dwim_ref bit is a double work (it has already been done by the
>   caller of this function), but I decided it would be more work to
>   pass this information along than to recompute it for the few
>   commandline refs that were relevant.

These details seem like good details for the commit message, so the
next puzzled person looking at the code can see what behavior is
deliberate and what are the incidental side-effects.

The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds
worth a test.

> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -18,6 +18,8 @@
>  #include "parse-options.h"
>  #include "quote.h"
> 
> +#define REF_HANDLED (ALL_REV_FLAGS + 1)

Could TMP_MARK be used for this?

[...]
> @@ -541,10 +543,34 @@ static void handle_reset(const char *name, struct object *object)
>  		       sha1_to_hex(object->sha1));
>  }
>  
> -static void handle_tags_and_duplicates(struct string_list *extra_refs)
> +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs)
>  {
>  	int i;
>  
> +	/* even if no commits were exported, we need to export the ref */
> +	for (i = 0; i < revs->cmdline.nr; i++) {

Might be clearer in a new function.

> +		struct rev_cmdline_entry *elem = &revs->cmdline.rev[i];
> +
> +		if (elem->flags & UNINTERESTING)
> +			continue;
> +
> +		if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT)
> +			continue;

Oh, neat.

> +
> +		char *full_name;

declaration-after-statement

> +		dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name);
> +
> +		if (!prefixcmp(full_name, "refs/tags/") &&

What happens if dwim_ref fails, perhaps because a ref was deleted in
the meantime?

> +			(tag_of_filtered_mode != REWRITE ||
> +			!get_object_mark(elem->item)))
> +			continue;

Style nit: this would be easier to read if the "if" condition doesn't
line up with the code below it:

		if (!prefixcmp(full_name, "refs/tags/")) {
			if (tag_of_filtered_mode != REWRITE ||
			    !get_object_mark(elem->item))
				continue;
		}

If tag_of_filtered_mode == ABORT, we are going to die() soon, right?
So this seems to be about tag_of_filtered_mode == DROP --- makes
sense.

When does the !get_object_mark() case come up?

> +
> +		if (!(elem->flags & REF_HANDLED)) {
> +			handle_reset(full_name, elem->item);
> +			elem->flags |= REF_HANDLED;
> +		}

Just curious: is the REF_HANDLED handling actually needed?  What
would happen if fast-export included the redundant resets?

> +	}
[...]
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -446,7 +446,7 @@ from $(git rev-parse master)
>  
>  EOF
>  
> -test_expect_failure 'refs are updated even if no commits need to be exported' '
> +test_expect_success 'refs are updated even if no commits need to be exported' '
>  	git fast-export master..master > actual &&
>  	test_cmp expected actual
>  '

Thanks for a pleasant read.