From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 14 Aug 2011 12:52:23 GMT
Subject: Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions
Message-ID: <20110814125223.GE18466@elie.gateway.2wire.net>
In-Reply-To: <1313310789-10216-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> Currently, prepare_revs calls setup_revisions with the (argc, argv) in
> the opts structure filled in by parse_args.  As a result, prepare_revs
> has to take up the responsibility of erroring out and printing usage
> information when (argc, argv) is malformed.  Since parse_args is doing
> this for other kinds of parse errors anyway, give it the task of
> calling setup_revisions as well.

I'm having trouble understanding the above.  Are you saying that we
want to concentrate all usage() calls in a single function for some
reason and prepare_revs() goes against this?  Why would I (the reader)
care about that?

Presumably a simpler explanation is that it makes my life easier in
two ways:

 1. If I want to understand what commandline arguments are accepted
    by cherry-pick/revert, after this patch there is just one function
    to look at.

 2. If I want to ask the replay machinery to do something different,
    as a caller I can set my options in a "struct rev_info" instead
    of forging commandline arguments with the same effect.

> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Since I wrote the patch modulo the commit message and small
improvements, shouldn't I be blamed as the author?  (git log
--grep="[Oo]riginal patch" and git log --grep=jc: give some examples.)
You can have my sign-off if you'd like.

[...]
> +	if (opts->subcommand == REPLAY_NONE) {
> +		opts->revs = xmalloc(sizeof(*opts->revs));

My fault: this never gets freed.  As long as this is private to the
cherry-pick/revert builtin, it's a one-time tiny leak cleaned up by
_exit --- no harm done, but probably worth a comment.