Re: [PATCH 3/2] pretty: support right alignment
On Sun, Sep 23, 2012 at 01:17:43AM -0700, Junio C Hamano wrote: Nguyen Thai Ngoc Duy pclo...@gmail.com writes: ... On the other hand, I don't really wish to turn pretty format machinery into a full feature text layout engine (by ripping of links/lynx?). That is very true. We should restrain ourselves and avoid going overboard piling shiny new toys on a not-so-useful foundation that is not expressive enough. One feature that is probably much more needed on the foundation side would be some form of conditional output, without which built-in output elements like --show-notes cannot be emulated with --format option. The embedded lua patch series I just posted may interest (or horrify) the both of you: http://article.gmane.org/gmane.comp.version-control.git/206335 -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: ... On the other hand, I don't really wish to turn pretty format machinery into a full feature text layout engine (by ripping of links/lynx?). That is very true. We should restrain ourselves and avoid going overboard piling shiny new toys on a not-so-useful foundation that is not expressive enough. One feature that is probably much more needed on the foundation side would be some form of conditional output, without which built-in output elements like --show-notes cannot be emulated with --format option. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
On Thu, Sep 20, 2012 at 11:40 PM, Junio C Hamano gits...@pobox.com wrote: I think this is a great feature at the conceptual level, and you know but is coming ;-). I'm still not sure if it's useful beyond my simple example. For example, will it be useful in multiline log format, not just --oneline? - Shouldn't it be everything from there until the end of the current line than everything after that? The patch does that. I wasn't specific in my patch description. - How is the display width determined and is it fixed once it gets computed? term_columns(). But I'd rather have a (user-configurable) max limit. It's really hard to line up two distant text parts of a 200 char line without a physical ruler. In my patch I just hard code the max limit around 120 char or so. - How does this interact with the wrapped output? Should it? We have to deal with it anyway when the left aligned text takes all the space. On one hand, I don't want to break the terminal width, leading to ugly output, so it'll interact. On the other hand, I don't really wish to turn pretty format machinery into a full feature text layout engine (by ripping of links/lynx?). So we have a few options: 1. ellipses, line cutting means i18n issues ahead 2. just put the right-aligned text on another line. We do something similar in parse-options. When the option syntax is too long, we put help description on the next line. 3. bring in html/css box model for arranging text so that both left/right aligned texts can share the same line. 4. tell users upfront it's not supported. don't do that I'd vote 2, or 4. - I am wondering if somebody ever want to do this with a follow-up patch: Left %h%|Center %cd%|Right %ad Is %| a sensible choice for flush right? I am wondering if it makes more sense to make %|, % and % as multi-column introducer (the example defines output with three columns) that also tells how text inside each column is flushed inside the column, e.g. %col 1 right flushed%|col 2 centered% col 3 left flushed or something like that (we may want explicit column width specifiers if we were to do this kind of thing). Yeah that crossed my mind. But I'll need to convince myself it's actually useful. Once you're on that road, you may want =4 column tables. We can extend column.c to do that. That hard part is converting pretty format to use column functions. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
On Thu, Sep 20, 2012 at 09:40:49AM -0700, Junio C Hamano wrote:
- I am wondering if somebody ever want to do this with a follow-up
patch:
Left %h%|Center %cd%|Right %ad
Is %| a sensible choice for flush right? I am wondering if it
makes more sense to make %|, % and % as multi-column
introducer (the example defines output with three columns) that
also tells how text inside each column is flushed inside the
column, e.g.
%col 1 right flushed%|col 2 centered% col 3 left flushed
or something like that (we may want explicit column width
specifiers if we were to do this kind of thing).
Instead of thinking of columns, we could go back to placeholders,
or in printf terms, an %s. In addition to plain %s, we need
something similar to %*s and %-*s to pad right and left.
Conceptually it's simpler. We don't have to deal with a bunch of
problems in your quotes that I cut out. Still it allows users to do
flush right, flush left and so on within limits. They just need to
think in terms of fixed-size cells. So...
%(N)%? is transformed roughly to printf(%-*s, N, %?). Similarly
%(N)%? becomes printf(%*s, N, %?). We could have %|(N) to pad both
%left and right (aka centered).
Better?
We might need a modifier or something to allow cutting (and maybe
putting ellipsis in place) to keep oversized cells from breaking the
layout.
The demonstration patch follows. You can't build because I don't post
the whole series.
-- 8 --
diff --git a/pretty.c b/pretty.c
index b1cec71..543c309 100644
--- a/pretty.c
+++ b/pretty.c
@@ -617,6 +617,12 @@ struct chunk {
size_t len;
};
+enum flush_type {
+ no_flush,
+ flush_right,
+ flush_left
+};
+
struct format_commit_context {
const struct commit *commit;
const struct pretty_print_context *pretty_ctx;
@@ -624,13 +630,14 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
unsigned use_color:1;
+ enum flush_type flush_type;
struct {
char *gpg_output;
char good_bad;
char *signer;
} signature;
char *message;
- size_t width, indent1, indent2;
+ size_t width, indent1, indent2, padding;
/* These offsets are relative to the start of the commit message. */
struct chunk author;
@@ -944,6 +951,24 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return end - placeholder + 1;
} else
return 0;
+
+ case '':
+ case '':
+ if (placeholder[1] == '(') {
+ const char *start = placeholder + 2;
+ const char *end = strchr(start, ')');
+ char *next;
+ int width;
+ if (!end || end == start)
+ return 0;
+ width = strtoul(start, next, 10);
+ if (next == start || width == 0)
+ return 0;
+ c-padding = width;
+ c-flush_type = *placeholder == '' ? flush_right :
flush_left;
+ return end - placeholder + 1;
+ }
+ return 0;
}
/* these depend on the commit */
@@ -1102,6 +1127,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
void *context)
{
+ struct format_commit_context *c = context;
+ struct strbuf local_sb = STRBUF_INIT;
int consumed;
size_t orig_len;
enum {
@@ -1127,10 +1154,23 @@ static size_t format_commit_item(struct strbuf *sb,
const char *placeholder,
if (magic != NO_MAGIC)
placeholder++;
- orig_len = sb-len;
- consumed = format_commit_one(sb, placeholder, context);
- if (magic == NO_MAGIC)
- return consumed;
+ if (c-flush_type != no_flush) {
+ int len;
+ consumed = format_commit_one(local_sb, placeholder, context);
+ /* the number of column, esc seq skipped */
+ len = utf8_strnwidth(local_sb.buf, -1, 1);
+ strbuf_addf(sb,
+ c-flush_type == flush_right ? %-*s : %*s,
+ (int)(c-padding + (local_sb.len - len)),
+ local_sb.buf);
+ strbuf_release(local_sb);
+ c-flush_type = no_flush;
+ } else {
+ orig_len = sb-len;
+ consumed = format_commit_one(sb, placeholder, context);
+ if (magic == NO_MAGIC)
+ return consumed;
+ }
if ((orig_len == sb-len) magic == DEL_LF_BEFORE_EMPTY) {
while (sb-len
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: - How does this interact with the wrapped output? Should it? We have to deal with it anyway when the left aligned text takes all the space. On one hand, I don't want to break the terminal width, leading to ugly output, so it'll interact. On the other hand, I don't really wish to turn pretty format machinery into a full feature text layout engine (by ripping of links/lynx?). So we have a few options: 1. ellipses, line cutting means i18n issues ahead 2. just put the right-aligned text on another line. We do something similar in parse-options. When the option syntax is too long, we put help description on the next line. 3. bring in html/css box model for arranging text so that both left/right aligned texts can share the same line. 4. tell users upfront it's not supported. don't do that I'd vote 2, or 4. I am fine with 4 personally. - I am wondering if somebody ever want to do this with a follow-up patch: Left %h%|Center %cd%|Right %ad Is %| a sensible choice for flush right? I am wondering if it makes more sense to make %|, % and % as multi-column introducer (the example defines output with three columns) that also tells how text inside each column is flushed inside the column, e.g. %col 1 right flushed%|col 2 centered% col 3 left flushed or something like that (we may want explicit column width specifiers if we were to do this kind of thing). Yeah that crossed my mind. But I'll need to convince myself it's actually useful. Once you're on that road, you may want =4 column tables. We can extend column.c to do that. That hard part is converting pretty format to use column functions. Reading the above again, I realize that I may have sounded as if saying With 'flush to right', we are inviting wishes for 'left' and 'center', and a patch that only does 'right' is unacceptable., but that was not what I meant. I am perfectly fine with 'flush to right' without anything else as the first step. The only concern I had was that somebody who later tries to add 'left', 'center', etc. to accompany the 'right' that you are adding with your patch will find it unfortunate that the natural choice for 'center', i.e. %|, has been taken to mean a wrong thing and that mistake cannot be corrected without breaking backward compatibility. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
And this is a for-fun patch that adds %| to right align everything
after that. I'm ignoring problems with line wrapping, i18n and so
on. %C+%h %s%|%d looks quite nice. I'm not sure how much useful it
is beyond --oneline though. It looks something like this
cc543b2 pretty: support placeholders %C+ and %C-
(HEAD, master)
da6001e pretty: share code between format_decoration and show_decorations
b0576a6 Update draft release notes to 1.8.0
(origin/master, origin/HEAD)
3d7535e Merge branch 'jc/maint-log-grep-all-match'
06e211a Merge branch 'jc/make-static'
8db3865 Merge branch 'pw/p4-submit-conflicts'
3387423 Merge branch 'mv/cherry-pick-s'
d71abd9 Merge branch 'nd/fetch-status-alignment'
3c7d509 Sync with 1.7.12.1
304b7d9 Git 1.7.12.1
(tag: v1.7.12.1, origin/maint)
39e2e02 Merge branch 'er/doc-fast-import-done' into maint
8ffc331 Merge branch 'jk/config-warn-on-inaccessible-paths' into maint
01f7d7f Doc: Improve shallow depth wording
8093ae8 Documentation/git-filter-branch: Move note about effect of removing
commits
-- 8 --
diff --git a/pretty.c b/pretty.c
index b1cec71..6e96f83 100644
--- a/pretty.c
+++ b/pretty.c
@@ -624,6 +624,7 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
unsigned use_color:1;
+ unsigned right_alignment:1;
struct {
char *gpg_output;
char good_bad;
@@ -645,6 +646,8 @@ struct format_commit_context {
struct chunk abbrev_tree_hash;
struct chunk abbrev_parent_hashes;
size_t wrap_start;
+
+ struct strbuf *right_sb;
};
static int add_again(struct strbuf *sb, struct chunk *chunk)
@@ -944,6 +947,10 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return end - placeholder + 1;
} else
return 0;
+
+ case '|':
+ c-right_alignment = 1;
+ return 1;
}
/* these depend on the commit */
@@ -1099,9 +1106,44 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return 0; /* unknown placeholder */
}
+static void right_align(struct strbuf *sb,
+ struct format_commit_context *c,
+ int flush)
+{
+ const char *p;
+ int llen, rlen, len, total = term_columns() - 1;
+ if (!c-right_alignment)
+ return;
+ p = strchr(c-right_sb-buf, '\n');
+ if (!p flush)
+ p = c-right_sb-buf + c-right_sb-len;
+ if (!p)
+ return;
+
+ c-right_alignment = 0;
+ len = p - c-right_sb-buf;
+ if (!len)
+ return;
+ if (total 110)
+ total = 110;
+ rlen = utf8_strnwidth(c-right_sb-buf, len);
+ p = strrchr(sb-buf, '\n');
+ if (!p)
+ p = sb-buf;
+ else
+ p++;
+ llen = utf8_strwidth(p);
+ strbuf_addf(sb, %*s,
+ total - llen + (len - rlen),
+ c-right_sb-buf);
+ strbuf_reset(c-right_sb);
+}
+
static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
void *context)
{
+ struct format_commit_context *c = context;
+ struct strbuf *real_sb;
int consumed;
size_t orig_len;
enum {
@@ -1127,10 +1169,13 @@ static size_t format_commit_item(struct strbuf *sb,
const char *placeholder,
if (magic != NO_MAGIC)
placeholder++;
+ if (c-right_alignment c-right_sb) {
+ real_sb = sb;
+ sb = c-right_sb;
+ }
+
orig_len = sb-len;
consumed = format_commit_one(sb, placeholder, context);
- if (magic == NO_MAGIC)
- return consumed;
if ((orig_len == sb-len) magic == DEL_LF_BEFORE_EMPTY) {
while (sb-len sb-buf[sb-len - 1] == '\n')
@@ -1141,7 +1186,13 @@ static size_t format_commit_item(struct strbuf *sb,
const char *placeholder,
else if (magic == ADD_SP_BEFORE_NON_EMPTY)
strbuf_insert(sb, orig_len, , 1);
}
- return consumed + 1;
+
+ if (real_sb)
+ right_align(real_sb, c, 0);
+
+ if (magic != NO_MAGIC)
+ consumed++;
+ return consumed;
}
static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
@@ -1180,12 +1231,14 @@ void format_commit_message(const struct commit *commit,
struct format_commit_context context;
static const char utf8[] = UTF-8;
const char *output_enc = pretty_ctx-output_encoding;
+ struct strbuf right_sb = STRBUF_INIT;
memset(context, 0, sizeof(context));
context.commit = commit;
context.pretty_ctx =
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: And this is a for-fun patch that adds %| to right align everything after that. I'm ignoring problems with line wrapping, i18n and so on. %C+%h %s%|%d looks quite nice. I'm not sure how much useful it is beyond --oneline though. It looks something like this ... [oneline output ellided] ... I think this is a great feature at the conceptual level, and you know but is coming ;-). - Shouldn't it be everything from there until the end of the current line than everything after that? - How is the display width determined and is it fixed once it gets computed? - How does this interact with the wrapped output? Should it? - I am wondering if somebody ever want to do this with a follow-up patch: Left %h%|Center %cd%|Right %ad Is %| a sensible choice for flush right? I am wondering if it makes more sense to make %|, % and % as multi-column introducer (the example defines output with three columns) that also tells how text inside each column is flushed inside the column, e.g. %col 1 right flushed%|col 2 centered% col 3 left flushed or something like that (we may want explicit column width specifiers if we were to do this kind of thing). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
