Re: [PATCH v6] Add new git-related helper to contrib
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 23, 2013, 16:54 UTC
- Message-ID
- <7v8v35hc11.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAMP44s18ps9JtY7+mvXVbuZu3cm7bHnK1vCzMWqBdvawLr+zzA@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 20 quoted lines
> On Wed, May 22, 2013 at 11:07 PM, Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> On Wed, May 22, 2013 at 7:08 PM, Junio C Hamano <gitster@pobox.com> wrote: >>> Felipe Contreras <felipe.contreras@gmail.com> writes: >>> >>>>> IIRC, git-gui runs two blames, one without any -C and one with (I do >>>>> not offhand recall how many -C it uses) to show both. >>>> >>>> 'git blame' is a very expensive operation, perhaps we should add >>>> another option so users don't need to run two blames to find this. >>> >>> Yes, you could add a "struct origin *suspect_in_the_same_file" next >>> to the current "struct origin *suspect" to the blame_entry in order >>> to represent the origin you would get if you were to stop at a "move >>> across files" event, and keep digging, when you are operating under >>> "-C -C" or higher mode. >> >> No, this is what I meant: > > But that would not print the right line numbers, so perhaps:
Users of full-output may want to be able to see the same thing.
I have a suspicion that you misread what assign_blame() does. The first inner loop to find one "suspect" is really what it says. It loops over blame entries in the scoreboard but it is not about finding one "entry", but is to find one "suspect". The "ent" found in the loop that you store in tmp_ent is no more special than any other "ent" that shares the same "suspect" as its origin.
Imagine that your scoreboard originally has three blocks of text (i.e. blame_entry) A, B, and C, and the current suspect for A and C are the same, while we already know what the final guilty party for B is (which may be some descendant of the "suspect").
Once we find one "suspect" in the first inner loop, the call to pass_blame() does everything it can do to exonerate that "suspect".
It runs a single diff between the suspect and each of its parents to find lines in both A and C that can be blamed to one of these parents, and blame entries A and C are split into pieces, some of which still have the same suspect as their origin, which are where their lines originate from, while others are attributable to these parents or their ancestors.
If you keep the original entry for A to do something special, like printing what the original range of A was before it was split by pass_blame(), wouldn't you need to do the same for C? We will not be visiting C later in the outer while(1) loop, as a single call to pass_blame() for "suspect" we did when we found it in A has already taken care of it.
I am not sure what you are trying to achieve with that found-guilty logic, even if the "save away in tmp_ent" logic is changed to keep all the blame entries that have "suspect" we are looking at as their origin. When the current suspect is found to have passed all lines intact from its parents, we will see found-guilty left to be false.
How would it make the original blame_entry (perhaps halfway) guilty in that situation?
Show 85 quoted lines
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1500,25 +1500,16 @@ static int emit_one_suspect_detail(struct
> origin *suspect, int repeat)
> return 1;
> }
>
> -/*
> - * The blame_entry is found to be guilty for the range. Mark it
> - * as such, and show it in incremental output.
> - */
> -static void found_guilty_entry(struct blame_entry *ent)
> +static void print_guilty_entry(struct blame_entry *ent)
> {
> - if (ent->guilty)
> - return;
> - ent->guilty = 1;
> - if (incremental) {
> - struct origin *suspect = ent->suspect;
> -
> - printf("%s %d %d %d\n",
> - sha1_to_hex(suspect->commit->object.sha1),
> - ent->s_lno + 1, ent->lno + 1, ent->num_lines);
> - emit_one_suspect_detail(suspect, 0);
> - write_filename_info(suspect->path);
> - maybe_flush_or_die(stdout, "stdout");
> - }
> + struct origin *suspect = ent->suspect;
> +
> + printf("%s %d %d %d\n",
> + sha1_to_hex(suspect->commit->object.sha1),
> + ent->s_lno + 1, ent->lno + 1, ent->num_lines);
> + emit_one_suspect_detail(suspect, 0);
> + write_filename_info(suspect->path);
> + maybe_flush_or_die(stdout, "stdout");
> }
>
> /*
> @@ -1531,14 +1522,19 @@ static void assign_blame(struct scoreboard *sb, int opt)
> struct rev_info *revs = sb->revs;
>
> while (1) {
> - struct blame_entry *ent;
> + struct blame_entry *ent, tmp_ent;
> struct commit *commit;
> struct origin *suspect = NULL;
> + int found_guilty = 0;
>
> /* find one suspect to break down */
> - for (ent = sb->ent; !suspect && ent; ent = ent->next)
> - if (!ent->guilty)
> + for (ent = sb->ent; ent; ent = ent->next)
> + if (!ent->guilty) {
> + tmp_ent = *ent;
> suspect = ent->suspect;
> + break;
> + }
> +
> if (!suspect)
> return; /* all done */
>
> @@ -1564,9 +1560,20 @@ static void assign_blame(struct scoreboard *sb, int opt)
> commit->object.flags |= UNINTERESTING;
>
> /* Take responsibility for the remaining entries */
> - for (ent = sb->ent; ent; ent = ent->next)
> - if (same_suspect(ent->suspect, suspect))
> - found_guilty_entry(ent);
> + for (ent = sb->ent; ent; ent = ent->next) {
> + if (same_suspect(ent->suspect, suspect)) {
> + if (ent->guilty)
> + continue;
> + found_guilty = ent->guilty = 1;
> + if (incremental)
> + print_guilty_entry(ent);
> + }
> + }
> +
> + if (incremental && !found_guilty &&
> + !is_null_sha1(suspect->commit->object.sha1))
> + print_guilty_entry(&tmp_ent);
> +
> origin_decref(suspect);
>
> if (DEBUG) /* sanity */