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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 1, 2007, 21:37 UTC
Message-ID: <Pine.LNX.4.63.0703012230290.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vzm6xqj0d.fsf@assigned-by-dhcp.cox.net>

Hi,

On Thu, 1 Mar 2007, Junio C Hamano wrote:

Show 16 quoted lines

> 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 tried. Of course, it helped tremendously that there were test cases. Nevertheless would I appreciate review from the list...

> I have a few gripes on the behaviour of these commands, and it might be 
> worthwhile to address them in the later rounds.

Yes, that was part of the reason I rewrote these two in C: For example, I would like a little bit better locking (double locking index would be a good start).

Of course, it makes it easier to write cmd_rebase(): I plan to put it into the same file so that the function revert_or_cherry_pick() can easily be reused.

Show 9 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 ;-).

I briefly considered this, until I realized that merge-recursive is no builtin yet!

Speaking of this issue: Would it be conceivable to make the standalones into builtins? (This adds a dependency on libcurl to core Git programs, but I could live with that... It would make a builtin fetch easier, too.)

Show 9 quoted lines

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

Okay. I have no idea how much the tests rely on that locations, let alone other scripts. Therefore I think it is sane to do it in a separate step.

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

Yeah, let's do that, too.

BTW it took me so long because there was a hard-to-catch bug with the encodings (of course!). In the end I realized that I only forgot to call git_config(git_default_config)... D'oh.

Ciao, Dscho