Download raw body.
[patch] add a one-line-per-commit output mode to log
Thanks for the detailed feedback! Thoughts inline and a revised patch at
the bottom.
Stefan Sperling <stsp@stsp.name> 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 <evan@jklol.net>
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
[patch] add a one-line-per-commit output mode to log