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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Chris

On 23/09/2022 19:55, Stefan Xenos via GitGitGadget wrote:
> 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>
> ---

> +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));

> +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