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

From: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
Date: Mar 24, 2015, 00:09 UTC
Message-ID: <87r3sfz25t.fsf@gmail.com>
In-Reply-To: <xmqqpp87mfqx.fsf@gitster.dls.corp.google.com>

On Tue, Mar 17 2015 at 06:16:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

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

Show 7 quoted lines

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

Show 17 quoted lines

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