[BUG?] git revert option parsing too lenient?

[BUG?] git revert option parsing too lenient?

[Sverre Rabbelier]

Heya,

I just did 'git revert -p <commit>', hoping I might be able to
partially revert a commit, but it just ignored the -p (it's not
supported) rather than erroring out. Is this expected behavior?

-- 
Cheers,

Sverre Rabbelier

Re: [BUG?] git revert option parsing too lenient?

[Jeff King]

On Wed, May 11, 2011 at 12:40:27PM +0200, Sverre Rabbelier wrote:

> I just did 'git revert -p <commit>', hoping I might be able to
> partially revert a commit, but it just ignored the -p (it's not
> supported) rather than erroring out. Is this expected behavior?

Yeah, this is a problem with many commands that take revision
parameters. The revision option parser will parse all of the log and
diff options, modifying the rev_info struct, and then the resulting
command may or may not do anything useful with the values. You can even
do:

  git revert --pretty=oneline -p --date=iso --renames $sha1

all of which is equivalent to just:

  git revert $sha1

So yeah, it's a bug, but it is a known one. The trouble is that fixing
it means splitting the revision options into a set of "these are OK for
picking a revision or range of revisions" and "other options", and
letting commands like revert use one set but not the other. And I'm not
even sure if that distinction is sufficient for all of the commands that
use setup_revisions.

-Peff

Re: [BUG?] git revert option parsing too lenient?

[Sverre Rabbelier]

Heya,

On Wed, May 11, 2011 at 22:36, Jeff King <peff@peff.net> wrote:
> So yeah, it's a bug, but it is a known one. The trouble is that fixing
> it means splitting the revision options into a set of "these are OK for
> picking a revision or range of revisions" and "other options", and
> letting commands like revert use one set but not the other.

Doing the splitting doesn't sound that hard, I assume the real problem
is getting the option parser to look through both tables?

-- 
Cheers,

Sverre Rabbelier

Re: [BUG?] git revert option parsing too lenient?

[Jeff King]

On Wed, May 11, 2011 at 10:44:48PM +0200, Sverre Rabbelier wrote:

> On Wed, May 11, 2011 at 22:36, Jeff King <peff@peff.net> wrote:
> > So yeah, it's a bug, but it is a known one. The trouble is that fixing
> > it means splitting the revision options into a set of "these are OK for
> > picking a revision or range of revisions" and "other options", and
> > letting commands like revert use one set but not the other.
> 
> Doing the splitting doesn't sound that hard, I assume the real problem
> is getting the option parser to look through both tables?

No, it wouldn't be that hard. You'd split handle_revision_opt into two
functions, and then probably callers of setup_revisions would pass in a
flag to say whether which subset they were interested in.

Maybe the splitting isn't hard. But I suspect it is one of those "the
devil is in the details" tasks where you will find that the options do
not fit as plainly into categories as you want. But I might be wrong.
And I don't want to discourage you from trying to work on it. :)

-Peff

Re: [BUG?] git revert option parsing too lenient?

[Sverre Rabbelier]

Heya,

On Wed, May 11, 2011 at 22:48, Jeff King <peff@peff.net> wrote:
> And I don't want to discourage you from trying to work on it. :)

Who knows, maybe I'll have some time to work on git(-remote-hg) sometime :).

-- 
Cheers,

Sverre Rabbelier