Re: [PATCH 5/7] revert: Make the argument parser responsible for setup_revisions

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 14, 2011, 12:52 UTC
Message-ID: <20110814125223.GE18466@elie.gateway.2wire.net>
In-Reply-To: <1313310789-10216-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 6 quoted lines

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