From: Evan Silberman Subject: Re: [patch] add a one-line-per-commit output mode to log To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Sat, 04 Jun 2022 16:05:18 -0700 Thanks for the detailed feedback! Thoughts inline and a revised patch at the bottom. Stefan Sperling wrote: > 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. Ya learn something new every day. > 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.) Since you suggested it I went for it. -s for short is nice. > 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. I leave this to more capable hackers, not to exclude the possibility that "future me" is one such person. > 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. Check. > > 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. Documented and implemented the mutual exclusion, but I haven't added a regress for it. > 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. That sounds like a nicely ergonomic default and I'd be happy if it existed but again I leave this for future generations. > 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. Thanks again for looking. Evan ----------------------------------------------- commit 96278aa32031e19df6e537b3d87667a02ea11134 from: Evan Silberman date: Sat Jun 4 22:59:12 2022 UTC Add one-line output mode to got log with -s flag -s for short. Moves log search to -S. M got/got.1 M got/got.c M regress/cmdline/log.sh diff b801cfa6010340244886ed8bcc5895a2831497e7 d1b50da87f207800cabd80ab4a8867b926d6c60f blob - b91cfb2cb4963721a9a86be9c2cdb397bc1a9278 blob + ef8a26678a9cc998c2aa65d3b1595102c2fdd008 --- got/got.1 +++ got/got.1 @@ -756,7 +756,7 @@ does not support negated ignore patterns prefixed with and gives no special significance to the location of path component separators, .Dq / , in a pattern. -.It Cm log Oo Fl b Oc Oo Fl c Ar commit Oc Oo Fl C Ar number Oc Oo Fl l Ar N Oc Oo Fl p Oc Oo Fl P Oc Oo Fl s Ar search-pattern Oc Oo Fl r Ar repository-path Oc Oo Fl R Oc Oo Fl x Ar commit Oc Op Ar path +.It Cm log Oo Fl b Oc Oo Fl c Ar commit Oc Oo Fl C Ar number Oc Oo Fl l Ar N Oc Oo Fl pP | s Oc Oo Fl S Ar search-pattern Oc Oo Fl r Ar repository-path Oc Oo Fl R Oc Oo Fl x Ar commit Oc Op Ar path Display history of a repository. If a .Ar path @@ -802,6 +802,9 @@ Display the patch of modifications made in each commit If a .Ar path is specified, only show the patch of modifications at or within this path. +Cannot be used with the +.Fl s +option. .It Fl P Display the list of file paths changed in each commit, using the following status codes: @@ -811,7 +814,20 @@ status codes: .It A Ta new file was added .It m Ta modified file modes (executable bit only) .El -.It Fl s Ar search-pattern +Cannot be used with the +.Fl s +option. +.It Fl s +Display a short one-line summary of each commit, instead of the default +history format. +The one-line format prints the beginning of the SHA1 hash of each commit, +followed by a space and the first line of the log message. +Cannot be used together with the +.Fl p +or +.Fl P +option. +.It Fl S Ar search-pattern If specified, show only commits with a log message matched by the extended regular expression .Ar search-pattern . blob - d88f23ce5b63a9d80862b5fac05c212d054a0b74 blob + cd52e408578f5342aeeeefb3f8b97080f9229345 --- got/got.c +++ got/got.c @@ -3841,6 +3841,45 @@ build_refs_str(char **refs_str, struct got_reflist_hea } 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); + + if (fflush(stdout) != 0 && err == NULL) + err = got_error_from_errno("fflush"); +done: + free(id_str); + free(logmsg0); + return err; +} + +static const struct got_error * print_commit(struct got_commit_object *commit, struct got_object_id *id, struct got_repository *repo, const char *path, struct got_pathlist_head *changed_paths, int show_patch, @@ -3941,7 +3980,7 @@ print_commits(struct got_object_id *root_id, struct go struct got_repository *repo, const char *path, int show_changed_paths, int show_patch, const char *search_pattern, int diff_context, int limit, int log_branches, int reverse_display_order, - struct got_reflist_object_id_map *refs_idmap) + struct got_reflist_object_id_map *refs_idmap, int one_line) { const struct got_error *err; struct got_commit_graph *graph; @@ -4020,9 +4059,12 @@ print_commits(struct got_object_id *root_id, struct go STAILQ_INSERT_HEAD(&reversed_commits, qid, entry); got_object_commit_close(commit); } else { - err = print_commit(commit, id, repo, path, - show_changed_paths ? &changed_paths : NULL, - show_patch, diff_context, refs_idmap, NULL); + if (one_line) + err = print_commit_oneline(commit, id); + else + err = print_commit(commit, id, repo, path, + show_changed_paths ? &changed_paths : NULL, + show_patch, diff_context, refs_idmap, NULL); got_object_commit_close(commit); if (err) break; @@ -4049,9 +4091,12 @@ print_commits(struct got_object_id *root_id, struct go if (err) break; } - err = print_commit(commit, &qid->id, repo, path, - show_changed_paths ? &changed_paths : NULL, - show_patch, diff_context, refs_idmap, NULL); + if (one_line) + err = print_commit_oneline(commit, &qid->id); + else + err = print_commit(commit, &qid->id, repo, path, + show_changed_paths ? &changed_paths : NULL, + show_patch, diff_context, refs_idmap, NULL); got_object_commit_close(commit); if (err) break; @@ -4082,9 +4127,9 @@ done: __dead static void usage_log(void) { - fprintf(stderr, "usage: %s log [-b] [-c commit] [-C number] [ -l N ] " - "[-p] [-P] [-x commit] [-s search-pattern] [-r repository-path] " - "[-R] [path]\n", getprogname()); + fprintf(stderr, "usage: %s log [-b] [-c commit] [-C number] " + "[ -l N ] [ -s | [-pP] ] [-x commit] [-S search-pattern] " + "[-r repository-path] [-R] [path]\n", getprogname()); exit(1); } @@ -4116,7 +4161,7 @@ cmd_log(int argc, char *argv[]) const char *search_pattern = NULL; int diff_context = -1, ch; int show_changed_paths = 0, show_patch = 0, limit = 0, log_branches = 0; - int reverse_display_order = 0; + int reverse_display_order = 0, one_line = 0; const char *errstr; struct got_reflist_head refs; struct got_reflist_object_id_map *refs_idmap = NULL; @@ -4132,7 +4177,7 @@ cmd_log(int argc, char *argv[]) limit = get_default_log_limit(); - while ((ch = getopt(argc, argv, "bpPc:C:l:r:Rs:x:")) != -1) { + while ((ch = getopt(argc, argv, "bpPc:C:l:r:RsS:x:")) != -1) { switch (ch) { case 'p': show_patch = 1; @@ -4170,6 +4215,9 @@ cmd_log(int argc, char *argv[]) reverse_display_order = 1; break; case 's': + one_line = 1; + break; + case 'S': search_pattern = optarg; break; case 'x': @@ -4189,6 +4237,9 @@ cmd_log(int argc, char *argv[]) else if (!show_patch) errx(1, "-C requires -p"); + if (one_line && (show_patch || show_changed_paths)) + errx(1, "cannot use -s with -p or -P"); + cwd = getcwd(NULL, 0); if (cwd == NULL) { error = got_error_from_errno("getcwd"); @@ -4307,7 +4358,7 @@ cmd_log(int argc, char *argv[]) error = print_commits(start_id, end_id, repo, path ? path : "", show_changed_paths, show_patch, search_pattern, diff_context, - limit, log_branches, reverse_display_order, refs_idmap); + limit, log_branches, reverse_display_order, refs_idmap, one_line); done: free(path); free(repo_path); blob - f13bd9029079658e7f658b35c11dc4ca7a6f70f7 blob + 41cee9ee65ad11031857077637457e6954320463 --- regress/cmdline/log.sh +++ regress/cmdline/log.sh @@ -297,6 +297,40 @@ test_log_limit() { test_done "$testroot" "0" } +test_log_oneline() { + local testroot=`test_init log_oneline` + local commit_id0=`git_show_head $testroot/repo` + got checkout $testroot/repo $testroot/wt > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + test_done "$testroot" "$ret" + return 1 + fi + + echo "modified alpha" > $testroot/wt/alpha + (cd $testroot/wt && got commit -m "test oneline +no" > /dev/null) + local commit_id1=`git_show_head $testroot/repo` + + echo "modified beta" > $testroot/wt/beta + (cd $testroot/wt && got commit -m " test oneline +no" > /dev/null) + local commit_id2=`git_show_head $testroot/repo` + + printf "%.7s test oneline\n" $commit_id2 > $testroot/stdout.expected + printf "%.7s test oneline\n" $commit_id1 >> $testroot/stdout.expected + + (cd $testroot/repo && got log -s | head -n 2 > $testroot/stdout) + cmp -s $testroot/stdout.expected $testroot/stdout + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stdout.expected $testroot/stdout + test_done "$testroot" "$ret" + return 1 + fi + test_done "$testroot" "0" +} + test_log_patch_added_file() { local testroot=`test_init log_patch_added_file` local commit_id0=`git_show_head $testroot/repo` @@ -587,7 +621,7 @@ test_log_reverse_display() { # commit matching with -s applies before -R echo "commit $commit_id1" > $testroot/stdout.expected echo "commit $commit_id2" >> $testroot/stdout.expected - (cd $testroot/wt && got log -R -s 'commit[12]' | \ + (cd $testroot/wt && got log -R -S 'commit[12]' | \ grep ^commit > $testroot/stdout) cmp -s $testroot/stdout.expected $testroot/stdout ret=$? @@ -815,6 +849,7 @@ run_test test_log_in_worktree run_test test_log_in_worktree_with_path_prefix run_test test_log_tag run_test test_log_limit +run_test test_log_oneline run_test test_log_patch_added_file run_test test_log_nonexistent_path run_test test_log_end_at_commit