Re: [PATCH] Make "git reset" a builtin. (incomplete)

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

[I thought that it was high time in this thread to review the code]

On Wed, 22 Aug 2007, Carlos Rica wrote:

> The tests I made for it are not finished so they are not included, but 
> it seems to pass the rest of the test suite.

AFAICS there are only "reset --hard"s in the test suite.

> diff --git a/builtin-reset.c b/builtin-reset.c
> [...]
> +
> +static int unmerged_files(void)
> +{
> +	char b;
> +	ssize_t len;
> +	struct child_process cmd;
> +	const char *argv_ls_files[] = {"ls-files", "--unmerged", NULL};
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.argv = argv_ls_files;
> +	cmd.git_cmd = 1;
> +	cmd.out = -1;
> +
> +	if (start_command(&cmd))
> +		die("Could not run sub-command: git ls-files");
> +
> +	len = xread(cmd.out, &b, 1);
> +	if (len < 0)
> +		die("Could not read output from git ls-files: %s",
> +						strerror(errno));
> +	finish_command(&cmd);
> +
> +	return len;
> +}

I think it is a good idea to start out using run_command, and if we ever 
run into performance issues, we can always switch to calling the functions 
directly.

> +static int print_line_current_head(void)
> +{
> +	const char *argv_log[] = {"log", "--max-count=1", "--pretty=oneline",
> +					"--abbrev-commit", "HEAD", NULL};
> +	printf("HEAD is now at ");
> +	unsetenv("GIT_PAGER");
> +	return run_command_v_opt(argv_log, RUN_GIT_CMD);
> +}

This is a candidate to refactoring, using commit.c's get_one_line() 
function.

> +int cmd_reset(int argc, const char **argv, const char *prefix)
> +{
> +	int i = 1, reset_type = MIXED, update_ref_status = 0;
> +	const char *rev = "HEAD";
> +	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
> +				*old_orig = NULL, sha1_old_orig[20];
> +	struct object *obj;
> +	char *reflog_action;
> +
> +	git_config(git_default_config);
> +
> +	reflog_action = args_to_str(argv);
> +	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
> +
> +	if (i < argc) {
> +		if (!strcmp(argv[i], "--mixed")) {
> +			reset_type = MIXED;
> +			i++;
> +		}
> +		else if (!strcmp(argv[i], "--soft")) {
> +			reset_type = SOFT;
> +			i++;
> +		}
> +		else if (!strcmp(argv[i], "--hard")) {
> +			reset_type = HARD;
> +			i++;
> +		}
> +	}
> +
> +	if (i < argc && argv[i][0] != '-')
> +		rev = argv[i++];
> +
> +	if (get_sha1(rev, sha1))
> +		die("Failed to resolve '%s' as a valid ref.", rev);
> +
> +	obj = deref_tag(parse_object(sha1), sha1_to_hex(sha1), 40);

IMHO it would be better to use "..., rev, strlen(rev));" instead.

> +	if (!obj)
> +		die("Could not parse object '%s'.", rev);
> +	memcpy(sha1, obj->sha1, sizeof(sha1));
> +
> +	if (i < argc && argv[i][0] == '-') {
> +		if (strcmp(argv[i], "--"))
> +			usage(builtin_reset_usage);
> +		else
> +			i++;
> +	}

IMHO this would be clearer:

	if (i < argc && !strcmp(argv[i], "--"))
		i++;
	else if (i < argc && argv[i][0] == '-')
		usage(builtin_reset_usage);

but I do not care _that_ deeply.

> +	/* git reset --mixed tree [--] paths... can be used to
> +	 * load chosen paths from the tree into the index without
> +	 * affecting the working tree nor HEAD. */
> +	if (i < argc) {
> +		if (reset_type != MIXED)
> +			die("Cannot do partial %s reset.", argv[1]);

Hmm.  Maybe use a static const array of "hard", "mixed" and "soft"?

> +		/*
> +		git diff-index --cached $rev -- "$@" |
> +		sed -e 's/^:\([0-7][0-7]*\) [0-7][0-7]* \([0-9a-f][0-9a-f]*\) [0-9a-f][0-9a-f]* [A-Z]	\(.*\)$/\1 \2	\3/' |
> +		git update-index --add --remove --index-info || exit
> +		*/
> +		update_index_refresh();

AFAICT this code misses out on added files, i.e. files which are in $rev, 
but not in the index.

> +	/* Any resets update HEAD to the head being switched to,
> +	 * saving the previous head in ORIG_HEAD before. */
> +	if (!get_sha1("ORIG_HEAD", sha1_old_orig))
> +		old_orig = sha1_old_orig;
> +	if (!get_sha1("HEAD", sha1_orig)) {
> +		orig = sha1_orig;
> +		update_ref("updating ORIG_HEAD", "ORIG_HEAD", orig, old_orig);
> +	}
> +	else if (old_orig)
> +		delete_ref("ORIG_HEAD", old_orig);

Why not put the get_sha1() into the else if()?  You spare a variable and a 
few lines there.

Otherwise it looks good to me.  It would be good if you could post your 
test script, though, so that people can get a feel what works and what 
needs work.

Ciao,
Dscho