* [PATCH 1/2] blame: add --abbrev command line option
@ 2011-04-01 1:54 Namhyung Kim
2011-04-01 1:54 ` [PATCH 2/2] blame: honor core.abbrev Namhyung Kim
0 siblings, 1 reply; 4+ messages in thread
From: Namhyung Kim @ 2011-04-01 1:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
builtin/blame.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index f6b03f7..253b480 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -41,6 +41,7 @@ static int reverse;
static int blank_boundary;
static int incremental;
static int xdl_opts;
+static int abbrev = 8;
static enum date_mode blame_date_mode = DATE_ISO8601;
static size_t blame_date_width;
@@ -1670,7 +1671,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
cp = nth_line(sb, ent->lno);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
char ch;
- int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? 40 : 8;
+ int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? 40 : abbrev;
if (suspect->commit->object.flags & UNINTERESTING) {
if (blank_boundary)
@@ -2310,6 +2311,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
+ OPT__ABBREV(&abbrev),
OPT_END()
};
--
1.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] blame: honor core.abbrev
2011-04-01 1:54 [PATCH 1/2] blame: add --abbrev command line option Namhyung Kim
@ 2011-04-01 1:54 ` Namhyung Kim
2011-04-01 22:44 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Namhyung Kim @ 2011-04-01 1:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
If user sets config.abbrev option, use it as if --abbrev was given.
We can't set abbrev to default_abbrev unconditionally IMHO, because
historically default abbrev length of the blame command is 8 and
DEFAULT_ABBREV is 7.
As you may see, this is the default value and user can use different
abbrev length by specifing --abbrev=N command line option.
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
builtin/blame.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 253b480..93693d2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2317,12 +2317,16 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
struct parse_opt_ctx_t ctx;
int cmd_is_annotate = !strcmp(argv[0], "annotate");
+ int orig_abbrev = DEFAULT_ABBREV;
git_config(git_blame_config, NULL);
init_revisions(&revs, NULL);
revs.date_mode = blame_date_mode;
DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
+ /* if user sets config.abbrev, honor it */
+ if (orig_abbrev != default_abbrev)
+ abbrev = default_abbrev;
save_commit_buffer = 0;
dashdash_pos = 0;
--
1.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] blame: honor core.abbrev
2011-04-01 1:54 ` [PATCH 2/2] blame: honor core.abbrev Namhyung Kim
@ 2011-04-01 22:44 ` Junio C Hamano
2011-04-02 1:37 ` Namhyung Kim
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2011-04-01 22:44 UTC (permalink / raw)
To: Namhyung Kim; +Cc: git
Namhyung Kim <namhyung@gmail.com> writes:
> If user sets config.abbrev option, use it as if --abbrev was given.
> We can't set abbrev to default_abbrev unconditionally IMHO, because
> historically default abbrev length of the blame command is 8 and
> DEFAULT_ABBREV is 7.
Isn't the one-letter difference because we sometimes need to show the
boundary commit with a caret at the beginning?
I think the way this patch initializes orig_abbrev using DEFAULT_ABBREV is
wrong (at that point, I don't think you have called git_config() to get
the user config for DEFAULT_ABBREV).
See the patch to describe.c in dce9648 (Make the default abbrev length
configurable, 2010-10-28) for the right way to do this.
- initialize the variable to -1;
- call git_config() to get correct value in DEFAULT_ABBREV;
- call parse_options() to potentially update the variable; then
- if variable is still -1, assign DEFAULT_ABBREV to it.
After all that, add 1 to it to account for the possible boundary caret.
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 253b480..93693d2 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2317,12 +2317,16 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>
> struct parse_opt_ctx_t ctx;
> int cmd_is_annotate = !strcmp(argv[0], "annotate");
> + int orig_abbrev = DEFAULT_ABBREV;
>
> git_config(git_blame_config, NULL);
> init_revisions(&revs, NULL);
> revs.date_mode = blame_date_mode;
> DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>
> + /* if user sets config.abbrev, honor it */
> + if (orig_abbrev != default_abbrev)
> + abbrev = default_abbrev;
> save_commit_buffer = 0;
> dashdash_pos = 0;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] blame: honor core.abbrev
2011-04-01 22:44 ` Junio C Hamano
@ 2011-04-02 1:37 ` Namhyung Kim
0 siblings, 0 replies; 4+ messages in thread
From: Namhyung Kim @ 2011-04-02 1:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hello, Junio-san.
2011-04-01 (금), 15:44 -0700, Junio C Hamano:
> Namhyung Kim <namhyung@gmail.com> writes:
>
> > If user sets config.abbrev option, use it as if --abbrev was given.
> > We can't set abbrev to default_abbrev unconditionally IMHO, because
> > historically default abbrev length of the blame command is 8 and
> > DEFAULT_ABBREV is 7.
>
> Isn't the one-letter difference because we sometimes need to show the
> boundary commit with a caret at the beginning?
>
Yeah, I guessed so.
> I think the way this patch initializes orig_abbrev using DEFAULT_ABBREV is
> wrong (at that point, I don't think you have called git_config() to get
> the user config for DEFAULT_ABBREV).
>
My intention was that I want to check whether the user sets the option
or not and apply the value only if it is set (ig. different from "7").
Please see below.
> See the patch to describe.c in dce9648 (Make the default abbrev length
> configurable, 2010-10-28) for the right way to do this.
>
> - initialize the variable to -1;
> - call git_config() to get correct value in DEFAULT_ABBREV;
> - call parse_options() to potentially update the variable; then
> - if variable is still -1, assign DEFAULT_ABBREV to it.
>
> After all that, add 1 to it to account for the possible boundary caret.
>
I saw the patch already, and, yes, it would be a simple and probably the
right way to do. But it could give a bit of confusion to the users IMHO.
Say, A user specify --abbbrev=10 then he/she might expect 10 hexdigits
in the output but actually [s]he will get 11.
So I decided to use the number as is, not adding 1. And this would be a
constant behavior from the patch 1/2 and all other git command which use
--abbrev option, I guess. But I kept to use 8 in case the user doesn't
give the config or command-line option, just for backward compatibility.
I'm not sure this is really a concern. Maybe we can give them 11 as you
said and document the behavior in the manual page.
Thanks.
--
Regards,
Namhyung Kim
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-02 1:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-01 1:54 [PATCH 1/2] blame: add --abbrev command line option Namhyung Kim
2011-04-01 1:54 ` [PATCH 2/2] blame: honor core.abbrev Namhyung Kim
2011-04-01 22:44 ` Junio C Hamano
2011-04-02 1:37 ` Namhyung Kim
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).