From: Stefan Sperling Subject: Re: [patch] add a one-line-per-commit output mode to log To: Evan Silberman Cc: gameoftrees@openbsd.org Date: Tue, 17 May 2022 10:43:38 +0200 On Mon, May 16, 2022 at 10:40:35PM -0700, Evan Silberman wrote: > I do git log --oneline reasonably often and it seemed like exactly the > sort of thing I could take a stab at a patch for without having to do > much other than harvest parts of other functions, and so it was. > There's probably something wrong with this given it may be the most C > I've ever written at once. And also the choice of -1 as the flag was too > cute to resist proposing but also probably too cute to actually use. I don't think using -1 for this is a good idea for two reasons. First, we've been exclusively using letters as option names so far. This is part of what makes our UI consistent and easy to memorize in the long term. Numbers only appear as option arguments so far. Second, git log -1 exists and it does something else than git log --oneline. Switching between got and git on a whim becomes harder if we introduce subtle incompatibilities. Instead of -1, you could pick another free letter, such as -o or -z. That would be the simplest solution. Or we could rename the existing -s option (search) to -S, and then you could add your feature as -s. (We are not yet at the point where parts of the system are set in stone for backwards compatibility, so moving such things around if needed is fine.) If we want to be ambitious, this patch seems like a good first step towards merge-graph-style output. If the printing routine kept track of the number of open branches currently known to the commit graph, it could probably print a graph? What do you think about that? If you would like to extend this feature to show a graph, then we could use the -G option (like Mercurial hg log -G, check it out, it looks nice); -G would imply -b. A few more suggestions below: > @@ -771,6 +771,16 @@ The options for > .Cm got log > are as follows: > .Bl -tag -width Ds > +.It Fl 1 > +Display a compact one-line summary of each commit, instead of the default > +history format. > +The one-line format prints the first seven characters of the SHA1 hash of each I would not document any specific character limit. This could just say "abbreviated SHA1 hash". We might eventually want to scale the length of abbreviated hashes to the number of total commits in the repository. The documentation as written above would then become outdated. Better write it in a way that won't need to be adjusted later. It is fine if regression tests assume a specific length. We can always adjust them if needed. > +commit, followed by a space and the first line of the log message. > +The > +.Fl p > +and > +.Fl P > +options are ignored. We are usually more strict than this. The man page would say: "Cannot be used together with the -p or -P options." And 'got log' would error out if incompatible options are used together, and ideally a test would check for this behaviour of erroring out. This approach can suck for scripting, where the list of options might be derived from several independent parts of a script. But it makes the tool more robust during interactive use. And there is less chance of unexpected behaviour in case an unsupported option combo lacks test coverage since we exit early rather than running the program with a set of incompatible flags. > static const struct got_error * > +print_commit_oneline(struct got_commit_object *commit, struct got_object_id *id) > +{ > + const struct got_error *err = NULL; > + char *id_str, *s, *nl, *logmsg0, *logmsg = NULL; > + > + err = got_object_id_str(&id_str, id); > + if (err) > + return err; > + > + err = got_object_commit_get_logmsg(&logmsg0, commit); > + if (err) > + goto done; > + > + s = logmsg0; > + while (isspace((unsigned char)s[0])) > + s++; > + > + logmsg = strdup(s); > + if (logmsg == NULL) { > + err = got_error_from_errno("strdup"); > + goto done; > + } > + > + nl = strchr(logmsg, '\n'); > + if (nl) { > + *nl = '\0'; > + } > + > + printf("%.7s %s\n", id_str, logmsg); This output format doesn't include any information about references. Have you considered picking a reference name to display instead of a short hash, in case a suitable reference is available? It would be fine to display just one ref, even if multiple references match. You could prioritise those in refs/heads, then refs/tags, then refs/remotes, then any others? Of course, it would filter out anything from refs/got. I have made many suggestions. Please just pick whatever you think is reasonable for you to work through for the next iteration of this patch. Much of what I suggested could be done as follow-up patches, even by people other than you if they like the ideas, or by myself.