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@xxxxxxxxxxxxxx> 于2021年1月14日周四 下午2:39写道: > > On Fri, Jan 8, 2021 at 9:36 AM ZheNing Hu via GitGitGadget > <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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.