Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

On Tue, Mar 17 2015 at 06:16:38 PM, Junio C Hamano [off-list ref] wrote:

I also notice that handle_revision_arg() would die() by calling it
directly or indirectly via verify_non_filename(), etc., but the
caller actually is expecting it to silently return non-zero when it
finds an argument that cannot be interpreted as a revision or as a
revision range.  

If we feed the function a string that has ".." in it, with
cant_be_filename unset, and if that string _can_ be parsed as a
valid range (e.g. "master..next"), we would check if a file whose
name is that string and die, e.g.

    $ >master..next ; git log master..next
    fatal: ambigous argument 'master..next': both revision and filename

If we swap the order to do the "revision" first before "option",
however, we would end up getting the same for a name that begins
with "-" and has ".." in it.  I see no guarantee that future
possible option name cannot be misinterpreted as a range to trigger
this check.

If I'm understanding correctly, the problem of checking revisions before
arg is that an option fed to handle_revision_arg() might die() before getting
checked as an option in cases where a file with the same name exists?

But doesn't verify_non_filename() already return silently if arg begins
with "-"? It die() only after making that check.

If an option with ".." in it such as -$opt..ion is really given to
handle_revision_arg() then verify_non_filename should not be a problem.

But "git cmd -$option" for any value of $option does not have to be
disambiguated when there is a file whose name is "-$option".  The
existing die()'s in the handle_revision_arg() function _will_ break
that promise.  Currently, because we check the options first,
handle_revision_arg() does not cause us any problem, but swapping
the order will have fallouts.

The only other way handle_revision_arg() can die() is if given a ".."
range, either revisions return null when passed their sha1 to
parse_object().

So something like you proposed earlier:

      if(try to see if it is a revision or a revision range) {
              /* if failed ... */
              if (starts with '-') {
                      do the option thing;
                      continue;
              }
              /*
               * args must be pathspecs from here on.
               * We already checked that rev arg cannot be
               * interpreted as a filename at this point
               */
              if(dashdash)
                      verify_filename()
                     
      } else {
              got_rev_arg = 1;
      }

should work. I'm still getting familiar to how it works so I might be missing
something but shouldn't this be fine? At least concerning the possible fallouts
that you've raised.

If we want to really do the swapping (and I think that is the only
sensible way if we wanted to allow "-" and any extended SHA-1 that
begins with "-" as "the previous branch"), I think the "OK, it looks
like a revision (or revision range); as we didn't see dashdash, it
must not be a filename" check has to be moved to the caller, perhaps
like this:

	if (try to see if it is a revision or a revision range) {
        	/* failed */
                ...
	} else {
        	/* it can be read as a revision or a revision range */
                if (!seen_dashdash)
			verify_non_filename(arg);
		got_rev_arg = 1;
	}

If what I'm saying makes sense, then verify_non_filename(arg) would be
already working as intended in handle_revision_arg(), so moving it to
the caller wouldn't be necessary.

The "missing" cases should also silently return failure and have the
caller deal with that.