Re: [PATCH 07/10] evolve: implement the git change command

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 25, 2022, 09:10 UTC
Message-ID: <61a01fdf-9805-ff8d-6306-ff49f31e93c2@gmail.com>
In-Reply-To: <914028341842a4d57e02ec42a7426d3aa83640f9.1663959325.git.gitgitgadget@gmail.com>

Hi Chris

On 23/09/2022 19:55, Stefan Xenos via GitGitGadget wrote:

Show 16 quoted lines

> From: Stefan Xenos <sxenos@google.com>
> 
> Implement the git change update command, which
> are sufficient for constructing change graphs.
> 
> For example, to create a new change (a stable name) that refers to HEAD:
> 
> git change update -c HEAD
> 
> To record a rebase or amend in the change graph:
> 
> git change update -c <new_commit> -r <old_commit>
> 
> To record a cherry-pick in the change graph:
> 
> git change update -c <new_commit> -o <original_commit>

While it is good to have this example it would be better to have some documentation about how to use this command. It would be very helpful to have the documentation added before the code so that reviewers have an overview of the command when they come to review the code.

The commit message should also discuss why it is called "change" rather than "evolve". For more details on commit messages for this project see "Describe your changes well" in Documentation/SubmittingPatches. Having some tests would make it clear how this command is intended to be used as well as demonstrating that the implementation works.

> Signed-off-by: Stefan Xenos <sxenos@google.com>
> Signed-off-by: Chris Poucet <poucet@google.com>
> ---

Show 15 quoted lines

> +struct update_state {
> +	int options;
> +	const char* change;
> +	const char* content;
> +	struct string_list replace;
> +	struct string_list origin;
> +};
> +
> +static void init_update_state(struct update_state *state)
> +{
> +	memset(state, 0, sizeof(*state));
> +	state->content = "HEAD";
> +	string_list_init_nodup(&state->replace);
> +	string_list_init_nodup(&state->origin);
> +}

In general we prefer to use initializer macros over functions. So this would become

#define UPDATE_STATE_INIT {			\
	.content = "HEAD",			\
	.replace = STRING_LIST_INIT_NODUP,	\
	.origin = STRING_LIST_INIT_NODUP	\
}

and lower down we'd have

struct update_state state = UPDATE_STATE_INIT;

Likewise we prefer

	struct foo = { 0 };

over

	struct foo foo;
	memset(&foo, 0, sizeof(foo));

Show 15 quoted lines

> +int cmd_change(int argc, const char **argv, const char *prefix)
> +{
> +	/* No options permitted before subcommand currently */
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	int result = 1;
> +
> +	argc = parse_options(argc, argv, prefix, options, builtin_change_usage,
> +		PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (argc < 1)
> +		usage_with_options(builtin_change_usage, options);
> +	else if (!strcmp(argv[0], "update"))
> +		result = change_update(argc, argv, prefix);

Since Stefan wrote this code the parse options api has been improved to support sub commands so this should be updated to use that support.

Thanks again for picking up these patches, I'm excited to see an evolve command for git.

Best Wishes

Phillip