Re: [PATCH] [RFC] diff: introduce scope option
From: ZheNing Hu <hidden>
Date: 2022-11-06 02:17:19
inHi, Elijah Newren [off-list ref] 于2022年11月1日周二 13:18写道：
Hi, On Sun, Oct 30, 2022 at 9:11 PM ZheNing Hu via GitGitGadget [off-list ref] wrote:quoted
From: ZheNing Hu <redacted> 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/pull.1367.v3.git.1665269538608.gitgitgadget@gmail.com/ (local)
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
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.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.
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
`--no-scope` is 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
Add `diff.scope={sparse, all}` config, which can also have the same capabilities as `--scope`, and it will be covered by `--scope` option.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
Signed-off-by: ZheNing Hu <redacted> --- [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.log & grep should accept a similar flag. format-patch should not, and should ignore any config in this area.quoted
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
[1]: https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/ (local) [2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ (local) [3]: https://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/ (local) [4]: https://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/ (local) 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.shdiff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt index 35a7bf86d77..52707e1b2d6 100644 --- a/Documentation/config/diff.txt +++ b/Documentation/config/diff.txt@@ -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.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
diff.wsErrorHighlight:: Highlight whitespace errors in the `context`, `old` or `new` lines of the diff. Multiple values are separated by comma,diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 3674ac48e92..04bf83e8be1 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt@@ -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[]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
+ --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 graphdiff --git a/builtin/diff.c b/builtin/diff.c index 854d2c5a5c4..6b450f7184c 100644 --- a/builtin/diff.c +++ b/builtin/diff.c@@ -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 c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e 72d42bd856228c15f702fa3c353432f4f1defe03 (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.
Yeah, it's buggy that I forget to check two blobs without paths.
quoted
+ if (opt->flags.reverse_diff) { SWAP(old_mode, new_mode); SWAP(old_oid, new_oid);diff --git a/diff-lib.c b/diff-lib.c index 2edea41a234..a3381f2e0ff 100644 --- a/diff-lib.c +++ b/diff-lib.c@@ -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.
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
+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
+} + + 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.
Agree.
quoted
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;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
/*@@ -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
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; + }...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
+ /* 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)));quoted
diff --git a/diff-no-index.c b/diff-no-index.c index 18edbdf4b59..ea94a104ea4 100644 --- a/diff-no-index.c +++ b/diff-no-index.c@@ -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.
Yeah, we should care about untracked files sparse-checkout only when we are using git add/rm/update-index...
quoted
+ revs->diffopt.skip_stat_unmatch = 1; if (!revs->diffopt.output_format) revs->diffopt.output_format = DIFF_FORMAT_PATCH;diff --git a/diff.c b/diff.c index 285d6e2d575..9de4044ae05 100644 --- a/diff.c +++ b/diff.c@@ -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;Why is this called "external"?
Learn from external_diff_cmd_cfg, I should remove it.
quoted
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;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
+ } else if (optarg) {...which would also allow you to drop this if and dedent the rest of the function.
Agree.
quoted
+ 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?
Agree.
quoted
+ } 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
+ + 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"?
OK.
quoted
+ 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 --git a/diff.h b/diff.h index 8ae18e5ab1e..90f7512034c 100644 --- a/diff.h +++ b/diff.h@@ -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 */diff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh new file mode 100644This needs to be fixed.quoted
index 00000000000..dca75a3308b--- /dev/null +++ b/t/t4070-diff-sparse-checkout-scope.sh@@ -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 + ) +' + +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
+} + +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
+} + +change_worktree_and_index() { + ( + cd repo && + mkdir sub2 sub3 && + echo sub1/file3 >sub1/file3 && + 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.
Nice addition. So I should use git update-index --skip-worktree to set skip_worktree bit for index entries.
quoted
+ +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~") +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
+ +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 +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
+ +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[@]}" + +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
+ +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).
Agree.
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