| Message ID | a09a5098aa66ea0ed89fe0fcde3f016b4a65814d.1610116600.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/ls-files.c:add git ls-file --dedup option | expand |
On Fri, Jan 8, 2021 at 9:36 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > builtin:ls-files.c:add git ls-file --dedup option This subject concisely explains the purpose of the patch. That's good. A more typical way to write it would be: ls-files: add --dedup option > This commit standardizes the code format. Fixing problems pointed out by reviewers is good. Normally, however, when you submit a new version of your patch or patch series, you should apply these fixes directly to the patch(es) which introduced the problems in the first place rather than adding one or more additional patches to fix problems introduced in earlier patches. To do this, you typically would use `git rebase -i` or `git commit --amend` to squash the fixes into the problematic patches. Thus, when you re-submit the patches, they will appear to be "perfect". For this particular two-patch series, patch [2/2] is doing two things: (1) fixing style problems from patch [1/2], and (2) adding documentation and tests which logically belong with the feature added by patch [1/2]. Taking the above advice into account, a better presentation when you re-submit this series would be to squash these two patches into a single patch. > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt > @@ -81,6 +82,9 @@ OPTIONS > +--dedup:: > + Suppress duplicates entries when conflicts happen or > + specify -d -m at the same time. For consistency with typesetting elsewhere in this file, use backticks around the command-line options. It also often is a good idea to spell the options using long form since it is typically easier to search for the long form of an option in documentation. So, perhaps the above can be written like this: Suppress duplicate entries when `--deleted` and `--modified` are combined. > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > - const struct cache_entry *last_stage=NULL; > + const struct cache_entry *last_stage = NULL; > - if(show_cached && delete_dup){ > + if (show_cached && delete_dup) { > - last_stage=ce; > + last_stage = ce; > - if(delete_dup){ > + if (delete_dup) { > - if(delete_dup && show_deleted && show_modified && err) > + if (delete_dup && show_deleted && show_modified && err) > - else{ > - if (show_deleted && err)/* you can't find it,so it's actually removed at all! */ > + else { > + if (show_deleted && err) As mentioned above, these style fixes should be squashed into the first patch, rather than being done in a separate patch, so that reviewers see a nicely polished patch rather than a patch which requires later fixing up. > diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh > @@ -0,0 +1,63 @@ > +test_expect_success 'master branch setup and write expect1 expect2 and commit' ' We usually give this test a simple title such as "setup" so that we don't have to worry about the title becoming outdated as people make changes to the test itself. > + touch a.txt && > + touch b.txt && > + touch delete.txt && On this project, we use `touch` when the timestamp of the empty files is important to the test. If the timestamp is not important, then we just use `>`, like this: >a.txt && >b.txt && >delete.txt && > + cat <<-EOF >expect1 && > + M a.txt > + H b.txt > + H delete.txt > + H expect1 > + H expect2 > + EOF > + cat <<-EOF >expect2 && > + C a.txt > + R delete.txt > + EOF When no variables are being interpolated in the here-doc content, we use -\EOF to let readers know that the here-doc body is literal. So: cat >expect1 <<-\EOF && ... EOF > + git add a.txt b.txt delete.txt expect1 expect2 && > + git commit -m master:1 > +' > + > +test_expect_success 'main commit again' ' > + echo a>a.txt && > + echo b>b.txt && > + echo delete>delete.txt && > + git add a.txt b.txt delete.txt && > + git commit -m master:2 > +' > + > +test_expect_success 'dev commit' ' > + git checkout HEAD~ && > + git switch -c dev && > + echo change>a.txt && > + git add a.txt && > + git commit -m dev:1 > +' These two tests following the "setup" test also seem to be doing setup tasks rather than testing the new --dedup functionality. If this is the case, then it probably would make sense to combine all three tests into a single "setup" test. > +test_expect_success 'dev merge master' ' > + test_must_fail git merge master && > + git ls-files -t --dedup >actual1 && > + test_cmp expect1 actual1 && > + rm delete.txt && > + git ls-files -d -m -t --dedup >actual2 && > + test_cmp expect2 actual2 > +' Do you foresee that people will add more tests to this file which will use the files and branches set up by the "setup" test(s)? If not, if those branches and files are only ever going to be used by this one test, then it probably would be better to combine all the above code into a single test.
You can see that the coding and documentation of GIT community are really very standard, which may be one of the things I lack and need to improve ;) Thanks for patiently correct my errors. Eric Sunshine <sunshine@sunshineco.com> 于2021年1月14日周四 下午2:39写道： > > On Fri, Jan 8, 2021 at 9:36 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > builtin:ls-files.c:add git ls-file --dedup option > > This subject concisely explains the purpose of the patch. That's good. > A more typical way to write it would be: > > ls-files: add --dedup option > OK.I will correct it more specification. > > This commit standardizes the code format. > > Fixing problems pointed out by reviewers is good. Normally, however, > when you submit a new version of your patch or patch series, you > should apply these fixes directly to the patch(es) which introduced > the problems in the first place rather than adding one or more > additional patches to fix problems introduced in earlier patches. To > do this, you typically would use `git rebase -i` or `git commit > --amend` to squash the fixes into the problematic patches. Thus, when > you re-submit the patches, they will appear to be "perfect". > > For this particular two-patch series, patch [2/2] is doing two things: > (1) fixing style problems from patch [1/2], and (2) adding > documentation and tests which logically belong with the feature added > by patch [1/2]. Taking the above advice into account, a better > presentation when you re-submit this series would be to squash these > two patches into a single patch. > I thought before this was gitgitgadget would sent duplicate patch over and over again. It seems like I really should go straight ahead and squash my commits , so I know what I should do. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt > > @@ -81,6 +82,9 @@ OPTIONS > > +--dedup:: > > + Suppress duplicates entries when conflicts happen or > > + specify -d -m at the same time. > > For consistency with typesetting elsewhere in this file, use backticks > around the command-line options. It also often is a good idea to spell > the options using long form since it is typically easier to search for > the long form of an option in documentation. So, perhaps the above can > be written like this: > > Suppress duplicate entries when `--deleted` and `--modified` are > combined. > > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > > - const struct cache_entry *last_stage=NULL; > > + const struct cache_entry *last_stage = NULL; > > - if(show_cached && delete_dup){ > > + if (show_cached && delete_dup) { > > - last_stage=ce; > > + last_stage = ce; > > - if(delete_dup){ > > + if (delete_dup) { > > - if(delete_dup && show_deleted && show_modified && err) > > + if (delete_dup && show_deleted && show_modified && err) > > - else{ > > - if (show_deleted && err)/* you can't find it,so it's actually removed at all! */ > > + else { > > + if (show_deleted && err) > > As mentioned above, these style fixes should be squashed into the > first patch, rather than being done in a separate patch, so that > reviewers see a nicely polished patch rather than a patch which > requires later fixing up. > > > diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh > > @@ -0,0 +1,63 @@ > > +test_expect_success 'master branch setup and write expect1 expect2 and commit' ' > > We usually give this test a simple title such as "setup" so that we > don't have to worry about the title becoming outdated as people make > changes to the test itself. > > > + touch a.txt && > > + touch b.txt && > > + touch delete.txt && > > On this project, we use `touch` when the timestamp of the empty files > is important to the test. If the timestamp is not important, then we > just use `>`, like this: > > >a.txt && > >b.txt && > >delete.txt && > OK,maybe because I always use touch to generate files. > > + cat <<-EOF >expect1 && > > + M a.txt > > + H b.txt > > + H delete.txt > > + H expect1 > > + H expect2 > > + EOF > > + cat <<-EOF >expect2 && > > + C a.txt > > + R delete.txt > > + EOF > > When no variables are being interpolated in the here-doc content, we > use -\EOF to let readers know that the here-doc body is literal. So: > > cat >expect1 <<-\EOF && > ... > EOF > > > + git add a.txt b.txt delete.txt expect1 expect2 && > > + git commit -m master:1 > > +' > > + > > +test_expect_success 'main commit again' ' > > + echo a>a.txt && > > + echo b>b.txt && > > + echo delete>delete.txt && > > + git add a.txt b.txt delete.txt && > > + git commit -m master:2 > > +' > > + > > +test_expect_success 'dev commit' ' > > + git checkout HEAD~ && > > + git switch -c dev && > > + echo change>a.txt && > > + git add a.txt && > > + git commit -m dev:1 > > +' > > These two tests following the "setup" test also seem to be doing setup > tasks rather than testing the new --dedup functionality. If this is > the case, then it probably would make sense to combine all three tests > into a single "setup" test. > > > +test_expect_success 'dev merge master' ' > > + test_must_fail git merge master && > > + git ls-files -t --dedup >actual1 && > > + test_cmp expect1 actual1 && > > + rm delete.txt && > > + git ls-files -d -m -t --dedup >actual2 && > > + test_cmp expect2 actual2 > > +' > > Do you foresee that people will add more tests to this file which will > use the files and branches set up by the "setup" test(s)? If not, if > those branches and files are only ever going to be used by this one > test, then it probably would be better to combine all the above code > into a single test. No,the test file may just need only one.
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt index cbcf5263dd0..41a9c5a8b27 100644 --- a/Documentation/git-ls-files.txt +++ b/Documentation/git-ls-files.txt @@ -13,6 +13,7 @@ SYNOPSIS (--[cached|deleted|others|ignored|stage|unmerged|killed|modified])* (-[c|d|o|i|s|u|k|m])* [--eol] + [--dedup] [-x <pattern>|--exclude=<pattern>] [-X <file>|--exclude-from=<file>] [--exclude-per-directory=<file>] @@ -81,6 +82,9 @@ OPTIONS \0 line termination on output and do not quote filenames. See OUTPUT below for more information. +--dedup:: + Suppress duplicates entries when conflicts happen or + specify -d -m at the same time. -x <pattern>:: --exclude=<pattern>:: Skip untracked files matching pattern. diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 66a7e251a46..bc4eded19ab 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -302,7 +302,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) { int i; struct strbuf fullname = STRBUF_INIT; - const struct cache_entry *last_stage=NULL; + const struct cache_entry *last_stage = NULL; /* For cached/deleted files we don't need to even do the readdir */ if (show_others || show_killed) { @@ -317,7 +317,8 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (show_cached || show_stage) { for (i = 0; i < repo->index->cache_nr; i++) { const struct cache_entry *ce = repo->index->cache[i]; - if(show_cached && delete_dup){ + + if (show_cached && delete_dup) { switch (ce_stage(ce)) { case 0: default: @@ -328,7 +329,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (last_stage && !strcmp(last_stage->name, ce->name)) continue; - last_stage=ce; + last_stage = ce; } } construct_fullname(&fullname, repo, ce); @@ -351,7 +352,8 @@ static void show_files(struct repository *repo, struct dir_struct *dir) const struct cache_entry *ce = repo->index->cache[i]; struct stat st; int err; - if(delete_dup){ + + if (delete_dup) { switch (ce_stage(ce)) { case 0: default: @@ -362,7 +364,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (last_stage && !strcmp(last_stage->name, ce->name)) continue; - last_stage=ce; + last_stage = ce; } } construct_fullname(&fullname, repo, ce); @@ -375,10 +377,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir) if (ce_skip_worktree(ce)) continue; err = lstat(fullname.buf, &st); - if(delete_dup && show_deleted && show_modified && err) + if (delete_dup && show_deleted && show_modified && err) show_ce(repo, dir, ce, fullname.buf, tag_removed); - else{ - if (show_deleted && err)/* you can't find it,so it's actually removed at all! */ + else { + if (show_deleted && err) show_ce(repo, dir, ce, fullname.buf, tag_removed); if (show_modified && ie_modified(repo->index, ce, &st, 0)) show_ce(repo, dir, ce, fullname.buf, tag_modified); @@ -610,7 +612,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) N_("pretend that paths removed since <tree-ish> are still present")), OPT__ABBREV(&abbrev), OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")), - OPT_BOOL(0, "dedup", &delete_dup, N_("delete duplicate entry in index")), + OPT_BOOL(0, "dedup", &delete_dup, N_("suppress duplicate entries")), OPT_END() }; diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh new file mode 100755 index 00000000000..00c7f65cfc1 --- /dev/null +++ b/t/t3012-ls-files-dedup.sh @@ -0,0 +1,63 @@ +#!/bin/sh + +test_description='git ls-files --dedup test. + +This test prepares the following in the cache: + + a.txt - a file(base) + a.txt - a file(master) + a.txt - a file(dev) + b.txt - a file + delete.txt - a file + expect1 - a file + expect2 - a file + +' + +. ./test-lib.sh + +test_expect_success 'master branch setup and write expect1 expect2 and commit' ' + touch a.txt && + touch b.txt && + touch delete.txt && + cat <<-EOF >expect1 && + M a.txt + H b.txt + H delete.txt + H expect1 + H expect2 + EOF + cat <<-EOF >expect2 && + C a.txt + R delete.txt + EOF + git add a.txt b.txt delete.txt expect1 expect2 && + git commit -m master:1 +' + +test_expect_success 'main commit again' ' + echo a>a.txt && + echo b>b.txt && + echo delete>delete.txt && + git add a.txt b.txt delete.txt && + git commit -m master:2 +' + +test_expect_success 'dev commit' ' + git checkout HEAD~ && + git switch -c dev && + echo change>a.txt && + git add a.txt && + git commit -m dev:1 +' + +test_expect_success 'dev merge master' ' + test_must_fail git merge master && + git ls-files -t --dedup >actual1 && + test_cmp expect1 actual1 && + rm delete.txt && + git ls-files -d -m -t --dedup >actual2 && + test_cmp expect2 actual2 +' + +test_done