From: Vikrant Varma Date: Thu, 09 May 2013 21:04:22 GMT Subject: Re: [PATCH v2 1/2] help: add help_unknown_ref Message-ID: <518C0F56.7070600@gmail.com> In-Reply-To: <7vzjw5axzk.fsf@alter.siamese.dyndns.org> 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.