On Fri, Jan 28, 2022 at 4:34 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Mon, Jan 24, 2022 at 1:37 AM Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> wrote: > > lib-read-tree-m-3way: modernize a test script (style) > > Prefixing the subject with "t/" would help better explain which part > of the project this patch is touching. Perhaps: > > t/lib-read-tree-m-3way: modernize style Thanks, this tip is really helpful and I forgot to add the directory prefix. > > As a microproject, I found another small fix regarding styling to do. > > Everything above the "---" line below your sign-off will become part > of the permanent project history; everything below the "---" is mere > commentary for reviewers. Reviewers may be interested in knowing that > this is a microproject, but it likely won't be meaningful to future > readers of the project history. As such, place this sort of commentary > below the "---" line. Sure, I did not realize this. I should've put the comments below "---" instead. > > I changed the old style '\' (backslash) to new style "'" (single > > quotes). > > Not sure what this means. I _think_ you mean that you changed: > > test_expect_success \ > 'title' \ > 'body' > > to: > > test_expect_success 'title' ' > body > ' > > Sometimes you can convey such a meaning more clearly by a simple > example as illustrated above. > > > And I also fixed some double quotes misuse. > > Again, it is unclear what this means. Providing an example can help > readers understand what you changed. Will do, definitely a helpful tip. > > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> > > --- > > Other than that, I forgot to introduce myself in the last patch and > > here it goes: > > > > I'm Shaoxuan Yuan, currently a sophomore majors in Computer Science and Engineering > > (CSE) @ University of California, Irvine. > > > > I have prior open-source experience in which I was [maintaining|contributing to] the > > Casbin community. My main language is Python, and I'm a C newbie > > because I'm quite interested in contributing to git (since it is in my main daily > > toolkit and it is a charm to wield :-) ). > > > > And for now I'm still taking baby steps trying to crack some test script > > styling issues. After getting more familiar with the git contribution > > process, I will try something bigger (though not THAT big) to get a > > firmer grasp of git. > > Welcome. Please understand that all review comments (above and below) > are meant to be constructive and help familiarize you with the local > customs of the Git project; they are not meant as any sort of > criticism. Thanks, I learned a lot from your review comments :) > > diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh > > @@ -8,16 +8,16 @@ do > > echo This is Z/$p from the original tree. >Z/$p > > - test_expect_success \ > > - "adding test file $p and Z/$p" \ > > - 'git update-index --add $p && > > - git update-index --add Z/$p' > > + test_expect_success 'adding test file $p and Z/$p' ' > > + git update-index --add $p && > > + git update-index --add Z/$p > > + ' > > Taking into consideration the difference in behavior of single-quoted > strings and double-quoted strings, changing this test's title from > double- to single-quoted is undesirable since `$p` is supposed to be > interpolated into the title; the title should not contain a literal > "$p". (Unlike the test title which is simply echo'd/print'd, the test > body gets eval'd, which is why `$p` in the body is acceptable even > though the body is in single-quotes.) Agree, I didn't realize the difference between single and double quotes in shell scripts (I was just imitating other similar style fixes). > > -test_expect_success \ > > - 'adding test file SS' \ > > - 'git update-index --add SS' > > +test_expect_success 'adding test file SS' ' > > + git update-index --add SS > > +' > > Since you're touching this anyhow as part of fixing style issues, you > should also fix the indentation to use tabs rather than spaces. The > same comment applies to the rest of the patch, as well. Sure. > > for p in M? Z/M? > > do > > echo This is modified $p in the branch A. >$p > > - test_expect_success \ > > - 'change in branch A (modification)' \ > > - "git update-index $p" > > + test_expect_success 'change in branch A (modification)' ' > > + git update-index $p > > + ' > > done > > In this case, the indentation of the entire body of the for-loop needs > to be fixed to use tabs rather than spaces, however, fixing all the > indentation problems together with the other problems can make for a > too-noisy patch for reviewers to really see what is going on. As such, > it may be better to make this a multi-patch series in which one patch > fixes indentation problems only. > As mentioned above, changing the body of the test from double- to > single-quoted string should (presumably) be okay since the body gets > eval'd and `$p` will be visible at the time of `eval`, however, mixing > this sort of change in with all the other changes being made makes it > hard for reviewers to spot such little details, let alone reason about > correctness. As such, switching of quote types is also probably the > sort of change which should be split out into its own patch. The same > comment applies to other changes following this one. I think so. I was thinking fixing all the general styling issues in one patch (since they are all style related), but now I realize that the general style patch can be divided into sub-patches for clearer review experience. And my question is, should I do this "multi-patch series" thing in the form of "-v<n>" (all under this thread), e.g. "v2" or "v3"? Or I just submit multiple patches separately (a new thread for each one)? > Overall, with the exception of the test title which needs to > interpolate `$p`, the rest of the changes are probably reasonable and > benign. It's important to understand that lengthy patches like this > full of mechanical changes tend to be quite taxing on reviewers, so > it's a good idea to help in any way you can to ease the review burden. > This can be done, for instance, by making only a single type of change > per patch (i.e. indentation fixes), or by limiting the scope or > breadth of the changes, which is especially important for this sort of I'm not quite sure what this means, and I quote, "limiting the scope or breadth of the changes". Are you suggesting, for example, fixing fewer occurrences of tab indentation issue in a patch; or, for example, limiting the fix to a specific "test_expect_success" block, and do multiple patches sequentially? > "noise" change -- a change just for the sake of change -- which > doesn't have any immediate practical benefit. Overall, thanks for the reply and it is really helpful! I did make a lot of mistakes in this patch, but I'm learning a lot :) And I will amend my patch base on these suggestions then submit a v2. -- Thanks, Shaoxuan
