Re: [PATCH 1/2] help: add help_unknown_ref
From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:06
Vikrant Varma wrote:
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.
Interesting. Thanks for working on this. You say advice, but you're not invoking advise() or guarding the advice with an advice.* -- the advice is undoubtedly helpful, but not everyone wants to see it.
quoted hunk ↗ jump to hunk
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 {
I see that there are other structs in our codebase suffixing _cb, to indicate "callback data". I normally reserve _cb for callback functions.
+ const char *base_ref; + struct string_list similar_refs;
Okay, so you might have more than one matching candidate.
+static int append_similar_ref(const char* refname, const unsigned char *sha1, int flags, void *cb_data)
+{
+ int i;
+ struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);
+ for (i = strlen(refname); refname[i] != '/'; i--)
+ ;Er, what is this? A re-implementation of strrchr()?
+ /* A remote branch of the same name is deemed similar */ + if (!prefixcmp(refname, "refs/remotes/") && !strcmp(refname + i + 1, cb->base_ref)) + string_list_append(&(cb->similar_refs), refname + 13);
What is 13? Please use strlen("refs/remotes/") for readability.
I don't like the + i + 1 thing, but you should be able to get rid of
it with strrchr().
+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;Why are you casting 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);Hm, "fatal: " was emitted by die() earlier. I wonder if something like error() will be nicer than hard-coding "fatal: ".
+ 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));Hm, why did you use Q_?
+ for (i = 0; i < ref_cb.similar_refs.nr; i++) + fprintf(stderr, "\t%s\n", ref_cb.similar_refs.items[i].string); + } + exit(1);
die() exits with 128, no? Why are you exiting with 1 now?