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

Re: [RFC PATCH 03/13] commit-reach: move commit_contains from ref-filter

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 29, 2018, 22:00 UTC
Message-ID
<xmqq36x5tfqu.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<20180629161223.229661-4-dstolee@microsoft.com>
Derrick Stolee <dstolee@microsoft.com> writes:
> +int commit_contains(struct ref_filter *filter, struct commit *commit,
> +		    struct commit_list *list, struct contains_cache *cache)

This is a symbol that is used to be file-local private. Is it named appropriately in the new context, which is "globally visible throughout the system"? The convention to call into it now must be documented a lot better (e.g. how should list/cache etc are to be prepared?).

Show 43 quoted lines
> +{
> +	if (filter->with_commit_tag_algo)
> +		return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
> +	return is_descendant_of(commit, list);
> +}
> diff --git a/commit-reach.h b/commit-reach.h
> index 35ec9f0ddb..986fb388d5 100644
> --- a/commit-reach.h
> +++ b/commit-reach.h
> @@ -2,42 +2,24 @@
>  #define __COMMIT_REACH_H__
>  
>  #include "commit.h"
> +#include "commit-slab.h"
> +#include "ref-filter.h"
>  
> -struct commit_list *get_merge_bases_many(struct commit *one,
> -					 int n,
> -					 struct commit **twos);
> -struct commit_list *get_merge_bases_many_dirty(struct commit *one,
> -					       int n,
> -					       struct commit **twos);
> -struct commit_list *get_merge_bases(struct commit *one, struct commit *two);
> -struct commit_list *get_octopus_merge_bases(struct commit_list *in);
> -
> -/* To be used only when object flags after this call no longer matter */
> -struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, struct commit **twos);
> -
> -int is_descendant_of(struct commit *commit, struct commit_list *with_commit);
> -int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference);
> -int in_merge_bases(struct commit *commit, struct commit *reference);
> -
> +int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
>  
>  /*
> - * Takes a list of commits and returns a new list where those
> - * have been removed that can be reached from other commits in
> - * the list. It is useful for, e.g., reducing the commits
> - * randomly thrown at the git-merge command and removing
> - * redundant commits that the user shouldn't have given to it.
> - *
> - * This function destroys the STALE bit of the commit objects'
> - * flags.

The above removal of lines is sloppy; they are mostly duplicates in commit.h that should never have been moved here in the first place, no?

Show 9 quoted lines
> + * Unknown has to be "0" here, because that's the default value for
> + * contains_cache slab entries that have not yet been assigned.
>   */
> -struct commit_list *reduce_heads(struct commit_list *heads);
> +enum contains_result {
> +	CONTAINS_UNKNOWN = 0,
> +	CONTAINS_NO,
> +	CONTAINS_YES
> +};

Are these names specific enough, or were they OK in the limited context inside ref-filter but now are overly broad as globally visible names? I suspect it might be the latter.

Previous: Derrick StoleeNext: Derrick Stolee
Message 9 of 29 in “Consolidate reachability logic”
  1. 00/13 Consolidate reachability logicDerrick Stolee, Jun 29, 2018
  2. 01/13 commit-reach: move walk methods from commit.cDerrick Stolee, Jun 29, 2018
  3. Stefan BellerJun 29, 2018
  4. Junio C HamanoJun 29, 2018
  5. 02/13 commit-reach: move ref_newer from remote.cDerrick Stolee, Jun 29, 2018
  6. 03/13 commit-reach: move commit_contains from ref-filterDerrick Stolee, Jun 29, 2018
  7. Stefan BellerJun 29, 2018
  8. Derrick StoleeJun 30, 2018
  9. Junio C HamanoJun 29, 2018
  10. 04/13 upload-pack: make reachable() more genericDerrick Stolee, Jun 29, 2018
  11. Junio C HamanoJun 29, 2018
  12. 06/13 commit-reach: move can_all_from_reach_with_flag()Derrick Stolee, Jun 29, 2018
  13. Stefan BellerJun 29, 2018
  14. Derrick StoleeJun 30, 2018
  15. 10/13 commit-reach: test is_descendant_ofDerrick Stolee, Jun 29, 2018
  16. 09/13 commit-reach: test can_all_from_reachDerrick Stolee, Jun 29, 2018
  17. 12/13 commit-reach: use is_descendant_of for ref_newerDerrick Stolee, Jun 29, 2018
  18. 13/13 commit-reach: use can_all_from_reachDerrick Stolee, Jun 29, 2018
  19. Stefan BellerJun 29, 2018
  20. 08/13 test-reach: test reduce_heads()Derrick Stolee, Jun 29, 2018
  21. Stefan BellerJun 29, 2018
  22. 11/13 commit-reach: make can_all_from_reach... linearDerrick Stolee, Jun 29, 2018
  23. Stefan BellerJun 29, 2018
  24. 05/13 upload-pack: refactor ok_to_give_up()Derrick Stolee, Jun 29, 2018
  25. Stefan BellerJun 29, 2018
  26. 07/13 test-reachDerrick Stolee, Jun 29, 2018
  27. Stefan BellerJun 29, 2018
  28. Derrick StoleeJun 30, 2018
  29. Derrick StoleeJun 29, 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.