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

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2013, 22:49 UTC
Message-ID: <7vzjw5axzk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1367625860-20746-2-git-send-email-vikrant.varma94@gmail.com>

Vikrant Varma <vikrant.varma94@gmail.com> writes:

> When a ref is not known, currently functions call die() with an
> error message.

The first part read somewhat awkward, so I started rewriting the above like so:

    When the user gives an unknown string to a command that expects
    to see a ref, we could be more helpful than just saying "that's
    not a ref" and die.

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

> Add helper function help_unknown_ref to take care of displaying an
> error message along with a list of suggested refs the user might
> have meant.

OK.

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

Show 6 quoted lines

>
> 	Did you mean one of these?
> 	    origin/foo
> 	    upstream/foo
>
> Signed-off-by: Vikrant Varma <vikrant.varma94@gmail.com>

Show 31 quoted lines

> ...
> +struct string_list guess_refs(const char *ref)
> +{
> +	struct similar_ref_cb ref_cb;
> +	struct string_list similar_refs = STRING_LIST_INIT_NODUP;
> +
> +	ref_cb.base_ref = ref;
> +	ref_cb.similar_refs = &similar_refs;
> +	for_each_ref(append_similar_ref, &ref_cb);
> +	return similar_refs;
> +}
> +
> +void help_unknown_ref(const char *ref, const char *cmd, const char *error)
> +{
> +	int i;
> +	struct string_list suggested_refs = guess_refs(ref);
> +
> +	fprintf_ln(stderr, _("%s: %s - %s"), cmd, ref, error);
> +
> +	if (suggested_refs.nr > 0) {
> +		fprintf_ln(stderr,
> +			   Q_("\nDid you mean this?",
> +			      "\nDid you mean one of these?",
> +			      suggested_refs.nr));
> +		for (i = 0; i < suggested_refs.nr; i++)
> +			fprintf(stderr, "\t%s\n", suggested_refs.items[i].string);
> +	}
> +
> +	string_list_clear(&suggested_refs, 0);
> +	exit(1);
> +}

Looks sensible.

Show 12 quoted lines

> diff --git a/help.h b/help.h
> index 0ae5a12..5003423 100644
> --- a/help.h
> +++ b/help.h
> @@ -27,4 +27,9 @@ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
>  extern int is_in_cmdlist(struct cmdnames *cmds, const char *name);
>  extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
>  
> +/*
> + * This function has been called because ref is not known.
> + * Print a list of refs that the user might have meant, and exit.
> + */

The wording is a bit funny; I'll amend it somehow before queuing.

> +extern void help_unknown_ref(const char *ref, const char *cmd, const char *error);
>  #endif /* HELP_H */