Re: [PATCH 2/3] add--interactive: add builtin helper for interactive add

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 6, 2017 at 1:13 AM, Daniel Ferreira (theiostream)
<bnmvco@xxxxxxxxx> wrote:
> On Fri, May 5, 2017 at 7:30 PM, Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
>>> +static int git_add_interactive_config(const char *var,
>>
>> Not git_add_interactive__helper_config()? ;-)
>
> I don't get if you mean this ironically (because of the verbosity) or
> if you do think this would be a good name ;P
>
>>> +     for (i = 0; i < q->nr; i++) {
>>> +             struct diff_filepair *p;
>>> +             p = q->queue[i];
>>> +             diff_flush_stat(p, options, &stat);
>>> +     }
>>> +
>>> +     for (i = 0; i < stat.nr; i++) {
>>> +             int file_index = s->file_count;
>>> +             for (j = 0; j < s->file_count; j++) {
>>> +                     if (!strcmp(s->files[j].path, stat.files[i]->name)) {
>>> +                             file_index = j;
>>> +                             break;
>>> +                     }
>>> +             }
>>
>> So basically, this is looking up in a list whether we saw the file in
>> question already, and the reason we have to do that is that we run the
>> entire shebang twice, once with the worktree and once with the index.
>>
>> I wonder whether it would not make sense to switch away s->files from a
>> list to a hashmap.
>> [...]
>> BTW in the first pass, we pretty much know that we only get unique names,
>> so the entire lookup is unnecessary and will just increase the time
>> complexity from O(n) to O(n^2). So let's avoid that.
>>
>> By moving to a hashmap, you can even get the second phase down to an
>> expected O(n).
>
> How would you go about implementing that hashmap (i.e. what should be
> the hash)? Does Git have any interface for it, or is there any example
> I can look after in the codebase?

Git has a hashmap API already. 7d4558c462 is a good example of using it.

>>> +     printf(ADD_INTERACTIVE_HEADER_INDENT);
>>> +     color_fprintf(stdout, header_color, modified_fmt, _("staged"),
>>> +                     _("unstaged"), _("path"));
>>
>> I think these _() need to become N_().
>
> I cannot find any call to N_() outside of Perl code. What should that
> even do differently?

N_() is to mark a string for later translation, not to return it. See
my "how to use getopt" patch for an example of that, but this doesn't
look like such a case, since _() is returning the string to
color_fprintf, surely...

    $ git grep -P '\bN_\(' -- '*.c'



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]