Re: [PATCH] Make git-revert & git-cherry-pick a builtin

From: Junio C Hamano <junkio@cox.net>
Date: Mar 1, 2007, 11:51 UTC
Message-ID: <7vzm6xqj0d.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0703010526080.22628@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 10 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Makefile         |   11 +-
>  builtin-revert.c |  406 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  builtin.h        |    2 +
>  git-revert.sh    |  197 --------------------------
>  git.c            |    2 +
>  5 files changed, 414 insertions(+), 204 deletions(-)
>  create mode 100644 builtin-revert.c
>  delete mode 100755 git-revert.sh

Thanks. I haven't looked the patch closely but I trust this round is a fairly faithful straight translation from the shell script, which is good. I have a few gripes on the behaviour of these commands, and it might be worthwhile to address them in the later rounds.

Show 6 quoted lines

> +static int merge_recursive(const char *base_sha1,
> +		const char *head_sha1, const char *head_name,
> +		const char *next_sha1, const char *next_name)
> +{
> + ...
> +}

Somehow I would have expected you to call merge-recursive not spawn, but this is saner ;-).

> +static int revert_or_cherry_pick(int argc, const char **argv)
> +{
> + ...
> +	msg_fd = hold_lock_file_for_update(&msg_file, ".msg", 1);

Although the detailed reason escapes me, we earlier moved the temporary files to $GIT_DIR/. This .msg in the current directory somehow survived. It probably is a good idea to move this to $GIT_DIR/ in later rounds.

Show 14 quoted lines

> +		if (action == CHERRY_PICK) {
> +			fprintf(stderr, "You may choose to use the following "
> +				"when making the commit:\n"
> +				"GIT_AUTHOR_NAME=\"%s\"\n",
> +				getenv("GIT_AUTHOR_NAME"));
> +			fprintf(stderr, "GIT_AUTHOR_EMAIL=\"%s\"\n",
> +				getenv("GIT_AUTHOR_EMAIL"));
> +			fprintf(stderr, "GIT_AUTHOR_DATE=\"%s\"\n"
> +				"export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL "
> +				"GIT_AUTHOR_DATE\n",
> +				getenv("GIT_AUTHOR_DATE"));
> +		}
> +		exit(1);
> +	}

I am to blame for this crap, but I think this part of code was done before we invented "git commit -C <commit>". It would be a lot better to suggest using that command.