From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Thu, 09 Apr 2009 10:53:27 GMT
Subject: Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory
Message-ID: <fcaeb9bf0904090353s4ec770bfk3cd3f6559c367a20@mail.gmail.com>
In-Reply-To: <7vws9u2ov4.fsf@gitster.siamese.dyndns.org>

2009/4/9 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> @@ -168,6 +171,22 @@ int write_archive_entries(struct archiver_args *args,
>>       context.args = args;
>>       context.write_entry = write_entry;
>>
>> +     /*
>> +      * Setup index and instruct attr to read index only
>> +      */
>> +     if (!args->worktree_attributes) {
>> +             memset(&opts, 0, sizeof(opts));
>> +             opts.index_only = 1;
>> +             opts.head_idx = -1;
>> +             opts.src_index = &the_index;
>> +             opts.dst_index = &the_index;
>> +             opts.fn = oneway_merge;
>> +             init_tree_desc(&t, args->tree->buffer, args->tree->size);
>> +             if (unpack_trees(1, &t, &opts))
>> +                     return -1;
>> +             git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
>
> Why use unpack_trees with oneway_merge?  You won't be doing "is this file
> up-to-date in the work tree?", and you won't be writing the index out
> either, so there is nothing gained by keeping the cached stat information
> fresh, which is the major justification of using that mechanism.  I think
> using tree.c::read_tree() would be more appropriate.

Because I'm more familiar with unpack stuff than read-tree (or to be
honest, haven't touched tree stuff at all). Will look at read_tree().

>> diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
>> index 0713bca..69a93fc 100644
>> --- a/builtin-tar-tree.c
>> +++ b/builtin-tar-tree.c
>> @@ -36,6 +36,11 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
>>               argv++;
>>               argc--;
>>       }
>> +     if (2 <= argc && !strcmp(argv[1], "--fix-attributes")) {
>> +             nargv[nargc++] = argv[1];
>> +             argv++;
>> +             argc--;
>> +     }
>
> I am not sure if it is worth backporting this new option to tar-tree which
> is an essentially backward-compatibility interface, and worse yet, doing
> it poorly (i.e. --fix-attributes must come after --remote= for unexplained
> reason).

It's because git-tar-tree is used in tests and I don't want to migrate
all to git-archive. I don't want to change too much in a deprecated
command. Maybe just remove the option and make --fix-attributes
default for git-tar-tree. In other words, keep git-tar-tree's current
behaviour.

> It would affect a bit more tests, but I think you would want to test both
> the new "normal" mode of operation (generate archives with "git archive"
> and "git tar-tree" without options and compare, for example), instead of
> adding --fix-attributes everywhere.

There is a new test to test the new "normal" mode. I'll think of more.
-- 
Duy