git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] log: make --abbrev-commit's ellipsis configurable
@ 2009-02-13 12:58 Thomas Rast
  2009-02-13 13:20 ` Johannes Schindelin
  2009-02-13 21:32 ` Junio C Hamano
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Rast @ 2009-02-13 12:58 UTC (permalink / raw)
  To: git; +Cc: Adeodato Simó, Junio C Hamano

'git log --abbrev-commit' adds an ellipsis to all commit names that
were abbreviated.  This is annoying if you want to cut&paste the sha1
from the terminal, since selecting by word will pick up '...' too.
(And this cannot be fixed by making '.' a non-word character; in other
instances, such as the '123457..abcdef0' from git-fetch, it's part of
the expression.)

So we introduce a new variable format.abbrevEllipsis that defaults to
true (previous behaviour).  If disabled, the formatting routines for
log/show/whatchanged do not append an ellipsis.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Asked for by dato (Adeodato Simó) on IRC.  The bad interaction with
word selection always bothered me, so I jumped at the opportunity.

BTW, how strict is the alphabetic order of Documentation/config.txt
supposed to be?  I noticed several options are in the wrong place,
e.g., format.headers, format.suffix, pack.indexVersion, and
core.autocrlf.


 Documentation/config.txt |    6 ++++++
 builtin-log.c            |    4 ++++
 log-tree.c               |   19 ++++++++++++++-----
 log-tree.h               |    3 +++
 pretty.c                 |    6 ++----
 t/t4202-log.sh           |   23 +++++++++++++++++++++++
 6 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1806a60..ee5b76f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -656,6 +656,12 @@ fetch.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+format.abbrevEllipsis::
+	Whether --abbrev-commit for log/show/whatchanged should append
+	an ellipsis (...) to the abbreviated SHA-1.  Defaults to true.
+	See linkgit:git-log[1], linkgit:git-show[1],
+	linkgit:git-whatchanged[1].
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..f85e469 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -227,6 +227,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		default_show_root = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.abbrevellipsis")) {
+		log_abbrev_commit_ellipsis = git_config_bool(var, value);
+		return 0;
+	}
 	return git_diff_ui_config(var, value, cb);
 }
 
diff --git a/log-tree.c b/log-tree.c
index 194ddb1..ec7d3e4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -8,6 +8,7 @@
 #include "refs.h"
 
 struct decoration name_decoration = { "object names" };
+int log_abbrev_commit_ellipsis = 1;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -43,12 +44,20 @@ void load_ref_decorations(void)
 	}
 }
 
+const char *log_unique_abbrev(const unsigned char *sha1, int abbrev)
+{
+	if (log_abbrev_commit_ellipsis)
+		return diff_unique_abbrev(sha1, abbrev);
+	else
+		return find_unique_abbrev(sha1, abbrev);
+}
+
 static void show_parents(struct commit *commit, int abbrev)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", diff_unique_abbrev(parent->object.sha1, abbrev));
+		printf(" %s", log_unique_abbrev(parent->object.sha1, abbrev));
 	}
 }
 
@@ -280,7 +289,7 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
+		fputs(log_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		show_decorations(opt, commit);
@@ -348,14 +357,14 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit),
+		fputs(log_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		if (parent)
 			printf(" (from %s)",
-			       diff_unique_abbrev(parent->object.sha1,
-						  abbrev_commit));
+			       log_unique_abbrev(parent->object.sha1,
+						 abbrev_commit));
 		show_decorations(opt, commit);
 		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
diff --git a/log-tree.h b/log-tree.h
index f2a9008..af99c69 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -3,6 +3,8 @@
 
 #include "revision.h"
 
+extern int log_abbrev_commit_ellipsis;
+
 struct log_info {
 	struct commit *commit, *parent;
 };
@@ -18,5 +20,6 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
 void load_ref_decorations(void);
+const char *log_unique_abbrev(const unsigned char *sha1, int abbrev);
 
 #endif
