diff: introduce scope option
RFC PATCH 1/1 diff: introduce scope option
From: ZheNing Hu <[email protected]>
When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout specification.
So introduce the --scope option to git diff, which have two
value: "sparse" and "all". "sparse" mean that diff is performed restrict to paths which matching sparse-checkout specification, "all" mean that diff is performed regardless of whether the path meets the sparse-checkout specification. --no-scope is the default option for now.
Add diff.scope={sparse, all} config, which can also have the same capabilities as --scope, and it will be covered by --scope option.
---
[RFC] diff: introduce scope option
In [1], we discovered that users working on different sparse-checkout specification may download unnecessary blobs from each other's specification in collaboration. In [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout specification to elegantly solve this problem above. In [3]: Newren and Derrick Stolee prefer to name the option --scope={sparse, all}.
So this patch is attempt to do this thing on git diff:
v1:
1. add --restrict option to git diff, which restrict diff filespec in sparse-checkout specification. [4] v2.
2. rename --restrict to --scope={sparse, all}, support --no-scope.
3. add config: diff.scope={sparse,all}.
Unresolved work:
1. how to properly pass this --scope={sparse, all} to other commands like git log, git format-patch, etc.
2. how to set the default value of scope for different diff commands.
[1]:
https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/[email protected]/ [3]:
https://lore.kernel.org/git/[email protected]/
[4]:
https://lore.kernel.org/git/[email protected]/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1398
Documentation/config/diff.txt | 12 + Documentation/diff-options.txt | 18 + builtin/diff.c | 4 + diff-lib.c | 36 +- diff-no-index.c | 4 + diff.c | 39 +++ diff.h | 11 + t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++ tree-diff.c | 5 + 9 files changed, 597 insertions(+), 1 deletion(-) create mode 100644 t/t4070-diff-sparse-checkout-scope.sh
Documentation/config/diff.txt +12 -0
@@ -201,6 +201,18 @@ diff.algorithm:: -- + +diff.scope:: + Choose diff scope. The variants are as follows: ++ +-- +`sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`all`;; + Without restriction, diff is performed regardless of whether the path + meets the sparse-checkout specification. +-- ++ + diff.wsErrorHighlight:: Highlight whitespace errors in the `context`, `old` or `new` lines of the diff. Multiple values are separated by comma,
Documentation/diff-options.txt +18 -0
@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifndef::git-format-patch[] +ifndef::git-log[] + +--scope={sparse|all}:: + Choose diff scope. The variants are as follows: ++ +-- +`--sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`--all`;; + Without restriction, diff is performed regardless of whether the path + meets the sparse-checkout specification. +-- ++ + +endif::git-log[] +endif::git-format-patch[] + --stat[=<width>[,<name-width>[,<count>]]]:: Generate a diffstat. By default, as much space as necessary will be used for the filename part, and the rest for the graph
builtin/diff.c +4 -0
@@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt, oideq(old_oid, new_oid) && (old_mode == new_mode))return ; +if (opt->scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (old_path, new_path)) +return ; +if (opt->flags.reverse_diff) {SWAP (old_mode, new_mode);SWAP (old_oid, new_oid);
diff-lib.c +35 -1
@@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,return changed; } +int diff_path_in_sparse_checkout (const char *path) { +if (core_sparse_checkout_cone) +return path_in_cone_mode_sparse_checkout (path, the_repository->index); +else +return path_in_sparse_checkout (path, the_repository->index); +} + +int diff_paths_in_sparse_checkout (const char *one,const char *two) { +if (one == two || !strcmp (one, two)) +return diff_path_in_sparse_checkout (one); +else +return diff_path_in_sparse_checkout (one) && +diff_path_in_sparse_checkout (two); +} + +int run_diff_files (struct rev_info *revs,unsigned int option) {int entries, i;
@@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)if (diff_can_quit_early (&revs->diffopt))break ; +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_path_in_sparse_checkout (ce->name)) +continue ;if (!ce_path_match (istate, ce, &revs->prune_data,NULL ))continue ;
@@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)continue ; } -if (ce_uptodate (ce) ||ce_skip_worktree (ce)) +if (ce_uptodate (ce) || + (revs->diffopt.scope !=DIFF_SCOPE_ALL &&ce_skip_worktree (ce)))continue ;/*
@@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,return ;/* nothing to diff.. */ } +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE ) { +if (idx && tree) { +if (!diff_paths_in_sparse_checkout (idx->name, tree->name)) +return ; + }else if (idx) { +if (!diff_path_in_sparse_checkout (idx->name)) +return ; + }else if (tree) { +if (!diff_path_in_sparse_checkout (tree->name)) +return ; + }else +return ; + } +/* if the entry is not checked out, don't examine work tree */ cached = o->index_only || (idx && ((idx->ce_flags &CE_VALID ) ||ce_skip_worktree (idx)));
diff-no-index.c +4 -0
@@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,fixup_paths (paths, &replacement); +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (paths[0 ], paths[1 ])) + goto out; + revs->diffopt.skip_stat_unmatch =1 ;if (!revs->diffopt.output_format) revs->diffopt.output_format =DIFF_FORMAT_PATCH ;
diff.c +39 -0
@@ -48,6 +48,7 @@ static int diff_interhunk_context_default;static const char *diff_word_regex_cfg;static const char *external_diff_cmd_cfg;static const char *diff_order_file_cfg; +static const char *external_diff_scope_cfg;int diff_auto_refresh_index =1 ;static int diff_mnemonic_prefix;static int diff_no_prefix;
@@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;static struct diff_options default_diff_options;static long diff_algorithm;static unsigned ws_error_highlight_default =WSEH_NEW ; +static enum diff_scope external_diff_scope;static char diff_colors[][COLOR_MAXLEN ] = {GIT_COLOR_RESET ,
@@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"diff.scope" )) { +git_config_string (&external_diff_scope_cfg, var, value); +if (!strcmp (value,"all" )) + external_diff_scope =DIFF_SCOPE_ALL ; +else if (!strcmp (value,"sparse" )) + external_diff_scope =DIFF_SCOPE_SPARSE ; +else +return -1 ; + } +if (git_color_config (var, value, cb) <0 )return -1 ;
@@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; + options->scope = external_diff_scope;prep_parse_options (options); }
@@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)return 1 ; } +static int diff_opt_diff_scope (const struct option *option, +const char *optarg,int unset) +{ +struct diff_options *opt = option->value; + +if (unset) { + opt->scope =DIFF_SCOPE_NONE ; + }else if (optarg) { +if (!strcmp (optarg,"all" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_ALL ; + } + }else if (!strcmp (optarg,"sparse" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_SPARSE ; + } + }else +return error (_ ("invalid --scope value: %s" ), optarg); + } + +return 0 ; +} +static int diff_opt_diff_filter (const struct option *option,const char *optarg,int unset) {
@@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)OPT_CALLBACK_F (0 ,"diff-filter" , options,N_ ("[(A|C|D|M|R|T|U|X|B)...[*]]" ),N_ ("select files by diff type" ),PARSE_OPT_NONEG , diff_opt_diff_filter), +OPT_CALLBACK_F (0 ,"scope" , options,N_ ("[sparse|all]" ), +N_ ("choose diff scope" ), +PARSE_OPT_OPTARG , diff_opt_diff_scope), { OPTION_CALLBACK, 0, "output", options, N_("<file>"),N_ ("output to a specific file" ), PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
diff.h +11 -0
@@ -230,6 +230,12 @@ enum diff_submodule_format { DIFF_SUBMODULE_INLINE_DIFF }; +enum diff_scope { +DIFF_SCOPE_NONE =0 , +DIFF_SCOPE_ALL , +DIFF_SCOPE_SPARSE , +}; +/** * the set of options the calling program wants to affect the operation of * diffcore library with.
@@ -285,6 +291,9 @@ struct diff_options {/* diff-filter bits */ unsigned int filter, filter_not; +/* diff sparse-checkout scope */ +enum diff_scope scope; +int use_color;/* Number of context lines to generate in patch output. */
@@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files, int insertions, int deletions);void setup_diff_pager (struct diff_options *); +int diff_path_in_sparse_checkout (const char *path); +int diff_paths_in_sparse_checkout (const char *one,const char *two);#endif /* DIFF_H */
t/t4070-diff-sparse-checkout-scope.sh new file +469 -0
469 lines not on this page · raw message
tree-diff.c +5 -0
@@ -76,6 +76,11 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)static int emit_diff_first_parent_only (struct diff_options *opt,struct combine_diff_path *p) {struct combine_diff_parent *p0 = &p->parent[0 ]; + +if (opt->scope ==DIFF_SCOPE_SPARSE && + !diff_path_in_sparse_checkout (p->path)) +return 0 ; +if (p->mode && p0->mode) { opt->change (opt, p0->mode, p->mode, &p0->oid, &p->oid,1 ,1 , p->path,0 ,0 );
base-commit: 63bba4fdd86d80ef061c449daa97a981a9be0792
Signature · 1 line
gitgitgadget
Quoted 1 line · ZheNing Hu via GitGitGadget
t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++
It looks like this test is non-executable, leading to the following error:
~/s/git [nand] (zh/diff--scope) $ make test
SUBDIR templates
make -C t/ all
make[1]: Entering directory '/home/ttaylorr/src/git/t' rm -f -r 'test-results'
non-executable tests: t4070-diff-sparse-checkout-scope.sh make[1]: *** [Makefile:120: test-lint-executable] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: Leaving directory '/home/ttaylorr/src/git/t'
make: *** [Makefile:3083: test] Error 2
Thanks,
Taylor
Quoted 7 lines · Taylor Blau <[email protected]> 于2022年11月1日周二 09:34写道：
Quoted 1 line · ZheNing Hu via GitGitGadget
t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++It looks like this test is non-executable, leading to the following error:
Quoted 11 lines
~/s/git [nand] (zh/diff--scope) $ make test
SUBDIR templates
make -C t/ all
make[1]: Entering directory '/home/ttaylorr/src/git/t' rm -f -r 'test-results'
non-executable tests:t4070-diff-sparse-checkout-scope.shmake[1]: *** [Makefile:120: test-lint-executable] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: Leaving directory '/home/ttaylorr/src/git/t'
make: *** [Makefile:3083: test] Error 2
Thanks for pointing out that I had overlooked this problem.
Quoted 2 lines
Thanks,
Taylor
ZheNing Hu
Hi,
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout specification.
It seems a bit premature to send this, when the guideline document[*] detailing how these options should work is still in the "Needs Review" state. I know, it's been waiting for a while, but it's a _long_ document.
[*] https://lore.kernel.org/git/[email protected]/
Quoted 5 lines
So introduce the
--scopeoption to git diff, which have two
value: "sparse" and "all". "sparse" mean that diff is performed restrict to paths which matching sparse-checkout specification, "all" mean that diff is performed regardless of whether the path meets the sparse-checkout specification.
The wording probably needs some care to reflect the fact that it only affects cases where either --cached or revisions are passed. In particular, your wording for "all" suggests behavior very different from today, whereas "all" is probably best thought of as today's current behavior. For example, a plain git diff without --cached or revisions, should be unaffected by either of these flags.
Quoted 2 lines
--no-scopeis the default
option for now.
What does --no-scope even mean? You didn't explain it, and I don't see how it could make sense. We explicitly avoided a --no- prefix by allowing the --scope option to take a value. I don't think there should be a --no-scope option.
Quoted 2 lines
Add
diff.scope={sparse, all}config, which can also have the same capabilities as--scope, and it will be covered by--scopeoption.
This is not what we want. The high level usecases should not need to be configured per-command. There should be a config option which reflects the high level use cases (e.g. sparse.scope) and then all relevant commands (diff, log, grep, etc.) can key off it.
Quoted 20 lines
Signed-off-by ZheNing Hu <[email protected]>---
[RFC] diff: introduce scope optionIn [1], we discovered that users working on different sparse-checkout specification may download unnecessary blobs from each other's specification in collaboration. In [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout specification to elegantly solve this problem above. In [3]: Newren and Derrick Stolee prefer to name the option --scope={sparse, all}.
So this patch is attempt to do this thing on git diff:
v1:
1. add --restrict option to git diff, which restrict diff filespec in sparse-checkout specification. [4] v2.
2. rename --restrict to --scope={sparse, all}, support --no-scope.
3. add config: diff.scope={sparse,all}.
Quoted 4 lines
Unresolved work:
1. how to properly pass this --scope={sparse, all} to other commands like git log, git format-patch, etc.
log & grep should accept a similar flag. format-patch should not, and should ignore any config in this area.
Quoted 1 line
2. how to set the default value of scope for different diff commands.
I don't understand this.
Quoted 30 lines
[1]: https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/ [2]: https://lore.kernel.org/git/[email protected]/ [3]: https://lore.kernel.org/git/[email protected]/ [4]: https://lore.kernel.org/git/[email protected]/Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1398Documentation/config/diff.txt | 12 + Documentation/diff-options.txt | 18 + builtin/diff.c | 4 + diff-lib.c | 36 +- diff-no-index.c | 4 + diff.c | 39 +++ diff.h | 11 + t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++ tree-diff.c | 5 + 9 files changed, 597 insertions(+), 1 deletion(-) create mode 100644 t/t4070-diff-sparse-checkout-scope.shDocumentation/config/diff.txt +0
-0@@ -201,6 +201,18 @@ diff.algorithm:: -- +
Documentation/config/diff.txt diff.algorithm:: · line 212
Quoted 10 lines
Documentation/config/diff.txt +9
-0+diff.scope:: + Choose diff scope. The variants are as follows: ++ +-- +`sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`all`;; + Without restriction, diff is performed regardless of whether the path + meets the sparse-checkout specification.
As noted above, this is the wrong level to specify things. The description for "all" is misleading as well and suggests something other than "behavior B" from the direction document.
Quoted 18 lines
Documentation/config/diff.txt +0
-0diff.wsErrorHighlight:: Highlight whitespace errors in the `context`, `old` or `new` lines of the diff. Multiple values are separated by comma,Documentation/diff-options.txt +7
-0@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifndef::git-format-patch[] +ifndef::git-log[] + +--scope={sparse|all}:: + Choose diff scope. The variants are as follows: ++ +--
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a · line 214
Quoted 10 lines
Documentation/diff-options.txt +10
-0+`--sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`--all`;; + Without restriction, diff is performed regardless of whether the path + meets the sparse-checkout specification. +-- ++ + +endif::git-log[] +endif::git-format-patch[]
What about diff-files, diff-index, diff-tree, and show?
Quoted 9 lines
Documentation/diff-options.txt +1
-0+ --stat[=<width>[,<name-width>[,<count>]]]:: Generate a diffstat. By default, as much space as necessary will be used for the filename part, and the rest for the graphbuiltin/diff.c +0
-0
builtin/diff.c static void stuff_change(struct diff_options *opt, · line 59
Quoted 6 lines
builtin/diff.c +3
-0@@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt, oideq(old_oid, new_oid) && (old_mode == new_mode))return ; +if (opt->scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (old_path, new_path)) +return ;
This can't be right.
git diff c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e72d42bd856228c15f702fa3c353432f4f1defe03
(to directly diff two known blobs) will go through this function, with old_path == c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e and new_path == 72d42bd856228c15f702fa3c353432f4f1defe03. But those aren't real paths, and sparse-checkout should not restrict what is shown in those cases.
Quoted 9 lines
builtin/diff.c +1
-0+if (opt->flags.reverse_diff) {SWAP (old_mode, new_mode);SWAP (old_oid, new_oid);diff-lib.c +0
-0
diff-lib.c static int match_stat_with_submodule(struct diff_options *diffopt, · line 96
Quoted 9 lines
diff-lib.c +6
-0@@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,return changed; } +int diff_path_in_sparse_checkout (const char *path) { +if (core_sparse_checkout_cone) +return path_in_cone_mode_sparse_checkout (path, the_repository->index); +else +return path_in_sparse_checkout (path, the_repository->index); +}
This says we are including the path if it matches the sparsity patterns. Thus, we have to be careful when we use this function, because the relevant paths are ones that match the sparsity specification. The sparsity specification will always match the sparsity patterns when diffing two commits, but when either the index or the working tree is part of the diff, the sparsity specification *might* be temporarily expanded.
diff-lib.c static int match_stat_with_submodule(struct diff_options *diffopt, · line 103
Quoted 6 lines
diff-lib.c +6
-0+int diff_paths_in_sparse_checkout (const char *one,const char *two) { +if (one == two || !strcmp (one, two)) +return diff_path_in_sparse_checkout (one); +else +return diff_path_in_sparse_checkout (one) && +diff_path_in_sparse_checkout (two);
Why && rather than || ?
diff-lib.c int run_diff_files(struct rev_info *revs, unsigned int option) · line 134
Quoted 13 lines
diff-lib.c +6
-0+} + +int run_diff_files (struct rev_info *revs,unsigned int option) {int entries, i;@@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)if (diff_can_quit_early (&revs->diffopt))break ; +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_path_in_sparse_checkout (ce->name)) +continue ;
Here you've cut off the possibility of showing diffs for anything outside the sparsity patterns, which is a mistake. We need to handle a temporarily expanded sparse specification too.
diff-lib.c int run_diff_files(struct rev_info *revs, unsigned int option) · line 226
Quoted 10 lines
diff-lib.c +2
-1if (!ce_path_match (istate, ce, &revs->prune_data,NULL ))continue ;@@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)continue ; } -if (ce_uptodate (ce) ||ce_skip_worktree (ce)) +if (ce_uptodate (ce) || + (revs->diffopt.scope !=DIFF_SCOPE_ALL &&ce_skip_worktree (ce)))continue ;
Here you make --scope=all show files even if they are skip-worktree, making them appear to have been deleted. I called out your description earlier as potentially misleading, because it could imply this behavior. It looks like you were consistent with the description and implementation, it just doesn't match what we want.
diff-lib.c static void do_oneway_diff(struct unpack_trees_options *o,
Quoted 2 lines
diff-lib.c +0
-0/* @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,
do_oneway_diff is for cases where we are diffing against the index...
Quoted 6 lines
diff-lib.c +3
-0return ;/* nothing to diff.. */ } +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE ) { +if (idx && tree) { +if (!diff_paths_in_sparse_checkout (idx->name, tree->name))
diff-lib.c static void do_oneway_diff(struct unpack_trees_options *o, · line 474
Quoted 10 lines
diff-lib.c +10
-0+return ; + }else if (idx) { +if (!diff_path_in_sparse_checkout (idx->name)) +return ; + }else if (tree) { +if (!diff_path_in_sparse_checkout (tree->name)) +return ; + }else +return ; + }
...and you again mistakenly only compare to the sparsity patterns instead of the sparse specification.
Quoted 12 lines
diff-lib.c +1
-0+/* if the entry is not checked out, don't examine work tree */ cached = o->index_only || (idx && ((idx->ce_flags &CE_VALID ) ||ce_skip_worktree (idx)));diff-no-index.c +0
-0
diff-no-index.c int diff_no_index(struct rev_info *revs, · line 286
Quoted 6 lines
diff-no-index.c +3
-0@@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,fixup_paths (paths, &replacement); +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (paths[0 ], paths[1 ])) + goto out;
--no-index means we're diffing two files that are not tracked, or at least treating them as not tracked. sparse-checkout should not affect such files.
Quoted 17 lines
diff-no-index.c +1
-0+ revs->diffopt.skip_stat_unmatch =1 ;if (!revs->diffopt.output_format) revs->diffopt.output_format =DIFF_FORMAT_PATCH ;diff.c +1
-0@@ -48,6 +48,7 @@ static int diff_interhunk_context_default;static const char *diff_word_regex_cfg;static const char *external_diff_cmd_cfg;static const char *diff_order_file_cfg; +static const char *external_diff_scope_cfg;int diff_auto_refresh_index =1 ;static int diff_mnemonic_prefix;static int diff_no_prefix;
diff.c static int diff_dirstat_permille_default = 30; · line 61
Quoted 4 lines
diff.c +1
-0@@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;static struct diff_options default_diff_options;static long diff_algorithm;static unsigned ws_error_highlight_default =WSEH_NEW ; +static enum diff_scope external_diff_scope;
Why is this called "external"?
Quoted 28 lines
diff.c +11
-0static char diff_colors[][COLOR_MAXLEN ] = {GIT_COLOR_RESET ,@@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"diff.scope" )) { +git_config_string (&external_diff_scope_cfg, var, value); +if (!strcmp (value,"all" )) + external_diff_scope =DIFF_SCOPE_ALL ; +else if (!strcmp (value,"sparse" )) + external_diff_scope =DIFF_SCOPE_SPARSE ; +else +return -1 ; + } +if (git_color_config (var, value, cb) <0 )return -1 ;@@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; + options->scope = external_diff_scope;prep_parse_options (options); }
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4936
Quoted 10 lines
diff.c +7
-0@@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)return 1 ; } +static int diff_opt_diff_scope (const struct option *option, +const char *optarg,int unset) +{ +struct diff_options *opt = option->value; + +if (unset) { + opt->scope =DIFF_SCOPE_NONE ;
I think we should instead have a
BUG_ON_OPT_NEG(unset)
or, even better, a
BUG_ON_OPT_NEG_NOARG(unset, optarg)
at the beginning of this function...
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4937
Quoted 1 line
diff.c +1
-0+ }else if (optarg) {
...which would also allow you to drop this if and dedent the rest of the function.
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4945
Quoted 8 lines
diff.c +8
-0+if (!strcmp (optarg,"all" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_ALL ; + } + }else if (!strcmp (optarg,"sparse" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_SPARSE ; + }
If core_apply_sparse_checkout is false, should we perhaps throw an error instead of just silently ignoring the option the user passed?
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4948
Quoted 3 lines
diff.c +3
-0+ }else +return error (_ ("invalid --scope value: %s" ), optarg); + }
As written with no follow-on else clause here, wouldn't this silently accept "--scope" without an "=<something>" argument without an error and without properly initializing opt->scope?
diff.c static void prep_parse_options(struct diff_options *options) · line 5723
Quoted 13 lines
diff.c +6
-0+ +return 0 ; +} +static int diff_opt_diff_filter (const struct option *option,const char *optarg,int unset) {@@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)OPT_CALLBACK_F (0 ,"diff-filter" , options,N_ ("[(A|C|D|M|R|T|U|X|B)...[*]]" ),N_ ("select files by diff type" ),PARSE_OPT_NONEG , diff_opt_diff_filter), + OPT_CALLBACK_F(0, "scope", options, N_("[sparse|all]"), + N_("choose diff scope"),
maybe "choose diff scope in sparse checkouts"?
t/t4070-diff-sparse-checkout-scope.sh
Quoted 40 lines
diff.c +1
-0+ PARSE_OPT_OPTARG, diff_opt_diff_scope), {OPTION_CALLBACK ,0 ,"output" , options,N_ ("<file>" ),N_ ("output to a specific file" ), PARSE_OPT_NONEG, NULL, 0, diff_opt_output },diff.h +11
-0@@ -230,6 +230,12 @@ enum diff_submodule_format { DIFF_SUBMODULE_INLINE_DIFF }; +enum diff_scope { +DIFF_SCOPE_NONE =0 , +DIFF_SCOPE_ALL , +DIFF_SCOPE_SPARSE , +}; +/** * the set of options the calling program wants to affect the operation of * diffcore library with. @@ -285,6 +291,9 @@ struct diff_options {/* diff-filter bits */ unsigned int filter, filter_not; +/* diff sparse-checkout scope */ +enum diff_scope scope; +int use_color;/* Number of context lines to generate in patch output. */ @@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files, int insertions, int deletions);void setup_diff_pager (struct diff_options *); +int diff_path_in_sparse_checkout (const char *path); +int diff_paths_in_sparse_checkout (const char *one,const char *two);#endif /* DIFF_H */ t/t4070-diff-sparse-checkout-scope.sh new file +0
-0
This needs to be fixed.
Quoted 38 lines
index 00000000000..dca75a3308b
t/t4070-diff-sparse-checkout-scope.sh new file +34
-0@@ -0,0 +1,469 @@ +#!/bin/sh + +test_description='diff sparse-checkout scope' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + + +test_expect_success 'setup' ' + git init temp && + ( + cd temp && + mkdir sub1 && + mkdir sub2 && + echo sub1/file1 >sub1/file1 && + echo sub2/file2 >sub2/file2 && + echo file1 >file1 && + echo file2 >file2 && + git add --all && + git commit -m init && + echo sub1/file1 >>sub1/file1 && + echo sub1/file2 >>sub1/file2 && + echo sub2/file1 >>sub2/file1 && + echo sub2/file2 >>sub2/file2 && + echo file1 >>file1 && + echo file2 >>file2 && + git add --all && + git commit -m change1 && + echo sub1/file1 >>sub1/file1 && + echo sub1/file2 >>sub1/file2 && + echo sub2/file1 >>sub2/file1 && + echo sub2/file2 >>sub2/file2 &&
t/t4070-diff-sparse-checkout-scope.sh line 44
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+ echo file1 >>file1 && + echo file2 >>file2 && + git add --all && + git commit -m change2 + ) +' + +reset_repo () { + rm -rf repo && + git clone --no-checkout temp repo
Why --no-checkout rather than say --sparse?
t/t4070-diff-sparse-checkout-scope.sh line 50
Quoted 6 lines
t/t4070-diff-sparse-checkout-scope.sh new file +6
-0+} + +reset_with_sparse_checkout() { + reset_repo && + git -C repo sparse-checkout set $1 sub1 && + git -C repo checkout
Fixing the above would let us get rid of this really weird extra checkout command too.
Quoted 7 lines
t/t4070-diff-sparse-checkout-scope.sh new file +7
-0+} + +change_worktree_and_index() { + ( + cd repo && + mkdir sub2 sub3 && + echo sub1/file3 >sub1/file3 &&
t/t4070-diff-sparse-checkout-scope.sh line 67
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+ echo sub2/file3 >sub2/file3 && + echo sub3/file3 >sub3/file3 && + echo file3 >file3 && + git add --all --sparse && + echo sub1/file3 >>sub1/file3 && + echo sub2/file3 >>sub2/file3 && + echo sub3/file3 >>sub3/file3 && + echo file3 >>file3 + ) +}
It would be nice to modify different paths in the working tree and index, to see if we can handle cases where the sparse specification does not match the sparsity patterns better. (So, modify files inside and outside the sparsity patterns, stage those changes, and then do a git sparse-checkout reapply to make the files outside the sparsity patterns disappear from the working tree...then modify different files in the working tree both inside and outside the sparsity patterns. And also remove some file that matches the sparsity patterns and manually mark it as SKIP_WORKTREE.) That'd give us much better coverage.
Quoted 65 lines
t/t4070-diff-sparse-checkout-scope.sh new file +65
-0+ +diff_scope() { + title=$1 + need_change_worktree_and_index=$2 + sparse_checkout_option=$3 + scope_option=$4 + expect=$5 + shift 5 + args=("$@") + + test_expect_success "$title $sparse_checkout_option $scope_option" " + reset_with_sparse_checkout $sparse_checkout_option && + if test \"$need_change_worktree_and_index\" = \"true\" ; then + change_worktree_and_index + fi && + git -C repo diff $scope_option ${args[*]} >actual && + if test -z \"$expect\" ; then + >expect + else + cat > expect <<-EOF +$expect + EOF + fi && + test_cmp expect actual + " +} + +args=("--name-only" "HEAD" "HEAD~") +diff_scope builtin_diff_tree false "--no-cone" "--scope=sparse" \ +"sub1/file1 +sub1/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--no-cone" "--scope=all" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--no-cone" "--no-scope" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--scope=sparse" \ +"file1 +file2 +sub1/file1 +sub1/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--scope=all" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--no-scope" \ +"file1 +file2
t/t4070-diff-sparse-checkout-scope.sh line 142
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +args=("--name-only" "HEAD~") +diff_scope builtin_diff_index true "--no-cone" "--scope=sparse" \ +"sub1/file1 +sub1/file2 +sub1/file3" "${args[@]}"
Here's a good example where the testcase is doing the wrong thing. The expected answer here would also include file3, sub2/file3, and sub3/file3.
Quoted 22 lines
t/t4070-diff-sparse-checkout-scope.sh new file +22
-0+ +diff_scope builtin_diff_index true "--no-cone" "--scope=all" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--no-cone" "--no-scope" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2
t/t4070-diff-sparse-checkout-scope.sh line 174
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--cone" "--scope=sparse" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3" "${args[@]}"
This is also wrong; it's missing sub2/file3 and sub3/file3.
Quoted 19 lines
t/t4070-diff-sparse-checkout-scope.sh new file +19
-0+ +diff_scope builtin_diff_index true "--cone" "--scope=all" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--cone" "--no-scope" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2
t/t4070-diff-sparse-checkout-scope.sh line 203
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" + +args=("--name-only" "file3" "sub1/" "sub2/") + +diff_scope builtin_diff_files true "--no-cone" "--scope=sparse" \ +"sub1/file3" "${args[@]}"
This should also include file3, sub2/file3, and sub3/file3. --scope= should not affect diff output at all if neither --cached nor revision arguments are supplied.
t/t4070-diff-sparse-checkout-scope.sh line 210
Quoted 7 lines
t/t4070-diff-sparse-checkout-scope.sh new file +7
-0+ +diff_scope builtin_diff_files true "--no-cone" "--scope=all" \ +"file3 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3" "${args[@]}"
This is wrong due to including too much; it should not include sub2/file1 or sub2/file2 (it is only including those because it is showing them as deleted, when they are not deleted but are SKIP_WORKTREE).
I think I'm going to stop reviewing here. I'm probably just going to keep repeating the same issues I identified earlier if I continue.
inHi,
Quoted 12 lines · Elijah Newren <[email protected]> 于2022年11月1日周二 13:18写道：
Hi,
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout specification.
Quoted 7 lines
It seems a bit premature to send this, when the guideline document[*] detailing how these options should work is still in the "Needs Review" state. I know, it's been waiting for a while, but it's a _long_ document.
Fine, I just want to start trying to experiment with this feature in git-diff earlier,
and I can wait for the sparse-checkout.txt documentation to finish first if needed :)
Quoted 6 lines
Quoted 5 lines
So introduce the
--scopeoption to git diff, which have two
value: "sparse" and "all". "sparse" mean that diff is performed restrict to paths which matching sparse-checkout specification, "all" mean that diff is performed regardless of whether the path meets the sparse-checkout specification.
Quoted 7 lines
The wording probably needs some care to reflect the fact that it only affects cases where either --cached or revisions are passed. In particular, your wording for "all" suggests behavior very different from today, whereas "all" is probably best thought of as today's current behavior. For example, a plain
git diffwithout --cached or revisions, should be unaffected by either of these flags.
Yes, after re-reading your sparse-checkout.txt patch, I realized that I misinterpreted "--scope=sparse" as sparse patterns instead of sparse specification, and misinterpreted "-scope=all" as diff on all files.
Quoted 8 lines
Quoted 2 lines
--no-scopeis the default
option for now.What does --no-scope even mean? You didn't explain it, and I don't see how it could make sense. We explicitly avoided a
--no-prefix by allowing the --scope option to take a value. I don't think there should be a --no-scope option.
I think the “--no-scope” here does nothing, as if it were unaffected by scope (just like correctly "--scope=all", right?)
Quoted 8 lines
Quoted 2 lines
Add
diff.scope={sparse, all}config, which can also have the same capabilities as--scope, and it will be covered by--scopeoption.This is not what we want. The high level usecases should not need to be configured per-command. There should be a config option which reflects the high level use cases (e.g. sparse.scope) and then all relevant commands (diff, log, grep, etc.) can key off it.
Ok, using a global config should indeed be more useful.
Quoted 25 lines
Quoted 20 lines
Signed-off-by ZheNing Hu <[email protected]>---
[RFC] diff: introduce scope optionIn [1], we discovered that users working on different sparse-checkout specification may download unnecessary blobs from each other's specification in collaboration. In [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout specification to elegantly solve this problem above. In [3]: Newren and Derrick Stolee prefer to name the option --scope={sparse, all}.
So this patch is attempt to do this thing on git diff:
v1:
1. add --restrict option to git diff, which restrict diff filespec in sparse-checkout specification. [4] v2.
2. rename --restrict to --scope={sparse, all}, support --no-scope.
3. add config: diff.scope={sparse,all}.Quoted 4 lines
Unresolved work:
1. how to properly pass this --scope={sparse, all} to other commands like git log, git format-patch, etc.
Quoted 7 lines
log & grep should accept a similar flag. format-patch should not, and should ignore any config in this area.
Quoted 1 line
2. how to set the default value of scope for different diff commands.
I don't understand this.
Oh, I was just curious if the config defaults for scope needed to be configured separately for the different diff commands git diff-files, git diff-index, git diff-no-index, git diff-tree, since sparse-checkout.txt mentions the different
behavior of scope for them. Now I think this just needs to be handled in command code logic and no additional command level configuration is needed.
Quoted 31 lines
Quoted 31 lines
[1]: https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/ [2]: https://lore.kernel.org/git/[email protected]/ [3]: https://lore.kernel.org/git/[email protected]/ [4]: https://lore.kernel.org/git/[email protected]/Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1398Documentation/config/diff.txt | 12 + Documentation/diff-options.txt | 18 + builtin/diff.c | 4 + diff-lib.c | 36 +- diff-no-index.c | 4 + diff.c | 39 +++ diff.h | 11 + t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++ tree-diff.c | 5 + 9 files changed, 597 insertions(+), 1 deletion(-) create mode 100644 t/t4070-diff-sparse-checkout-scope.shDocumentation/config/diff.txt +0
-0@@ -201,6 +201,18 @@ diff.algorithm:: -- +
Quoted 14 lines
Documentation/config/diff.txt diff.algorithm:: · line 212
Quoted 9 lines
Documentation/config/diff.txt +9
-0+diff.scope:: + Choose diff scope. The variants are as follows: ++ +-- +`sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`all`;; + Without restriction, diff is performed regardless of whether the path + meets the sparse-checkout specification.As noted above, this is the wrong level to specify things. The description for "all" is misleading as well and suggests something other than "behavior B" from the direction document.
So do we need to make "--scope=all" correspond to the "behavior B", The correct description of it should be: "worktree-sparse-history-dense"...
Quoted 22 lines
Quoted 22 lines
Documentation/config/diff.txt +0
-0diff.wsErrorHighlight:: Highlight whitespace errors in the `context`, `old` or `new` lines of the diff. Multiple values are separated by comma,Documentation/diff-options.txt +11
-0@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifndef::git-format-patch[] +ifndef::git-log[] + +--scope={sparse|all}:: + Choose diff scope. The variants are as follows: ++ +-- +`--sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`--all`;; + Without restriction, diff is performed regardless of whether the path
Quoted 9 lines
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a · line 214
Quoted 6 lines
Documentation/diff-options.txt +6
-0+ meets the sparse-checkout specification. +-- ++ + +endif::git-log[] +endif::git-format-patch[]What about diff-files, diff-index, diff-tree, and show?
diff-options.txt included in their documents, and git-format-patch.txt, git-log.txt, but should not in git-format-patch.txt. I don't know if it should be included in git-diff-files.txt, because git diff-files compare the files in the working tree and the index (so it's the same as "simple" git-diff, which should not be affected by scope?)
Quoted 12 lines
Quoted 12 lines
Documentation/diff-options.txt +1
-0+ --stat[=<width>[,<name-width>[,<count>]]]:: Generate a diffstat. By default, as much space as necessary will be used for the filename part, and the rest for the graphbuiltin/diff.c +0
-0@@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt, oideq(old_oid, new_oid) && (old_mode == new_mode))return ;
Quoted 13 lines
builtin/diff.c static void stuff_change(struct diff_options *opt, · line 59
Quoted 3 lines
builtin/diff.c +3
-0+if (opt->scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (old_path, new_path)) +return ;This can't be right.
git diffc231e0f26fe9b2ea9ec46aa68ff95ba984ce592e72d42bd856228c15f702fa3c353432f4f1defe03
(to directly diff two known blobs) will go through this function, withold_path==c231e0f26fe9b2ea9ec46aa68ff95ba984ce592eandnew_path==72d42bd856228c15f702fa3c353432f4f1defe03. But those aren't real paths, and sparse-checkout should not restrict what is shown in those cases.
Yeah, it's buggy that I forget to check two blobs without paths.
Quoted 12 lines
Quoted 12 lines
builtin/diff.c +1
-0+if (opt->flags.reverse_diff) {SWAP (old_mode, new_mode);SWAP (old_oid, new_oid);diff-lib.c +0
-0@@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,return changed; }
Quoted 15 lines
diff-lib.c static int match_stat_with_submodule(struct diff_options *diffopt, · line 96
Quoted 6 lines
diff-lib.c +6
-0+int diff_path_in_sparse_checkout (const char *path) { +if (core_sparse_checkout_cone) +return path_in_cone_mode_sparse_checkout (path, the_repository->index); +else +return path_in_sparse_checkout (path, the_repository->index); +}This says we are including the path if it matches the sparsity patterns. Thus, we have to be careful when we use this function, because the relevant paths are ones that match the sparsity specification. The sparsity specification will always match the sparsity patterns when diffing two commits, but when either the index or the working tree is part of the diff, the sparsity specification *might* be temporarily expanded.
Yes, I may have to look at more code to better understand how and when the "sparsity specification" is extended. Any recommendations for places to read?
Quoted 9 lines
diff-lib.c static int match_stat_with_submodule(struct diff_options *diffopt, · line 103
Quoted 6 lines
diff-lib.c +6
-0+int diff_paths_in_sparse_checkout (const char *one,const char *two) { +if (one == two || !strcmp (one, two)) +return diff_path_in_sparse_checkout (one); +else +return diff_path_in_sparse_checkout (one) && +diff_path_in_sparse_checkout (two);Why && rather than || ?
Agree, we do need to use || here, because the one diff side in the sparse specification, we should diff the two files.
Quoted 8 lines
Quoted 8 lines
diff-lib.c +3
-0+} + +int run_diff_files (struct rev_info *revs,unsigned int option) {int entries, i;@@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
Quoted 10 lines
diff-lib.c int run_diff_files(struct rev_info *revs, unsigned int option) · line 134
Quoted 5 lines
diff-lib.c +3
-0if (diff_can_quit_early (&revs->diffopt))break ; +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_path_in_sparse_checkout (ce->name)) +continue ;Here you've cut off the possibility of showing diffs for anything outside the sparsity patterns, which is a mistake. We need to handle a temporarily expanded sparse specification too.
Agree.
Quoted 6 lines
Quoted 6 lines
diff-lib.c +0
-0if (!ce_path_match (istate, ce, &revs->prune_data,NULL ))continue ;@@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)continue ; }
Quoted 11 lines
diff-lib.c int run_diff_files(struct rev_info *revs, unsigned int option) · line 226
Quoted 4 lines
diff-lib.c +2
-1-if (ce_uptodate (ce) ||ce_skip_worktree (ce)) +if (ce_uptodate (ce) || + (revs->diffopt.scope !=DIFF_SCOPE_ALL &&ce_skip_worktree (ce)))continue ;Here you make --scope=all show files even if they are skip-worktree, making them appear to have been deleted. I called out your description earlier as potentially misleading, because it could imply this behavior. It looks like you were consistent with the description and implementation, it just doesn't match what we want.
Agree too. So we should not do anything in run_diff_files.
Quoted 22 lines
diff-lib.c static void do_oneway_diff(struct unpack_trees_options *o,
Quoted 2 lines
diff-lib.c +0
-0/* @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,
do_oneway_diffis for cases where we are diffing against the index...Quoted 6 lines
diff-lib.c +3
-0return ;/* nothing to diff.. */ } +if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE ) { +if (idx && tree) { +if (!diff_paths_in_sparse_checkout (idx->name, tree->name))diff-lib.c static void do_oneway_diff(struct unpack_trees_options *o, · line 474
Quoted 10 lines
diff-lib.c +10
-0+return ; + }else if (idx) { +if (!diff_path_in_sparse_checkout (idx->name)) +return ; + }else if (tree) { +if (!diff_path_in_sparse_checkout (tree->name)) +return ; + }else +return ; + }
Quoted 3 lines
...and you again mistakenly only compare to the sparsity patterns instead of the sparse specification.
So here we should use ce_skip_worktree(idx) to check if this index entry matches sparse specification.
Quoted 15 lines
Quoted 15 lines
diff-lib.c +1
-0+/* if the entry is not checked out, don't examine work tree */ cached = o->index_only || (idx && ((idx->ce_flags &CE_VALID ) ||ce_skip_worktree (idx)));diff-no-index.c +0
-0@@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,fixup_paths (paths, &replacement);
Quoted 8 lines
diff-no-index.c int diff_no_index(struct rev_info *revs, · line 286
Quoted 3 lines
diff-no-index.c +3
-0+if (revs->diffopt.scope ==DIFF_SCOPE_SPARSE && + !diff_paths_in_sparse_checkout (paths[0 ], paths[1 ])) + goto out;--no-index means we're diffing two files that are not tracked, or at least treating them as not tracked. sparse-checkout should not affect such files.
Yeah, we should care about untracked files sparse-checkout only when we are using git add/rm/update-index...
Quoted 15 lines
Quoted 15 lines
diff-no-index.c +1
-0+ revs->diffopt.skip_stat_unmatch =1 ;if (!revs->diffopt.output_format) revs->diffopt.output_format =DIFF_FORMAT_PATCH ;diff.c +1
-0@@ -48,6 +48,7 @@ static int diff_interhunk_context_default;static const char *diff_word_regex_cfg;static const char *external_diff_cmd_cfg;static const char *diff_order_file_cfg; +static const char *external_diff_scope_cfg;int diff_auto_refresh_index =1 ;static int diff_mnemonic_prefix;
Quoted 9 lines
diff.c static int diff_dirstat_permille_default = 30; · line 61
Quoted 6 lines
diff.c +1
-0static int diff_no_prefix;@@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;static struct diff_options default_diff_options;static long diff_algorithm;static unsigned ws_error_highlight_default =WSEH_NEW ; +static enum diff_scope external_diff_scope;Why is this called "external"?
Learn from external_diff_cmd_cfg, I should remove it.
Quoted 45 lines
Quoted 28 lines
diff.c +11
-0static char diff_colors[][COLOR_MAXLEN ] = {GIT_COLOR_RESET ,@@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)return 0 ; } +if (!strcmp (var,"diff.scope" )) { +git_config_string (&external_diff_scope_cfg, var, value); +if (!strcmp (value,"all" )) + external_diff_scope =DIFF_SCOPE_ALL ; +else if (!strcmp (value,"sparse" )) + external_diff_scope =DIFF_SCOPE_SPARSE ; +else +return -1 ; + } +if (git_color_config (var, value, cb) <0 )return -1 ;@@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; + options->scope = external_diff_scope;prep_parse_options (options); }diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4936
Quoted 10 lines
diff.c +7
-0@@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)return 1 ; } +static int diff_opt_diff_scope (const struct option *option, +const char *optarg,int unset) +{ +struct diff_options *opt = option->value; + +if (unset) { + opt->scope =DIFF_SCOPE_NONE ;I think we should instead have a
BUG_ON_OPT_NEG(unset)
or, even better, a
BUG_ON_OPT_NEG_NOARG(unset, optarg)
at the beginning of this function...
Quoted 5 lines
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4937
Quoted 1 line
diff.c +1
-0+ }else if (optarg) {...which would also allow you to drop this if and dedent the rest of the function.
Agree.
Quoted 9 lines
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4945
Quoted 8 lines
diff.c +8
-0+if (!strcmp (optarg,"all" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_ALL ; + } + }else if (!strcmp (optarg,"sparse" )) { +if (core_apply_sparse_checkout) { + opt->scope =DIFF_SCOPE_SPARSE ; + }
Quoted 3 lines
If
core_apply_sparse_checkoutis false, should we perhaps throw an error instead of just silently ignoring the option the user passed?
Agree.
Quoted 8 lines
diff.c static int parse_dirstat_opt(struct diff_options *options, const char *params) · line 4948
Quoted 3 lines
diff.c +3
-0+ }else +return error (_ ("invalid --scope value: %s" ), optarg); + }As written with no follow-on else clause here, wouldn't this silently accept "--scope" without an "=<something>" argument without an error and without properly initializing
opt->scope?
Because opt will be initializing with default_diff_options in repo_diff_setup(), and opt->scope should respect config value first. So I don't think there's a mistake here.
Quoted 7 lines
Quoted 7 lines
diff.c +4
-0+ +return 0 ; +} +static int diff_opt_diff_filter (const struct option *option,const char *optarg,int unset) {
Quoted 9 lines
diff.c static void prep_parse_options(struct diff_options *options) · line 5723
Quoted 6 lines
diff.c +2
-0@@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)OPT_CALLBACK_F (0 ,"diff-filter" , options,N_ ("[(A|C|D|M|R|T|U|X|B)...[*]]" ),N_ ("select files by diff type" ),PARSE_OPT_NONEG , diff_opt_diff_filter), + OPT_CALLBACK_F(0, "scope", options, N_("[sparse|all]"), + N_("choose diff scope"),maybe "choose diff scope in sparse checkouts"?
OK.
Quoted 85 lines
t/t4070-diff-sparse-checkout-scope.sh
Quoted 40 lines
diff.c +1
-0+ PARSE_OPT_OPTARG, diff_opt_diff_scope), {OPTION_CALLBACK ,0 ,"output" , options,N_ ("<file>" ),N_ ("output to a specific file" ), PARSE_OPT_NONEG, NULL, 0, diff_opt_output },diff.h +11
-0@@ -230,6 +230,12 @@ enum diff_submodule_format { DIFF_SUBMODULE_INLINE_DIFF }; +enum diff_scope { +DIFF_SCOPE_NONE =0 , +DIFF_SCOPE_ALL , +DIFF_SCOPE_SPARSE , +}; +/** * the set of options the calling program wants to affect the operation of * diffcore library with. @@ -285,6 +291,9 @@ struct diff_options {/* diff-filter bits */ unsigned int filter, filter_not; +/* diff sparse-checkout scope */ +enum diff_scope scope; +int use_color;/* Number of context lines to generate in patch output. */ @@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files, int insertions, int deletions);void setup_diff_pager (struct diff_options *); +int diff_path_in_sparse_checkout (const char *path); +int diff_paths_in_sparse_checkout (const char *one,const char *two);#endif /* DIFF_H */ t/t4070-diff-sparse-checkout-scope.sh new file +0
-0This needs to be fixed.
Quoted 42 lines
index 00000000000..dca75a3308b
t/t4070-diff-sparse-checkout-scope.sh new file +38
-0@@ -0,0 +1,469 @@ +#!/bin/sh + +test_description='diff sparse-checkout scope' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + + +test_expect_success 'setup' ' + git init temp && + ( + cd temp && + mkdir sub1 && + mkdir sub2 && + echo sub1/file1 >sub1/file1 && + echo sub2/file2 >sub2/file2 && + echo file1 >file1 && + echo file2 >file2 && + git add --all && + git commit -m init && + echo sub1/file1 >>sub1/file1 && + echo sub1/file2 >>sub1/file2 && + echo sub2/file1 >>sub2/file1 && + echo sub2/file2 >>sub2/file2 && + echo file1 >>file1 && + echo file2 >>file2 && + git add --all && + git commit -m change1 && + echo sub1/file1 >>sub1/file1 && + echo sub1/file2 >>sub1/file2 && + echo sub2/file1 >>sub2/file1 && + echo sub2/file2 >>sub2/file2 && + echo file1 >>file1 && + echo file2 >>file2 && + git add --all && + git commit -m change2
Quoted 9 lines
t/t4070-diff-sparse-checkout-scope.sh line 44
Quoted 6 lines
t/t4070-diff-sparse-checkout-scope.sh new file +6
-0+ ) +' + +reset_repo () { + rm -rf repo && + git clone --no-checkout temp repoWhy --no-checkout rather than say --sparse?
This is because I accidentally associated it with a partial clone. I often use "git clone -filter=blob:none -no-checkout" to do a partial clone, then "git sparse- checkout set <pattern>" to set sparse-checkout patterns, and "git checkout" to download the missing blobs and checkout to a branch. But in this test file, we only need sparse-checkout, so it's true that I should not do such strange no-checkout thing.
Quoted 30 lines
t/t4070-diff-sparse-checkout-scope.sh line 50
Quoted 6 lines
t/t4070-diff-sparse-checkout-scope.sh new file +6
-0+} + +reset_with_sparse_checkout() { + reset_repo && + git -C repo sparse-checkout set $1 sub1 && + git -C repo checkoutFixing the above would let us get rid of this really weird extra checkout command too.
Quoted 7 lines
t/t4070-diff-sparse-checkout-scope.sh new file +7
-0+} + +change_worktree_and_index() { + ( + cd repo && + mkdir sub2 sub3 && + echo sub1/file3 >sub1/file3 &&t/t4070-diff-sparse-checkout-scope.sh line 67
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+ echo sub2/file3 >sub2/file3 && + echo sub3/file3 >sub3/file3 && + echo file3 >file3 && + git add --all --sparse && + echo sub1/file3 >>sub1/file3 && + echo sub2/file3 >>sub2/file3 && + echo sub3/file3 >>sub3/file3 && + echo file3 >>file3 + ) +}It would be nice to modify different paths in the working tree and index, to see if we can handle cases where the sparse specification
Quoted 9 lines
does not match the sparsity patterns better. (So, modify files inside and outside the sparsity patterns, stage those changes, and then do a
git sparse-checkout reapplyto make the files outside the sparsity patterns disappear from the working tree...then modify different files in the working tree both inside and outside the sparsity patterns. And also remove some file that matches the sparsity patterns and manually mark it asSKIP_WORKTREE.) That'd give us much better coverage.
Nice addition. So I should use git update-index --skip-worktree to set skip_worktree bit for index entries.
Quoted 71 lines
Quoted 71 lines
t/t4070-diff-sparse-checkout-scope.sh new file +71
-0+ +diff_scope() { + title=$1 + need_change_worktree_and_index=$2 + sparse_checkout_option=$3 + scope_option=$4 + expect=$5 + shift 5 + args=("$@") + + test_expect_success "$title $sparse_checkout_option $scope_option" " + reset_with_sparse_checkout $sparse_checkout_option && + if test \"$need_change_worktree_and_index\" = \"true\" ; then + change_worktree_and_index + fi && + git -C repo diff $scope_option ${args[*]} >actual && + if test -z \"$expect\" ; then + >expect + else + cat > expect <<-EOF +$expect + EOF + fi && + test_cmp expect actual + " +} + +args=("--name-only" "HEAD" "HEAD~") +diff_scope builtin_diff_tree false "--no-cone" "--scope=sparse" \ +"sub1/file1 +sub1/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--no-cone" "--scope=all" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--no-cone" "--no-scope" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--scope=sparse" \ +"file1 +file2 +sub1/file1 +sub1/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--scope=all" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +diff_scope builtin_diff_tree false "--cone" "--no-scope" \ +"file1 +file2 +sub1/file1 +sub1/file2 +sub2/file1 +sub2/file2" "${args[@]}" + +args=("--name-only" "HEAD~")
Quoted 9 lines
t/t4070-diff-sparse-checkout-scope.sh line 142
Quoted 4 lines
t/t4070-diff-sparse-checkout-scope.sh new file +4
-0+diff_scope builtin_diff_index true "--no-cone" "--scope=sparse" \ +"sub1/file1 +sub1/file2 +sub1/file3" "${args[@]}"Here's a good example where the testcase is doing the wrong thing. The expected answer here would also include file3, sub2/file3, and sub3/file3.
Yeah. Files that are not part of the sparse-checkout patterns are temporarily extended into the sparse specification.
Quoted 60 lines
Quoted 22 lines
t/t4070-diff-sparse-checkout-scope.sh new file +22
-0+ +diff_scope builtin_diff_index true "--no-cone" "--scope=all" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--no-cone" "--no-scope" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2t/t4070-diff-sparse-checkout-scope.sh line 174
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--cone" "--scope=sparse" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3" "${args[@]}"This is also wrong; it's missing sub2/file3 and sub3/file3.
Quoted 25 lines
t/t4070-diff-sparse-checkout-scope.sh new file +25
-0+ +diff_scope builtin_diff_index true "--cone" "--scope=all" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" + +diff_scope builtin_diff_index true "--cone" "--no-scope" \ +"file1 +file2 +file3 +sub1/file1 +sub1/file2 +sub1/file3 +sub2/file1 +sub2/file2 +sub2/file3 +sub3/file3" "${args[@]}" +
Quoted 9 lines
t/t4070-diff-sparse-checkout-scope.sh line 203
Quoted 4 lines
t/t4070-diff-sparse-checkout-scope.sh new file +4
-0+args=("--name-only" "file3" "sub1/" "sub2/") + +diff_scope builtin_diff_files true "--no-cone" "--scope=sparse" \ +"sub1/file3" "${args[@]}"This should also include file3, sub2/file3, and sub3/file3.
--scope=should not affect diff output at all if neither --cached nor revision arguments are supplied.
Agree.
Quoted 4 lines
Quoted 4 lines
t/t4070-diff-sparse-checkout-scope.sh new file +4
-0+ +diff_scope builtin_diff_files true "--no-cone" "--scope=all" \ +"file3 +sub1/file3
Quoted 9 lines
t/t4070-diff-sparse-checkout-scope.sh line 210
Quoted 3 lines
t/t4070-diff-sparse-checkout-scope.sh new file +3
-0+sub2/file1 +sub2/file2 +sub2/file3" "${args[@]}"This is wrong due to including too much; it should not include sub2/file1 or sub2/file2 (it is only including those because it is showing them as deleted, when they are not deleted but are
SKIP_WORKTREE).
Agree.
Quoted 2 lines
I think I'm going to stop reviewing here. I'm probably just going to keep repeating the same issues I identified earlier if I continue.
Thank you very much for your review, you have pointed out very many problems with this patch :)
--
ZheNing Hu
Hi,
Quoted 15 lines · ZheNing Hu
Quoted 14 lines · Elijah Newren <[email protected]> 于2022年11月1日周二 13:18写道：
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout specification.
It seems a bit premature to send this, when the guideline document[*] detailing how these options should work is still in the "Needs Review" state. I know, it's been waiting for a while, but it's a _long_ document.
Quoted 7 lines
Quoted 2 lines
Fine, I just want to start trying to experiment with this feature in git-diff earlier,
and I can wait for the sparse-checkout.txt documentation to finish first if needed :)
Note that you may be able to help reduce the wait by reviewing the document. (You commented on v1 -- thanks! -- but no one has commented on any of the newer versions, despite being out for over a month and still being marked as "Needs Review" in the "What's cooking" reports.)
Quoted 10 lines
Quoted 7 lines
Quoted 2 lines
--no-scopeis the default
option for now.What does --no-scope even mean? You didn't explain it, and I don't see how it could make sense. We explicitly avoided a
--no-prefix by allowing the --scope option to take a value. I don't think there should be a --no-scope option.I think the “--no-scope” here does nothing, as if it were unaffected by scope (just like correctly "--scope=all", right?)
Reading your patch, I was unable to determine where you made --no-scope behave differently than how you implemented --scope=all. Maybe there was a difference and I missed it.
Anyway, I don't think there should be a --no-scope option.
Quoted 29 lines
Quoted 19 lines
Quoted 19 lines
Documentation/diff-options.txt +11
-0@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifndef::git-format-patch[] +ifndef::git-log[] + +--scope={sparse|all}:: + Choose diff scope. The variants are as follows: ++ +-- +`--sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`--all`;; + Without restriction, diff is performed regardless of whether the pathQuoted 9 lines
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a · line 214
Quoted 6 lines
Documentation/diff-options.txt +6
-0+ meets the sparse-checkout specification. +-- ++ + +endif::git-log[] +endif::git-format-patch[]What about diff-files, diff-index, diff-tree, and show?
Quoted 5 lines
diff-options.txt included in their documents, and git-format-patch.txt, git-log.txt, but should not in git-format-patch.txt. I don't know if it should be included in git-diff-files.txt, because git diff-files compare the files in the working tree and the index (so it's the same as "simple" git-diff, which should not be affected by scope?)
Oh, good point. Yeah, git-diff-files.txt should not get this option as it won't have any affect. git-diff-index and git-diff-tree and git-show and git-log should get it, though.
Quoted 15 lines
Quoted 7 lines
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a
Quoted 6 lines
Documentation/diff-options.txt +6
-0+int diff_path_in_sparse_checkout(const char *path) { + if (core_sparse_checkout_cone) + return path_in_cone_mode_sparse_checkout(path, the_repository->index); + else + return path_in_sparse_checkout(path, the_repository->index); +}Quoted 7 lines
This says we are including the path if it matches the sparsity patterns. Thus, we have to be careful when we use this function, because the relevant paths are ones that match the sparsity specification. The sparsity specification will always match the sparsity patterns when diffing two commits, but when either the index or the working tree is part of the diff, the sparsity specification *might* be temporarily expanded.
Quoted 2 lines
Yes, I may have to look at more code to better understand how and when the "sparsity specification" is extended. Any recommendations for places to read?
Yes, please review the newer version of my sparse-checkout.txt directions file; it covers this in more detail.
Quoted 21 lines
Quoted 21 lines
Documentation/diff-options.txt static void do_oneway_diff(struct unpack_trees_options *o,
Quoted 1 line
Documentation/diff-options.txt +0
-0@@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,
do_oneway_diffis for cases where we are diffing against the index...Quoted 16 lines
Documentation/diff-options.txt +13
-0return; /* nothing to diff.. */ } + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) { + if (idx && tree) { + if (!diff_paths_in_sparse_checkout(idx->name, tree->name)) + return; + } else if (idx) { + if (!diff_path_in_sparse_checkout(idx->name)) + return; + } else if (tree) { + if (!diff_path_in_sparse_checkout(tree->name)) + return; + } else + return; + }
Quoted 6 lines
Quoted 3 lines
...and you again mistakenly only compare to the sparsity patterns instead of the sparse specification.
So here we should use
ce_skip_worktree(idx)to check if this index entry matches sparse specification.
ce_skip_worktree(idx) only checks for expansions to the sparse specification in the working tree. If dealing with index files, the sparse specification is also expanded to handle all files in the index that differ from HEAD.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Documentation/diff-options.txt +3
-0+ } else + return error(_("invalid --scope value: %s"), optarg); + }
Quoted 8 lines
Quoted 4 lines
As written with no follow-on else clause here, wouldn't this silently accept "--scope" without an "=<something>" argument without an error and without properly initializing
opt->scope?Because opt will be initializing with
default_diff_optionsinrepo_diff_setup(), andopt->scopeshould respect config value first. So I don't think there's a mistake here.
Okay, it's good that you've got the variables initialized somehow, but that's only half the point here. The main point is that the user can specify something that makes no sense and if they do, we should throw an error informing them of their mistake. --scope should not be passed without an argument (either "all" or "sparse", currently), but this code allowed it.
Quoted 4 lines
Quoted 4 lines
t/t4070-diff-sparse-checkout-scope.sh
Quoted 2 lines
t/t4070-diff-sparse-checkout-scope.sh new file +0
-0This needs to be fixed.
Since you didn't comment on this but usually do comment on thing, I'll just re-ping it to make sure you don't miss the comment about the incorrect file mode here.
Quoted 7 lines
Quoted 6 lines
t/t4070-diff-sparse-checkout-scope.sh line 44
Quoted 3 lines
t/t4070-diff-sparse-checkout-scope.sh new file +3
-0+reset_repo () { + rm -rf repo && + git clone --no-checkout temp repoWhy --no-checkout rather than say --sparse?
Quoted 7 lines
This is because I accidentally associated it with a partial clone. I often use "git clone -filter=blob:none -no-checkout" to do a partial clone, then "git sparse- checkout set <pattern>" to set sparse-checkout patterns, and "git checkout" to download the missing blobs and checkout to a branch. But in this test file, we only need sparse-checkout, so it's true that I should not do such strange no-checkout thing.
Yeah, no need to involve partial clones here. (As an aside, though, I think even with a partial clone that --sparse makes more sense than --no-checkout.)
Quoted 28 lines
Quoted 18 lines
Quoted 5 lines
t/t4070-diff-sparse-checkout-scope.sh new file +5
-0+change_worktree_and_index() { + ( + cd repo && + mkdir sub2 sub3 && + echo sub1/file3 >sub1/file3 &&t/t4070-diff-sparse-checkout-scope.sh line 67
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+ echo sub2/file3 >sub2/file3 && + echo sub3/file3 >sub3/file3 && + echo file3 >file3 && + git add --all --sparse && + echo sub1/file3 >>sub1/file3 && + echo sub2/file3 >>sub2/file3 && + echo sub3/file3 >>sub3/file3 && + echo file3 >>file3 + ) +}It would be nice to modify different paths in the working tree and index, to see if we can handle cases where the sparse specification
Quoted 9 lines
does not match the sparsity patterns better. (So, modify files inside and outside the sparsity patterns, stage those changes, and then do a
git sparse-checkout reapplyto make the files outside the sparsity patterns disappear from the working tree...then modify different files in the working tree both inside and outside the sparsity patterns. And also remove some file that matches the sparsity patterns and manually mark it asSKIP_WORKTREE.) That'd give us much better coverage.
Quoted 2 lines
Nice addition. So I should use git update-index --skip-worktree to set
skip_worktreebit for index entries.
Well, I'd say use normal "sparse-checkout" commands to set most of them. However, adding one or two extra SKIP_WORKTREE paths via running git update-index --skip-worktree $PATH (and removing the corresponding local file) would make the testcases more interesting.
Quoted 5 lines
Quoted 2 lines
I think I'm going to stop reviewing here. I'm probably just going to keep repeating the same issues I identified earlier if I continue.
Thank you very much for your review, you have pointed out very many problems with this patch :)
I'm glad it was helpful. :)
Quoted 27 lines · Elijah Newren <[email protected]> 于2022年11月6日周日 14:58写道：
Hi,
Quoted 15 lines · ZheNing Hu
Quoted 14 lines · Elijah Newren <[email protected]> 于2022年11月1日周二 13:18写道：
Quoted 6 lines · ZheNing Hu via GitGitGadget
From: ZheNing Hu <[email protected]>
When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout specification.
It seems a bit premature to send this, when the guideline document[*] detailing how these options should work is still in the "Needs Review" state. I know, it's been waiting for a while, but it's a _long_ document.
Quoted 7 lines
Quoted 2 lines
Fine, I just want to start trying to experiment with this feature in git-diff earlier,
and I can wait for the sparse-checkout.txt documentation to finish first if needed :)
Quoted 5 lines
Note that you may be able to help reduce the wait by reviewing the document. (You commented on v1 -- thanks! -- but no one has commented on any of the newer versions, despite being out for over a month and still being marked as "Needs Review" in the "What's cooking" reports.)
I've skimmed v3 in general, but haven't looked at v4 yet, I'll take a closer look at it.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
--no-scopeis the default
option for now.What does --no-scope even mean? You didn't explain it, and I don't see how it could make sense. We explicitly avoided a
--no-prefix by allowing the --scope option to take a value. I don't think there should be a --no-scope option.
Quoted 9 lines
Quoted 2 lines
I think the “--no-scope” here does nothing, as if it were unaffected by scope (just like correctly "--scope=all", right?)
Reading your patch, I was unable to determine where you made --no-scope behave differently than how you implemented --scope=all. Maybe there was a difference and I missed it.
Anyway, I don't think there should be a --no-scope option.
Fine, I'll get rid of it.
Quoted 54 lines
Quoted 29 lines
Quoted 19 lines
Quoted 19 lines
Documentation/diff-options.txt +11
-0@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifndef::git-format-patch[] +ifndef::git-log[] + +--scope={sparse|all}:: + Choose diff scope. The variants are as follows: ++ +-- +`--sparse`;; + Restrict diff paths to those matching sparse-checkout specification. +`--all`;; + Without restriction, diff is performed regardless of whether the pathQuoted 9 lines
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a · line 214
Quoted 6 lines
Documentation/diff-options.txt +6
-0+ meets the sparse-checkout specification. +-- ++ + +endif::git-log[] +endif::git-format-patch[]What about diff-files, diff-index, diff-tree, and show?
Quoted 5 lines
diff-options.txt included in their documents, and git-format-patch.txt, git-log.txt, but should not in git-format-patch.txt. I don't know if it should be included in git-diff-files.txt, because git diff-files compare the files in the working tree and the index (so it's the same as "simple" git-diff, which should not be affected by scope?)
Oh, good point. Yeah, git-diff-files.txt should not get this option as it won't have any affect. git-diff-index and git-diff-tree and git-show and git-log should get it, though.
Quoted 15 lines
Quoted 14 lines
Documentation/diff-options.txt For instance, if you configured the `diff.algorithm` variable to a
Quoted 6 lines
Documentation/diff-options.txt +6
-0+int diff_path_in_sparse_checkout(const char *path) { + if (core_sparse_checkout_cone) + return path_in_cone_mode_sparse_checkout(path, the_repository->index); + else + return path_in_sparse_checkout(path, the_repository->index); +}This says we are including the path if it matches the sparsity patterns. Thus, we have to be careful when we use this function, because the relevant paths are ones that match the sparsity specification. The sparsity specification will always match the sparsity patterns when diffing two commits, but when either the index or the working tree is part of the diff, the sparsity specification *might* be temporarily expanded.
Quoted 6 lines
Quoted 2 lines
Yes, I may have to look at more code to better understand how and when the "sparsity specification" is extended. Any recommendations for places to read?
Yes, please review the newer version of my sparse-checkout.txt directions file; it covers this in more detail.
Ok, thanks.
Quoted 25 lines
Quoted 25 lines
Quoted 24 lines
Documentation/diff-options.txt static void do_oneway_diff(struct unpack_trees_options *o,
Quoted 1 line
Documentation/diff-options.txt +0
-0@@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,
do_oneway_diffis for cases where we are diffing against the index...Quoted 6 lines
Documentation/diff-options.txt +3
-0return; /* nothing to diff.. */ } + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) { + if (idx && tree) { + if (!diff_paths_in_sparse_checkout(idx->name, tree->name))Documentation/diff-options.txt static void do_oneway_diff(struct unpack_trees_options *o, · line 474
Quoted 10 lines
Documentation/diff-options.txt +10
-0+ return; + } else if (idx) { + if (!diff_path_in_sparse_checkout(idx->name)) + return; + } else if (tree) { + if (!diff_path_in_sparse_checkout(tree->name)) + return; + } else + return; + }...and you again mistakenly only compare to the sparsity patterns instead of the sparse specification.
Quoted 8 lines
Quoted 2 lines
So here we should use
ce_skip_worktree(idx)to check if this index entry matches sparse specification.
ce_skip_worktree(idx)only checks for expansions to the sparse specification in the working tree. If dealing with index files, the sparse specification is also expanded to handle all files in the index that differ from HEAD.
I will read more code to understand this detail.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Documentation/diff-options.txt +3
-0+ } else + return error(_("invalid --scope value: %s"), optarg); + }Quoted 8 lines
Quoted 4 lines
As written with no follow-on else clause here, wouldn't this silently accept "--scope" without an "=<something>" argument without an error and without properly initializing
opt->scope?Because opt will be initializing with
default_diff_optionsinrepo_diff_setup(), andopt->scopeshould respect config value first. So I don't think there's a mistake here.
Quoted 7 lines
Okay, it's good that you've got the variables initialized somehow, but that's only half the point here. The main point is that the user can specify something that makes no sense and if they do, we should throw an error informing them of their mistake. --scope should not be passed without an argument (either "all" or "sparse", currently), but this code allowed it.
Ah, it makes sense, I will fix it.
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
t/t4070-diff-sparse-checkout-scope.sh
Quoted 2 lines
t/t4070-diff-sparse-checkout-scope.sh new file +0
-0This needs to be fixed.
Since you didn't comment on this but usually do comment on thing, I'll just re-ping it to make sure you don't miss the comment about the incorrect file mode here.
Yes, I've noticed the file permissions issue here, but there may be some other issues with this test file: it fails when running CI tests, presumably because of
some shell syntax incompatibility.
Quoted 15 lines
Quoted 7 lines
Quoted 6 lines
t/t4070-diff-sparse-checkout-scope.sh line 44
Quoted 3 lines
t/t4070-diff-sparse-checkout-scope.sh new file +3
-0+reset_repo () { + rm -rf repo && + git clone --no-checkout temp repoWhy --no-checkout rather than say --sparse?
Quoted 7 lines
This is because I accidentally associated it with a partial clone. I often use "git clone -filter=blob:none -no-checkout" to do a partial clone, then "git sparse- checkout set <pattern>" to set sparse-checkout patterns, and "git checkout" to download the missing blobs and checkout to a branch. But in this test file, we only need sparse-checkout, so it's true that I should not do such strange no-checkout thing.
Quoted 4 lines
Yeah, no need to involve partial clones here. (As an aside, though, I think even with a partial clone that --sparse makes more sense than --no-checkout.)
Using --sparse may indeed be better than -no-checkout, as there is no need to perform an additional checkout after using clone --no-checkout and sparse-checkout. But it uses sparse-checkout cone mode by default.
A little curious, why can't we specify --no-cone mode when doing git clone and why can't we specify sparse-checkout patterns here? If such a feature is available, git clone and git sparse-checkout will be combined in one step.
Quoted 28 lines
Quoted 28 lines
Quoted 27 lines
Quoted 5 lines
t/t4070-diff-sparse-checkout-scope.sh new file +5
-0+change_worktree_and_index() { + ( + cd repo && + mkdir sub2 sub3 && + echo sub1/file3 >sub1/file3 &&t/t4070-diff-sparse-checkout-scope.sh line 67
Quoted 10 lines
t/t4070-diff-sparse-checkout-scope.sh new file +10
-0+ echo sub2/file3 >sub2/file3 && + echo sub3/file3 >sub3/file3 && + echo file3 >file3 && + git add --all --sparse && + echo sub1/file3 >>sub1/file3 && + echo sub2/file3 >>sub2/file3 && + echo sub3/file3 >>sub3/file3 && + echo file3 >>file3 + ) +}It would be nice to modify different paths in the working tree and index, to see if we can handle cases where the sparse specification does not match the sparsity patterns better. (So, modify files inside and outside the sparsity patterns, stage those changes, and then do a
git sparse-checkout reapplyto make the files outside the sparsity patterns disappear from the working tree...then modify different files in the working tree both inside and outside the sparsity patterns. And also remove some file that matches the sparsity patterns and manually mark it asSKIP_WORKTREE.) That'd give us much better coverage.
Quoted 8 lines
Quoted 2 lines
Nice addition. So I should use git update-index --skip-worktree to set
skip_worktreebit for index entries.Well, I'd say use normal "sparse-checkout" commands to set most of them. However, adding one or two extra
SKIP_WORKTREEpaths via runninggit update-index --skip-worktree $PATH(and removing the corresponding local file) would make the testcases more interesting.
Get it.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
I think I'm going to stop reviewing here. I'm probably just going to keep repeating the same issues I identified earlier if I continue.
Thank you very much for your review, you have pointed out very many problems with this patch :)
I'm glad it was helpful. :)
Thanks.
ZheNing Hu
RFC PATCH v2 1/1 diff: introduce --scope option
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
Many of git commands, such as "git grep", "git diff", they will search the "full-tree" scope of the entire git repository, which is reasonable under normal circumstances, but if the user uses sparse checkout in a git monorepo, it's very possible that he just wants to use files within the sparse specification, perhaps because:
* He wants to be able to focus on his subprojects, the output of other subprojects will only interfere with him.
* He's using partial cloning at the same time, and he doesn't want to be able to execute the above git commands download a large number of blobs which out of sparse specification, which is a waste of time and may cause the size of the git repository to gradually expand.
So we need a way to restrict git commands to the sparse specification. Implementing "diff --scope" is the first step in this plan. We are looking for a suitable option to choose:
restrict the path scope of diff to the sparse specification or keep the full tree scope (default action now). "--scope=sparse", "--scope=all" are the parameters corresponding to these two cases.
It is worth noting that "--scope" option only works on diff commands specify "--cached" or "REVISION", because normal "git diff" has retrict the scope of diff files to the sparse specificaiton by default, while "git diff --cached" or "git diff REVSION" will compare to the commit history, and "--scope" options can works here to restrict or not.
Add "--scope" option to git "diff-index" and "git diff-tree" too, because they also meet the above: specify "--cached", or "REVISION". Meanwhile, "git diff-no-index", "git diff-files" don't have this option.
---
[RFC] diff: introduce scope option
In [1], we discovered that users working on different sparse-checkout specification may download unnecessary blobs from each other's specification in collaboration. In [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout specification to elegantly solve this problem above. In [3]: Newren and Derrick Stolee prefer to name the option --scope={sparse, all}.
So this patch is attempt to do this thing on git diff:
v1:
1. add --restrict option to git diff, which restrict diff filespec in sparse-checkout specification. [4]
v2.
1. rename --restrict to --scope={sparse, all}, support --no-scope.
2. add config: diff.scope={sparse,all}.
v3.
1. with the help of newren's review, fix the wrong --scope behavior, its previous meaning was misrepresented as sparse patterns, and now it is fixed to match sparse specification. [5]
2. remove wrong diff.scope config.
3. apply --scope to git diff, git diff-index, git diff-tree.
Since I split --scope into a separate option, this option will not be directly inherited by git commands such as git log, git format-patch, etc. If necessary, we can add it to git log or other commands in a similar way later.
Global scope config haven’t implement yet... Since we haven't decided on an appropriate name for scope config. [6]
[1]:
https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/[email protected]/ [3]:
https://lore.kernel.org/git/[email protected]/
[4]:
https://lore.kernel.org/git/[email protected]/
[5]:
https://lore.kernel.org/git/CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com/
[6]:
https://lore.kernel.org/git/CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1398
Contributor requested no range-diff. You can review it using these commands:
git fetch https://github.com/gitgitgadget/git 63bba4fd 471a0691 git range-diff <options> 63bba4fd..93ddbcbd c000d916..471a0691
Documentation/diff-options.txt | 33 +++ builtin/diff-index.c | 29 ++- builtin/diff-tree.c | 15 ++ builtin/diff.c | 46 ++++- cache.h | 5 + diff-lib.c | 44 ++++ diff.c | 2 + diff.h | 8 + dir.c | 20 ++ dir.h | 4 + t/t4070-diff-sparse-checkout-scope.sh | 286 ++++++++++++++++++++++++++ tree-diff.c | 11 + 12 files changed, 497 insertions(+), 6 deletions(-) create mode 100755 t/t4070-diff-sparse-checkout-scope.sh
Documentation/diff-options.txt +33 -0
@@ -195,6 +195,39 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifdef::git-diff[] +ifdef::git-diff-index[] +ifdef::git-diff-tree[] + +--scope=[sparse|all]:: + Restrict or not restrict diff path scope in sparse specification. + The variants are as follows: + ++ +-- +`sparse`;; + When using diff to compare commit history, restrict the + scope of file path comparisons to the sparse specification. + See sparse specification in link:technical/sparse-checkout.html + [the sparse-checkout design document] for more information. +`all`;; + When using diff to compare commit history, the file comparison + scope is full-tree. This is consistent with the current default + behavior. +-- ++ + +Note that `--scope` option only take effect if diff command specify +`--cached` or `REVISION`. + +The behavior of this `--scope` option is experimental and may change +in the future. See link:technical/sparse-checkout.html [the sparse-checkout +design document] for more information. + +endif::git-diff-tree[] +endif::git-diff-index[] +endif::git-diff[] + --stat[=<width>[,<name-width>[,<count>]]]:: Generate a diffstat. By default, as much space as necessary will be used for the filename part, and the rest for the graph
builtin/diff-index.c +26 -3
@@ -20,6 +20,14 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)unsigned int option =0 ;int i;int result; +enum sparse_scope scope; + +struct option sparse_scope_options[] = { +OPT_CALLBACK_F (0 ,"scope" , &scope,N_ ("[sparse|all]" ), +N_ ("restrict path scope in sparse specification" ), +PARSE_OPT_NONEG , diff_opt_sparse_scope), +OPT_END () + };if (argc ==2 && !strcmp (argv[1 ],"-h" ))usage (diff_cache_usage);
@@ -36,6 +44,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)diff_merges_suppress_m_parsing (); argc =setup_revisions (argc, argv, &rev,NULL ); + + argc =parse_options (argc, argv, prefix, sparse_scope_options,NULL , +PARSE_OPT_KEEP_DASHDASH | +PARSE_OPT_KEEP_UNKNOWN_OPT | +PARSE_OPT_KEEP_ARGV0 | +PARSE_OPT_NO_INTERNAL_HELP ); + + rev.diffopt.scope = scope; +for (i =1 ; i < argc; i++) {const char *arg = argv[i];
@@ -66,9 +83,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)perror ("read_cache_preload" );return -1 ; } - }else if (read_cache () <0 ) { -perror ("read_cache" ); -return -1 ; + }else { +if (read_cache () <0 ) { +perror ("read_cache" ); +return -1 ; + } +if (rev.diffopt.scope ==SPARSE_SCOPE_SPARSE && +strcmp (rev.pending.objects[0 ].name,"HEAD" )) +diff_collect_changes_index (&rev.diffopt.pathspec, + &rev.diffopt.change_index_files); } result =run_diff_index (&rev, option); result =diff_result_code (&rev.diffopt, result);
builtin/diff-tree.c +15 -0
@@ -114,6 +114,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)struct userformat_want w;int read_stdin =0 ;int merge_base =0 ; +enum sparse_scope scope; + +struct option sparse_scope_options[] = { +OPT_CALLBACK_F (0 ,"scope" , &scope,N_ ("[sparse|all]" ), +N_ ("restrict path scope in sparse specification" ), +PARSE_OPT_NONEG , diff_opt_sparse_scope), +OPT_END () + };if (argc ==2 && !strcmp (argv[1 ],"-h" ))usage (diff_tree_usage);
@@ -131,6 +139,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) prefix =precompose_argv_prefix (argc, argv, prefix); argc =setup_revisions (argc, argv, opt, &s_r_opt); + argc =parse_options (argc, argv, prefix, sparse_scope_options,NULL , +PARSE_OPT_KEEP_DASHDASH | +PARSE_OPT_KEEP_UNKNOWN_OPT | +PARSE_OPT_KEEP_ARGV0 | +PARSE_OPT_NO_INTERNAL_HELP ); + opt->diffopt.scope = scope; +memset (&w,0 ,sizeof (w));userformat_find_requirements (NULL , &w);
builtin/diff.c +43 -3
70 lines not on this page · raw message
cache.h +5 -0
@@ -1082,6 +1082,11 @@ extern int core_apply_sparse_checkout;extern int core_sparse_checkout_cone;extern int sparse_expect_files_outside_of_patterns; +enum sparse_scope { +SPARSE_SCOPE_ALL =0 , +SPARSE_SCOPE_SPARSE , +}; +/* * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value). */
diff-lib.c +44 -0
56 lines not on this page · raw message
diff.c +2 -0
@@ -4663,6 +4663,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; +strset_init (&options->change_index_files);prep_parse_options (options); }
@@ -6511,6 +6512,7 @@ void diff_free(struct diff_options *options)diff_free_ignore_regex (options);clear_pathspec (&options->pathspec);FREE_AND_NULL (options->parseopts); +strset_clear (&options->change_index_files); }void diff_flush (struct diff_options *options)
diff.h +8 -0
@@ -8,6 +8,7 @@#include "pathspec.h" #include "object.h" #include "oidset.h" +#include "strmap.h" /** * The diff API is for programs that compare two sets of files (e.g. two trees,
@@ -285,6 +286,9 @@ struct diff_options {/* diff-filter bits */ unsigned int filter, filter_not; +/* diff sparse-checkout scope */ +enum sparse_scope scope; +int use_color;/* Number of context lines to generate in patch output. */
@@ -397,6 +401,7 @@ struct diff_options {struct option *parseopts;struct strmap *additional_path_headers; +struct strset change_index_files;int no_free; };
@@ -696,4 +701,7 @@ void print_stat_summary(FILE *fp, int files, int insertions, int deletions);void setup_diff_pager (struct diff_options *); +void diff_collect_changes_index (struct pathspec *pathspec,struct strset *files); +int diff_opt_sparse_scope (const struct option *option,const char *optarg,int unset); +#endif /* DIFF_H */
dir.c +20 -0
@@ -1503,6 +1503,26 @@ int path_in_cone_mode_sparse_checkout(const char *path,return path_in_sparse_checkout_1 (path, istate,1 ); } +int path_in_sparse_patterns (const char *path) { +return path_in_sparse_checkout_1 (path, the_repository->index, core_sparse_checkout_cone); +} + +/* Expand sparse-checkout specification (worktree) */ +int worktree_file_in_sparse_specification (const struct cache_entry *worktree_check_ce) +{ +return worktree_check_ce && !ce_skip_worktree (worktree_check_ce); +} + +/* Expand sparse-checkout specification (index) */ +int index_file_in_sparse_specification (const struct cache_entry *ce,struct strset *change_index_files) +{ +if (!ce->ce_namelen) +return 0 ; +if (change_index_files &&strset_contains (change_index_files, ce->name)) +return 1 ; +return path_in_sparse_patterns (ce->name); +} + static struct path_pattern *last_matching_pattern_from_lists( struct dir_struct *dir, struct index_state *istate, const char *pathname, int pathlen,
dir.h +4 -0
@@ -4,6 +4,7 @@#include "cache.h" #include "hashmap.h" #include "strbuf.h" +#include "strmap.h" /** * The directory listing API is used to enumerate paths in the work tree,
@@ -401,6 +402,9 @@ int path_in_sparse_checkout(const char *path, struct index_state *istate);int path_in_cone_mode_sparse_checkout (const char *path,struct index_state *istate); +int path_in_sparse_patterns (const char *path); +int index_file_in_sparse_specification (const struct cache_entry *ce,struct strset *change_index_files); +int worktree_file_in_sparse_specification (const struct cache_entry *worktree_check_ce); struct dir_entry *dir_add_ignored(struct dir_struct *dir, struct index_state *istate,
t/t4070-diff-sparse-checkout-scope.sh new file +286 -0
286 lines not on this page · raw message
tree-diff.c +11 -0
@@ -5,6 +5,7 @@#include "diff.h" #include "diffcore.h" #include "tree.h" +#include "dir.h" /* * internal mode marker, saying a tree entry != entry of tp[imin]
@@ -76,6 +77,16 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)static int emit_diff_first_parent_only (struct diff_options *opt,struct combine_diff_path *p) {struct combine_diff_parent *p0 = &p->parent[0 ]; + +if (opt->scope ==SPARSE_SCOPE_SPARSE ) { +struct strbuf sb =STRBUF_INIT ; + +strbuf_addstr (&sb, p->path); +if (S_ISDIR (p->mode) ||S_ISDIR (p0->mode)) +strbuf_addch (&sb,'/' ); +if (!path_in_sparse_patterns (sb.buf)) +return 0 ; + }if (p->mode && p0->mode) { opt->change (opt, p0->mode, p->mode, &p0->oid, &p->oid,1 ,1 , p->path,0 ,0 );
base-commit: c000d916380bb59db69c78546928eadd076b9c7d
Signature · 1 line
gitgitgadget
v3 cover diff: introduce scope option
In [1], we discovered that users working on different sparse-checkout specification may download unnecessary blobs from each other's specification in collaboration. In [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout specification to elegantly solve this problem above. In [3]: Newren and Derrick Stolee prefer to name the option --scope={sparse, all}.
So this patch is attempt to do this thing on git diff:
v1:
1. add --restrict option to git diff, which restrict diff filespec in sparse-checkout specification. [4]
v2.
1. rename --restrict to --scope={sparse, all}, support --no-scope.
2. add config: diff.scope={sparse,all}.
v3.
1. with the help of newren's review, fix the wrong --scope behavior, its previous meaning was misrepresented as sparse patterns, and now it is fixed to match sparse specification. [5]
2. remove wrong diff.scope config.
3. apply --scope to git diff, git diff-index, git diff-tree.
v4.
1. create a OPT_SPARSE_SCOPE macro for easier add --scope option to other git commands later.
2. introduce --scope option to "git grep ".
Since I split --scope into a separate option, this option will not be directly inherited by git commands such as git log, git format-patch, etc. If necessary, we can add it to git log or other commands in a similar way later.
Global scope config haven’t implement yet... Since we haven't decided on an appropriate name for scope config. [6]
Note why I don't add --scope to "git grep --cached", because we can't use "" with "--cached", whether we use --scope=sparse or not , the resulting set of files it gets is the same.
[1]:
https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/[email protected]/ [3]:
https://lore.kernel.org/git/[email protected]/
[4]:
https://lore.kernel.org/git/[email protected]/
[5]:
https://lore.kernel.org/git/CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com/
[6]:
https://lore.kernel.org/git/CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com/
ZheNing Hu (2):
[RFC] diff: introduce --scope option
[RPC] grep: introduce --scope option
Documentation/diff-options.txt | 33 ++++ Documentation/git-grep.txt | 24 +++ builtin/diff-index.c | 24 ++- builtin/diff-tree.c | 11 ++ builtin/diff.c | 23 ++- builtin/grep.c | 10 + cache.h | 5 + diff-lib.c | 43 +++++ diff.c | 2 + diff.h | 7 + dir.c | 52 ++++++ dir.h | 4 + grep.h | 2 + parse-options.h | 7 + t/t1090-sparse-checkout-scope.sh | 303 +++++++++++++++++++++++++++++++ tree-diff.c | 7 + 16 files changed, 551 insertions(+), 6 deletions(-)
base-commit: 815c1e82021edbd99a2c423cf27f28863f28cef3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1398
Contributor requested no range-diff. You can review it using these commands:
git fetch https://github.com/gitgitgadget/git c000d916 1d1b6618 git range-diff <options> c000d916..471a0691 083e0127..1d1b6618
Signature · 1 line
gitgitgadget
RFC PATCH v3 1/2 diff: introduce --scope option
From: ZheNing Hu <[email protected]>
Many of git commands, such as "git grep", "git diff", they will search the "full-tree" scope of the entire git repository, which is reasonable under normal circumstances, but if the user uses sparse checkout in a git monorepo, it's very possible that he just wants to use files within the sparse specification, perhaps because:
* He wants to be able to focus on his subprojects, the output of other subprojects will only interfere with him.
* He's using partial cloning at the same time, and he doesn't want to be able to execute the above git commands download a large number of blobs which out of sparse specification, which is a waste of time and may cause the size of the git repository to gradually expand.
So we need a way to restrict git commands to the sparse specification. Implementing "diff --scope" is the first step in this plan. We are looking for a suitable option to choose:
restrict the path scope of diff to the sparse specification or keep the full tree scope (default action now). "--scope=sparse", "--scope=all" are the parameters corresponding to these two cases.
It is worth noting that "--scope" option only works on diff commands specify "--cached" or "REVISION", because normal "git diff" has retrict the scope of diff files to the sparse specificaiton by default, while "git diff --cached" or "git diff REVSION" will compare to the commit history, and "--scope" options can works here to restrict or not.
Add "--scope" option to git "diff-index" and "git diff-tree" too, because they also meet the above: specify "--cached", or "REVISION". Meanwhile, "git diff-no-index", "git diff-files" don't have this option.
--- Documentation/diff-options.txt | 33 ++++ builtin/diff-index.c | 24 ++- builtin/diff-tree.c | 11 ++ builtin/diff.c | 23 ++- cache.h | 5 + diff-lib.c | 43 +++++ diff.c | 2 + diff.h | 7 + dir.c | 52 ++++++ dir.h | 4 + parse-options.h | 7 + t/t1090-sparse-checkout-scope.sh | 276 +++++++++++++++++++++++++++++++ tree-diff.c | 7 + 13 files changed, 488 insertions(+), 6 deletions(-)
Documentation/diff-options.txt +33 -0
@@ -195,6 +195,39 @@ For instance, if you configured the `diff.algorithm` variable to a non-default value and want to use the default one, then you have to use `--diff-algorithm=default` option. +ifdef::git-diff[] +ifdef::git-diff-index[] +ifdef::git-diff-tree[] + +--scope=[sparse|all]:: + Restrict or not restrict diff path scope in sparse specification. + The variants are as follows: + ++ +-- +`sparse`;; + When using diff to compare commit history, restrict the + scope of file path comparisons to the sparse specification. + See sparse specification in link:technical/sparse-checkout.html + [the sparse-checkout design document] for more information. +`all`;; + When using diff to compare commit history, the file comparison + scope is full-tree. This is consistent with the current default + behavior. +-- ++ + +Note that `--scope` option only take effect if diff command specify +`--cached` or `REVISION`. + +The behavior of this `--scope` option is experimental and may change +in the future. See link:technical/sparse-checkout.html [the sparse-checkout +design document] for more information. + +endif::git-diff-tree[] +endif::git-diff-index[] +endif::git-diff[] + --stat[=<width>[,<name-width>[,<count>]]]:: Generate a diffstat. By default, as much space as necessary will be used for the filename part, and the rest for the graph
builtin/diff-index.c +21 -3
@@ -20,6 +20,11 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)int i;int result; +struct option sparse_scope_options[] = { +OPT_SPARSE_SCOPE (&rev.diffopt.scope), +OPT_END () + }; +if (argc ==2 && !strcmp (argv[1 ],"-h" ))usage (diff_cache_usage);
@@ -35,6 +40,13 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)diff_merges_suppress_m_parsing (); argc =setup_revisions (argc, argv, &rev,NULL ); + + argc =parse_options (argc, argv, prefix, sparse_scope_options,NULL , +PARSE_OPT_KEEP_DASHDASH | +PARSE_OPT_KEEP_UNKNOWN_OPT | +PARSE_OPT_KEEP_ARGV0 | +PARSE_OPT_NO_INTERNAL_HELP ); +for (i =1 ; i < argc; i++) {const char *arg = argv[i];
@@ -65,9 +77,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)perror ("repo_read_index_preload" );return -1 ; } - }else if (repo_read_index (the_repository) <0 ) { -perror ("repo_read_index" ); -return -1 ; + }else { +if (repo_read_index (the_repository) <0 ) { +perror ("read_cache" ); +return -1 ; + } +if (rev.diffopt.scope ==SPARSE_SCOPE_SPARSE && +strcmp (rev.pending.objects[0 ].name,"HEAD" )) +diff_collect_changes_index (&rev.diffopt.pathspec, + &rev.diffopt.change_index_files); } result =run_diff_index (&rev, option); result =diff_result_code (&rev.diffopt, result);
builtin/diff-tree.c +11 -0
@@ -115,6 +115,11 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)int read_stdin =0 ;int merge_base =0 ; +struct option sparse_scope_options[] = { +OPT_SPARSE_SCOPE (&opt->diffopt.scope), +OPT_END () + }; +if (argc ==2 && !strcmp (argv[1 ],"-h" ))usage (diff_tree_usage);
@@ -131,6 +136,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) prefix =precompose_argv_prefix (argc, argv, prefix); argc =setup_revisions (argc, argv, opt, &s_r_opt); + argc =parse_options (argc, argv, prefix, sparse_scope_options,NULL , +PARSE_OPT_KEEP_DASHDASH | +PARSE_OPT_KEEP_UNKNOWN_OPT | +PARSE_OPT_KEEP_ARGV0 | +PARSE_OPT_NO_INTERNAL_HELP ); +memset (&w,0 ,sizeof (w));userformat_find_requirements (NULL , &w);
builtin/diff.c +20 -3
@@ -162,9 +162,15 @@ static int builtin_diff_index(struct rev_info *revs,perror ("repo_read_index_preload" );return -1 ; } - }else if (repo_read_index (the_repository) <0 ) { -perror ("repo_read_cache" ); -return -1 ; + }else { +if (repo_read_index (the_repository) <0 ) { +perror ("read_cache" ); +return -1 ; + } +if (revs->diffopt.scope ==SPARSE_SCOPE_SPARSE && +strcmp (revs->pending.objects[0 ].name,"HEAD" )) +diff_collect_changes_index (&revs->diffopt.pathspec, + &revs->diffopt.change_index_files); }return run_diff_index (revs, option); }
@@ -403,6 +409,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)int result =0 ;struct symdiff sdiff; +struct option sparse_scope_options[] = { +OPT_SPARSE_SCOPE (&rev.diffopt.scope), +OPT_END () + }; +/* * We could get N tree-ish in the rev.pending_objects list. * Also there could be M blobs there, and P pathspecs. --cached may
@@ -507,6 +518,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)diff_setup_done (&rev.diffopt); } + argc =parse_options (argc, argv, prefix, sparse_scope_options,NULL , +PARSE_OPT_KEEP_DASHDASH | +PARSE_OPT_KEEP_UNKNOWN_OPT | +PARSE_OPT_KEEP_ARGV0 | +PARSE_OPT_NO_INTERNAL_HELP ); + rev.diffopt.flags.recursive =1 ; rev.diffopt.rotate_to_strict =1 ;
cache.h +5 -0
@@ -1058,6 +1058,11 @@ extern int core_apply_sparse_checkout;extern int core_sparse_checkout_cone;extern int sparse_expect_files_outside_of_patterns; +enum sparse_scope { +SPARSE_SCOPE_ALL =0 , +SPARSE_SCOPE_SPARSE , +}; +/* * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value). */
diff-lib.c +43 -0
55 lines not on this page · raw message
diff.c +2 -0
@@ -4663,6 +4663,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; +strset_init (&options->change_index_files);prep_parse_options (options); }
@@ -6514,6 +6515,7 @@ void diff_free(struct diff_options *options)diff_free_ignore_regex (options);clear_pathspec (&options->pathspec);FREE_AND_NULL (options->parseopts); +strset_clear (&options->change_index_files); }void diff_flush (struct diff_options *options)
diff.h +7 -0
@@ -8,6 +8,7 @@#include "pathspec.h" #include "object.h" #include "oidset.h" +#include "strmap.h" /** * The diff API is for programs that compare two sets of files (e.g. two trees,
@@ -285,6 +286,9 @@ struct diff_options {/* diff-filter bits */ unsigned int filter, filter_not; +/* diff sparse-checkout scope */ +enum sparse_scope scope; +int use_color;/* Number of context lines to generate in patch output. */
@@ -397,6 +401,7 @@ struct diff_options {struct option *parseopts;struct strmap *additional_path_headers; +struct strset change_index_files;int no_free; };
@@ -696,4 +701,6 @@ void print_stat_summary(FILE *fp, int files, int insertions, int deletions);void setup_diff_pager (struct diff_options *); +void diff_collect_changes_index (struct pathspec *pathspec,struct strset *files); +#endif /* DIFF_H */
dir.c +52 -0
64 lines not on this page · raw message
dir.h +4 -0
@@ -4,6 +4,7 @@#include "cache.h" #include "hashmap.h" #include "strbuf.h" +#include "strmap.h" /** * The directory listing API is used to enumerate paths in the work tree,
@@ -401,6 +402,9 @@ int path_in_sparse_checkout(const char *path, struct index_state *istate);int path_in_cone_mode_sparse_checkout (const char *path,struct index_state *istate); +int path_in_sparse_patterns (const char *path,int is_dir); +int index_file_in_sparse_specification (const struct cache_entry *ce,struct strset *change_index_files); +int worktree_file_in_sparse_specification (const struct cache_entry *worktree_check_ce); struct dir_entry *dir_add_ignored(struct dir_struct *dir, struct index_state *istate,
parse-options.h +7 -0
@@ -356,6 +356,13 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);/* value is enum branch_track* */ int parse_opt_tracking_mode (const struct option *,const char *,int ); +int opt_sparse_scope (const struct option *option, +const char *optarg,int unset); + +#define OPT_SPARSE_SCOPE (var) OPT_CALLBACK_F(0, "scope", (var), N_("[sparse|all]"), \ + N_("restrict path scope in sparse specification"), \ + PARSE_OPT_NONEG, opt_sparse_scope) +#define OPT__VERBOSE (var, h) OPT_COUNTUP('v', "verbose", (var), (h))#define OPT__QUIET (var, h) OPT_COUNTUP('q', "quiet", (var), (h))#define OPT__VERBOSITY (var) \
t/t1090-sparse-checkout-scope.sh +276 -0
280 lines not on this page · raw message
tree-diff.c +7 -0
@@ -5,6 +5,7 @@#include "diff.h" #include "diffcore.h" #include "tree.h" +#include "dir.h" /* * internal mode marker, saying a tree entry != entry of tp[imin]
@@ -76,6 +77,12 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)static int emit_diff_first_parent_only (struct diff_options *opt,struct combine_diff_path *p) {struct combine_diff_parent *p0 = &p->parent[0 ]; + +if (opt->scope ==SPARSE_SCOPE_SPARSE && + !path_in_sparse_patterns (p->path, +S_ISDIR (p->mode) || +S_ISDIR (p0->mode))) +return 0 ;if (p->mode && p0->mode) { opt->change (opt, p0->mode, p->mode, &p0->oid, &p->oid,1 ,1 , p->path,0 ,0 );
Signature · 1 line
gitgitgadget
PATCH v3 2/2 grep: introduce --scope option
In reply to ZheNing Hu via GitGitGadgetFrom: ZheNing Hu <[email protected]>
Because we may want to restrict grep's file scope to sparse specification, Apply the --scope option we implemented in diff to grep as well.
--scope=sparse mean that the search file scope restrict to sparse specification when we grep something in commit history, and --scope=all mean that the search file scope will be full-tree.
Note that --scope option only oly takes effect when "git grep <tree>" is specified.
--- Documentation/git-grep.txt | 24 ++++++++++++++++++++++++ builtin/grep.c | 10 ++++++++++ grep.h | 2 ++ t/t1090-sparse-checkout-scope.sh | 27 +++++++++++++++++++++++++++ 4 files changed, 63 insertions(+)
Documentation/git-grep.txt +24 -0
@@ -28,6 +28,7 @@ SYNOPSIS [-f <file>] [-e] <pattern> [--and|--or|--not|(|)|-e <pattern>...] [--recurse-submodules] [--parent-basename <basename>] + [--scope=(sparse|all)] [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...] [--] [<pathspec>...]
@@ -296,6 +297,29 @@ question doesn't support them. Do not output matched lines; instead, exit with status 0 when there is a match and with non-zero status when there isn't. +--scope=(sparse|all):: + Restrict or not restrict grep path scope in sparse specification. + The variants are as follows: + ++ +-- +`sparse`;; + When grep in commit history, restrict the scope of file path + to the sparse specification. See sparse specification in + link:technical/sparse-checkout.html [the sparse-checkout design + document] for more information. +`all`;; + When grep in commit history, the file path scope is full-tree. + This is consistent with the current default behavior. +-- ++ + +Note that `--scope` option only take effect if git command specify `<tree>`. + +The behavior of this `--scope` option is experimental and may change +in the future. See link:technical/sparse-checkout.html [the sparse-checkout +design document] for more information. + <tree>...:: Instead of searching tracked files in the working tree, search blobs in the given trees.
builtin/grep.c +10 -0
@@ -640,6 +640,15 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, }strbuf_add (base, entry.path, te_len); +if (opt->scope ==SPARSE_SCOPE_SPARSE && + base->len != tn_len && + !path_in_sparse_patterns (base->buf + tn_len, +S_ISDIR (entry.mode) || +S_ISGITLINK (entry.mode))) { +strbuf_setlen (base, old_baselen); +continue ; + } +if (S_ISREG (entry.mode)) { hit |= grep_oid(opt, &entry.oid, base->buf, tn_len,
@@ -999,6 +1008,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) PARSE_OPT_NOCOMPLETE),OPT_INTEGER ('m' ,"max-count" , &opt.max_count,N_ ("maximum number of results per file" )), +OPT_SPARSE_SCOPE (&opt.scope), OPT_END() }; grep_prefix = prefix;
grep.h +2 -0
@@ -22,6 +22,7 @@ typedef int pcre2_general_context;#endif #include "thread-utils.h" #include "userdiff.h" +#include "cache.h" struct repository ;
@@ -175,6 +176,7 @@ struct grep_opt {void (*output)(struct grep_opt *opt,const void *data,size_t size);void *output_priv; +enum sparse_scope scope; };#define GREP_OPT_INIT { \
t/t1090-sparse-checkout-scope.sh +27 -0
@@ -382,4 +382,31 @@ M in test_cmp expected actual ' +# git grep TREE + +test_expect_success 'git grep --scope=all' ' + reset_sparse_checkout_state && + cat >expected <<-EOF && +HEAD~:in/1 +HEAD~:out1/1 +HEAD~:out1/2 +HEAD~:out1/3 +HEAD~:out1/4 +HEAD~:out1/5 +HEAD~:out1/6 +HEAD~:out2/1 + EOF + git -C repo grep --name-only --scope=all 1 HEAD~ >actual && + test_cmp expected actual +' + +test_expect_success 'git grep --scope=sparse' ' + reset_sparse_checkout_state && + cat >expected <<-EOF && +HEAD~:in/1 + EOF + git -C repo grep --name-only --scope=sparse 1 HEAD~ >actual && + test_cmp expected actual +' + test_done
Signature · 1 line
gitgitgadget