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.