diff --git a/pretty.c b/pretty.c
index cc460b5..c64ad23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -210,15 +210,13 @@ static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
 	while (parent) {
 		struct commit *p = parent->item;
 		const char *hex = NULL;
-		const char *dots;
 		if (abbrev)
-			hex = find_unique_abbrev(p->object.sha1, abbrev);
+			hex = log_unique_abbrev(p->object.sha1, abbrev);
 		if (!hex)
 			hex = sha1_to_hex(p->object.sha1);
-		dots = (abbrev && strlen(hex) != 40) ?  "..." : "";
 		parent = parent->next;
 
-		strbuf_addf(sb, " %s%s", hex, dots);
+		strbuf_addf(sb, " %s", hex);
 	}
 	strbuf_addch(sb, '\n');
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 7b976ee..730036e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -134,5 +134,28 @@ test_expect_success 'log --grep -i' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git log --abbrev-commit (default: ellipsis enabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1)\\.\\.\\."
+'
+
+test_expect_success 'enable format.abbrevEllipsis' '
+	git config format.abbrevEllipsis true
+'
+
+test_expect_success 'git log --abbrev-commit (ellipsis enabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1)\\.\\.\\."
+'
+
+test_expect_success 'disable format.abbrevEllipsis' '
+	git config format.abbrevEllipsis false
+'
+
+test_expect_success 'git log --abbrev-commit (ellipsis disabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1) "
+'
+
 test_done
 
-- 
1.6.2.rc0.274.g97213

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 12:58 [PATCH] log: make --abbrev-commit's ellipsis configurable Thomas Rast
@ 2009-02-13 13:20 ` Johannes Schindelin
  2009-02-13 13:37   ` Thomas Rast
  2009-02-13 21:32 ` Junio C Hamano
  1 sibling, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2009-02-13 13:20 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Adeodato Simó, Junio C Hamano

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1171 bytes --]

Hi,

On Fri, 13 Feb 2009, Thomas Rast wrote:

> 'git log --abbrev-commit' adds an ellipsis to all commit names that
> were abbreviated.  This is annoying if you want to cut&paste the sha1
> from the terminal, since selecting by word will pick up '...' too.
> (And this cannot be fixed by making '.' a non-word character; in other
> instances, such as the '123457..abcdef0' from git-fetch, it's part of
> the expression.)
> 
> So we introduce a new variable format.abbrevEllipsis that defaults to
> true (previous behaviour).  If disabled, the formatting routines for
> log/show/whatchanged do not append an ellipsis.
> 
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>[...]
>
>  Documentation/config.txt |    6 ++++++
>  builtin-log.c            |    4 ++++
>  log-tree.c               |   19 ++++++++++++++-----
>  log-tree.h               |    3 +++
>  pretty.c                 |    6 ++----

I am slightly worried that you overshoot here, as log-tree.c has plumbing 
users, too, no?

How about making this an option, and passing it in rev_opts instead?  This 
option could then be defaulted to in git-log, when the user said 
--abbrev-commit.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 13:20 ` Johannes Schindelin
@ 2009-02-13 13:37   ` Thomas Rast
  2009-02-13 13:47     ` Johannes Schindelin
  2009-02-13 19:27     ` [PATCH] " Jeff King
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Rast @ 2009-02-13 13:37 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Adeodato Simó, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

Johannes Schindelin wrote:
> On Fri, 13 Feb 2009, Thomas Rast wrote:
[...]
> >  log-tree.c               |   19 ++++++++++++++-----
> >  log-tree.h               |    3 +++
> 
> I am slightly worried that you overshoot here, as log-tree.c has plumbing 
> users, too, no?
> 
> How about making this an option, and passing it in rev_opts instead?  This 
> option could then be defaulted to in git-log, when the user said 
> --abbrev-commit.

But the 'git_config(git_log_config, NULL);' that sets the new variable
to false is only called from cmd_{log,show,whatchanged,reflog}.  I
should have indicated this in the commit messaged, sorry.

The real problem with stuffing it in rev_opts (actually rev_info :-)
is that it seems inconsistent to not change the 'Merge: blah' line's
format.  But that is generated in pretty.c, in add_merge_info via
pp_header from pretty_print_commit, which has a bunch of users in
various 'builtin-*.c'.

So do I sacrifice symmetry (abbrev_commit is indeed stored in
rev_info), or touch the other ~7 users of pretty_print_commit too?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 13:37   ` Thomas Rast
@ 2009-02-13 13:47     ` Johannes Schindelin
  2009-02-13 14:24       ` [PATCH v2] " Thomas Rast
  2009-02-13 19:27     ` [PATCH] " Jeff King
  1 sibling, 1 reply; 11+ messages in thread
From: Johannes Schindelin @ 2009-02-13 13:47 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Adeodato Simó, Junio C Hamano

Hi,

On Fri, 13 Feb 2009, Thomas Rast wrote:

> Johannes Schindelin wrote:
> > On Fri, 13 Feb 2009, Thomas Rast wrote:
> [...]
> > >  log-tree.c               |   19 ++++++++++++++-----
> > >  log-tree.h               |    3 +++
> > 
> > I am slightly worried that you overshoot here, as log-tree.c has plumbing 
> > users, too, no?
> > 
> > How about making this an option, and passing it in rev_opts instead?  This 
> > option could then be defaulted to in git-log, when the user said 
> > --abbrev-commit.
> 
> But the 'git_config(git_log_config, NULL);' that sets the new variable
> to false is only called from cmd_{log,show,whatchanged,reflog}.  I
> should have indicated this in the commit messaged, sorry.
> 
> The real problem with stuffing it in rev_opts (actually rev_info :-)
> is that it seems inconsistent to not change the 'Merge: blah' line's
> format.  But that is generated in pretty.c, in add_merge_info via
> pp_header from pretty_print_commit, which has a bunch of users in
> various 'builtin-*.c'.
> 
> So do I sacrifice symmetry (abbrev_commit is indeed stored in
> rev_info), or touch the other ~7 users of pretty_print_commit too?

Thanks, I understand much better now.

Hmm... I cannot really make my mind up what I prefer.  But you have 
implemented one of the two options, so...

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 13:47     ` Johannes Schindelin
@ 2009-02-13 14:24       ` Thomas Rast
  2009-02-13 15:32         ` Johannes Sixt
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Rast @ 2009-02-13 14:24 UTC (permalink / raw)
  To: git; +Cc: Adeodato Simó, Junio C Hamano, Johannes Schindelin

