From: Junio C Hamano <junkio@cox.net>
Date: Thu, 01 Mar 2007 11:51:30 GMT
Subject: Re: [PATCH] Make git-revert & git-cherry-pick a builtin
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:

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

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

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