Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]

I agree that this is the right to do, since this is how the built-in
formats work (the ‘commit ...’ line follows the semantics of your %H,
and ‘Merge: ...’ line your %p, for example).

Documentation and tests?

Tests, noted. I'll include some in the next version of the patch.
As for documentation, I'm not entirely sure what to add, as it seemed
like the change
merely implements what I would expect when reading the docs for
git-log. Still, noted.
I'll stick a short note in each of %H, %h, %P, %p, and %t

Shortlog doesn’t print commit hashes, does it?

Shortlog accepts --format, though this doesn't seem to be documented
(if I type "man" and search
for "format"), so perhaps it should be.

quoted

diff --git a/commit.h b/commit.h
index b6caf91..7a476a0 100644
--- a/commit.h
+++ b/commit.h

@@ -72,6 +72,7 @@ struct pretty_print_context

      int need_8bit_cte;
      int show_notes;
      int use_color;
+     int abbrev_commit;
      struct reflog_walk_info *reflog_info;
 };

nitpick: I’d stick this up with abbrev and maybe add a comment to
explain their distinct uses.

Noted. Thanks

quoted

diff --git a/log-tree.c b/log-tree.c
index 6bb4748..0a2309c 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -282,6 +282,8 @@ void show_log(struct rev_info *opt)

      int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
      const char *extra_headers = opt->extra_headers;
      struct pretty_print_context ctx = {0};
+     ctx.abbrev = opt->abbrev;
+     ctx.abbrev_commit = opt->abbrev_commit;
      ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);

      opt->loginfo = NULL;

There is a

ctx.abbrev = opt->diffopt.abbrev;

later in the same function; how do these interact?

I hadn't caught that. My guess: stupidly doing the same thing twice.
I'll double-check,
and take out one of them if that's the case.

What all this
has shown me is that there are really too many ways to specify "context when
printing information about a commit". I don't like it, and the lot of
them can probably
be re-factored, perhaps by getting rid of pretty_context and passing
rev_info around
everywhere. I don't know the full implications of that and it seems
outside the scope
of this change.

quoted

@@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,

[...]

quoted

                      strbuf_addstr(sb, find_unique_abbrev(
-                                     p->item->object.sha1, DEFAULT_ABBREV));
+                                   p->item->object.sha1,
+                                   c->pretty_ctx->abbrev));

nitpick: the new indentation makes these look like parameters to
strbuf_addstr.

Noted. I'll restore the extra indent, which I had assumed was a typo.

I had been thinking that this wouldn't be safe, but then that was my
being overly-cautious:
it's just been xstrdup()ed, so what we're parsing is ours, no real
reason not to do it.
I think the "must be specified after --abbrev-commit" is a rather
large nail, though. If you work
out the bugs mentioned by Jeff King, and it works, I'll stick it in
there, as I don't like
falling through case statements any more than the next guy. (well,
maybe a little more than
the next guy).

Thanks for the review!
-- Will Palmer