git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 6, 2011, 05:01 UTC
Message-ID
<20111106050126.GO27272@elie.hsd1.il.comcast.net>
In-Reply-To
<1320535407-4933-4-git-send-email-srabbelier@gmail.com>
Sverre Rabbelier wrote:
Show 7 quoted lines
> 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).

Show 18 quoted lines
> ---
>   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.

Show 7 quoted lines
> --- 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?
[...]
Show 11 quoted lines
> @@ -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.
Show 7 quoted lines
> +		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?
Show 5 quoted lines
> +
> +		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?

> +	}
[...]
Show 11 quoted lines
> --- 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.
Previous: Sverre RabbelierNext: Sverre Rabbelier
Message 36 of 42 in “fast-export fixes”
  1. 0/3 fast-export fixesSverre Rabbelier, Nov 5, 2011
  2. 1/3 t9350: point out that refs are not updated correctlySverre Rabbelier, Nov 5, 2011
  3. Jonathan NiederNov 6, 2011
  4. Sverre RabbelierNov 6, 2011
  5. Jonathan NiederNov 7, 2011
  6. Felipe ContrerasOct 24, 2012
  7. Jonathan NiederOct 24, 2012
  8. Felipe ContrerasOct 24, 2012
  9. Jonathan NiederOct 24, 2012
  10. Felipe ContrerasOct 25, 2012
  11. Jonathan NiederOct 25, 2012
  12. Felipe ContrerasOct 25, 2012
  13. Jonathan NiederOct 25, 2012
  14. Sverre RabbelierOct 25, 2012
  15. Felipe ContrerasOct 25, 2012
  16. Sverre RabbelierOct 25, 2012
  17. Felipe ContrerasOct 25, 2012
  18. Sverre RabbelierOct 25, 2012
  19. Jonathan NiederOct 25, 2012
  20. Sverre RabbelierOct 25, 2012
  21. Jonathan NiederOct 25, 2012
  22. Sverre RabbelierOct 25, 2012
  23. Felipe ContrerasOct 25, 2012
  24. Felipe ContrerasOct 25, 2012
  25. Jonathan NiederOct 25, 2012
  26. Felipe ContrerasOct 25, 2012
  27. Jonathan NiederOct 25, 2012
  28. Felipe ContrerasOct 25, 2012
  29. Johannes SchindelinOct 24, 2012
  30. Felipe ContrerasOct 25, 2012
  31. 2/3 fast-export: do not refer to non-existing marksSverre Rabbelier, Nov 5, 2011
  32. Jonathan NiederNov 6, 2011
  33. Sverre RabbelierNov 6, 2011
  34. Johannes SchindelinJan 29, 2019
  35. 3/3 fast-export: output reset command for commandline revsSverre Rabbelier, Nov 5, 2011
  36. Jonathan NiederNov 6, 2011
  37. Sverre RabbelierNov 6, 2011
  38. Jonathan NiederNov 7, 2011
  39. Junio C HamanoNov 7, 2011
  40. Junio C HamanoNov 7, 2011
  41. Thomas RastNov 30, 2011
  42. Felipe ContrerasOct 24, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.