On Tue, Mar 17 2015 at 06:16:38 PM, Junio C Hamano <gitster@xxxxxxxxx> 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.
--
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