'git log --abbrev-commit' adds an ellipsis to all commit names that
were abbreviated.  This is annoying if you want to cut&paste the sha1
from the terminal, since selecting by word will pick up '...' too.
(And this cannot be fixed by making '.' a non-word character; in other
instances, such as the '123457..abcdef0' from git-fetch, it's part of
the expression.)

So we introduce a new variable format.abbrevEllipsis that defaults to
true (previous behaviour).  If disabled, the formatting routines for
log/show/whatchanged do not append an ellipsis.

Internally we use a new global variable that is only set to false in
git_log_config, which is only called from log, show, whatchanged and
reflog, thus not affecting plumbing.  For symmetry with abbrev_commit,
it should go in rev_info, but that way we could not reach the
formatting in add_merge_info() without touching all calls of
pretty_print_commit().

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Changed the commit message to reflect the discussion in this thread.


 Documentation/config.txt |    6 ++++++
 builtin-log.c            |    4 ++++
 log-tree.c               |   19 ++++++++++++++-----
 log-tree.h               |    3 +++
 pretty.c                 |    6 ++----
 t/t4202-log.sh           |   23 +++++++++++++++++++++++
 6 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1806a60..ee5b76f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -656,6 +656,12 @@ fetch.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+format.abbrevEllipsis::
+	Whether --abbrev-commit for log/show/whatchanged should append
+	an ellipsis (...) to the abbreviated SHA-1.  Defaults to true.
+	See linkgit:git-log[1], linkgit:git-show[1],
+	linkgit:git-whatchanged[1].
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..f85e469 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -227,6 +227,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		default_show_root = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.abbrevellipsis")) {
+		log_abbrev_commit_ellipsis = git_config_bool(var, value);
+		return 0;
+	}
 	return git_diff_ui_config(var, value, cb);
 }
 
