Re: [PATCH v2 1/2] help: add help_unknown_ref

On Thursday 09 May 2013 04:19 AM, Junio C Hamano wrote:
 > [...] which in turn made me realize that some commands may not even know
 > if the user mistyped a ref.  It is not an objection to this patch
 > per-se, but a useful future enhancement may be to allow the callers
 > call guess_mistyped_ref() directly and let them decide what to do
 > when they suspect the string they did not understand is not a
 > mistyped ref but something else, i.e. not let help_unknown_ref() die
 > unconditionally but allow it to return.  Then the caller can do:
 >
 >          commit = get_commit_from_string(argv[i]);
 >          if (!commit) {
 >              ... I do not understand argv[i], but ...
 >              ... it may be a mistyped ref ...
 >              help_unknown_ref(argv[i], "expected a revision");
 >              ... it is not likely to be a typo ...
 >              ... perhaps it was meant to be a filename? ...
 >              if (file_exists(argv[i])) {
 >                  ... yes! ...
 >                  ... do the "file" thing instead ...
 >              }
 >          }
 >

I'm apprehensive about calling guess_mistyped_ref() (or it's equivalent, which happens to be guess_refs()) directly, because it doesn't seem like a clean enough separation. When the caller thinks it's got a bad refname, it should just hand it over to help_unknown_ref, for further processing.

If autocorrect is enabled, it can get back a single corrected refname (that is what my next patch will include - is it okay to base it on pu?).

If the need ever did arise to get that kind of information from help_unknown_ref, it could always be done using callback data?

	commit = get_commit_from_string(argv[i]);
	if (!commit) {
		... maybe mistyped ref, maybe something else ...
		struct unknown_ref_cb data;
		help_unknown_ref(argv[i], "expected something else",
				 &data);
		if (data.autocorrect)
			commit = get_commit_from_string(
					data.corrected_ref);
		else if (data.is_file)
			... do the file thing instead ...
    	}

I didn't see the need for this right away.

 >> Example:
 >> 	$ git merge foo
 >> 	merge: foo - not something we can merge
 >
 > That leading "merge: " looks somewhat strange, especially when it
 > immediately follows the command line to invoke "merge", making it
 > appear to waste space by stating the obvious.
 >
 > Our messages are generally marked with "error:", "fatal:",
 > "warning:", etc. at the beginning.

I agree, it looks strange. However the alternatives seem to be:

1) hard code 'fatal' into the error message
2) print the corrections before using die()
3) create and store the corrections string beforehand, and then call die()

1 and 3 are not elegant, and 2's output seems harder to read. I haven't been able to figure out a way to do this well.