Re: [PATCH 1/2] help: add help_unknown_ref
Vikrant Varma <vikrant.varma94@gmail.com> writes:
Show 12 quoted lines
> Give better advice when trying to merge a branch that doesn't exist. If
> the branch exists in any remotes, display a list of suggestions.
>
> Example:
> $ git merge foo
> fatal: foo - not something we can merge
>
> Did you mean this?
> bar/foo
>
> Signed-off-by: Vikrant Varma <vikrant.varma94@gmail.com>
> ---
Nicely explained.
If you step back a bit, you would notice two things.
(1) Saying 'foo' when the user means 'origin/foo' is hardly the
only (or even the most common) kind of mistake that the code
you need to add to 'git merge' would encounter and could help
the user with. "git merge origin/mastre" and "orign/master"
may benefit from a typofix as well, and the mechanism to come
up with the suggestion is likely to hook to the same codepath
you are modifying with this patch, even though the logic to
come up with the suggested alternatives may be different. (2) "merge" is not the single command that user may make this kind
of mistake the command could help and use the same helper.
"git branch myfoo foo" may want to suggest "origin/foo", for
example. I just typed "git checkout mater", which could have
been easily corrected to "git checkout master" with a mechanism
like this.Show 27 quoted lines
> help.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> help.h | 6 ++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/help.c b/help.c
> index 02ba043..faf18b9 100644
> --- a/help.c
> +++ b/help.c
> @@ -7,6 +7,7 @@
> #include "string-list.h"
> #include "column.h"
> #include "version.h"
> +#include "refs.h"
>
> void add_cmdname(struct cmdnames *cmds, const char *name, int len)
> {
> @@ -404,3 +405,46 @@ int cmd_version(int argc, const char **argv, const char *prefix)
> printf("git version %s\n", git_version_string);
> return 0;
> }
> +
> +struct similar_ref_cb {
> + const char *base_ref;
> + struct string_list similar_refs;
> +};
> +
> +static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)An asterisk sticks to the parameter name, not type, like this:
..._ref(const char *refname, ...
There are other places with the same style problem in this patch.
Show 5 quoted lines
> +{
> + int i;
> + struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
> + for (i = strlen(refname); refname[i] != '/'; i--)
> + ;Indent with two HT, not HT followed by a run of SPs.
> + /* A remote branch of the same name is deemed similar */
> + if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref))
An overlong line can and should be split, perhaps like this:
if (!prefixcmp(... very long parameter list ...) &&
!strcmp(... another very long parameter list ...))> + string_list_append(&(cb->similar_refs), refname + 13);
To suggest "orign/foo" => "origin/foo", "foz" => "origin/foo", and "mastre" => "master", using levenshtein.c would help here.
You would special case the distance between "foo" and "origin/foo" as "very low", e.g. 0, and compute levenshtein distance with refname and cb->base_ref, store the result in the .util field of the string list, and sort it at the end after you finish iterating using the computed distance to come up with the list of suggestions.
Show 12 quoted lines
> + return 0;
> +}
> +
> +void help_unknown_ref(const char* ref) {
> + int i;
> + struct similar_ref_cb ref_cb;
> + ref_cb.similar_refs = (struct string_list)STRING_LIST_INIT_NODUP;
> + ref_cb.base_ref = ref;
> +
> + for_each_ref(append_similar_ref, &ref_cb);
> +
> + fprintf_ln(stderr, _("fatal: %s - not something we can merge"), ref);When you consider the point (2) above, it becomes clear why this message does not belong to a helper function with a bland and generic name "help unknown ref".
This API is misdesigned. A possible alternative that may be better reusable would be to have a helper that is used to come up with a list of suggestions and make the caller responsible for emitting the error message.
Show 15 quoted lines
> + if (ref_cb.similar_refs.nr > 0) {
> + fprintf_ln(stderr,
> + Q_("\nDid you mean this?",
> + "\nDid you mean one of these?",
> + ref_cb.similar_refs.nr));
> +
> + for (i = 0; i < ref_cb.similar_refs.nr; i++)
> + fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string);
> + }
> + exit(1);
> +}
> +
> +
> +
> +Do not add trailing blank lines.
Show 15 quoted lines
> diff --git a/help.h b/help.h
> index 0ae5a12..613cb45 100644
> --- a/help.h
> +++ b/help.h
> @@ -27,4 +27,10 @@ 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);
>
> +/*
> + * ref is not something that can be merged. Print a list of remote branches of the
> + * same name that the user might have meant.
> + */
> +extern void help_unknown_ref(const char* ref);
> +
> #endif /* HELP_H */
Vikrant Varma
