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

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

From
Jeff King <peff@peff.net>
Date
Dec 1, 2018, 19:44 UTC
Message-ID
<20181201194424.GB28918@sigill.intra.peff.net>
In-Reply-To
<19c82fb0-e0d6-0b15-06ab-cfba4d699d94@comcast.net>
On Fri, Nov 30, 2018 at 05:32:47PM -0800, Matthew DeVore wrote:
Show 20 quoted lines
> > Speaking of which, would this flag work better as a field in
> > setup_revision_opt, which is passed to setup_revisions()? The intent
> > seem to be to influence how we parse command-line arguments, and that's
> > where other similar flags are (e.g., assume_dashdash).
> 
> Good idea. This would solve the problem of mistakenly believing the flag
> matters when it doesn't, since it is in the struct which is used as the
> arguments of the exact function that cares about it. Here's a new patch -
> I'm tweaking e-mail client settings so hopefully this makes it to the list
> without mangling - if not I'll resend it with `git-send-email` later.
> 
> From 941c89fe1e226ed4d210ce35d0d906526b8277ed Mon Sep 17 00:00:00 2001
> From: Matthew DeVore <matvore@google.com>
> Date: Fri, 30 Nov 2018 16:43:32 -0800
> Subject: [PATCH] revisions.c: put promisor option in specialized struct
> 
> Put the allow_exclude_promisor_objects flag in setup_revision_opt. When
> it was in rev_info, it was unclear when it was used, since rev_info is
> passed to functions that don't use the flag. This resulted in
> unnecessary setting of the flag in prune.c, so fix that as well.
Thanks, this looks pretty reasonable overall. Two comments:
Show 8 quoted lines
>  	repo_init_revisions(the_repository, &revs, NULL);
>  	save_commit_buffer = 0;
> -	revs.allow_exclude_promisor_objects_opt = 1;
> -	setup_revisions(ac, av, &revs, NULL);
> +
> +	memset(&s_r_opt, 0, sizeof(s_r_opt));
> +	s_r_opt.allow_exclude_promisor_objects = 1;
> +	setup_revisions(ac, av, &revs, &s_r_opt);

I wonder if a static initializer for setup_revision_opt is worth it. It would remove the need for this memset. Probably not a big deal either way, though.

Show 5 quoted lines
>  static int handle_revision_opt(struct rev_info *revs, int argc, const char
> **argv,
> -			       int *unkc, const char **unkv)
> +			       int *unkc, const char **unkv,
> +			       int allow_exclude_promisor_objects)

Why not pass in the whole setup_revision_opt struct? We don't need anything else from it yet, but it seems like the point of that struct is to pass around preferences like this.

-Peff
Previous: Matthew DeVoreNext: Matthew DeVore
Message 9 of 19 in “[RFC 0/2] explicitly support or not support --exclude-promisor-objects”
  1. Matthew DeVoreOct 23, 2018
  2. 1/2 Documentation/git-log.txt: do not show --exclude-promisor-objectsMatthew DeVore, Oct 23, 2018
  3. 2/2 exclude-promisor-objects: declare when option is allowedMatthew DeVore, Oct 23, 2018
  4. Junio C HamanoOct 23, 2018
  5. Matthew DeVoreOct 23, 2018
  6. Junio C HamanoOct 24, 2018
  7. Jeff KingNov 21, 2018
  8. Matthew DeVoreDec 1, 2018
  9. Jeff KingDec 1, 2018
  10. Matthew DeVoreDec 3, 2018
  11. Jeff KingDec 3, 2018
  12. Matthew DeVoreDec 3, 2018
  13. Junio C HamanoDec 4, 2018
  14. revisions.c: put promisor option in specialized structMatthew DeVore, Dec 3, 2018
  15. Jeff KingDec 3, 2018
  16. Matthew DeVoreDec 3, 2018
  17. Matthew DeVoreOct 23, 2018
  18. Junio C HamanoOct 23, 2018
  19. Matthew DeVoreOct 23, 2018

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.