| Message ID | 9d915748c1953cc2683fa3189c3c98b1e9a1e299.1571832176.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | apply: fix merge.conflictStyle bug in --3way | expand |
On Wed, Oct 23, 2019 at 8:04 AM Denton Liu <liu.denton@gmail.com> wrote: > Before, the output of `git diff HEAD` would always be piped to > sanitize_conflicted_diff(). However, since the Git command was upstream > of the pipe, in case the Git command fails, the return code would be > lost. Rewrite into separate statements so that the return code is no > longer lost. > > Since only the command `git diff HEAD` was being piped to > sanitize_conflicted_diff(), move the command into the function and rename > it to print_sanitized_diff(). > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh > @@ -4,11 +4,12 @@ test_description='git apply --3way' > -sanitize_conflicted_diff () { > +print_sanitized_diff () { > + git diff HEAD >diff.raw && > sed -e ' > /^index /d > s/^\(+[<>][<>][<>][<>]*\) .*/\1/ > - ' > + ' diff.raw > } Nit: By hard-coding "HEAD" in this function, you lose the flexibility of the original. An alternative would have been to accept the ref against which to diff as an argument to this function: print_sanitized_diff () { git diff "$@" >diff.raw && ... Or, better yet, keep the original design and pass the diff in as the shell function's input, so a caller would say: git diff HEAD >diff.raw && sanitize_conflicted_diff <diff.raw >expect.diff && However, not necessarily worth a re-roll if we never expect anyone to pass anything other than "HEAD".
On Wed, Oct 23, 2019 at 09:32:26AM -0400, Eric Sunshine wrote: > On Wed, Oct 23, 2019 at 8:04 AM Denton Liu <liu.denton@gmail.com> wrote: > > Before, the output of `git diff HEAD` would always be piped to > > sanitize_conflicted_diff(). However, since the Git command was upstream > > of the pipe, in case the Git command fails, the return code would be > > lost. Rewrite into separate statements so that the return code is no > > longer lost. > > > > Since only the command `git diff HEAD` was being piped to > > sanitize_conflicted_diff(), move the command into the function and rename > > it to print_sanitized_diff(). > > > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > > --- > > diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh > > @@ -4,11 +4,12 @@ test_description='git apply --3way' > > -sanitize_conflicted_diff () { > > +print_sanitized_diff () { > > + git diff HEAD >diff.raw && > > sed -e ' > > /^index /d > > s/^\(+[<>][<>][<>][<>]*\) .*/\1/ > > - ' > > + ' diff.raw > > } > > Nit: By hard-coding "HEAD" in this function, you lose the flexibility > of the original. An alternative would have been to accept the ref > against which to diff as an argument to this function: > > print_sanitized_diff () { > git diff "$@" >diff.raw && > ... > > Or, better yet, keep the original design and pass the diff in as the > shell function's input, so a caller would say: > > git diff HEAD >diff.raw && > sanitize_conflicted_diff <diff.raw >expect.diff && > > However, not necessarily worth a re-roll if we never expect anyone to > pass anything other than "HEAD". Since it doesn't really make sense to commmit conflicts, I decided to hardcode it to be a diff against HEAD and the worktree since that's the only sensible place where the conflict should live. Speaking of conflicts, I dropped the "conflicted" part of the old function name. I think that removes a lot of clarity so I'll reroll renaming the function to print_sanitized_conflicted_diff() or something like that.
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh index b109ecbd9f..49739ce8b4 100755 --- a/t/t4108-apply-threeway.sh +++ b/t/t4108-apply-threeway.sh @@ -4,11 +4,12 @@ test_description='git apply --3way' . ./test-lib.sh -sanitize_conflicted_diff () { +print_sanitized_diff () { + git diff HEAD >diff.raw && sed -e ' /^index /d s/^\(+[<>][<>][<>][<>]*\) .*/\1/ - ' + ' diff.raw } test_expect_success setup ' @@ -54,14 +55,14 @@ test_expect_success 'apply with --3way' ' git checkout master^0 && test_must_fail git merge --no-commit side && git ls-files -s >expect.ls && - git diff HEAD | sanitize_conflicted_diff >expect.diff && + print_sanitized_diff >expect.diff && # should fail to apply git reset --hard && git checkout master^0 && test_must_fail git apply --index --3way P.diff && git ls-files -s >actual.ls && - git diff HEAD | sanitize_conflicted_diff >actual.diff && + print_sanitized_diff >actual.diff && # The result should resemble the corresponding merge test_cmp expect.ls actual.ls && @@ -114,7 +115,7 @@ test_expect_success 'apply -3 with add/add conflict setup' ' git checkout adder^0 && test_must_fail git merge --no-commit another && git ls-files -s >expect.ls && - git diff HEAD | sanitize_conflicted_diff >expect.diff + print_sanitized_diff >expect.diff ' test_expect_success 'apply -3 with add/add conflict' ' @@ -124,7 +125,7 @@ test_expect_success 'apply -3 with add/add conflict' ' test_must_fail git apply --index --3way P.diff && # ... and leave conflicts in the index and in the working tree git ls-files -s >actual.ls && - git diff HEAD | sanitize_conflicted_diff >actual.diff && + print_sanitized_diff >actual.diff && # The result should resemble the corresponding merge test_cmp expect.ls actual.ls &&
Before, the output of `git diff HEAD` would always be piped to sanitize_conflicted_diff(). However, since the Git command was upstream of the pipe, in case the Git command fails, the return code would be lost. Rewrite into separate statements so that the return code is no longer lost. Since only the command `git diff HEAD` was being piped to sanitize_conflicted_diff(), move the command into the function and rename it to print_sanitized_diff(). Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t4108-apply-threeway.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)