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

Re: [PATCH 5/5] repo-settings: create feature.experimental setting

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jul 23, 2019, 15:20 UTC
Message-ID
<nycvar.QRO.7.76.6.1907231708280.21907@tvgsbejvaqbjf.bet>
In-Reply-To
<da7685936f44a32121ec0568df2a9f23e4003506.1563818059.git.gitgitgadget@gmail.com>
Hi Stolee,
On Mon, 22 Jul 2019, Derrick Stolee via GitGitGadget wrote:
Show 20 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The 'feature.experimental' setting includes config options that are
> not committed to become defaults, but could use additional testing.
>
> Update the following config settings to take new defaults, and to
> use the repo_settings struct if not already using them:
>
> * 'pack.useSparse=true'
>
> * 'merge.directoryRenames=true'
>
> * 'fetch.negotiationAlgorithm=skipping'
>
> In the case of fetch.negotiationAlgorithm, the existing logic
> would load the config option only when about to use the setting,
> so had a die() statement on an unknown string value. This is
> removed as now the config is parsed under prepare_repo_settings().
> In general, this die() is probably misplaced and not valuable.
> A test was removed that checked this die() statement executed.
Good.
Show 22 quoted lines
> [...]
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 12300131fc..162d5a4753 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -28,6 +28,7 @@
>  #include "submodule.h"
>  #include "revision.h"
>  #include "commit-reach.h"
> +#include "repo-settings.h"
>
>  struct path_hashmap_entry {
>  	struct hashmap_entry e;
> @@ -1375,10 +1376,14 @@ static int handle_rename_via_dir(struct merge_options *opt,
>  	 * there is no content merge to do; just move the file into the
>  	 * desired final location.
>  	 */
> +	struct repository *r = the_repository;
>  	const struct rename *ren = ci->ren1;
>  	const struct diff_filespec *dest = ren->pair->two;
>  	char *file_path = dest->path;
> -	int mark_conflicted = (opt->detect_directory_renames == 1);

I actually don't think that we want to do that; the `opt` parameter is passed to the merge recursive algorithm specifically so that it can be overridden by the caller.

Instead, what we should do, I think, is to change `init_merge_options()` (which already gets a parameter of type `struct repository *`) so that it does not hard-code the `detect_directory_renames` value to `1` but to query the repo settings instead.

Show 89 quoted lines
> +	int mark_conflicted;
> +
> +	prepare_repo_settings(r);
> +	mark_conflicted = (r->settings->merge_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);
>  	assert(ren->dir_rename_original_dest);
>
>  	if (!opt->call_depth && would_lose_untracked(opt, dest->path)) {
> @@ -2850,6 +2855,7 @@ static int detect_and_process_renames(struct merge_options *opt,
>  				      struct string_list *entries,
>  				      struct rename_info *ri)
>  {
> +	struct repository *r = the_repository;
>  	struct diff_queue_struct *head_pairs, *merge_pairs;
>  	struct hashmap *dir_re_head, *dir_re_merge;
>  	int clean = 1;
> @@ -2863,7 +2869,8 @@ static int detect_and_process_renames(struct merge_options *opt,
>  	head_pairs = get_diffpairs(opt, common, head);
>  	merge_pairs = get_diffpairs(opt, common, merge);
>
> -	if (opt->detect_directory_renames) {
> +	prepare_repo_settings(r);
> +	if (r->settings->merge_directory_renames) {
>  		dir_re_head = get_directory_renames(head_pairs);
>  		dir_re_merge = get_directory_renames(merge_pairs);
>
> @@ -3112,6 +3119,7 @@ static int handle_rename_normal(struct merge_options *opt,
>  				const struct diff_filespec *b,
>  				struct rename_conflict_info *ci)
>  {
> +	struct repository *r = the_repository;
>  	struct rename *ren = ci->ren1;
>  	struct merge_file_info mfi;
>  	int clean;
> @@ -3121,7 +3129,9 @@ static int handle_rename_normal(struct merge_options *opt,
>  	clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
>  				     o, a, b, ci);
>
> -	if (clean && opt->detect_directory_renames == 1 &&
> +	prepare_repo_settings(r);
> +	if (clean &&
> +	    r->settings->merge_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
>  	    ren->dir_rename_original_dest) {
>  		if (update_stages(opt, path,
>  				  NULL,
> @@ -3155,6 +3165,7 @@ static void dir_rename_warning(const char *msg,
>  static int warn_about_dir_renamed_entries(struct merge_options *opt,
>  					  struct rename *ren)
>  {
> +	struct repository *r = the_repository;
>  	const char *msg;
>  	int clean = 1, is_add;
>
> @@ -3166,12 +3177,13 @@ static int warn_about_dir_renamed_entries(struct merge_options *opt,
>  		return clean;
>
>  	/* Sanity checks */
> -	assert(opt->detect_directory_renames > 0);
> +	prepare_repo_settings(r);
> +	assert(r->settings->merge_directory_renames > 0);
>  	assert(ren->dir_rename_original_type == 'A' ||
>  	       ren->dir_rename_original_type == 'R');
>
>  	/* Check whether to treat directory renames as a conflict */
> -	clean = (opt->detect_directory_renames == 2);
> +	clean = (r->settings->merge_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE);
>
>  	is_add = (ren->dir_rename_original_type == 'A');
>  	if (ren->dir_rename_original_type == 'A' && clean) {
> @@ -3662,15 +3674,6 @@ static void merge_recursive_config(struct merge_options *opt)
>  		opt->merge_detect_rename = git_config_rename("merge.renames", value);
>  		free(value);
>  	}
> -	if (!git_config_get_string("merge.directoryrenames", &value)) {
> -		int boolval = git_parse_maybe_bool(value);
> -		if (0 <= boolval) {
> -			opt->detect_directory_renames = boolval ? 2 : 0;
> -		} else if (!strcasecmp(value, "conflict")) {
> -			opt->detect_directory_renames = 1;
> -		} /* avoid erroring on values from future versions of git */
> -		free(value);
> -	}
>  	git_config(git_xmerge_config, NULL);
>  }
>
> @@ -3687,7 +3690,6 @@ void init_merge_options(struct merge_options *opt,
>  	opt->renormalize = 0;
>  	opt->diff_detect_rename = -1;
>  	opt->merge_detect_rename = -1;
> -	opt->detect_directory_renames = 1;
In other words: here.
Show 66 quoted lines
>  	merge_recursive_config(opt);
>  	merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
>  	if (merge_verbosity)
> diff --git a/merge-recursive.h b/merge-recursive.h
> index c2b7bb65c6..b8eba244ee 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -22,7 +22,6 @@ struct merge_options {
>  	unsigned renormalize : 1;
>  	long xdl_opts;
>  	int verbosity;
> -	int detect_directory_renames;
>  	int diff_detect_rename;
>  	int merge_detect_rename;
>  	int diff_rename_limit;
> diff --git a/repo-settings.c b/repo-settings.c
> index 9e4b8e6268..5e9249c437 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -9,6 +9,12 @@ static int git_repo_config(const char *key, const char *value, void *cb)
>  {
>  	struct repo_settings *rs = (struct repo_settings *)cb;
>
> +	if (!strcmp(key, "feature.experimental")) {
> +		UPDATE_DEFAULT(rs->pack_use_sparse, 1);
> +		UPDATE_DEFAULT(rs->merge_directory_renames, MERGE_DIRECTORY_RENAMES_TRUE);
> +		UPDATE_DEFAULT(rs->fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);
> +		return 0;
> +	}
>  	if (!strcmp(key, "feature.manycommits")) {
>  		UPDATE_DEFAULT(rs->core_commit_graph, 1);
>  		UPDATE_DEFAULT(rs->gc_write_commit_graph, 1);
> @@ -50,6 +56,23 @@ static int git_repo_config(const char *key, const char *value, void *cb)
>  				"using 'keep' default value"), value);
>  		return 0;
>  	}
> +	if (!strcmp(key, "merge.directoryrenames")) {
> +		int bool_value = git_parse_maybe_bool(value);
> +		if (0 <= bool_value) {
> +			rs->merge_directory_renames = bool_value ? MERGE_DIRECTORY_RENAMES_TRUE : 0;
> +		} else if (!strcasecmp(value, "conflict")) {
> +			rs->merge_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT;
> +		}
> +		return 0;
> +	}
> +	if (!strcmp(key, "fetch.negotiationalgorithm"))	{
> +		if (!strcasecmp(value, "skipping")) {
> +			rs->fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
> +		} else {
> +			rs->fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
> +		}
> +		return 0;
> +	}
>
>  	return 1;
>  }
> @@ -64,10 +87,14 @@ void prepare_repo_settings(struct repository *r)
>  	/* Defaults */
>  	r->settings->core_commit_graph = -1;
>  	r->settings->gc_write_commit_graph = -1;
> -	r->settings->pack_use_sparse = -1;
> +
>  	r->settings->index_version = -1;
>  	r->settings->core_untracked_cache = -1;
>
> +	r->settings->pack_use_sparse = -1;

This reordering at this stage in the patch series is a bit confusing. Maybe add it at the location where you want it to end up to begin with?

Or use `memset(..., -1, ...)`.
Show 28 quoted lines
> +	r->settings->merge_directory_renames = -1;
> +	r->settings->fetch_negotiation_algorithm = -1;
> +
>  	repo_config(r, git_repo_config, r->settings);
>
>  	/* Hack for test programs like test-dump-untracked-cache */
> @@ -75,4 +102,7 @@ void prepare_repo_settings(struct repository *r)
>  		r->settings->core_untracked_cache = CORE_UNTRACKED_CACHE_KEEP;
>  	else
>  		UPDATE_DEFAULT(r->settings->core_untracked_cache, CORE_UNTRACKED_CACHE_KEEP);
> +
> +	UPDATE_DEFAULT(r->settings->merge_directory_renames, MERGE_DIRECTORY_RENAMES_CONFLICT);
> +	UPDATE_DEFAULT(r->settings->fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
>  }
> diff --git a/repo-settings.h b/repo-settings.h
> index bac9b87d49..cecf7d0028 100644
> --- a/repo-settings.h
> +++ b/repo-settings.h
> @@ -4,12 +4,22 @@
>  #define CORE_UNTRACKED_CACHE_WRITE (1 << 0)
>  #define CORE_UNTRACKED_CACHE_KEEP (1 << 1)
>
> +#define MERGE_DIRECTORY_RENAMES_CONFLICT 1
> +#define MERGE_DIRECTORY_RENAMES_TRUE 2
> +
> +#define FETCH_NEGOTIATION_DEFAULT 1
> +#define FETCH_NEGOTIATION_SKIPPING 2
> +
Again, I'd prefer enums.

The rest looks pretty fine to me (I have to admit that I only glanced over the tests, though).

Thanks! Dscho

Show 51 quoted lines
>  struct repo_settings {
>  	int core_commit_graph;
>  	int gc_write_commit_graph;
> -	int pack_use_sparse;
> +
>  	int index_version;
>  	int core_untracked_cache;
> +
> +	int pack_use_sparse;
> +	int merge_directory_renames;
> +	int fetch_negotiation_algorithm;
>  };
>
>  struct repository;
> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
> index 8a14be51a1..f70cbcc9ca 100755
> --- a/t/t5552-skipping-fetch-negotiator.sh
> +++ b/t/t5552-skipping-fetch-negotiator.sh
> @@ -60,29 +60,6 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
>  	have_not_sent c6 c4 c3
>  '
>
> -test_expect_success 'unknown fetch.negotiationAlgorithm values error out' '
> -	rm -rf server client trace &&
> -	git init server &&
> -	test_commit -C server to_fetch &&
> -
> -	git init client &&
> -	test_commit -C client on_client &&
> -	git -C client checkout on_client &&
> -
> -	test_config -C client fetch.negotiationAlgorithm invalid &&
> -	test_must_fail git -C client fetch "$(pwd)/server" 2>err &&
> -	test_i18ngrep "unknown fetch negotiation algorithm" err &&
> -
> -	# Explicit "default" value
> -	test_config -C client fetch.negotiationAlgorithm default &&
> -	git -C client -c fetch.negotiationAlgorithm=default fetch "$(pwd)/server" &&
> -
> -	# Implementation detail: If there is nothing to fetch, we will not error out
> -	test_config -C client fetch.negotiationAlgorithm invalid &&
> -	git -C client fetch "$(pwd)/server" 2>err &&
> -	test_i18ngrep ! "unknown fetch negotiation algorithm" err
> -'
> -
>  test_expect_success 'when two skips collide, favor the larger one' '
>  	rm -rf server client trace &&
>  	git init server &&
> --
> gitgitgadget
>
Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee
Message 12 of 61 in “Create 'feature.*' config area and some centralized config parsing”
  1. 0/5 Create 'feature.*' config area and some centralized config parsingDerrick Stolee via GitGitGadget, Jul 22, 2019
  2. 3/5 repo-settings: parse core.untrackedCacheDerrick Stolee via GitGitGadget, Jul 22, 2019
  3. Johannes SchindelinJul 23, 2019
  4. Derrick StoleeJul 24, 2019
  5. 2/5 repo-settings: add feature.manyCommits settingDerrick Stolee via GitGitGadget, Jul 22, 2019
  6. Johannes SchindelinJul 23, 2019
  7. Derrick StoleeJul 24, 2019
  8. 4/5 repo-settings: create feature.manyFiles settingDerrick Stolee via GitGitGadget, Jul 22, 2019
  9. 1/5 repo-settings: consolidate some config settingsDerrick Stolee via GitGitGadget, Jul 22, 2019
  10. Johannes SchindelinJul 23, 2019
  11. 5/5 repo-settings: create feature.experimental settingDerrick Stolee via GitGitGadget, Jul 22, 2019
  12. Johannes SchindelinJul 23, 2019
  13. Derrick StoleeJul 25, 2019
  14. 0/5 Create 'feature.*' config area and some centralized config parsingDerrick Stolee via GitGitGadget, Jul 25, 2019
  15. 1/5 repo-settings: consolidate some config settingsDerrick Stolee via GitGitGadget, Jul 25, 2019
  16. Johannes SchindelinJul 25, 2019
  17. 2/5 repo-settings: add feature.manyCommits settingDerrick Stolee via GitGitGadget, Jul 25, 2019
  18. 3/5 repo-settings: parse core.untrackedCacheDerrick Stolee via GitGitGadget, Jul 25, 2019
  19. Johannes SchindelinJul 25, 2019
  20. 4/5 repo-settings: create feature.manyFiles settingDerrick Stolee via GitGitGadget, Jul 25, 2019
  21. 5/5 repo-settings: create feature.experimental settingDerrick Stolee via GitGitGadget, Jul 25, 2019
  22. Johannes SchindelinJul 25, 2019
  23. 0/5 Create 'feature.*' config area and some centralized config parsingDerrick Stolee via GitGitGadget, Jul 30, 2019
  24. 2/5 repo-settings: add feature.manyCommits settingDerrick Stolee via GitGitGadget, Jul 30, 2019
  25. Junio C HamanoJul 30, 2019
  26. Johannes SchindelinJul 31, 2019
  27. Junio C HamanoJul 31, 2019
  28. Ævar Arnfjörð BjarmasonJul 31, 2019
  29. Derrick StoleeAug 1, 2019
  30. 1/5 repo-settings: consolidate some config settingsDerrick Stolee via GitGitGadget, Jul 30, 2019
  31. Junio C HamanoJul 30, 2019
  32. 4/5 repo-settings: create feature.manyFiles settingDerrick Stolee via GitGitGadget, Jul 30, 2019
  33. 5/5 repo-settings: create feature.experimental settingDerrick Stolee via GitGitGadget, Jul 30, 2019
  34. Elijah NewrenAug 8, 2019
  35. Derrick StoleeAug 8, 2019
  36. Junio C HamanoAug 8, 2019
  37. Derrick StoleeAug 8, 2019
  38. Elijah NewrenAug 8, 2019
  39. Derrick StoleeAug 8, 2019
  40. Elijah NewrenAug 8, 2019
  41. Junio C HamanoAug 8, 2019
  42. Derrick StoleeAug 8, 2019
  43. 3/5 repo-settings: parse core.untrackedCacheDerrick Stolee via GitGitGadget, Jul 30, 2019
  44. 0/6 Create 'feature.*' config area and some centralized config parsingDerrick Stolee via GitGitGadget, Aug 13, 2019
  45. 2/6 t6501: use 'git gc' in quiet modeDerrick Stolee via GitGitGadget, Aug 13, 2019
  46. 1/6 repo-settings: consolidate some config settingsDerrick Stolee via GitGitGadget, Aug 13, 2019
  47. 4/6 repo-settings: parse core.untrackedCacheDerrick Stolee via GitGitGadget, Aug 13, 2019
  48. 5/6 repo-settings: create feature.manyFiles settingDerrick Stolee via GitGitGadget, Aug 13, 2019
  49. 3/6 commit-graph: turn on commit-graph by defaultDerrick Stolee via GitGitGadget, Aug 13, 2019
  50. Junio C HamanoSep 20, 2021
  51. brian m. carlsonSep 20, 2021
  52. Phillip WoodSep 20, 2021
  53. Ævar Arnfjörð BjarmasonSep 20, 2021
  54. Jeff KingSep 20, 2021
  55. Phillip WoodSep 20, 2021
  56. Ævar Arnfjörð BjarmasonSep 20, 2021
  57. 6/6 repo-settings: create feature.experimental settingDerrick Stolee via GitGitGadget, Aug 13, 2019
  58. Junio C HamanoAug 13, 2019
  59. Junio C HamanoAug 13, 2019
  60. Derrick StoleeAug 14, 2019
  61. Derrick StoleeAug 14, 2019

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.