[darcs-devel] darcs patch: configure.ac: restore bytestring to default (and 25 more)

David Roundy droundy at darcs.net
Thu Apr 17 15:28:21 UTC 2008


I'm rejecting this patch.  I'm not going to include cabalization changes
that don't work (e.g. don't support ghc < 6.8), as I believe this will
confuse users and/or reduce the amount of testing darcs' real build process
receives.  I may not apply even a proper cabalization, but one that will
not build darcs on my system is not even going to be under consideration.

This set of patches would have been rejected even if I were eager to get
cabal support into darcs, due to its unreadable nature.  You need to pay
attention to what changes you're recording as you record patches, and you
need to avoid including unrelated changes in patches.  It also really helps
to send separate patches in separate bundles whenever possible.

>  AC_CONFIG_COMMANDS([config.command],
>                     [ echo > config.command "# configured $date"
>                       echo >>config.command "./configure $args"
> -		     chmod +x config.command
> +                     chmod +x config.command
>                     ],
>                     [date="`date`"; args="$ac_cmdline_args"])

Here, for instance, you've changed the whitespace, for no apparent reason.
But nevertheless, I am forced to read this line carefully to ensure that
nothing else was changed.

> hunk ./configure.ac 275
>  
>  if test "$with_docs" = "yes"; then
> -	AC_CHECK_PROG(LATEX, latex, latex)
> -	if test -z "$LATEX"; then
> -	AC_MSG_WARN([Cannot find latex in your path!])
> -	fi
> -	AC_CHECK_PROG(PDFLATEX, pdflatex, pdflatex)
> -	if test -z "$PDFLATEX"; then
> -	AC_MSG_WARN([Cannot find pdflatex in your path!])
> -	fi
> -	AC_CHECK_PROG(DVIPS, dvips, dvips)
> -	if test -z "$DVIPS"; then
> -	AC_MSG_WARN([Cannot find dvips in your path!])
> -	fi
> -	if test ! '(' '(' -z "$LATEX" ')' -o '(' -z "$DVIPS" ')' ')'; then
> -	TARGETS="$TARGETS doc/manual/darcs.ps"
> -	INSTALLWHAT="$INSTALLWHAT installdocs"
> -	BUILDDOC="yes"
> -	fi
> -	if test ! '(' -z "$PDFLATEX" ')'; then
> -	TARGETS="$TARGETS doc/manual/darcs.pdf doc/manual/patch-theory.pdf"
> -	fi
> -	AC_CHECK_PROG(LATEX2HTML, latex2html, latex2html)
> -	if test -z "$LATEX2HTML"; then
> -	AC_MSG_WARN([Cannot find latex2html in your path!])
> -	PREPROCHTML=""
> -	AC_CHECK_PROG(HTLATEX, htlatex, htlatex)
> -	if test -z "$HTLATEX"; then
> -		AC_MSG_WARN([Cannot find htlatex in your path either!])
> -		AC_CHECK_PROG(HEVEA, hevea, hevea)
> -		if test -z "$HEVEA"; then
> -		AC_MSG_WARN([Cannot find hevea in your path either!])
> -		MAKEMANUAL="touch doc/manual/index.html; echo Cannot make manual!"
> -		else
> -		TARGETS="$TARGETS doc/manual/index.html"
> -		MAKEMANUAL="$HEVEA -o doc/manual/index.html src/darcs.tex"
> -		fi
> -	else
> -		TARGETS="$TARGETS doc/manual/index.html"
> -		MAKEMANUAL="cd doc/manual && $HTLATEX ../../src/darcs.tex && ln -sf darcs.html index.html"
> -	fi
> -	else
> -	TARGETS="$TARGETS doc/manual/index.html"
> -	MAKEMANUAL="$LATEX2HTML -split +1 -dir doc/manual src/darcs.tex"
> -	dnl the following tells "preproc" to generate "rawhtml" sections for latex2html.
> -	PREPROCHTML="--html"
> -	fi
> +        AC_CHECK_PROG(LATEX, latex, latex)
> +        if test -z "$LATEX"; then
> +        AC_MSG_WARN([Cannot find latex in your path!])
> +        fi
> +        AC_CHECK_PROG(PDFLATEX, pdflatex, pdflatex)
> +        if test -z "$PDFLATEX"; then
> +        AC_MSG_WARN([Cannot find pdflatex in your path!])
> +        fi
> +        AC_CHECK_PROG(DVIPS, dvips, dvips)
> +        if test -z "$DVIPS"; then
> +        AC_MSG_WARN([Cannot find dvips in your path!])
> +        fi
> +        if test ! '(' '(' -z "$LATEX" ')' -o '(' -z "$DVIPS" ')' ')'; then
> +        TARGETS="$TARGETS doc/manual/darcs.ps"
> +        INSTALLWHAT="$INSTALLWHAT installdocs"
> +        BUILDDOC="yes"
> +        fi
> +        if test ! '(' -z "$PDFLATEX" ')'; then
> +        TARGETS="$TARGETS doc/manual/darcs.pdf doc/manual/patch-theory.pdf"
> +        fi
> +        AC_CHECK_PROG(LATEX2HTML, latex2html, latex2html)
> +        if test -z "$LATEX2HTML"; then
> +        AC_MSG_WARN([Cannot find latex2html in your path!])
> +        PREPROCHTML=""
> +        AC_CHECK_PROG(HTLATEX, htlatex, htlatex)
> +        if test -z "$HTLATEX"; then
> +                AC_MSG_WARN([Cannot find htlatex in your path either!])
> +                AC_CHECK_PROG(HEVEA, hevea, hevea)
> +                if test -z "$HEVEA"; then
> +                AC_MSG_WARN([Cannot find hevea in your path either!])
> +                MAKEMANUAL="touch doc/manual/index.html; echo Cannot make manual!"
> +                else
> +                TARGETS="$TARGETS doc/manual/index.html"
> +                MAKEMANUAL="$HEVEA -o doc/manual/index.html src/darcs.tex"
> +                fi
> +        else
> +                TARGETS="$TARGETS doc/manual/index.html"
> +                MAKEMANUAL="cd doc/manual && $HTLATEX ../../src/darcs.tex && ln -sf darcs.html index.html"
> +        fi
> +        else
> +        TARGETS="$TARGETS doc/manual/index.html"
> +        MAKEMANUAL="$LATEX2HTML -split +1 -dir doc/manual src/darcs.tex"
> +        dnl the following tells "preproc" to generate "rawhtml" sections for latex2html.
> +        PREPROCHTML="--html"
> +        fi
>  else

This hunk is completely unreadable.  You've changed whitespace in so many
places that I have no idea what actual changes you made.  When you are
prompted for a hunk like this while you record, you need to ask yourself
"Did I really modify that entire block of code?".  In this case the answer
is that no, you didn't modify that entire block of code, you only
reindented it.  But there's no convenient way for me to determine what you
*did* modify, since I presume (perhaps incorrectly?) that somewhere in here
is an actual change, or you wouldn't have sent me this patch.

> hunk ./configure.ac 321
> -	BUILDDOC="no"
> +        BUILDDOC="no"
>  fi

Similarly.

The rest of the patch bundle didn't look much better.  More use of
amend-record would also be helpful.
-- 
David Roundy
Department of Physics
Oregon State University


More information about the darcs-devel mailing list