Re: [PATCH v4 09/24] ls-files.c: use index api

2013-11-30 Thread Duy Nguyen
On Wed, Nov 27, 2013 at 7:00 PM, Thomas Gummerer t.gumme...@gmail.com wrote:
 @@ -447,6 +463,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 struct dir_struct dir;
 struct exclude_list *el;
 struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 +   struct filter_opts *opts = xmalloc(sizeof(*opts));
 struct option builtin_ls_files_options[] = {
 { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 N_(paths are separated with NUL character),
 @@ -512,9 +529,6 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 prefix_len = strlen(prefix);
 git_config(git_default_config, NULL);

 -   if (read_cache()  0)
 -   die(index file corrupt);
 -
 argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 ls_files_usage, 0);
 el = add_exclude_list(dir, EXC_CMDL, --exclude option);
 @@ -550,6 +564,24 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);

 +   if (!with_tree  !needs_trailing_slash_stripped()) {
 +   memset(opts, 0, sizeof(*opts));
 +   opts-pathspec = pathspec;
 +   opts-read_staged = 1;
 +   if (show_resolve_undo)
 +   opts-read_resolve_undo = 1;
 +   if (read_cache_filtered(opts)  0)
 +   die(index file corrupt);
 +   } else {
 +   if (read_cache()  0)
 +   die(index file corrupt);
 +   parse_pathspec(pathspec, 0,
 +  PATHSPEC_PREFER_CWD |
 +  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 +  prefix, argv);

So we ran parse_pathspec() once (not shown in the context), if found
trailing slashes, we read full cache and rerun parse_pathspec()
because the index is not loaded on the first run.

This is fine. Just a note for future improvement: as _SLASH_CHEAP only
needs to look at a few entries with cache_name_pos(), we could take
advantage of v5 to peek individual entries (or in v2, load full cache
first). Nothing needs to be done now, I think we have not decided
whether to combine _SLASH_CHEAP and _SLASH_EXPENSIVE into one.

 +   }
 +
 /* Find common prefix for all pathspec's */
 max_prefix = common_prefix(pathspec);
 max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 09/24] ls-files.c: use index api

2013-11-30 Thread Thomas Gummerer
Duy Nguyen pclo...@gmail.com writes:

 On Wed, Nov 27, 2013 at 7:00 PM, Thomas Gummerer t.gumme...@gmail.com wrote:
 @@ -447,6 +463,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 struct dir_struct dir;
 struct exclude_list *el;
 struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 +   struct filter_opts *opts = xmalloc(sizeof(*opts));
 struct option builtin_ls_files_options[] = {
 { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 N_(paths are separated with NUL character),
 @@ -512,9 +529,6 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 prefix_len = strlen(prefix);
 git_config(git_default_config, NULL);

 -   if (read_cache()  0)
 -   die(index file corrupt);
 -
 argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 ls_files_usage, 0);
 el = add_exclude_list(dir, EXC_CMDL, --exclude option);
 @@ -550,6 +564,24 @@ int cmd_ls_files(int argc, const char **argv, const 
 char *cmd_prefix)
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);

 +   if (!with_tree  !needs_trailing_slash_stripped()) {
 +   memset(opts, 0, sizeof(*opts));
 +   opts-pathspec = pathspec;
 +   opts-read_staged = 1;
 +   if (show_resolve_undo)
 +   opts-read_resolve_undo = 1;
 +   if (read_cache_filtered(opts)  0)
 +   die(index file corrupt);
 +   } else {
 +   if (read_cache()  0)
 +   die(index file corrupt);
 +   parse_pathspec(pathspec, 0,
 +  PATHSPEC_PREFER_CWD |
 +  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 +  prefix, argv);

 So we ran parse_pathspec() once (not shown in the context), if found
 trailing slashes, we read full cache and rerun parse_pathspec()
 because the index is not loaded on the first run.

 This is fine. Just a note for future improvement: as _SLASH_CHEAP only
 needs to look at a few entries with cache_name_pos(), we could take
 advantage of v5 to peek individual entries (or in v2, load full cache
 first). Nothing needs to be done now, I think we have not decided
 whether to combine _SLASH_CHEAP and _SLASH_EXPENSIVE into one.

Yes that makes sense.  Adding the ability to search for path entries
without reading the whole or part of the index was something I was
thinking about, but didn't have time to do so yet.  I'll add this to my
list of possible future improvements.

 +   }
 +
 /* Find common prefix for all pathspec's */
 max_prefix = common_prefix(pathspec);
 max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 -- 
 Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 09/24] ls-files.c: use index api

