From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 03 Nov 2008 04:58:44 GMT
Subject: Re: [PATCH] filter-branch: add git_commit_non_empty_tree and --prune-empty.
Message-ID: <7viqr5wgl7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1225445204-28000-1-git-send-email-madcoder@debian.org>

Pierre Habouzit <madcoder@debian.org> writes:

> +case "$prune_empty,$filter_commit" in
> +',')
> +	filter_commit='git commit-tree "$@"';;
> +'t,')
> +	filter_commit="$functions;"' git_commit_non_empty_tree "$@"';;
> +','*)
> +	;;
> +*)
> +	die "Cannot set --prune-empty and --filter-commit at the same time"
> +esac

This is only style issue, but I find the above extremely difficult to
read.  If it were either:

	case ... in
        ,) do "neither set case" ;;
        t,) do "prune but not filter case" ;;
        *) do "both set case" ;;
        esac

or (rather amateurish but conveys what it wants to do more clearly):
        
	case ... in
        '','') do "neither set case" ;;
        t,'') do "prune but not filter case" ;;
        t,t) do "both set case" ;;
        esac

I wouldn't have to wonder which sq pairs with which one.

> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index b0a9d7d..352b56b 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -262,4 +262,12 @@ test_expect_success 'Tag name filtering allows slashes in tag names' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'Prune empty commits' '
> +	make_commit to_remove &&
> +	(git rev-list HEAD | grep -v $(git rev-parse HEAD)) > expect &&

I am not sure what this one is doing.

 - Isn't this the same as "git rev-list HEAD^"?
 - Do you need a subshell?