diff --git a/log-tree.c b/log-tree.c
index 194ddb1..ec7d3e4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -8,6 +8,7 @@
 #include "refs.h"
 
 struct decoration name_decoration = { "object names" };
+int log_abbrev_commit_ellipsis = 1;
 
 static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
 {
@@ -43,12 +44,20 @@ void load_ref_decorations(void)
 	}
 }
 
+const char *log_unique_abbrev(const unsigned char *sha1, int abbrev)
+{
+	if (log_abbrev_commit_ellipsis)
+		return diff_unique_abbrev(sha1, abbrev);
+	else
+		return find_unique_abbrev(sha1, abbrev);
+}
+
 static void show_parents(struct commit *commit, int abbrev)
 {
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", diff_unique_abbrev(parent->object.sha1, abbrev));
+		printf(" %s", log_unique_abbrev(parent->object.sha1, abbrev));
 	}
 }
 
@@ -280,7 +289,7 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
+		fputs(log_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		show_decorations(opt, commit);
@@ -348,14 +357,14 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit),
+		fputs(log_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		if (parent)
 			printf(" (from %s)",
-			       diff_unique_abbrev(parent->object.sha1,
-						  abbrev_commit));
+			       log_unique_abbrev(parent->object.sha1,
+						 abbrev_commit));
 		show_decorations(opt, commit);
 		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 		if (opt->commit_format == CMIT_FMT_ONELINE) {
diff --git a/log-tree.h b/log-tree.h
index f2a9008..af99c69 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -3,6 +3,8 @@
 
 #include "revision.h"
 
+extern int log_abbrev_commit_ellipsis;
+
 struct log_info {
 	struct commit *commit, *parent;
 };
@@ -18,5 +20,6 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
 void load_ref_decorations(void);
+const char *log_unique_abbrev(const unsigned char *sha1, int abbrev);
 
 #endif
diff --git a/pretty.c b/pretty.c
index cc460b5..c64ad23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -210,15 +210,13 @@ static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
 	while (parent) {
 		struct commit *p = parent->item;
 		const char *hex = NULL;
-		const char *dots;
 		if (abbrev)
-			hex = find_unique_abbrev(p->object.sha1, abbrev);
+			hex = log_unique_abbrev(p->object.sha1, abbrev);
 		if (!hex)
 			hex = sha1_to_hex(p->object.sha1);
-		dots = (abbrev && strlen(hex) != 40) ?  "..." : "";
 		parent = parent->next;
 
-		strbuf_addf(sb, " %s%s", hex, dots);
+		strbuf_addf(sb, " %s", hex);
 	}
 	strbuf_addch(sb, '\n');
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 7b976ee..730036e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -134,5 +134,28 @@ test_expect_success 'log --grep -i' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git log --abbrev-commit (default: ellipsis enabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1)\\.\\.\\."
+'
+
+test_expect_success 'enable format.abbrevEllipsis' '
+	git config format.abbrevEllipsis true
+'
+
+test_expect_success 'git log --abbrev-commit (ellipsis enabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1)\\.\\.\\."
+'
+
+test_expect_success 'disable format.abbrevEllipsis' '
+	git config format.abbrevEllipsis false
+'
+
+test_expect_success 'git log --abbrev-commit (ellipsis disabled)' '
+	git log --pretty=oneline --abbrev-commit -1 |
+	grep "^$(git log --pretty=format:%h -1) "
+'
+
 test_done
 
-- 
1.6.2.rc0.274.g97213

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 14:24       ` [PATCH v2] " Thomas Rast
@ 2009-02-13 15:32         ` Johannes Sixt
       [not found]           ` <m363jeux00.fsf@localhost.localdomain>
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Sixt @ 2009-02-13 15:32 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Adeodato Simó, Junio C Hamano, Johannes Schindelin

Thomas Rast schrieb:
> So we introduce a new variable format.abbrevEllipsis that defaults to
> true (previous behaviour).

How about format.hideAbbrevDots that defaults to false? Then you can write
in the config file:

    [format]
         hideAbbrevDots

(note: without '= true').

Even though I usually detest double negations (and I would count
hideAbbrevDots=false as one), the ability to set the non-default value of
a Boolean variable in this way trumps my taste.

-- Hannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] log: make --abbrev-commit's ellipsis configurable
       [not found]           ` <m363jeux00.fsf@localhost.localdomain>
@ 2009-02-13 16:45             ` Jakub Narebski
  0 siblings, 0 replies; 11+ messages in thread
From: Jakub Narebski @ 2009-02-13 16:45 UTC (permalink / raw)
  To: git; +Cc: Johannes Sixt, Adeodato Simo, Junio C Hamano, Johannes Schindelin

Johannes Sixt <j.sixt@viscovery.net> writes:
> Thomas Rast schrieb:

> > So we introduce a new variable format.abbrevEllipsis that defaults to
> > true (previous behaviour).
>
> How about format.hideAbbrevDots that defaults to false? Then you can write
> in the config file:
>
>     [format]
>          hideAbbrevDots
>
> (note: without '= true').
>
> Even though I usually detest double negations (and I would count
> hideAbbrevDots=false as one), the ability to set the non-default value of
> a Boolean variable in this way trumps my taste.

By the way, should it be in [format] section? I thought that format.*
variables are about git-format-patch... Wouldn't it be better to put it
in [log] section?

On the other hand we have already format.pretty (but log.date and
log.showroot).  Nice consistency ;-(


--
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 13:37   ` Thomas Rast
  2009-02-13 13:47     ` Johannes Schindelin
@ 2009-02-13 19:27     ` Jeff King
  1 sibling, 0 replies; 11+ messages in thread
From: Jeff King @ 2009-02-13 19:27 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Johannes Schindelin, git, Adeodato Simó, Junio C Hamano

On Fri, Feb 13, 2009 at 02:37:39PM +0100, Thomas Rast wrote:

> > How about making this an option, and passing it in rev_opts instead?  This 
> > option could then be defaulted to in git-log, when the user said 
> > --abbrev-commit.
> 
> But the 'git_config(git_log_config, NULL);' that sets the new variable
> to false is only called from cmd_{log,show,whatchanged,reflog}.  I
> should have indicated this in the commit messaged, sorry.

We use that technique elsewhere in git, and personally I am not a fan of
it, as it comes down to setting a global variable. That worked fine when
we had a lot of one-shot programs that read the config, did a defined
piece of work, and then exited.

But more and more we are performing multiple actions in a single run
(especially as many scripts become builtin porcelains), and those
globals are applied to all actions. So a porcelain trying to do a
plumbing-ish thing can run into problems.

I can't recall the exact details, but I remember dealing with something
like this related to external diff. Using an ALLOW_EXTERNAL diffopt
ended up being more reliable, _and_ easier to read and follow in the
code.

Now I don't think this is probably a big problem for this particular
option, but I'd rather not see the technique propagated.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: make --abbrev-commit's ellipsis configurable
  2009-02-13 12:58 [PATCH] log: make --abbrev-commit's ellipsis configurable Thomas Rast
  2009-02-13 13:20 ` Johannes Schindelin
@ 2009-02-13 21:32 ` Junio C Hamano
  2009-02-13 22:10   ` [PATCH] log: do not print ellipses with --abbrev-commit Thomas Rast
  1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2009-02-13 21:32 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Adeodato Simó

Thomas Rast <trast@student.ethz.ch> writes:

> 'git log --abbrev-commit' adds an ellipsis to all commit names that
> were abbreviated.  This is annoying if you want to cut&paste the sha1
> from the terminal, since selecting by word will pick up '...' too.
> (And this cannot be fixed by making '.' a non-word character; in other
> instances, such as the '123457..abcdef0' from git-fetch, it's part of
> the expression.)

I actually think that it is a bug that --abbrev-commit uses ellipses.

It has been there since "diff --abbrev" was first introduced by commit
47dd0d5 (diff: --abbrev option, 2005-12-13).  It did make sense to align
columns in abbreviated raw diff output with ellipses, but it did not make
much sense to do that for commit object names.

Some scripts might be relying on the presense of ellipses but depending on
the object distribution, the length of the ellipses can range from zero to
three; if they are expecting to always see three dots, such scripts are
already broken.

For this reason, I do not think it is necessarily a bad idea to make this
change even unconditionally to both plumbing and Porcelain.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] log: do not print ellipses with --abbrev-commit
  2009-02-13 21:32 ` Junio C Hamano
@ 2009-02-13 22:10   ` Thomas Rast
  2009-02-13 22:24     ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Rast @ 2009-02-13 22:10 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Adeodato Simó, Jakub Narebski, git,
	Johannes Schindelin

'git log --abbrev-commit' added an ellipsis to all commit names that
were abbreviated.  This was particularly annoying if you wanted to
cut&paste the sha1 from the terminal, since selecting by word would
pick up '...'  too.

So use find_unique_abbrev() instead of diff_unique_abbrev() in all
log-related commit sha1 printing routines, and also change the
formatting of the 'Merge: parent1 parent2' line output via
pretty_print_commit().

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > 'git log --abbrev-commit' adds an ellipsis to all commit names that
[...]
> I actually think that it is a bug that --abbrev-commit uses ellipses.
> 
> It has been there since "diff --abbrev" was first introduced by commit
> 47dd0d5 (diff: --abbrev option, 2005-12-13).  It did make sense to align
> columns in abbreviated raw diff output with ellipses, but it did not make
> much sense to do that for commit object names.

Heh.  I was actually more careful (making it customizable) because I
was afraid it would get shot down because people are used to the other
format.  It arguably does make a bit of sense for --pretty=oneline
(and to some extent --graph --pretty=oneline), though for me the
doubleclick word selection by far outweighs the slight chance of
breaking the layout because of a longer unique sha1.

> For this reason, I do not think it is necessarily a bad idea to make this
> change even unconditionally to both plumbing and Porcelain.

That makes for a smaller change, but affects t4013, where a lot of the
tests contain a 'Merge:' line.


 log-tree.c                                         |    8 ++++----
 pretty.c                                           |    4 +---
 ....log_--patch-with-stat_--summary_master_--_dir_ |    2 +-
 t/t4013/diff.log_--patch-with-stat_master          |    2 +-
 t/t4013/diff.log_--patch-with-stat_master_--_dir_  |    2 +-
 ..._--root_--cc_--patch-with-stat_--summary_master |    2 +-
 ...f.log_--root_--patch-with-stat_--summary_master |    2 +-
 t/t4013/diff.log_--root_--patch-with-stat_master   |    2 +-
 ...og_--root_-c_--patch-with-stat_--summary_master |    2 +-
 t/t4013/diff.log_--root_-p_master                  |    2 +-
 t/t4013/diff.log_--root_master                     |    2 +-
 t/t4013/diff.log_-p_master                         |    2 +-
 t/t4013/diff.log_master                            |    2 +-
 t/t4013/diff.show_master                           |    2 +-
 ..._--root_--cc_--patch-with-stat_--summary_master |    2 +-
 ...ed_--root_-c_--patch-with-stat_--summary_master |    2 +-
 16 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 194ddb1..84a74e5 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -48,7 +48,7 @@ static void show_parents(struct commit *commit, int abbrev)
 	struct commit_list *p;
 	for (p = commit->parents; p ; p = p->next) {
 		struct commit *parent = p->item;
-		printf(" %s", diff_unique_abbrev(parent->object.sha1, abbrev));
+		printf(" %s", find_unique_abbrev(parent->object.sha1, abbrev));
 	}
 }
 
@@ -280,7 +280,7 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
+		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit), stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		show_decorations(opt, commit);
@@ -348,13 +348,13 @@ void show_log(struct rev_info *opt)
 					putchar('>');
 			}
 		}
-		fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit),
+		fputs(find_unique_abbrev(commit->object.sha1, abbrev_commit),
 		      stdout);
 		if (opt->print_parents)
 			show_parents(commit, abbrev_commit);
 		if (parent)
 			printf(" (from %s)",
-			       diff_unique_abbrev(parent->object.sha1,
+			       find_unique_abbrev(parent->object.sha1,
 						  abbrev_commit));
 		show_decorations(opt, commit);
 		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
diff --git a/pretty.c b/pretty.c
index cc460b5..428fbb6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -210,15 +210,13 @@ static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
 	while (parent) {
 		struct commit *p = parent->item;
 		const char *hex = NULL;
-		const char *dots;
 		if (abbrev)
 			hex = find_unique_abbrev(p->object.sha1, abbrev);
 		if (!hex)
 			hex = sha1_to_hex(p->object.sha1);
-		dots = (abbrev && strlen(hex) != 40) ?  "..." : "";
 		parent = parent->next;
 
-		strbuf_addf(sb, " %s%s", hex, dots);
+		strbuf_addf(sb, " %s", hex);
 	}
 	strbuf_addch(sb, '\n');
 }
diff --git a/t/t4013/diff.log_--patch-with-stat_--summary_master_--_dir_ b/t/t4013/diff.log_--patch-with-stat_--summary_master_--_dir_
index 3ceb8e7..bd7f5c0 100644
--- a/t/t4013/diff.log_--patch-with-stat_--summary_master_--_dir_
+++ b/t/t4013/diff.log_--patch-with-stat_--summary_master_--_dir_
@@ -1,6 +1,6 @@
 $ git log --patch-with-stat --summary master -- dir/
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--patch-with-stat_master b/t/t4013/diff.log_--patch-with-stat_master
index 43d7776..14595a6 100644
--- a/t/t4013/diff.log_--patch-with-stat_master
+++ b/t/t4013/diff.log_--patch-with-stat_master
@@ -1,6 +1,6 @@
 $ git log --patch-with-stat master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--patch-with-stat_master_--_dir_ b/t/t4013/diff.log_--patch-with-stat_master_--_dir_
index 5187a26..5a4e727 100644
--- a/t/t4013/diff.log_--patch-with-stat_master_--_dir_
+++ b/t/t4013/diff.log_--patch-with-stat_master_--_dir_
@@ -1,6 +1,6 @@
 $ git log --patch-with-stat master -- dir/
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_--cc_--patch-with-stat_--summary_master b/t/t4013/diff.log_--root_--cc_--patch-with-stat_--summary_master
index c964097..df0aaa9 100644
--- a/t/t4013/diff.log_--root_--cc_--patch-with-stat_--summary_master
+++ b/t/t4013/diff.log_--root_--cc_--patch-with-stat_--summary_master
@@ -1,6 +1,6 @@
 $ git log --root --cc --patch-with-stat --summary master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_--patch-with-stat_--summary_master b/t/t4013/diff.log_--root_--patch-with-stat_--summary_master
index ad050af..c11b5f2 100644
--- a/t/t4013/diff.log_--root_--patch-with-stat_--summary_master
+++ b/t/t4013/diff.log_--root_--patch-with-stat_--summary_master
@@ -1,6 +1,6 @@
 $ git log --root --patch-with-stat --summary master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_--patch-with-stat_master b/t/t4013/diff.log_--root_--patch-with-stat_master
index 628c6c0..5f0c98f 100644
--- a/t/t4013/diff.log_--root_--patch-with-stat_master
+++ b/t/t4013/diff.log_--root_--patch-with-stat_master
@@ -1,6 +1,6 @@
 $ git log --root --patch-with-stat master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_-c_--patch-with-stat_--summary_master b/t/t4013/diff.log_--root_-c_--patch-with-stat_--summary_master
index 5d4e0f1..e62c368 100644
--- a/t/t4013/diff.log_--root_-c_--patch-with-stat_--summary_master
+++ b/t/t4013/diff.log_--root_-c_--patch-with-stat_--summary_master
@@ -1,6 +1,6 @@
 $ git log --root -c --patch-with-stat --summary master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_-p_master b/t/t4013/diff.log_--root_-p_master
index 217a2eb..b42c334 100644
--- a/t/t4013/diff.log_--root_-p_master
+++ b/t/t4013/diff.log_--root_-p_master
@@ -1,6 +1,6 @@
 $ git log --root -p master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_--root_master b/t/t4013/diff.log_--root_master
index e17ccfc..e8f4615 100644
--- a/t/t4013/diff.log_--root_master
+++ b/t/t4013/diff.log_--root_master
@@ -1,6 +1,6 @@
 $ git log --root master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_-p_master b/t/t4013/diff.log_-p_master
index f8fefef..bf1326d 100644
--- a/t/t4013/diff.log_-p_master
+++ b/t/t4013/diff.log_-p_master
@@ -1,6 +1,6 @@
 $ git log -p master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.log_master b/t/t4013/diff.log_master
index e9d9e7b..a8f6ce5 100644
--- a/t/t4013/diff.log_master
+++ b/t/t4013/diff.log_master
@@ -1,6 +1,6 @@
 $ git log master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.show_master b/t/t4013/diff.show_master
index 9e6e1f2..fb08ce0 100644
--- a/t/t4013/diff.show_master
+++ b/t/t4013/diff.show_master
@@ -1,6 +1,6 @@
 $ git show master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.whatchanged_--root_--cc_--patch-with-stat_--summary_master b/t/t4013/diff.whatchanged_--root_--cc_--patch-with-stat_--summary_master
index 5facf25..e96ff1f 100644
--- a/t/t4013/diff.whatchanged_--root_--cc_--patch-with-stat_--summary_master
+++ b/t/t4013/diff.whatchanged_--root_--cc_--patch-with-stat_--summary_master
@@ -1,6 +1,6 @@
 $ git whatchanged --root --cc --patch-with-stat --summary master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
diff --git a/t/t4013/diff.whatchanged_--root_-c_--patch-with-stat_--summary_master b/t/t4013/diff.whatchanged_--root_-c_--patch-with-stat_--summary_master
index 10f6767..c0aff68 100644
--- a/t/t4013/diff.whatchanged_--root_-c_--patch-with-stat_--summary_master
+++ b/t/t4013/diff.whatchanged_--root_-c_--patch-with-stat_--summary_master
@@ -1,6 +1,6 @@
 $ git whatchanged --root -c --patch-with-stat --summary master
 commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
-Merge: 9a6d494... c7a2ab9...
+Merge: 9a6d494 c7a2ab9
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:04:00 2006 +0000
 
-- 
1.6.2.rc0.274.g97213

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] log: do not print ellipses with --abbrev-commit
  2009-02-13 22:10   ` [PATCH] log: do not print ellipses with --abbrev-commit Thomas Rast
@ 2009-02-13 22:24     ` Jeff King
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2009-02-13 22:24 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Junio C Hamano, Adeodato Simó, Jakub Narebski, git,
	Johannes Schindelin

On Fri, Feb 13, 2009 at 11:10:41PM +0100, Thomas Rast wrote:

> 'git log --abbrev-commit' added an ellipsis to all commit names that
> were abbreviated.  This was particularly annoying if you wanted to
> cut&paste the sha1 from the terminal, since selecting by word would
> pick up '...'  too.
> 
> So use find_unique_abbrev() instead of diff_unique_abbrev() in all
> log-related commit sha1 printing routines, and also change the
> formatting of the 'Merge: parent1 parent2' line output via
> pretty_print_commit().

I like this. I have always hated the dots, and even started typing extra
--pretty=tformat lines to get rid of them. I always just assumed we
didn't want to change the output, but I think Junio's reasoning that
scripts cannot be relying on this is sane.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2009-02-13 22:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-13 12:58 [PATCH] log: make --abbrev-commit's ellipsis configurable Thomas Rast
2009-02-13 13:20 ` Johannes Schindelin
2009-02-13 13:37   ` Thomas Rast
2009-02-13 13:47     ` Johannes Schindelin
2009-02-13 14:24       ` [PATCH v2] " Thomas Rast
2009-02-13 15:32         ` Johannes Sixt
     [not found]           ` <m363jeux00.fsf@localhost.localdomain>
2009-02-13 16:45             ` Jakub Narebski
2009-02-13 19:27     ` [PATCH] " Jeff King
2009-02-13 21:32 ` Junio C Hamano
2009-02-13 22:10   ` [PATCH] log: do not print ellipses with --abbrev-commit Thomas Rast
2009-02-13 22:24     ` Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).