Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

From: David Aguilar <davvid@gmail.com>
Date: Mar 13, 2013, 09:03 UTC
Message-ID: <CAJDDKr4swZzzv3e+Huz72CVmisFKU8T74jFj3-uGmZHReRGVBw@mail.gmail.com>
In-Reply-To: <1363137142-18606-3-git-send-email-kevin@bracey.fi>

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey <kevin@bracey.fi> wrote:

Show 10 quoted lines

> Commit 718135e improved the merge error reporting for the resolve
> strategy's merge conflict and permission conflict cases, but led to a
> malformed "ERROR:  in myfile.c" message in the case of a file added
> differently.
>
> This commit reverts that change, and uses an alternative approach without
> this flaw.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---

I wonder whether before these changes we should update the style in this file to follow Documentation/CodingGuidelines.

Not in this patch, but in the file right now there's this part that stands out:

	if [ "$2" ]; then
		echo "Removing $4"

I think that expression would read more clearly as:

	if test -n "$2"
	then
		echo "Removing $4"

Ditto `if [ "$1" = '' ]` is better written as `test -z "$1"`.

Can you please send a patch to true these up?

It'd be especially nice if the style patch could come first, followed by the fixes/features ;-)

Show 33 quoted lines

>  git-merge-one-file.sh | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 0f164e5..78b07a8 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
>                 ;;
>         esac
>
> +       ret=0
>         src1=$(git-unpack-file $2)
>         src2=$(git-unpack-file $3)
>         case "$1" in
>         '')
> -               echo "Added $4 in both, but differently."
> +               echo "ERROR: Added $4 in both, but differently."
> +               ret=1
>                 orig=$(git-unpack-file $2)
>                 create_virtual_base "$orig" "$src2"
>                 ;;
> @@ -121,10 +123,9 @@ case "${1:-.}${2:-.}${3:-.}" in
>         # Be careful for funny filename such as "-L" in "$4", which
>         # would confuse "merge" greatly.
>         git merge-file "$src1" "$orig" "$src2"
> -       ret=$?
> -       msg=
> -       if [ $ret -ne 0 ]; then
> -               msg='content conflict'
> +       if [ $? -ne 0 ]; then
> +               echo "ERROR: Content conflict in $4"
> +               ret=1

if test $? != 0 then

Also.. should this error not go to stderr? I guess the existing script was not doing that, but it seems like anything that says "ERROR" should go there.

Show 23 quoted lines

>         fi
>
>         # Create the working tree file, using "our tree" version from the
> @@ -133,18 +134,11 @@ case "${1:-.}${2:-.}${3:-.}" in
>         rm -f -- "$orig" "$src1" "$src2"
>
>         if [ "$6" != "$7" ]; then
> -               if [ -n "$msg" ]; then
> -                       msg="$msg, "
> -               fi
> -               msg="${msg}permissions conflict: $5->$6,$7"
> -               ret=1
> -       fi
> -       if [ "$1" = '' ]; then
> +               echo "ERROR: Permissions conflict: $5->$6,$7"
>                 ret=1
>         fi
>
>         if [ $ret -ne 0 ]; then
> -               echo "ERROR: $msg in $4"
>                 exit 1
>         fi
>         exec git update-index -- "$4"

same notes as above.  I think a style patch should come first.

-- 
David