"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Evan Silberman <evan@jklol.net>
Subject:
Re: [patch] add a one-line-per-commit output mode to log
To:
Stefan Sperling <stsp@stsp.name>
Cc:
gameoftrees@openbsd.org
Date:
Tue, 07 Jun 2022 21:43:27 -0700

Download raw body.

Thread
Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Jun 04, 2022 at 04:05:18PM -0700, Evan Silberman wrote:
> > -.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
> 
> Showing -p -P -s instead of [ -pP | -s ] would be more consistent with
> other parts of the man page, and with other existing usage() strings.

Check.

> So far, we don't represent exclusivity of options in the synopsis line.
> I suspect doing so consistently would become complicated very quickly
> for some of our commands. Synopsis and usage strings can just list all
> valid options. Details are documented in the option description text.
> 
> >  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);
> 
> There should be no need for strdup(). logmsg0 is already a copy of the
> commit's log message string so writing a '\0' to the logmsg0 buffer is fine.
> You forgot to free(logmsg), but this won't be needed if we decide to simply
> drop the strdup() call.

An artifact of my copy-paste-iterate-driven development process. My
skill level with C string handling is at the "guessing" stage.

> > +	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);
> 
> The current output format of log -s is quite sparse. It might be a good
> idea to extend it. For example, it could include committer date and name,
> like the default log display shown by 'tog log'. But this can be tweaked
> later.

This suggests still further interesting questions about the UI design
space. git log --pretty has 9 predefined formats, all orthogonal to the
--decorate flag, plus a formatter language with dozens of tokens
accessible as well as alignment and color output directives. I'd take it
as a given that got doesn't want to support _quite_ that much but
certainly my contribution here doesn't seem like the end of the line
either. (fwiw this patch makes -s behave basically the same as git log
--oneline --no-decorate.) I think you'd want to make a design decision
about what your UI should be for this before even more options and
combinations of options get into the mix.

Fresh patch attached.

Evan

-----------------------------------------------
commit 739f42edd1683838b8ce3e27eee12ed3078fed51 (log-oneline)
from: Evan Silberman <evan@jklol.net>
date: Wed Jun  8 04:27:45 2022 UTC
 
 Add one-line output mode to got log with -s flag
 
 -s for short. Moves log search to -S.
 
diff b801cfa6010340244886ed8bcc5895a2831497e7 d9a8a2a59e0d34f20aaa79c5101e0e9909f9ca42
blob - b91cfb2cb4963721a9a86be9c2cdb397bc1a9278
blob + 44f372d9146e3367bcbd15b4681c57b1b340faef
--- 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 p Oc Oo Fl P Oc Oo Fl 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 + 473318fc731afd091ffa6dcf1c0a27f6c47cda82
--- got/got.c
+++ got/got.c
@@ -3841,6 +3841,39 @@ 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;
+
+	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++;
+
+	nl = strchr(s, '\n');
+	if (nl) {
+		*nl = '\0';
+	}
+
+	printf("%.7s %s\n", id_str, s);
+
+	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 +3974,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 +4053,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 +4085,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 +4121,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] [-p] [-P] [-s] [-c commit] "
+	    "[-C number] [ -l N ] [-x commit] [-S search-pattern] "
+	    "[-r repository-path] [-R] [path]\n", getprogname());
 	exit(1);
 }
 
@@ -4116,7 +4155,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 +4171,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 +4209,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 +4231,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 +4352,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