Hi,

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

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

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

quoted text
> > +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.)

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

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

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html