* [PATCH 0/3] t6025: updating tests
@ 2020-01-16 20:36 Shourya Shukla
2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw)
To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla
Greetings everyone!
This is my first ever contribution in the Open-Source Community and I chose Git
for this purpose as I have been using this important tool to maintain my projects
regularly.
In this patch, I have:
- modernized these tests so that they meet current Git CodingGuidlines[1]
- replaced the pipe operator with the redirection operator so that one can
detect the errors easily and precisely
- used helper function 'test_path_is_file()' to replace 'test -f' checks in
in the program as it improves the readability of the code and provides
better error messages
Also, I have some questions to better my understanding of the code:
- In the statement,
> git hash-object -t blob -w --stdin
is it necessary to explicitly specify the type 'blob' of the hash-object?
I have this question because it is the default type of hash-object.
- In the statement,
> l=$(printf file | git hash-object -t blob -w --stdin)
I have not used the redirection operator as this sub-shell will be executed
separately, hence its error cannot be captured therefore the presence of '>'
will not matter. Will using '>' improve the code?
Thanks,
Shourya Shukla
Shourya Shukla (3):
t6025: modernize style
t6025: replace pipe with redirection operator
t6025: use helpers to replace test -f <path>
t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++-------------------
1 file changed, 50 insertions(+), 47 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 1/3] t6025: modernize style 2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla @ 2020-01-16 20:36 ` Shourya Shukla 2020-01-16 21:42 ` Johannes Schindelin 2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla ` (2 subsequent siblings) 3 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw) To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla The tests in `t6025-merge-symlinks.sh` were written a long time ago, and has a lot of style violations, including the mixed-use of tabs and spaces, missing indentations, and other shell script style violations. Update it to match the CodingGuidelines. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 47 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 433c4de08f..b9219af659 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -10,52 +10,55 @@ if core.symlinks is false.' . ./test-lib.sh -test_expect_success \ -'setup' ' -git config core.symlinks false && -> file && -git add file && -git commit -m initial && -git branch b-symlink && -git branch b-file && -l=$(printf file | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m master && -git checkout b-symlink && -l=$(printf file-different | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m b-symlink && -git checkout b-file && -echo plain-file > symlink && -git add symlink && -git commit -m b-file' - -test_expect_success \ -'merge master into b-symlink, which has a different symbolic link' ' -git checkout b-symlink && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge master into b-file, which has a file instead of a symbolic link' ' -git reset --hard && git checkout b-file && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge b-file, which has a file instead of a symbolic link, into master' ' -git reset --hard && -git checkout master && -test_must_fail git merge b-file' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' +test_expect_success 'setup' ' + git config core.symlinks false && + touch file && + git add file && + git commit -m initial && + git branch b-symlink && + git branch b-file && + l=$(printf file | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m master && + git checkout b-symlink && + l=$(printf file-different | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m b-symlink && + git checkout b-file && + echo plain-file >symlink && + git add symlink && + git commit -m b-file +' + +test_expect_success 'merge master into b-symlink, which has a different symbolic link' ' + git checkout b-symlink && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' + git reset --hard && + git checkout b-file && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' + git reset --hard && + git checkout master && + test_must_fail git merge b-file +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] t6025: modernize style 2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla @ 2020-01-16 21:42 ` Johannes Schindelin 0 siblings, 0 replies; 24+ messages in thread From: Johannes Schindelin @ 2020-01-16 21:42 UTC (permalink / raw) To: Shourya Shukla; +Cc: git, gitster Hi, On Fri, 17 Jan 2020, Shourya Shukla wrote: > The tests in `t6025-merge-symlinks.sh` were written a long time ago, and > has a lot of style violations, including the mixed-use of tabs and spaces, > missing indentations, and other shell script style violations. Update it to > match the CodingGuidelines. > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- Sounds good. Just one nit: > t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- > 1 file changed, 50 insertions(+), 47 deletions(-) > > diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh > index 433c4de08f..b9219af659 100755 > --- a/t/t6025-merge-symlinks.sh > +++ b/t/t6025-merge-symlinks.sh > @@ -10,52 +10,55 @@ if core.symlinks is false.' > > . ./test-lib.sh > > -test_expect_success \ > -'setup' ' > -git config core.symlinks false && > -> file && Here, `file` is written as a 0-byte file, and... > -git add file && > -git commit -m initial && > -git branch b-symlink && > -git branch b-file && > -l=$(printf file | git hash-object -t blob -w --stdin) && > -echo "120000 $l symlink" | git update-index --index-info && > -git commit -m master && > -git checkout b-symlink && > -l=$(printf file-different | git hash-object -t blob -w --stdin) && > -echo "120000 $l symlink" | git update-index --index-info && > -git commit -m b-symlink && > -git checkout b-file && > -echo plain-file > symlink && > -git add symlink && > -git commit -m b-file' > - > -test_expect_success \ > -'merge master into b-symlink, which has a different symbolic link' ' > -git checkout b-symlink && > -test_must_fail git merge master' > - > -test_expect_success \ > -'the merge result must be a file' ' > -test -f symlink' > - > -test_expect_success \ > -'merge master into b-file, which has a file instead of a symbolic link' ' > -git reset --hard && git checkout b-file && > -test_must_fail git merge master' > - > -test_expect_success \ > -'the merge result must be a file' ' > -test -f symlink' > - > -test_expect_success \ > -'merge b-file, which has a file instead of a symbolic link, into master' ' > -git reset --hard && > -git checkout master && > -test_must_fail git merge b-file' > - > -test_expect_success \ > -'the merge result must be a file' ' > -test -f symlink' > +test_expect_success 'setup' ' > + git config core.symlinks false && > + touch file && ... here we now use `touch` instead. We do prefer `>file` in this instance, though, I think. At least we do not prohibit it. Otherwise it looks very good! Johannes > + git add file && > + git commit -m initial && > + git branch b-symlink && > + git branch b-file && > + l=$(printf file | git hash-object -t blob -w --stdin) && > + echo "120000 $l symlink" | > + git update-index --index-info && > + git commit -m master && > + git checkout b-symlink && > + l=$(printf file-different | git hash-object -t blob -w --stdin) && > + echo "120000 $l symlink" | > + git update-index --index-info && > + git commit -m b-symlink && > + git checkout b-file && > + echo plain-file >symlink && > + git add symlink && > + git commit -m b-file > +' > + > +test_expect_success 'merge master into b-symlink, which has a different symbolic link' ' > + git checkout b-symlink && > + test_must_fail git merge master > +' > + > +test_expect_success 'the merge result must be a file' ' > + test -f symlink > +' > + > +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' > + git reset --hard && > + git checkout b-file && > + test_must_fail git merge master > +' > + > +test_expect_success 'the merge result must be a file' ' > + test -f symlink > +' > + > +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' > + git reset --hard && > + git checkout master && > + test_must_fail git merge b-file > +' > + > +test_expect_success 'the merge result must be a file' ' > + test -f symlink > +' > > test_done > -- > 2.20.1 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] t6025: replace pipe with redirection operator 2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla @ 2020-01-16 20:36 ` Shourya Shukla 2020-01-16 22:57 ` Junio C Hamano 2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla 2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin 3 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw) To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla The exit code of pipes(|) are always ignored, which will create errors in subsequent statements. Let's handle it by redirecting its output to a file and capturing return values. Replace pipe with redirect(>) operator. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index b9219af659..41bae56ea9 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -18,13 +18,13 @@ test_expect_success 'setup' ' git branch b-symlink && git branch b-file && l=$(printf file | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" >foo && + git update-index --index-info <foo && git commit -m master && git checkout b-symlink && l=$(printf file-different | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" >foo && + git update-index --index-info <foo && git commit -m b-symlink && git checkout b-file && echo plain-file >symlink && -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] t6025: replace pipe with redirection operator 2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla @ 2020-01-16 22:57 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2020-01-16 22:57 UTC (permalink / raw) To: Shourya Shukla; +Cc: git, johannes.schindelin Shourya Shukla <shouryashukla.oo@gmail.com> writes: > - echo "120000 $l symlink" | > - git update-index --index-info && > + echo "120000 $l symlink" >foo && > + git update-index --index-info <foo && If we had "git" on the left-hand-side (i.e. upstream) of a pipe, it would make sense to split the pipeline like this, but this (and the other one this patch touches) is on the right side, whose exit status is not lost. And we are not in the business of preparing for broken implementation of "echo". So this rewrite is unnecessary and unwarranted. By the way, I think the pipeline echo ... | git update-index --index-info && should be written on a single line in the previous step 1/3. > git commit -m master && > git checkout b-symlink && > l=$(printf file-different | git hash-object -t blob -w --stdin) && > - echo "120000 $l symlink" | > - git update-index --index-info && > + echo "120000 $l symlink" >foo && > + git update-index --index-info <foo && > git commit -m b-symlink && > git checkout b-file && > echo plain-file >symlink && ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] t6025: use helpers to replace test -f <path> 2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla @ 2020-01-16 20:36 ` Shourya Shukla 2020-01-16 22:58 ` Junio C Hamano 2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin 3 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-16 20:36 UTC (permalink / raw) To: git, gitster, johannes.schindelin; +Cc: Shourya Shukla Take advantage of helper function 'test_path_is_file()' to replace 'test -f' since the function makes the code more readable and gives better error messages. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 41bae56ea9..ebbbc03f1d 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' @@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' @@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link, ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] t6025: use helpers to replace test -f <path> 2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla @ 2020-01-16 22:58 ` Junio C Hamano 2020-01-17 20:44 ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2020-01-16 22:58 UTC (permalink / raw) To: Shourya Shukla; +Cc: git, johannes.schindelin Shourya Shukla <shouryashukla.oo@gmail.com> writes: > Take advantage of helper function 'test_path_is_file()' to > replace 'test -f' since the function makes the code more > readable and gives better error messages. > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > t/t6025-merge-symlinks.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Makes sense. > diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh > index 41bae56ea9..ebbbc03f1d 100755 > --- a/t/t6025-merge-symlinks.sh > +++ b/t/t6025-merge-symlinks.sh > @@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic > ' > > test_expect_success 'the merge result must be a file' ' > - test -f symlink > + test_path_is_file symlink > ' > > test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' > @@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym > ' > > test_expect_success 'the merge result must be a file' ' > - test -f symlink > + test_path_is_file symlink > ' > > test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' > @@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link, > ' > > test_expect_success 'the merge result must be a file' ' > - test -f symlink > + test_path_is_file symlink > ' > > test_done ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/3] t6025: amended changes after suggestions from the community 2020-01-16 22:58 ` Junio C Hamano @ 2020-01-17 20:44 ` Shourya Shukla 2020-01-17 20:44 ` [PATCH 1/3] t6025: modernize style Shourya Shukla ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw) To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo Greetings everyone! I have made the changes in my patch on the advise of Junio C Hamano and Johannes Schindelin. Thank you for looking into my changes, this has given me confidence to contribute more in the Git Community. Thank you, Shourya Shukla Shourya Shukla (3): t6025: modernize style t6025: replace pipe with redirection operator t6025: use helpers to replace test -f <path> t/t6025-merge-symlinks.sh | 96 ++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 47 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] t6025: modernize style 2020-01-17 20:44 ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla @ 2020-01-17 20:44 ` Shourya Shukla 2020-01-17 21:15 ` Eric Sunshine 2020-01-17 20:44 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla 2020-01-17 20:44 ` [PATCH 3/3] " Shourya Shukla 2 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw) To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo The tests in `t6025-merge-symlinks.sh` were written a long time ago, and has a lot of style violations, including the mixed-use of tabs and spaces, missing indentations, and other shell script style violations. Update it to match the CodingGuidelines. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 47 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 433c4de08f..7a19ba8520 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -10,52 +10,55 @@ if core.symlinks is false.' . ./test-lib.sh -test_expect_success \ -'setup' ' -git config core.symlinks false && -> file && -git add file && -git commit -m initial && -git branch b-symlink && -git branch b-file && -l=$(printf file | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m master && -git checkout b-symlink && -l=$(printf file-different | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m b-symlink && -git checkout b-file && -echo plain-file > symlink && -git add symlink && -git commit -m b-file' - -test_expect_success \ -'merge master into b-symlink, which has a different symbolic link' ' -git checkout b-symlink && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge master into b-file, which has a file instead of a symbolic link' ' -git reset --hard && git checkout b-file && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge b-file, which has a file instead of a symbolic link, into master' ' -git reset --hard && -git checkout master && -test_must_fail git merge b-file' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' +test_expect_success 'setup' ' + git config core.symlinks false && + >file && + git add file && + git commit -m initial && + git branch b-symlink && + git branch b-file && + l=$(printf file | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m master && + git checkout b-symlink && + l=$(printf file-different | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m b-symlink && + git checkout b-file && + echo plain-file >symlink && + git add symlink && + git commit -m b-file +' + +test_expect_success 'merge master into b-symlink, which has a different symbolic link' ' + git checkout b-symlink && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' + git reset --hard && + git checkout b-file && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' + git reset --hard && + git checkout master && + test_must_fail git merge b-file +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] t6025: modernize style 2020-01-17 20:44 ` [PATCH 1/3] t6025: modernize style Shourya Shukla @ 2020-01-17 21:15 ` Eric Sunshine 2020-01-17 21:28 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Eric Sunshine @ 2020-01-17 21:15 UTC (permalink / raw) To: Shourya Shukla; +Cc: Junio C Hamano, Git List, Johannes Schindelin On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > [PATCH 1/3] t6025: modernize style When sending a new version of a patch series, indicate this via "[PATCH v2 1/3]", for instance. The -v option of "git format-patch" can help automate this for you. > The tests in `t6025-merge-symlinks.sh` were written a long time ago, and > has a lot of style violations, including the mixed-use of tabs and spaces, > missing indentations, and other shell script style violations. Update it to > match the CodingGuidelines. > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh > @@ -10,52 +10,55 @@ if core.symlinks is false.' > +test_expect_success 'setup' ' > + git config core.symlinks false && > + >file && > + git add file && > + git commit -m initial && > + git branch b-symlink && > + git branch b-file && > + l=$(printf file | git hash-object -t blob -w --stdin) && > + echo "120000 $l symlink" | > + git update-index --index-info && As mentioned[1] in the review of v1, this should be written: echo "120000 $l symlink" | git update-index --index-info && [1]: https://lore.kernel.org/git/xmqqftgff1r0.fsf@gitster-ct.c.googlers.com/ > + git commit -m master && > + git checkout b-symlink && > + l=$(printf file-different | git hash-object -t blob -w --stdin) && > + echo "120000 $l symlink" | > + git update-index --index-info && > + git commit -m b-symlink && > + git checkout b-file && > + echo plain-file >symlink && > + git add symlink && > + git commit -m b-file > +' ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] t6025: modernize style 2020-01-17 21:15 ` Eric Sunshine @ 2020-01-17 21:28 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2020-01-17 21:28 UTC (permalink / raw) To: Eric Sunshine; +Cc: Shourya Shukla, Git List, Johannes Schindelin Eric Sunshine <sunshine@sunshineco.com> writes: >> + l=$(printf file | git hash-object -t blob -w --stdin) && >> + echo "120000 $l symlink" | >> + git update-index --index-info && > > As mentioned[1] in the review of v1, this should be written: > > echo "120000 $l symlink" | git update-index --index-info && > > [1]: https://lore.kernel.org/git/xmqqftgff1r0.fsf@gitster-ct.c.googlers.com/ > >> + git commit -m master && >> + git checkout b-symlink && >> + l=$(printf file-different | git hash-object -t blob -w --stdin) && >> + echo "120000 $l symlink" | >> + git update-index --index-info && Same here. Funnily, 2/3 improves on this, but I agree that we should get it right from the get-go. >> + git commit -m b-symlink && >> + git checkout b-file && >> + echo plain-file >symlink && >> + git add symlink && >> + git commit -m b-file >> +' ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] t6025: replace pipe with redirection operator 2020-01-17 20:44 ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla 2020-01-17 20:44 ` [PATCH 1/3] t6025: modernize style Shourya Shukla @ 2020-01-17 20:44 ` Shourya Shukla 2020-01-17 21:24 ` Eric Sunshine 2020-01-17 20:44 ` [PATCH 3/3] " Shourya Shukla 2 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw) To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo The exit code of pipes(|) are always ignored, which will create errors in subsequent statements. Let's handle it by redirecting its output to a file and capturing return values. Replace pipe with redirect(>) operator. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 7a19ba8520..5136bf1e13 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -17,14 +17,13 @@ test_expect_success 'setup' ' git commit -m initial && git branch b-symlink && git branch b-file && - l=$(printf file | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + printf file >file && + l=$(git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | git update-index --index-info && git commit -m master && git checkout b-symlink && l=$(printf file-different | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" | git update-index --index-info && git commit -m b-symlink && git checkout b-file && echo plain-file >symlink && -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] t6025: replace pipe with redirection operator 2020-01-17 20:44 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla @ 2020-01-17 21:24 ` Eric Sunshine 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla 0 siblings, 1 reply; 24+ messages in thread From: Eric Sunshine @ 2020-01-17 21:24 UTC (permalink / raw) To: Shourya Shukla; +Cc: Junio C Hamano, Git List, Johannes Schindelin On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > The exit code of pipes(|) are always ignored, which will create > errors in subsequent statements. Let's handle it by redirecting > its output to a file and capturing return values. Replace pipe > with redirect(>) operator. This is not an accurate description. The proper way to explain this is that exit code of a command upstream of a pipe is lost; the exit code of a command downstream is not lost. We don't want to lose the exit code of a git command, so a git command should not be upstream. (We don't care about non-git commands being upstream when that command's exit code is not relevant.) > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh > @@ -17,14 +17,13 @@ test_expect_success 'setup' ' > git commit -m initial && > git branch b-symlink && > git branch b-file && > - l=$(printf file | git hash-object -t blob -w --stdin) && This command is fine as-is. We are interested in the exit code of the git command (which is correctly downstream), and we don't care about the exit code of 'printf' (which is upstream), so there is no reason to rewrite this to use temporary files instead. > + printf file >file && > + l=$(git hash-object -t blob -w --stdin) && Sorry, but this just doesn't make sense. You're telling "git hash-object" to take its input from the standard input stream, yet you don't feed anything to it on that stream. If anything, it should have been written like this: l=$(git hash-object -t blob -w --stdin <file) && however, as noted above, there is no reason to avoid pipes in this case, so this rewrite is unnecessary. By the way, it's hard to imagine that this test passed once this change was made (and, if it did pass, then that would likely indicate that the test is somehow flawed.) ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/3] t6025: updating tests 2020-01-17 21:24 ` Eric Sunshine @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-18 8:33 ` [PATCH 1/3] t6025: modernize style Shourya Shukla ` (5 more replies) 0 siblings, 6 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo Greetings everyone! This is my first ever contribution in the Open-Source Community and I chose Git for this purpose as I have been using this important tool to maintain my projects regularly. In this patch, I have: - modernized these tests so that they meet current Git CodingGuidlines[1] - replaced the pipe operator with the redirection operator so that one can detect the errors easily and precisely - used helper function 'test_path_is_file()' to replace 'test -f' checks in in the program as it improves the readability of the code and provides better error messages Also, I have some questions to better my understanding of the code: - In the statement, > git hash-object -t blob -w --stdin is it necessary to explicitly specify the type 'blob' of the hash-object? I have this question because it is the default type of hash-object. - In the statement, > l=$(printf file | git hash-object -t blob -w --stdin) I have not used the redirection operator as this sub-shell will be executed separately, hence its error cannot be captured therefore the presence of '>' will not matter. Will using '>' improve the code? Thanks, Shourya Shukla Shourya Shukla (3): t6025: modernize style t6025: replace pipe with redirection operator t6025: use helpers to replace test -f <path> t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 47 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] t6025: modernize style 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-18 8:33 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla ` (4 subsequent siblings) 5 siblings, 0 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo The tests in `t6025-merge-symlinks.sh` were written a long time ago, and has a lot of style violations, including the mixed-use of tabs and spaces, missing indentations, and other shell script style violations. Update it to match the CodingGuidelines. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 47 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 433c4de08f..b9219af659 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -10,52 +10,55 @@ if core.symlinks is false.' . ./test-lib.sh -test_expect_success \ -'setup' ' -git config core.symlinks false && -> file && -git add file && -git commit -m initial && -git branch b-symlink && -git branch b-file && -l=$(printf file | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m master && -git checkout b-symlink && -l=$(printf file-different | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m b-symlink && -git checkout b-file && -echo plain-file > symlink && -git add symlink && -git commit -m b-file' - -test_expect_success \ -'merge master into b-symlink, which has a different symbolic link' ' -git checkout b-symlink && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge master into b-file, which has a file instead of a symbolic link' ' -git reset --hard && git checkout b-file && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge b-file, which has a file instead of a symbolic link, into master' ' -git reset --hard && -git checkout master && -test_must_fail git merge b-file' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' +test_expect_success 'setup' ' + git config core.symlinks false && + touch file && + git add file && + git commit -m initial && + git branch b-symlink && + git branch b-file && + l=$(printf file | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m master && + git checkout b-symlink && + l=$(printf file-different | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | + git update-index --index-info && + git commit -m b-symlink && + git checkout b-file && + echo plain-file >symlink && + git add symlink && + git commit -m b-file +' + +test_expect_success 'merge master into b-symlink, which has a different symbolic link' ' + git checkout b-symlink && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' + git reset --hard && + git checkout b-file && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' + git reset --hard && + git checkout master && + test_must_fail git merge b-file +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/3] t6025: replace pipe with redirection operator 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-18 8:33 ` [PATCH 1/3] t6025: modernize style Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-18 8:33 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla ` (3 subsequent siblings) 5 siblings, 0 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo The exit code of pipes(|) are always ignored, which will create errors in subsequent statements. Let's handle it by redirecting its output to a file and capturing return values. Replace pipe with redirect(>) operator. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index b9219af659..41bae56ea9 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -18,13 +18,13 @@ test_expect_success 'setup' ' git branch b-symlink && git branch b-file && l=$(printf file | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" >foo && + git update-index --index-info <foo && git commit -m master && git checkout b-symlink && l=$(printf file-different | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" >foo && + git update-index --index-info <foo && git commit -m b-symlink && git checkout b-file && echo plain-file >symlink && -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/3] t6025: use helpers to replace test -f <path> 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-18 8:33 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-18 8:33 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-18 8:33 ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla ` (2 subsequent siblings) 5 siblings, 0 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo Take advantage of helper function 'test_path_is_file()' to replace 'test -f' since the function makes the code more readable and gives better error messages. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 41bae56ea9..ebbbc03f1d 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -38,7 +38,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' @@ -48,7 +48,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' @@ -58,7 +58,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link, ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v3 0/2] t025: amended changes after suggestions from the community 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla ` (2 preceding siblings ...) 2020-01-18 8:33 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-18 8:33 ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla 2020-01-18 8:33 ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla 5 siblings, 0 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo Greetings everyone! After some advise from Eric Sunshine, I have removed the commit to replace pipes with redirection operators. Also, as Junio Habano and Eric Sunshine pointed out, I made a small mistake while committing changes in my first commit and it has been corrected as well. Thanks, Shourya Shukla Shourya Shukla (2): t6025: modernize style t6025: use helpers to replace test -f <path> t/t6025-merge-symlinks.sh | 95 ++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 47 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 1/2] t6025: modernize style 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla ` (3 preceding siblings ...) 2020-01-18 8:33 ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-21 21:57 ` Junio C Hamano 2020-01-18 8:33 ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla 5 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo The tests in `t6025-merge-symlinks.sh` were written a long time ago, and has a lot of style violations, including the mixed-use of tabs and spaces, missing indentations, and other shell script style violations. Update it to match the CodingGuidelines. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 95 ++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 47 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 433c4de08f..d257dcf34d 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -10,52 +10,53 @@ if core.symlinks is false.' . ./test-lib.sh -test_expect_success \ -'setup' ' -git config core.symlinks false && -> file && -git add file && -git commit -m initial && -git branch b-symlink && -git branch b-file && -l=$(printf file | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m master && -git checkout b-symlink && -l=$(printf file-different | git hash-object -t blob -w --stdin) && -echo "120000 $l symlink" | git update-index --index-info && -git commit -m b-symlink && -git checkout b-file && -echo plain-file > symlink && -git add symlink && -git commit -m b-file' - -test_expect_success \ -'merge master into b-symlink, which has a different symbolic link' ' -git checkout b-symlink && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge master into b-file, which has a file instead of a symbolic link' ' -git reset --hard && git checkout b-file && -test_must_fail git merge master' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' - -test_expect_success \ -'merge b-file, which has a file instead of a symbolic link, into master' ' -git reset --hard && -git checkout master && -test_must_fail git merge b-file' - -test_expect_success \ -'the merge result must be a file' ' -test -f symlink' +test_expect_success 'setup' ' + git config core.symlinks false && + >file && + git add file && + git commit -m initial && + git branch b-symlink && + git branch b-file && + l=$(printf file | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | git update-index --index-info && + git commit -m master && + git checkout b-symlink && + l=$(printf file-different | git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | git update-index --index-info && + git commit -m b-symlink && + git checkout b-file && + echo plain-file >symlink && + git add symlink && + git commit -m b-file +' + +test_expect_success 'merge master into b-symlink, which has a different symbolic link' ' + git checkout b-symlink && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' + git reset --hard && + git checkout b-file && + test_must_fail git merge master +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' + +test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' + git reset --hard && + git checkout master && + test_must_fail git merge b-file +' + +test_expect_success 'the merge result must be a file' ' + test -f symlink +' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/2] t6025: modernize style 2020-01-18 8:33 ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla @ 2020-01-21 21:57 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2020-01-21 21:57 UTC (permalink / raw) To: Shourya Shukla; +Cc: sunshine, git, johannes.schindelin Shourya Shukla <shouryashukla.oo@gmail.com> writes: > The tests in `t6025-merge-symlinks.sh` were written a long time ago, and > has a lot of style violations, including the mixed-use of tabs and spaces, > missing indentations, and other shell script style violations. Update it to > match the CodingGuidelines. Looks good. Will queue. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 2/2] t6025: use helpers to replace test -f <path> 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla ` (4 preceding siblings ...) 2020-01-18 8:33 ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla @ 2020-01-18 8:33 ` Shourya Shukla 2020-01-21 21:58 ` Junio C Hamano 5 siblings, 1 reply; 24+ messages in thread From: Shourya Shukla @ 2020-01-18 8:33 UTC (permalink / raw) To: sunshine; +Cc: git, gitster, johannes.schindelin, shouryashukla.oo Take advantage of helper function 'test_path_is_file()' to replace 'test -f' since the function makes the code more readable and gives better error messages. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index d257dcf34d..6c0a90d044 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -36,7 +36,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' @@ -46,7 +46,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' @@ -56,7 +56,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link, ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/2] t6025: use helpers to replace test -f <path> 2020-01-18 8:33 ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla @ 2020-01-21 21:58 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2020-01-21 21:58 UTC (permalink / raw) To: Shourya Shukla; +Cc: sunshine, git, johannes.schindelin Shourya Shukla <shouryashukla.oo@gmail.com> writes: > Take advantage of helper function 'test_path_is_file()' to > replace 'test -f' since the function makes the code more > readable and gives better error messages. Looks good. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] t6025: use helpers to replace test -f <path> 2020-01-17 20:44 ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla 2020-01-17 20:44 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-17 20:44 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla @ 2020-01-17 20:44 ` Shourya Shukla 2 siblings, 0 replies; 24+ messages in thread From: Shourya Shukla @ 2020-01-17 20:44 UTC (permalink / raw) To: gitster; +Cc: git, johannes.schindelin, shouryashukla.oo Take advantage of helper function 'test_path_is_file()' to replace 'test -f' since the function makes the code more readable and gives better error messages. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 5136bf1e13..18a204bb65 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -37,7 +37,7 @@ test_expect_success 'merge master into b-symlink, which has a different symbolic ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge master into b-file, which has a file instead of a symbolic link' ' @@ -47,7 +47,7 @@ test_expect_success 'merge master into b-file, which has a file instead of a sym ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_expect_success 'merge b-file, which has a file instead of a symbolic link, into master' ' @@ -57,7 +57,7 @@ test_expect_success 'merge b-file, which has a file instead of a symbolic link, ' test_expect_success 'the merge result must be a file' ' - test -f symlink + test_path_is_file symlink ' test_done -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 0/3] t6025: updating tests 2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla ` (2 preceding siblings ...) 2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla @ 2020-01-16 21:46 ` Johannes Schindelin 3 siblings, 0 replies; 24+ messages in thread From: Johannes Schindelin @ 2020-01-16 21:46 UTC (permalink / raw) To: Shourya Shukla; +Cc: git, gitster Hi, On Fri, 17 Jan 2020, Shourya Shukla wrote: > Greetings everyone! > > This is my first ever contribution in the Open-Source Community and I chose Git > for this purpose as I have been using this important tool to maintain my projects > regularly. > > In this patch, I have: > > - modernized these tests so that they meet current Git CodingGuidlines[1] > - replaced the pipe operator with the redirection operator so that one can > detect the errors easily and precisely > - used helper function 'test_path_is_file()' to replace 'test -f' checks in > in the program as it improves the readability of the code and provides > better error messages > > Also, I have some questions to better my understanding of the code: > - In the statement, > > git hash-object -t blob -w --stdin > is it necessary to explicitly specify the type 'blob' of the hash-object? > I have this question because it is the default type of hash-object. It is the default type, but: 1) the code is not broken, so why fix it? 2) it _might_ be possible that the default changes, or can be configured in the future. The original author might just have wanted to stay safe. > - In the statement, > > l=$(printf file | git hash-object -t blob -w --stdin) > I have not used the redirection operator as this sub-shell will be executed > separately, hence its error cannot be captured therefore the presence of '>' > will not matter. Will using '>' improve the code? It will be enhanced, though: printf file >file && l=$(git hash-object -t blob -w --stdin) will have a non-zero exit code if the `hash_object` call fails. > > Thanks, > Shourya Shukla > > Shourya Shukla (3): > t6025: modernize style > t6025: replace pipe with redirection operator > t6025: use helpers to replace test -f <path> Apart from one little issue in the first patch, this all looks very good to me. Thanks, Johannes > > t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- > 1 file changed, 50 insertions(+), 47 deletions(-) > > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-01-21 21:58 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-16 20:36 [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-16 20:36 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-16 21:42 ` Johannes Schindelin 2020-01-16 20:36 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla 2020-01-16 22:57 ` Junio C Hamano 2020-01-16 20:36 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla 2020-01-16 22:58 ` Junio C Hamano 2020-01-17 20:44 ` [PATCH 0/3] t6025: amended changes after suggestions from the community Shourya Shukla 2020-01-17 20:44 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-17 21:15 ` Eric Sunshine 2020-01-17 21:28 ` Junio C Hamano 2020-01-17 20:44 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla 2020-01-17 21:24 ` Eric Sunshine 2020-01-18 8:33 ` [PATCH 0/3] t6025: updating tests Shourya Shukla 2020-01-18 8:33 ` [PATCH 1/3] t6025: modernize style Shourya Shukla 2020-01-18 8:33 ` [PATCH 2/3] t6025: replace pipe with redirection operator Shourya Shukla 2020-01-18 8:33 ` [PATCH 3/3] t6025: use helpers to replace test -f <path> Shourya Shukla 2020-01-18 8:33 ` [PATCH v3 0/2] t025: amended changes after suggestions from the community Shourya Shukla 2020-01-18 8:33 ` [PATCH v3 1/2] t6025: modernize style Shourya Shukla 2020-01-21 21:57 ` Junio C Hamano 2020-01-18 8:33 ` [PATCH v3 2/2] t6025: use helpers to replace test -f <path> Shourya Shukla 2020-01-21 21:58 ` Junio C Hamano 2020-01-17 20:44 ` [PATCH 3/3] " Shourya Shukla 2020-01-16 21:46 ` [PATCH 0/3] t6025: updating tests Johannes Schindelin
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).