{"thread":{"id":"40853","subject":"[PATCH v1] blame: add support for --[no-]progress option","startedAt":"2015-11-22T05:11:56Z","lastAt":"2015-11-22T15:56:14Z","messageCount":4,"participants":["Edmundo Carmona Antoranz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273583","messageId":"1448169116-32335-1-git-send-email-eantoranz@gmail.com","threadId":"40853","inReplyTo":null,"subject":"[PATCH v1] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T05:11:56Z","receivedAt":"2015-11-22T05:11:56Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Will also affect annotate\n\nSigned-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>\n---\n Documentation/blame-options.txt |  7 +++++++\n Documentation/git-blame.txt     |  9 ++++++++-\n builtin/blame.c                 | 39 ++++++++++++++++++++++++++++++++++++---\n 3 files changed, 51 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 760eab7..43f4f08 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -69,6 +69,13 @@ include::line-range-format.txt[]\n \tiso format is used. For supported values, see the discussion\n \tof the --date option at linkgit:git-log[1].\n \n+--[no-]progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal. This flag\n+\tenables progress reporting even if not attached to a\n+\tterminal.\n+\n+\n -M|<num>|::\n \tDetect moved or copied lines within a file. When a commit\n \tmoves or copies a block of lines (e.g. the original file\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex e6e947c..2e63397 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -10,7 +10,8 @@ SYNOPSIS\n [verse]\n 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]\n \t    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]\n-\t    [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>] [--] <file>\n+\t    [--[no-]progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>]\n+\t    [--] <file>\n \n DESCRIPTION\n -----------\n@@ -88,6 +89,12 @@ include::blame-options.txt[]\n \tabbreviated object name, use <n>+1 digits. Note that 1 column\n \tis used for a caret to mark the boundary commit.\n \n+--[no-]progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal. This flag\n+\tenables progress reporting even if not attached to a\n+\tterminal.\n+\n \n THE PORCELAIN FORMAT\n --------------------\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 83612f5..31477d8 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -28,6 +28,7 @@\n #include \"line-range.h\"\n #include \"line-log.h\"\n #include \"dir.h\"\n+#include \"progress.h\"\n \n static char blame_usage[] = N_(\"git blame [<options>] [<rev-opts>] [<rev>] [--] <file>\");\n \n@@ -50,6 +51,7 @@ static int incremental;\n static int xdl_opts;\n static int abbrev = -1;\n static int no_whole_file_rename;\n+static int show_progress;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -1760,14 +1762,28 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t}\n }\n \n+static int count_blamed_lines(struct scoreboard *sb)\n+{\n+\tint counter = 0;\n+\tfor (struct blame_entry * entry = sb->ent; entry; entry = entry->next)\n+\t\tcounter += entry->num_lines;\n+\treturn counter;\n+}\n+\n /*\n  * The main loop -- while we have blobs with lines whose true origin\n  * is still unknown, pick one blob, and allow its lines to pass blames\n  * to its parents. */\n-static void assign_blame(struct scoreboard *sb, int opt)\n+static void assign_blame(struct scoreboard *sb, int opt, int show_progress)\n {\n \tstruct rev_info *revs = sb->revs;\n \tstruct commit *commit = prio_queue_get(&sb->commits);\n+\tstruct progress * progress = NULL;\n+\tint blamed_lines = -1;\n+\n+\tif (show_progress) {\n+\t\tprogress = start_progress(_(\"Blaming lines\"), sb->num_lines);\n+\t}\n \n \twhile (commit) {\n \t\tstruct blame_entry *ent;\n@@ -1822,9 +1838,21 @@ static void assign_blame(struct scoreboard *sb, int opt)\n \t\t}\n \t\torigin_decref(suspect);\n \n+\t\tif (progress) {\n+\t\t\tint current_blamed_lines = count_blamed_lines(sb);\n+\t\t\tif (current_blamed_lines > blamed_lines) {\n+\t\t\t\tblamed_lines = current_blamed_lines;\n+\t\t\t\tdisplay_progress(progress, blamed_lines);\n+\t\t\t}\n+\t\t}\n+\n \t\tif (DEBUG) /* sanity */\n \t\t\tsanity_check_refcnt(sb);\n \t}\n+\n+\tif (progress) {\n+\t\tstop_progress(&progress);\n+\t}\n }\n \n static const char *format_time(unsigned long time, const char *tz_str,\n@@ -2520,6 +2548,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('b', NULL, &blank_boundary, N_(\"Show blank SHA-1 for boundary commits (Default: off)\")),\n \t\tOPT_BOOL(0, \"root\", &show_root, N_(\"Do not treat root commits as boundaries (Default: off)\")),\n \t\tOPT_BOOL(0, \"show-stats\", &show_stats, N_(\"Show work cost statistics\")),\n+\t\tOPT_BOOL(0, \"progress\", &show_progress, N_(\"Force progress reporting\")),\n \t\tOPT_BIT(0, \"score-debug\", &output_option, N_(\"Show output score for blame entries\"), OUTPUT_SHOW_SCORE),\n \t\tOPT_BIT('f', \"show-name\", &output_option, N_(\"Show original filename (Default: auto)\"), OUTPUT_SHOW_NAME),\n \t\tOPT_BIT('n', \"show-number\", &output_option, N_(\"Show original linenumber (Default: off)\"), OUTPUT_SHOW_NUMBER),\n@@ -2555,6 +2584,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n+\tshow_progress = -1;\n \n \tparse_options_start(&ctx, argc, argv, prefix, options,\n \t\t\t    PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);\n@@ -2578,6 +2608,9 @@ parse_done:\n \tno_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);\n \tDIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);\n \targc = parse_options_end(&ctx);\n+\tif (show_progress < 0) {\n+\t\tshow_progress = isatty(2);\n+\t}\n \n \tif (0 < abbrev)\n \t\t/* one more abbrev length is needed for the boundary commit */\n@@ -2830,11 +2863,11 @@ parse_done:\n \n \tread_mailmap(&mailmap, NULL);\n \n+\tassign_blame(&sb, opt, show_progress);\n+\n \tif (!incremental)\n \t\tsetup_pager();\n \n-\tassign_blame(&sb, opt);\n-\n \tfree(final_commit_name);\n \n \tif (incremental)\n-- \n2.6.2\n"},{"id":"273584","messageId":"CAOc6etbfzrqT4-5dorCAq_=5Oh9wXV4LgRwK9hrEqjmSYHiBEg@mail.gmail.com","threadId":"40853","inReplyTo":"1448169116-32335-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v1] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T05:27:54Z","receivedAt":"2015-11-22T05:27:54Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Hey, guys!\n\nTime for some more patch destruction.\n\nOn Sat, Nov 21, 2015 at 11:11 PM, Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n-static void assign_blame(struct scoreboard *sb, int opt)\n+static void assign_blame(struct scoreboard *sb, int opt, int show_progress)\n\nWould it be better to include show_progress as a binary flag in opt\ninstead of a separate variable?\n\n> +       struct progress * progress = NULL;\n> +       int blamed_lines = -1;\n\nI'm wondering if it would be better to save the 'last number used in\nprogress' inside struct progress so that we could skip blamed_lines.\nThat would also allow progress itself (as in the API) to avoid\nprinting duplicated progress values. In my case, I'm going through a\nnumber of cycles where _I think_ I might have the same number of\nblamed lines detected. To avoid asking to do a new progress print out\nwith the same value, I'm checking the value I had used last time. If\nprogress took care of that check up, this variable would go for good\nand I would just ask progress to print with the current value (no\nmatter if it's duplicate or an increased value).\n\n> +\n> +       if (show_progress) {\n> +               progress = start_progress(_(\"Blaming lines\"), sb->num_lines);\n> +       }\n\nAlready noticed I can get rid of the curly brackets.\n\n>\n>         while (commit) {\n>                 struct blame_entry *ent;\n> @@ -1822,9 +1838,21 @@ static void assign_blame(struct scoreboard *sb, int opt)\n>                 }\n>                 origin_decref(suspect);\n>\n> +               if (progress) {\n> +                       int current_blamed_lines = count_blamed_lines(sb);\n> +                       if (current_blamed_lines > blamed_lines) {\n> +                               blamed_lines = current_blamed_lines;\n> +                               display_progress(progress, blamed_lines);\n> +                       }\n> +               }\n> +\n>                 if (DEBUG) /* sanity */\n>                         sanity_check_refcnt(sb);\n>         }\n> +\n> +       if (progress) {\n> +               stop_progress(&progress);\n> +       }\n>  }\n>\n\nIt first I tried to detect how many revisions where going to be\nchecked so that I could report progress on them but I found extracting\ninformation from scoreboard a little tricky (to be read: I could not\nextract anything out of it). Then I though of counting lines and it\nworks so.... (same thing with the curly brackets).\n\n\n> +       assign_blame(&sb, opt, show_progress);\n> +\n>         if (!incremental)\n>                 setup_pager();\n>\n> -       assign_blame(&sb, opt);\n> -\n\nsetup_pager() was breaking progress so I moved it _after_\nassign_blame() and it seems to behave. Hope that's not a problem.\n\nLooking forward to your feedback.\n"},{"id":"273586","messageId":"CAOc6etbnOCDPv+25dEgYGajCSbpixMu_ahhsWM7EOCcqYpamUg@mail.gmail.com","threadId":"40853","inReplyTo":"CAOc6etbfzrqT4-5dorCAq_=5Oh9wXV4LgRwK9hrEqjmSYHiBEg@mail.gmail.com","subject":"Re: [PATCH v1] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T09:07:06Z","receivedAt":"2015-11-22T09:07:06Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"I think I found where to calculate the number of blamed lines without\nhaving to do it from scratch every cycle. It's where the list of\nsb->ent is getting its nodes pointed.... around here:\n\nfor (;;) {\n    struct blame_entry *next = ent->next;\n    found_guilty_entry(ent);\n    if (next) {\n        ent = next;\n        continue;\n    }\n    ent->next = sb->ent;\n    sb->ent = suspect->suspects;\n    suspect->suspects = NULL;\n    break;\n}\n\nSo the function I added to blame.c, it's gone.\n"},{"id":"273588","messageId":"CAOc6etYXHjGRT7dBsP835VntTQL2NyQnYTQXwbzreBR-5sSiSQ@mail.gmail.com","threadId":"40853","inReplyTo":"CAOc6etbfzrqT4-5dorCAq_=5Oh9wXV4LgRwK9hrEqjmSYHiBEg@mail.gmail.com","subject":"Re: [PATCH v1] blame: add support for --[no-]progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-22T15:56:14Z","receivedAt":"2015-11-22T15:56:14Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sat, Nov 21, 2015 at 11:27 PM, Edmundo Carmona Antoranz\n<eantoranz@gmail.com> wrote:\n> -static void assign_blame(struct scoreboard *sb, int opt)\n> +static void assign_blame(struct scoreboard *sb, int opt, int show_progress)\n>\n> Would it be better to include show_progress as a binary flag in opt\n> instead of a separate variable?\n\nWhat is the point of having 'global' variable and not using it as such?\n\nI'm sending a second version of the patch where I'm taking care of\nmost of my previous comments. Let's hope that one holds water.\n\nThanks in advance.\n"}]}