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@xxxxxxxxx>
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html