2013-11-30 Thread Antoine Pelisse
On Wed, Nov 27, 2013 at 1:00 PM, Thomas Gummerer t.gumme...@gmail.com wrote:
 @@ -447,6 +463,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 struct dir_struct dir;
 struct exclude_list *el;
 struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 +   struct filter_opts *opts = xmalloc(sizeof(*opts));
 struct option builtin_ls_files_options[] = {
 { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 N_(paths are separated with NUL character),
 @@ -512,9 +529,6 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 prefix_len = strlen(prefix);
 git_config(git_default_config, NULL);

 -   if (read_cache()  0)
 -   die(index file corrupt);
 -
 argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 ls_files_usage, 0);
 el = add_exclude_list(dir, EXC_CMDL, --exclude option);
 @@ -550,6 +564,24 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);

 +   if (!with_tree  !needs_trailing_slash_stripped()) {
 +   memset(opts, 0, sizeof(*opts));
 +   opts-pathspec = pathspec;
 +   opts-read_staged = 1;
 +   if (show_resolve_undo)
 +   opts-read_resolve_undo = 1;
 +   if (read_cache_filtered(opts)  0)
 +   die(index file corrupt);
 +   } else {
 +   if (read_cache()  0)
 +   die(index file corrupt);
 +   parse_pathspec(pathspec, 0,
 +  PATHSPEC_PREFER_CWD |
 +  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 +  prefix, argv);
 +
 +   }
 +

Would it make sense to move the declaration of opts as a non-pointer
to the block where it's used ?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 09/24] ls-files.c: use index api

2013-11-30 Thread Thomas Gummerer
Antoine Pelisse apeli...@gmail.com writes:

 On Wed, Nov 27, 2013 at 1:00 PM, Thomas Gummerer t.gumme...@gmail.com wrote:
 @@ -447,6 +463,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 struct dir_struct dir;
 struct exclude_list *el;
 struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 +   struct filter_opts *opts = xmalloc(sizeof(*opts));
 struct option builtin_ls_files_options[] = {
 { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
 N_(paths are separated with NUL character),
 @@ -512,9 +529,6 @@ int cmd_ls_files(int argc, const char **argv, const char 
 *cmd_prefix)
 prefix_len = strlen(prefix);
 git_config(git_default_config, NULL);

 -   if (read_cache()  0)
 -   die(index file corrupt);
 -
 argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
 ls_files_usage, 0);
 el = add_exclude_list(dir, EXC_CMDL, --exclude option);
 @@ -550,6 +564,24 @@ int cmd_ls_files(int argc, const char **argv, const 
 char *cmd_prefix)
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);

 +   if (!with_tree  !needs_trailing_slash_stripped()) {
 +   memset(opts, 0, sizeof(*opts));
 +   opts-pathspec = pathspec;
 +   opts-read_staged = 1;
 +   if (show_resolve_undo)
 +   opts-read_resolve_undo = 1;
 +   if (read_cache_filtered(opts)  0)
 +   die(index file corrupt);
 +   } else {
 +   if (read_cache()  0)
 +   die(index file corrupt);
 +   parse_pathspec(pathspec, 0,
 +  PATHSPEC_PREFER_CWD |
 +  PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 +  prefix, argv);
 +
 +   }
 +

 Would it make sense to move the declaration of opts as a non-pointer
 to the block where it's used ?

Yes, I think that would make sense, will do so in the re-roll. Thanks!
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html