Git development
 help / color / mirror / Atom feed
* Re: linux-next: Tree for Aug 18 (GNU patch and "git diffs: Support symlinks")
From: Sedat Dilek @ 2011-08-18  9:35 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: linux-next, LKML, git, Jonathan Nieder, Christoph Berg

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Thu, Aug 18, 2011 at 8:00 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> The powerpc allyesconfig build still fails today.
>
> Changes since 20110817:
>
> The sound-asoc tree lost its build failure.
>
> The tip tree gained a conflict against Linus' tree.
>
> The moduleh tree lost 2 of its merge fixups, but I added another.
>
> ----------------------------------------------------------------------------
>
> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (patches at http://www.kernel.org/pub/linux/kernel/v2.6/next/ ).  If you
> are tracking the linux-next tree using git, you should not use "git pull"
> to do so as that will try to merge the new linux-next release with the
> old one.  You should use "git fetch" as mentioned in the FAQ on the wiki
> (see below).
>

Just FYI:

Yesterday and today, I had the same problem when applying the
linux-next single patch (see attached ErrMsg text file).

I am using a slightly modified kernel-buildsystem from Debian Kernel
Team (so not building out of a GIT repository).

The GNU/patch I have installed from Debian/sid does not include
symlink-support for git-diff [1].
Jonathan gave me the pointer to install patch package from Debian/experimental.
After reading its Changelog [2] and Debian-bug #243309 [3] I installed
the recommended version.
Indeed, it fixes the issue.

Cristoph, can we have a backport of [1] to patch/sid?

Thanks Jonathan!

- Sedat -

[1] http://git.savannah.gnu.org/cgit/patch.git/commit/?id=f9db7450fa2788377c4567b5d0565b0ddfc1015c
[2] http://packages.debian.org/changelogs/pool/main/p/patch/patch_2.6.1.85-423d-3/changelog
[3] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=243309

[-- Attachment #2: ErrMsg_linux-next-patch-did-not-apply.txt --]
[-- Type: text/plain, Size: 4455 bytes --]

[ patch-v3.1-rc2-next-20110818 ]
...
diff --git a/tools/power/cpupower/debug/x86_64/Makefile b/tools/power/cpupower/debug/x86_64/Makefile
index dbf1399..3326217 100644
--- a/tools/power/cpupower/debug/x86_64/Makefile
+++ b/tools/power/cpupower/debug/x86_64/Makefile
@@ -1,10 +1,10 @@
 default: all

-centrino-decode: centrino-decode.c
-       $(CC) $(CFLAGS) -o centrino-decode centrino-decode.c
+centrino-decode: ../i386/centrino-decode.c
+       $(CC) $(CFLAGS) -o $@ $<

-powernow-k8-decode: powernow-k8-decode.c
-       $(CC) $(CFLAGS) -o powernow-k8-decode powernow-k8-decode.c
+powernow-k8-decode: ../i386/powernow-k8-decode.c
+       $(CC) $(CFLAGS) -o $@ $<

 all: centrino-decode powernow-k8-decode

diff --git a/tools/power/cpupower/debug/x86_64/centrino-decode.c b/tools/power/cpupower/debug/x86_64/centrino-decode.c
deleted file mode 120000
index 26fb3f1..0000000
--- a/tools/power/cpupower/debug/x86_64/centrino-decode.c
+++ /dev/null
@@ -1 +0,0 @@
-../i386/centrino-decode.c
\ No newline at end of file
diff --git a/tools/power/cpupower/debug/x86_64/powernow-k8-decode.c b/tools/power/cpupower/debug/x86_64/powernow-k8-decode.c
deleted file mode 120000
index eb30c79..0000000
--- a/tools/power/cpupower/debug/x86_64/powernow-k8-decode.c
+++ /dev/null
@@ -1 +0,0 @@
-../i386/powernow-k8-decode.c
\ No newline at end of file
...


$ LC_ALL=C fakeroot make -f debian/rules.gen setup_i386_none_686-small 2>&1 | tee ../setup.log                                                           
make -f debian/rules.real setup-flavour ABINAME='-next20110818.3' ARCH='i386' COMPILER='gcc-4.6' FEATURESET='none' FLAVOUR='686-small' KCONFIG='debian/config/config debian/config/kernelarch-x86/config debian/config/kernelarch-x86/config-arch-32 debian/config/i386/none/config.686-iniza debian/config/custom-x86/config-debug debian/config/custom-x86/config-fs debian/config/custom-x86/config-infiniband debian/config/custom-x86/config-isa debian/config/custom-x86/config-isdn debian/config/custom-x86/config-multimedia debian/config/custom-x86/config-pseudofs debian/config/custom-x86/config-staging debian/config/i386/none/config.686-small' KERNEL_ARCH='x86' LOCALVERSION='-686-small' LOCALVERSION_HEADERS='' LOCALVERSION_IMAGE='-686-small' MODULES='True' SOURCEVERSION='3.1.0~rc2-1~next20110818.dileks3' TYPE='plain' UPSTREAMVERSION='3.1.0-rc2' VERSION='3.1.0'                                                                                                                            
make[1]: Entering directory `/mnt/sdb3/linux-kernel/linux-3.1-rc2'                                                                                                                                   
rm -rf 'debian/build/source'                                                                                                                                                                         
mkdir -p 'debian/build/source'                                                                                                                                                                       
cp -al COPYING CREDITS Documentation Kbuild Kconfig MAINTAINERS Makefile README REPORTING-BUGS arch block crypto drivers firmware fs include init ipc kernel lib mm net samples scripts security sound tools usr virt .gitignore .mailmap 'debian/build/source'                                                                                                                                           
cd 'debian/build/source'; python '/mnt/sdb3/linux-kernel/linux-3.1-rc2/debian/bin/patch.apply' --overwrite-home='/mnt/sdb3/linux-kernel/linux-3.1-rc2/debian/patches'                                
Warning: No version.Debian file, assuming Debian Linux 3.1.0~rc2                                                                                                                                     
1 out of 1 hunk FAILED -- saving rejects to file tools/power/cpupower/debug/x86_64/centrino-decode.c.rej                                                                                             
1 out of 1 hunk FAILED -- saving rejects to file tools/power/cpupower/debug/x86_64/powernow-k8-decode.c.rej
Error: Patch failed
--> Try to apply base.
  (+) FAIL linux-next/patch-v3.1-rc2-next-20110818
make[1]: *** [debian/stamps/source] Error 1
make[1]: Leaving directory `/mnt/sdb3/linux-kernel/linux-3.1-rc2'
make: *** [setup_i386_none_686-small_real] Error 2


-dileks // 18-Aug-2011


^ permalink raw reply related

* Re: [PATCH] rev-parse: Allow @{U} as a synonym for @{u}
From: Michael J Gruber @ 2011-08-18  9:31 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git, Conrad Irwin, Sitaram Chamarty
In-Reply-To: <CACsJy8Dr5q=h-9MhNV0gKtVEn13+DxWwcNn2Prem6-+DyrDmHA@mail.gmail.com>

Nguyen Thai Ngoc Duy venit, vidit, dixit 18.08.2011 10:54:
> On Thu, Aug 18, 2011 at 2:15 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> For a german keyboard layout, you get @{u} from
>>
>> hold AltGr (Meta)
>> press q
>> press 7
>> release AltGr
>> press u
>> hold AltGr
>> press 0
>>
>> For me, an improvement would be to have the alias
>>
>> @{↓}
> 
> While it's convenient to type, the output now looks strange. If you do
> a screencast, or someone is watching you, they may not understand what
> that syntax is.

just like with any shell or git alias.

> 
> Also..
> 
>> As a simpler case, a user could tailor to her keyboard layout with
>>
>> git config revalias.↓ u
> 
> We may also have encoding issue here. What if config file is in utf-8,
> but the console is not?

It's a user setting, tailored for the user's environment.

> So I don't we should go this way. There may be a better way to deal
> with different keyboard layouts.
> 
>> Well, this patch is actually specific to a particular keyboard layout!
>> It improves things only for a layout where you get @ and { using a shift
>> key.
> 
> Fully agreed. While it does not help non en-US keyboards, it does not
> cause any harm (jealousy is not classified as harm to me, rather
> motivation ;).

If you sell it as

"rev key is like a config key, so make it case insensitive. As a side
effect, it helps certain keyboard layouts and does not harm others."

it may fair a better chance ;)

I guess this should apply to rev^{<type>} as well, then.

Michael

^ permalink raw reply

* Re: [PATCH 4/4] mergetools/meld: Use '--output' when available
From: David Aguilar @ 2011-08-18  9:24 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <20110818081309.GP31888@elie.gateway.2wire.net>

On Thu, Aug 18, 2011 at 03:13:10AM -0500, Jonathan Nieder wrote:
> David Aguilar wrote:
> 
> > use the '--output' option when available.
> 
> Yay. :)
> 
> > --- a/mergetools/meld
> > +++ b/mergetools/meld
> > @@ -4,6 +4,37 @@ diff_cmd () {
> >  
> >  merge_cmd () {
> >  	touch "$BACKUP"
> > -	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > +	if test "$meld_has_output_option" = true
> > +	then
> > +		"$merge_tool_path" --output "$MERGED" \
> > +			"$BASE" "$LOCAL" "$REMOTE"
> 
> Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?

Yup, thanks.

> 
> > +	else
> > +		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > +	fi
> >  	check_unchanged
> 
> I wonder if the version test could be made a little simpler (perhaps
> to cope better if future versions use a different numbering system):
> 
> 	if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
> 	then
> 		"$merge_tool_path" --output ...
> 	else
> 		"$merge_tool_path" "$LOCAL" ...
> 	fi
> 
> Forgive my ignorance: is this function likely to be called in a loop?
> If so, it makes sense to precompute or cache the result of detection,
> like you already do.

Yes, it is called in a loop...


> 	check_meld_for_output_option () {
> 		if ...
> 		then
> 			meld_has_output_option=true
> 		else
> 			meld_has_output_option=false
> 		fi
> 	}
> 
> 	merge_cmd () {
> 		if test -z "${meld_has_output_option:+set}"
> 		then
> 			check_meld_for_output_option
> 		fi
> 
> 		if test "$meld_has_output_option" = true
> 		then
> 			...
> 
> 
> [...]
> > +	# Filter meld --version to contain numbers and dots only
> > +	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
> 
> \+ is not a BRE.  If parsing version numbers seems like the right
> thing to do, maybe "tr -cd 0-9."?
> 
> > +	meld="${meld:-0.0.0}"
> > +
> > +	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> > +	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
> 
> I think git avoids \{m,n\} ranges where possible (for portability).
> This could be:
> 
> 	meld_major=${meld%%.*}
> 	meld_nonmajor=${meld#${meld_major}.}
> 	meld_minor=${meld_nonmajor%%.*}
> 
> or:
> 
> 	case $meld in
> 	[2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*)	# >= 1.5.0
> 		meld_has_output_option=true ;;
> 	*)
> 		meld_has_output_option=false ;;
> 	esac
> 
> It's nice how self-contained this can be now that it's in its own
> file.  Thanks.

Right, I was using \{1,\} since that's what CodingStyle said to
use instead of \+ (which I forgot to fixup above as you saw).

The case statement is nice and simple enough to understand.
By doing meld --output /dev/null --help we're relying on
older versions blowing up with --output and --help returning
exit status 0.  That seems pretty reasonable.
The case statement does seem sufficient but just trying
--output and seeing what happens is even simpler.
Your example also uses $merge_tool_path, which I forgot to do,
so I'll be sure to include that too.

I'll wait until tomorrow to see if there are any more comments
and reroll.

Thanks,
-- 
					David

^ permalink raw reply

* Re: [PATCH v2] git svn : hook before 'git svn dcommit'
From: Peter Baumann @ 2011-08-18  9:12 UTC (permalink / raw)
  To: Frédéric Heitzmann; +Cc: Eric Wong, git, Junio C Hamano
In-Reply-To: <CALeToSUidWgiJ+PxuphnBZOQGNStNOO9==EswfLDpFr2GYy=nA@mail.gmail.com>

On Wed, Aug 17, 2011 at 04:35:03PM +0200, Frédéric Heitzmann wrote:
> Hi all.
> 
> Maybe I should give some more context to explain why a hook could be a
> potential improvement.
> 
> Let's consider the following workflow :
> 1) git svn clone from the SVN server, then git checkout -b topic
> 2) git commit some "reference data", before starting some optimization
> or code refactoring.
> ** These reference data are not supposed to find their way to the SVN server **
> Committing such "reference data" is just a convenience because git
> does a great job to show how these data may or may not change during
> the development process.
> 3) hack, test, commit ...
> 3 bis) it may happen that reference data change for some very good
> reason (for instance some protocol change)
> New reference data are then commited.
> 
> back to 3 ...
> 
> 4) Before merging back to master and commitng to SVN, it is necessary
> to remove commits with reference data (git rebase -i --onto master
> master topic ...)
> 5) merge topic branch with master and git svn dcommit
> 
> -- end --
> 
> It is very easy to forget step 4, and svn commit lots of useless data.
> 
> Proposal 1)
> * commit reference data with some specific mark in the commit message
> (e.g. "NO_SVN")
> * use pre-svn-dcommit hook to detect such commits
> 
> Proposal 2) (not fully feasable for what I know)
> * git svn clone to a bare repo
> * clone a working repo from the the bare repo.
> * steps 2, 3, maybe 3bis, ... then 4
> * push commits to the bare repo, while using pre-receive or update
> hook to look for wrong commits, and abort if so.
> * use post-receive hook to trigger git svn dcommit
> 
> Main drawback for proposal 2 (appart from needing 2 repo instead of
> one) is that each time you want to update your working  repo, you have
> to git svn rebase the bare repo, then git pull.
> 
> Proposal 2bis)
> * add a pre-send hook on the bare repo, and trigger some git svn
> rebase with this hook.
> I am not sure to see all the potential consequences of such a hook though.
> 
> All things begin equal, proposal 1 seems to be the easier path, but it
> is highly debatable.
> 

I have written a local script for exactly the problem you described after looking
for a git svn dcommit hook I could use (as you did).
I attached it, so feel free to use it. Simply add it to your bin and run it
with   git dcommit    instead of  git svn dcommit

Pls read the comment for further explanation how this script is used.


#!/bin/bash
# Copyright © Peter Baumann, 2011
#
# Wrapper script around git svn dcommit, which adds some useful functionality
#
# This script will prevent accidentally commiting some commits not yet ready
# into SVN. Commits starting with (case insensitive) debug, wip, fixup are
# considered not appropriate for putting them into SVN. The main reason for
# this functionality is the specific workflow I use. I always have some 
# internal debug commits (e.g. enhanced debug logging) or simply work in progress
# commits which should never be put into SVN. 
#
# To avoid putting those into SVN, I rebase all commits so that my WIP/DEBUG commits 
# are on top of the commits ment for SVN.
# Calling this script via "git dcommit" after the rebase makes sure only commits
# beneath the WIP commits are considered for SVN. 
# Furthermore, a shortlog of commits ment for SVN is shown and the user has
# to confirm before actually putting them into SVN.
#
# If this script is called via a specific commit (e.g. via its SHA1) as parameter,
# then only commits beneath and including the commit itself are committet to SVN.


SUBDIRECTORY_OK=Yes

. git-sh-setup
require_work_tree
cd_to_toplevel || die "foo"

# Upstream ref
upstream=remotes/trunk

# Stop at this commit
last=
if [ ! -z $1 ]; then
	$(git rev-parse $1^{commit})
	echo $last
fi

# The latest git commit we want to commit SVN
commit=

# Remembers the original head
orig_head=
if branch=$(git symbolic-ref -q HEAD)
then
	orig_head=${branch#refs/heads/}
else
	orig_head='(detached head)'
fi


function run()
{
	#echo "DEBUG: $@"
	$@
}	

IFS='
'
for c in $(git log --reverse --pretty="%H %s" HEAD --not "${upstream}"); do
	# Split the log output into its fields
	sha1="${c:0:40}"
	msg="${c:41}"

	# Check if the commit subject matches (case insenstive) to one of the
	# following patterns. Leading whitespace is fine
	# 	debug
	#	wip
	#	fixup!
	if echo "$msg"|egrep -i -q '^\s*(debug|wip|fixup)'; then
		break
	fi
	commit=${sha1}

	if [ "x${commit}" = "x${last}" ]; then
		break
	fi
done


if [ "x${commit}" == "x" ]; then
	die "Nothing to commit - Perhaps you have only stuff not ready for SVN?"
fi

echo ">>>> Committing the folling GIT commits to SVN <<<<"
git --no-pager log --pretty=oneline ${commit} --not "${upstream}"
echo

# Show the latest commit we are going to submit to SVN
#git show ${commit}

echo
echo "Commiting to SVN (y/N)?"
read yesno || die "Aborting"

if [ "x${yesno}" == "xy" ] || [ "x$yesno" == "xY" ]; then
	run git checkout -q "${commit}" || die "Checkout failed"
	run git svn dcommit || "Aborting - git svn dcommit failed!"

	if [ "${orig_head}" != "(detached head)" ]; then
		echo "DO THIS:"
		run git checkout "${orig_head}" && run git rebase "${upstream}"
	else
		echo "You have started this script being on a detached HEAD."
		echo "Please rebase manually!"
	fi
fi

^ permalink raw reply

* Re: [PATCH 1/4] difftool--helper: Make style consistent with git
From: David Aguilar @ 2011-08-18  9:08 UTC (permalink / raw)
  To: Sebastian Schuberth
  Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
	Charles Bailey
In-Reply-To: <CAHGBnuPRsh0WDVSFr0X1Y+nb=94pJVv8=evFQCjdcd_y9ux97A@mail.gmail.com>

On Thu, Aug 18, 2011 at 09:49:33AM +0200, Sebastian Schuberth wrote:
> On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> 
> > Use the predominant conditional style where "then" appears
> > alone on the line after the test expression.
> 
> I support your effort to unify the style, but for my personal taste it
> has gone the wrong way :-) Even if "then" on its own line was the
> predominant style in the merge scripts, I would have voted for putting
> all "then" in the same line as "if" as I find this easier to read and
> comprehend. But then again it's obvious that such a change would
> polarize ;-)
> 
> -- 
> Sebastian Schuberth

I am fine reworking it either way.
There's not a specific note about it in
Documentation/CodingStyle.

If someone wants to make the final call I can patch the docs
and rework these patches as needed.

Thanks,
-- 
					David

^ permalink raw reply

* Re: [PATCH] rev-parse: Allow @{U} as a synonym for @{u}
From: Nguyen Thai Ngoc Duy @ 2011-08-18  8:54 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Junio C Hamano, git, Conrad Irwin, Sitaram Chamarty
In-Reply-To: <4E4CBC29.7080703@drmicha.warpmail.net>

On Thu, Aug 18, 2011 at 2:15 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> For a german keyboard layout, you get @{u} from
>
> hold AltGr (Meta)
> press q
> press 7
> release AltGr
> press u
> hold AltGr
> press 0
>
> For me, an improvement would be to have the alias
>
> @{↓}

While it's convenient to type, the output now looks strange. If you do
a screencast, or someone is watching you, they may not understand what
that syntax is.

Also..

> As a simpler case, a user could tailor to her keyboard layout with
>
> git config revalias.↓ u

We may also have encoding issue here. What if config file is in utf-8,
but the console is not?

So I don't we should go this way. There may be a better way to deal
with different keyboard layouts.

> Well, this patch is actually specific to a particular keyboard layout!
> It improves things only for a layout where you get @ and { using a shift
> key.

Fully agreed. While it does not help non en-US keyboards, it does not
cause any harm (jealousy is not classified as harm to me, rather
motivation ;).
-- 
Duy

^ permalink raw reply

* Re: [PATCH 4/4] mergetools/meld: Use '--output' when available
From: Jonathan Nieder @ 2011-08-18  8:13 UTC (permalink / raw)
  To: David Aguilar
  Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <1313652227-48545-5-git-send-email-davvid@gmail.com>

David Aguilar wrote:

> use the '--output' option when available.

Yay. :)

> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -4,6 +4,37 @@ diff_cmd () {
>  
>  merge_cmd () {
>  	touch "$BACKUP"
> -	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> +	if test "$meld_has_output_option" = true
> +	then
> +		"$merge_tool_path" --output "$MERGED" \
> +			"$BASE" "$LOCAL" "$REMOTE"

Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?

> +	else
> +		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> +	fi
>  	check_unchanged

I wonder if the version test could be made a little simpler (perhaps
to cope better if future versions use a different numbering system):

	if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
	then
		"$merge_tool_path" --output ...
	else
		"$merge_tool_path" "$LOCAL" ...
	fi

Forgive my ignorance: is this function likely to be called in a loop?
If so, it makes sense to precompute or cache the result of detection,
like you already do.

	check_meld_for_output_option () {
		if ...
		then
			meld_has_output_option=true
		else
			meld_has_output_option=false
		fi
	}

	merge_cmd () {
		if test -z "${meld_has_output_option:+set}"
		then
			check_meld_for_output_option
		fi

		if test "$meld_has_output_option" = true
		then
			...


[...]
> +	# Filter meld --version to contain numbers and dots only
> +	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"

\+ is not a BRE.  If parsing version numbers seems like the right
thing to do, maybe "tr -cd 0-9."?

> +	meld="${meld:-0.0.0}"
> +
> +	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> +	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"

I think git avoids \{m,n\} ranges where possible (for portability).
This could be:

	meld_major=${meld%%.*}
	meld_nonmajor=${meld#${meld_major}.}
	meld_minor=${meld_nonmajor%%.*}

or:

	case $meld in
	[2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*)	# >= 1.5.0
		meld_has_output_option=true ;;
	*)
		meld_has_output_option=false ;;
	esac

It's nice how self-contained this can be now that it's in its own
file.  Thanks.

^ permalink raw reply

* Re: [PATCH] stash: Utilize config variable pager.stash.list in stash list command
From: Ingo Brückl @ 2011-08-18  7:55 UTC (permalink / raw)
  To: git
In-Reply-To: <20110818042620.GA19045@sigill.intra.peff.net>

Jeff King wrote on Wed, 17 Aug 2011 21:26:37 -0700:

> On Wed, Aug 17, 2011 at 11:44:32AM -0700, Junio C Hamano wrote:

>> What's so difficult to say "git stash list | less" or even "git -p
>> stash list"?

> Anyway, I think his problem is not "I want a pager but I am too lazy to
> type it", but rather that "git stash list" will auto-paginate by
> default, because it is chaining to "log", which auto-paginates. You can
> turn it off with "--no-pager", but pager.stash seems to have no effect.

I'd like to mention that in this particular case it is ok that stash
auto-paginates, it only is annoying that "stash list" does. And, Junio,
it is totally ok to say "git -p stash list" then, because this is exactely
what I'd do.

Ingo

^ permalink raw reply

* Re: [PATCH 0/4] Refactor git-mergetool--lib.sh
From: Sebastian Schuberth @ 2011-08-18  7:57 UTC (permalink / raw)
  To: David Aguilar
  Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
	Charles Bailey
In-Reply-To: <1313652227-48545-1-git-send-email-davvid@gmail.com>

On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:

> This series splits the git-mergetool--lib.sh file into separate
> tool scriptlets that define the commands for diff and merge.

Very nice as it simplifies adding new merge tools to adding a single
file with little code inside. Thanks!

-- 
Sebastian Schuberth

^ permalink raw reply

* Re: [RFC/PATCH v3] git-svn: New flag to emulate empty directories
From: Ray Chen @ 2011-08-18  7:55 UTC (permalink / raw)
  To: Eric Wong; +Cc: git, Michael Haggerty, Michael J Gruber
In-Reply-To: <20110817203934.GC24581@dcvr.yhbt.net>

On Wed, Aug 17, 2011 at 4:39 PM, Eric Wong <normalperson@yhbt.net> wrote:
>
> We haven't heard anything in a few weeks.  Everything looks acceptable
> to me.  I'm inclined to just accept it since it's unlikely to break
> existing functionality.
>

Great.  Thanks for keeping this patch in mind, Eric.

Let me know if there's anything else I should do,
- Ray

^ permalink raw reply

* Re: [PATCH 1/4] difftool--helper: Make style consistent with git
From: Sebastian Schuberth @ 2011-08-18  7:49 UTC (permalink / raw)
  To: David Aguilar
  Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
	Charles Bailey
In-Reply-To: <1313652227-48545-2-git-send-email-davvid@gmail.com>

On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:

> Use the predominant conditional style where "then" appears
> alone on the line after the test expression.

I support your effort to unify the style, but for my personal taste it
has gone the wrong way :-) Even if "then" on its own line was the
predominant style in the merge scripts, I would have voted for putting
all "then" in the same line as "if" as I find this easier to read and
comprehend. But then again it's obvious that such a change would
polarize ;-)

-- 
Sebastian Schuberth

^ permalink raw reply

* [PATCH 3/4] mergetool--lib: Refactor tools into separate files
From: David Aguilar @ 2011-08-18  7:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <1313652227-48545-1-git-send-email-davvid@gmail.com>

Individual merge tools are now defined in a mergetools/$tool
file which is sourced at runtime.

The individual files are installed into $(git --exec-path)/mergetools/.
New tools can be added by creating a new file instead of editing the
git-mergetool--lib.sh scriptlet.

http://thread.gmane.org/gmane.comp.version-control.git/134906/focus=135006

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Makefile                 |   11 ++
 git-mergetool--lib.sh    |  385 ++++++---------------------------------------
 mergetools/araxis        |   21 +++
 mergetools/bc3           |   20 +++
 mergetools/defaults      |   46 ++++++
 mergetools/diffuse       |   17 ++
 mergetools/ecmerge       |   16 ++
 mergetools/emerge        |   23 +++
 mergetools/kdiff3        |   24 +++
 mergetools/kompare       |    7 +
 mergetools/meld          |    9 +
 mergetools/opendiff      |   16 ++
 mergetools/p4merge       |   10 ++
 mergetools/tkdiff        |   12 ++
 mergetools/tortoisemerge |   17 ++
 mergetools/vim           |   44 ++++++
 mergetools/xxdiff        |   25 +++
 17 files changed, 370 insertions(+), 333 deletions(-)
 create mode 100644 mergetools/araxis
 create mode 100644 mergetools/bc3
 create mode 100644 mergetools/defaults
 create mode 100644 mergetools/diffuse
 create mode 100644 mergetools/ecmerge
 create mode 100644 mergetools/emerge
 create mode 100644 mergetools/kdiff3
 create mode 100644 mergetools/kompare
 create mode 100644 mergetools/meld
 create mode 100644 mergetools/opendiff
 create mode 100644 mergetools/p4merge
 create mode 100644 mergetools/tkdiff
 create mode 100644 mergetools/tortoisemerge
 create mode 100644 mergetools/vim
 create mode 100644 mergetools/xxdiff

diff --git a/Makefile b/Makefile
index 8dd782f..6a68e3a 100644
--- a/Makefile
+++ b/Makefile
@@ -302,6 +302,7 @@ bindir = $(prefix)/$(bindir_relative)
 mandir = share/man
 infodir = share/info
 gitexecdir = libexec/git-core
+mergetoolsdir = $(gitexecdir)/mergetools
 sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
@@ -2257,6 +2258,13 @@ endif
 gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
+ifneq ($(filter /%,$(firstword $(mergetoolsdir))),)
+mergetools_instdir = $(mergetoolsdir)
+else
+mergetools_instdir = $(prefix)/$(mergetoolsdir)
+endif
+mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
+
 install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
 
 install: all
@@ -2266,6 +2274,9 @@ install: all
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+	(cd mergetools && $(TAR) cf - .) | \
+	(cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C gitweb install
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 03cfb19..2421700 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -9,26 +9,7 @@ merge_mode() {
 }
 
 translate_merge_tool_path () {
-	case "$1" in
-	araxis)
-		echo compare
-		;;
-	bc3)
-		echo bcompare
-		;;
-	emerge)
-		echo emacs
-		;;
-	gvimdiff|gvimdiff2)
-		echo gvim
-		;;
-	vimdiff|vimdiff2)
-		echo vim
-		;;
-	*)
-		echo "$1"
-		;;
-	esac
+	echo "$1"
 }
 
 check_unchanged () {
@@ -49,40 +30,55 @@ check_unchanged () {
 	fi
 }
 
+valid_tool_config () {
+	if test -n "$(get_merge_tool_cmd "$1")"
+	then
+		return 0
+	else
+		return 1
+	fi
+}
+
 valid_tool () {
+	setup_tool "$1" || valid_tool_config "$1"
+}
+
+setup_tool () {
 	case "$1" in
-	araxis | bc3 | diffuse | ecmerge | emerge | gvimdiff | gvimdiff2 | \
-	kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
-		;; # happy
-	kompare)
-		if ! diff_mode
-		then
-			return 1
-		fi
-		;;
-	tortoisemerge)
-		if ! merge_mode
-		then
-			return 1
-		fi
+	vim*|gvim*)
+		tool=vim
 		;;
 	*)
-		if test -z "$(get_merge_tool_cmd "$1")"
-		then
-			return 1
-		fi
+		tool="$1"
 		;;
 	esac
+	mergetools="$(git --exec-path)/mergetools"
+
+	# Load the default definitions
+	. "$mergetools/defaults"
+	if ! test -f "$mergetools/$tool"
+	then
+		return 1
+	fi
+
+	# Load the redefined functions
+	. "$mergetools/$tool"
+
+	if merge_mode && ! can_merge
+	then
+		echo "error: '$tool' can not be used to resolve merges" >&2
+		exit 1
+	elif diff_mode && ! can_diff
+	then
+		echo "error: '$tool' can only be used to resolve merges" >&2
+		exit 1
+	fi
+	return 0
 }
 
 get_merge_tool_cmd () {
 	# Prints the custom command for a merge tool
-	if test -n "$1"
-	then
-		merge_tool="$1"
-	else
-		merge_tool="$(get_merge_tool)"
-	fi
+	merge_tool="$1"
 	if diff_mode
 	then
 		echo "$(git config difftool.$merge_tool.cmd ||
@@ -92,6 +88,7 @@ get_merge_tool_cmd () {
 	fi
 }
 
+# Entry point for running tools
 run_merge_tool () {
 	# If GIT_PREFIX is empty then we cannot use it in tools
 	# that expect to be able to chdir() to its value.
@@ -102,288 +99,15 @@ run_merge_tool () {
 	base_present="$2"
 	status=0
 
-	case "$1" in
-	araxis)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" -wait -merge -3 -a1 \
-					"$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
-					>/dev/null 2>&1
-			else
-				"$merge_tool_path" -wait -2 \
-					"$LOCAL" "$REMOTE" "$MERGED" \
-					>/dev/null 2>&1
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-		fi
-		;;
-	bc3)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
-					-mergeoutput="$MERGED"
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-mergeoutput="$MERGED"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	diffuse)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" \
-					"$LOCAL" "$MERGED" "$REMOTE" \
-					"$BASE" | cat
-			else
-				"$merge_tool_path" \
-					"$LOCAL" "$MERGED" "$REMOTE" | cat
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		fi
-		;;
-	ecmerge)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
-					--default --mode=merge3 --to="$MERGED"
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					--default --mode=merge2 --to="$MERGED"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" --default --mode=diff2 \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	emerge)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" \
-					-f emerge-files-with-ancestor-command \
-					"$LOCAL" "$REMOTE" "$BASE" \
-					"$(basename "$MERGED")"
-			else
-				"$merge_tool_path" \
-					-f emerge-files-command \
-					"$LOCAL" "$REMOTE" \
-					"$(basename "$MERGED")"
-			fi
-			status=$?
-		else
-			"$merge_tool_path" -f emerge-files-command \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	gvimdiff|vimdiff)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" -f -d -c "wincmd J" \
-					"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
-			else
-				"$merge_tool_path" -f -d -c "wincmd l" \
-					"$LOCAL" "$MERGED" "$REMOTE"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" -R -f -d -c "wincmd l" \
-				-c 'cd $GIT_PREFIX' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	gvimdiff2|vimdiff2)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" -f -d -c "wincmd l" \
-				"$LOCAL" "$MERGED" "$REMOTE"
-			check_unchanged
-		else
-			"$merge_tool_path" -R -f -d -c "wincmd l" \
-				-c 'cd $GIT_PREFIX' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	kdiff3)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" --auto \
-					--L1 "$MERGED (Base)" \
-					--L2 "$MERGED (Local)" \
-					--L3 "$MERGED (Remote)" \
-					-o "$MERGED" \
-					"$BASE" "$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-			else
-				"$merge_tool_path" --auto \
-					--L1 "$MERGED (Local)" \
-					--L2 "$MERGED (Remote)" \
-					-o "$MERGED" \
-					"$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-			fi
-			status=$?
-		else
-			"$merge_tool_path" --auto \
-				--L1 "$MERGED (A)" \
-				--L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
-			>/dev/null 2>&1
-		fi
-		;;
-	kompare)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-	meld)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	opendiff)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-ancestor "$BASE" \
-					-merge "$MERGED" | cat
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-merge "$MERGED" | cat
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		fi
-		;;
-	p4merge)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			$base_present || >"$BASE"
-			"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	tkdiff)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" -a "$BASE" \
-					-o "$MERGED" "$LOCAL" "$REMOTE"
-			else
-				"$merge_tool_path" \
-					-o "$MERGED" "$LOCAL" "$REMOTE"
-			fi
-			status=$?
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	tortoisemerge)
-		if $base_present
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" \
-				-base:"$BASE" -mine:"$LOCAL" \
-				-theirs:"$REMOTE" -merged:"$MERGED"
-			check_unchanged
-		else
-			echo "TortoiseMerge cannot be used without a base" 1>&2
-			status=1
-		fi
-		;;
-	xxdiff)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" -X --show-merged-pane \
-					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-					-R 'Accel.Search: "Ctrl+F"' \
-					-R 'Accel.SearchForward: "Ctrl-G"' \
-					--merged-file "$MERGED" \
-					"$LOCAL" "$BASE" "$REMOTE"
-			else
-				"$merge_tool_path" -X $extra \
-					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-					-R 'Accel.Search: "Ctrl+F"' \
-					-R 'Accel.SearchForward: "Ctrl-G"' \
-					--merged-file "$MERGED" \
-					"$LOCAL" "$REMOTE"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" \
-				-R 'Accel.Search: "Ctrl+F"' \
-				-R 'Accel.SearchForward: "Ctrl-G"' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	*)
-		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-		if test -z "$merge_tool_cmd"
-		then
-			if merge_mode
-			then
-				status=1
-			fi
-			break
-		fi
-		if merge_mode
-		then
-			trust_exit_code="$(git config --bool \
-				mergetool."$1".trustExitCode || echo false)"
-			if test "$trust_exit_code" = "false"
-			then
-				touch "$BACKUP"
-				( eval $merge_tool_cmd )
-				check_unchanged
-			else
-				( eval $merge_tool_cmd )
-				status=$?
-			fi
-		else
-			( eval $merge_tool_cmd )
-		fi
-		;;
-	esac
+	# Bring tool-specific functions into scope
+	setup_tool "$1"
+
+	if merge_mode
+	then
+		merge_cmd "$1"
+	else
+		diff_cmd "$1"
+	fi
 	return $status
 }
 
@@ -449,12 +173,7 @@ get_configured_merge_tool () {
 
 get_merge_tool_path () {
 	# A merge tool has been set, so verify that it's valid.
-	if test -n "$1"
-	then
-		merge_tool="$1"
-	else
-		merge_tool="$(get_merge_tool)"
-	fi
+	merge_tool="$1"
 	if ! valid_tool "$merge_tool"
 	then
 		echo >&2 "Unknown merge tool $merge_tool"
@@ -483,7 +202,7 @@ get_merge_tool_path () {
 
 get_merge_tool () {
 	# Check if a merge tool has been configured
-	merge_tool=$(get_configured_merge_tool)
+	merge_tool="$(get_configured_merge_tool)"
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
diff --git a/mergetools/araxis b/mergetools/araxis
new file mode 100644
index 0000000..69a9c8c
--- /dev/null
+++ b/mergetools/araxis
@@ -0,0 +1,21 @@
+diff_cmd () {
+	"$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" -wait -merge -3 -a1 \
+			"$BASE" "$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+	else
+		"$merge_tool_path" -wait -2 \
+			"$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+	fi
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	echo compare
+}
+
diff --git a/mergetools/bc3 b/mergetools/bc3
new file mode 100644
index 0000000..27b3dd4
--- /dev/null
+++ b/mergetools/bc3
@@ -0,0 +1,20 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+			-mergeoutput="$MERGED"
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-mergeoutput="$MERGED"
+	fi
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	echo bcompare
+}
diff --git a/mergetools/defaults b/mergetools/defaults
new file mode 100644
index 0000000..1d8f2a3
--- /dev/null
+++ b/mergetools/defaults
@@ -0,0 +1,46 @@
+# Redefined by builtin tools
+can_merge () {
+	return 0
+}
+
+can_diff () {
+	return 0
+}
+
+diff_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -z "$merge_tool_cmd"
+	then
+		status=1
+		break
+	fi
+	( eval $merge_tool_cmd )
+	status=$?
+	return $status
+}
+
+merge_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -z "$merge_tool_cmd"
+	then
+		status=1
+		break
+	fi
+	trust_exit_code="$(git config --bool \
+		mergetool."$1".trustExitCode || echo false)"
+	if test "$trust_exit_code" = "false"
+	then
+		touch "$BACKUP"
+		( eval $merge_tool_cmd )
+		status=$?
+		check_unchanged
+	else
+		( eval $merge_tool_cmd )
+		status=$?
+	fi
+	return $status
+}
+
+translate_merge_tool_path () {
+	echo "$1"
+}
diff --git a/mergetools/diffuse b/mergetools/diffuse
new file mode 100644
index 0000000..02e0843
--- /dev/null
+++ b/mergetools/diffuse
@@ -0,0 +1,17 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" \
+			"$LOCAL" "$MERGED" "$REMOTE" \
+			"$BASE" | cat
+	else
+		"$merge_tool_path" \
+			"$LOCAL" "$MERGED" "$REMOTE" | cat
+	fi
+	check_unchanged
+}
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
new file mode 100644
index 0000000..13c2e43
--- /dev/null
+++ b/mergetools/ecmerge
@@ -0,0 +1,16 @@
+diff_cmd () {
+	"$merge_tool_path" --default --mode=diff2 "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
+			--default --mode=merge3 --to="$MERGED"
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			--default --mode=merge2 --to="$MERGED"
+	fi
+	check_unchanged
+}
diff --git a/mergetools/emerge b/mergetools/emerge
new file mode 100644
index 0000000..f96d9e5
--- /dev/null
+++ b/mergetools/emerge
@@ -0,0 +1,23 @@
+diff_cmd () {
+	"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" \
+			-f emerge-files-with-ancestor-command \
+			"$LOCAL" "$REMOTE" "$BASE" \
+			"$(basename "$MERGED")"
+	else
+		"$merge_tool_path" \
+			-f emerge-files-command \
+			"$LOCAL" "$REMOTE" \
+			"$(basename "$MERGED")"
+	fi
+	status=$?
+}
+
+translate_merge_tool_path() {
+	echo emacs
+}
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
new file mode 100644
index 0000000..28fead4
--- /dev/null
+++ b/mergetools/kdiff3
@@ -0,0 +1,24 @@
+diff_cmd () {
+	"$merge_tool_path" --auto \
+		--L1 "$MERGED (A)" --L2 "$MERGED (B)" \
+		"$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" --auto \
+			--L1 "$MERGED (Base)" \
+			--L2 "$MERGED (Local)" \
+			--L3 "$MERGED (Remote)" \
+			-o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" \
+		>/dev/null 2>&1
+	else
+		"$merge_tool_path" --auto \
+			--L1 "$MERGED (Local)" \
+			--L2 "$MERGED (Remote)" \
+			-o "$MERGED" "$LOCAL" "$REMOTE" \
+		>/dev/null 2>&1
+	fi
+	status=$?
+}
diff --git a/mergetools/kompare b/mergetools/kompare
new file mode 100644
index 0000000..433686c
--- /dev/null
+++ b/mergetools/kompare
@@ -0,0 +1,7 @@
+can_merge () {
+	return 1
+}
+
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
diff --git a/mergetools/meld b/mergetools/meld
new file mode 100644
index 0000000..73d70ae
--- /dev/null
+++ b/mergetools/meld
@@ -0,0 +1,9 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	check_unchanged
+}
diff --git a/mergetools/opendiff b/mergetools/opendiff
new file mode 100644
index 0000000..0942b2a
--- /dev/null
+++ b/mergetools/opendiff
@@ -0,0 +1,16 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-ancestor "$BASE" -merge "$MERGED" | cat
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-merge "$MERGED" | cat
+	fi
+	check_unchanged
+}
diff --git a/mergetools/p4merge b/mergetools/p4merge
new file mode 100644
index 0000000..1a45c1b
--- /dev/null
+++ b/mergetools/p4merge
@@ -0,0 +1,10 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	$base_present || >"$BASE"
+	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	check_unchanged
+}
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
new file mode 100644
index 0000000..618c438
--- /dev/null
+++ b/mergetools/tkdiff
@@ -0,0 +1,12 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+	else
+		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+	fi
+}
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
new file mode 100644
index 0000000..ed7db49
--- /dev/null
+++ b/mergetools/tortoisemerge
@@ -0,0 +1,17 @@
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		"$merge_tool_path" \
+			-base:"$BASE" -mine:"$LOCAL" \
+			-theirs:"$REMOTE" -merged:"$MERGED"
+		check_unchanged
+	else
+		echo "TortoiseMerge cannot be used without a base" 1>&2
+		return 1
+	fi
+}
diff --git a/mergetools/vim b/mergetools/vim
new file mode 100644
index 0000000..619594a
--- /dev/null
+++ b/mergetools/vim
@@ -0,0 +1,44 @@
+diff_cmd () {
+	case "$1" in
+	gvimdiff|vimdiff)
+		"$merge_tool_path" -R -f -d \
+			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
+		;;
+	gvimdiff2|vimdiff2)
+		"$merge_tool_path" -R -f -d \
+			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
+		;;
+	esac
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	case "$1" in
+	gvimdiff|vimdiff)
+		if $base_present
+		then
+			"$merge_tool_path" -f -d -c 'wincmd J' \
+				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+		else
+			"$merge_tool_path" -f -d -c 'wincmd l' \
+				"$LOCAL" "$MERGED" "$REMOTE"
+		fi
+		;;
+	gvimdiff2|vimdiff2)
+		"$merge_tool_path" -f -d -c 'wincmd l' \
+			"$LOCAL" "$MERGED" "$REMOTE"
+		;;
+	esac
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	case "$1" in
+	gvimdiff|gvimdiff2)
+		echo gvim
+		;;
+	vimdiff|vimdiff2)
+		echo vim
+		;;
+	esac
+}
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
new file mode 100644
index 0000000..05b4433
--- /dev/null
+++ b/mergetools/xxdiff
@@ -0,0 +1,25 @@
+diff_cmd () {
+	"$merge_tool_path" \
+		-R 'Accel.Search: "Ctrl+F"' \
+		-R 'Accel.SearchForward: "Ctrl-G"' \
+		"$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" -X --show-merged-pane \
+			-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+			-R 'Accel.Search: "Ctrl+F"' \
+			-R 'Accel.SearchForward: "Ctrl-G"' \
+			--merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+	else
+		"$merge_tool_path" -X $extra \
+			-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+			-R 'Accel.Search: "Ctrl+F"' \
+			-R 'Accel.SearchForward: "Ctrl-G"' \
+			--merged-file "$MERGED" "$LOCAL" "$REMOTE"
+	fi
+	check_unchanged
+}
-- 
1.7.6.476.g57292

^ permalink raw reply related

* [PATCH 4/4] mergetools/meld: Use '--output' when available
From: David Aguilar @ 2011-08-18  7:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <1313652227-48545-1-git-send-email-davvid@gmail.com>

meld 1.5.0 and newer allow the output file to be specified
when merging multiple files.  Check the meld version and use
the '--output' option when available.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 mergetools/meld |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/mergetools/meld b/mergetools/meld
index 73d70ae..1923797 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -4,6 +4,37 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	if test "$meld_has_output_option" = true
+	then
+		"$merge_tool_path" --output "$MERGED" \
+			"$BASE" "$LOCAL" "$REMOTE"
+	else
+		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	fi
 	check_unchanged
 }
+
+check_meld_version () {
+	if test -n "$meld_version_checked"
+	then
+		return
+	fi
+	# Whether 'meld --output <file>' is supported
+	meld_has_output_option=false
+
+	# Filter meld --version to contain numbers and dots only
+	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
+	meld="${meld:-0.0.0}"
+
+	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
+	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
+
+	# 'meld --output <file>' was introduced in meld 1.5.0
+	if test "$meld_major" -gt 1 ||
+		(test "$meld_major" = 1 && test "$meld_minor" -ge 5)
+	then
+		meld_has_output_option=true
+	fi
+	meld_version_checked=true
+}
+check_meld_version
-- 
1.7.6.476.g57292

^ permalink raw reply related

* [PATCH 2/4] mergetool--lib: Make style consistent with git
From: David Aguilar @ 2011-08-18  7:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <1313652227-48545-1-git-send-email-davvid@gmail.com>

Use the predominant conditional style where "then" appears
alone on the line after the test expression.
Remove spaces after ">" output redirections.
Remove unnecessary parentheses around the kdiff3 commands.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh |  153 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 100 insertions(+), 53 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 91f90ac..03cfb19 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -32,10 +32,12 @@ translate_merge_tool_path () {
 }
 
 check_unchanged () {
-	if test "$MERGED" -nt "$BACKUP"; then
+	if test "$MERGED" -nt "$BACKUP"
+	then
 		status=0
 	else
-		while true; do
+		while true
+		do
 			echo "$MERGED seems unchanged."
 			printf "Was the merge successful? [y/n] "
 			read answer
@@ -53,17 +55,20 @@ valid_tool () {
 	kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
 		;; # happy
 	kompare)
-		if ! diff_mode; then
+		if ! diff_mode
+		then
 			return 1
 		fi
 		;;
 	tortoisemerge)
-		if ! merge_mode; then
+		if ! merge_mode
+		then
 			return 1
 		fi
 		;;
 	*)
-		if test -z "$(get_merge_tool_cmd "$1")"; then
+		if test -z "$(get_merge_tool_cmd "$1")"
+		then
 			return 1
 		fi
 		;;
@@ -72,12 +77,14 @@ valid_tool () {
 
 get_merge_tool_cmd () {
 	# Prints the custom command for a merge tool
-	if test -n "$1"; then
+	if test -n "$1"
+	then
 		merge_tool="$1"
 	else
 		merge_tool="$(get_merge_tool)"
 	fi
-	if diff_mode; then
+	if diff_mode
+	then
 		echo "$(git config difftool.$merge_tool.cmd ||
 		        git config mergetool.$merge_tool.cmd)"
 	else
@@ -97,9 +104,11 @@ run_merge_tool () {
 
 	case "$1" in
 	araxis)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -wait -merge -3 -a1 \
 					"$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
 					>/dev/null 2>&1
@@ -115,9 +124,11 @@ run_merge_tool () {
 		fi
 		;;
 	bc3)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
 					-mergeoutput="$MERGED"
 			else
@@ -130,9 +141,11 @@ run_merge_tool () {
 		fi
 		;;
 	diffuse)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" \
 					"$LOCAL" "$MERGED" "$REMOTE" \
 					"$BASE" | cat
@@ -146,9 +159,11 @@ run_merge_tool () {
 		fi
 		;;
 	ecmerge)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
 					--default --mode=merge3 --to="$MERGED"
 			else
@@ -162,8 +177,10 @@ run_merge_tool () {
 		fi
 		;;
 	emerge)
-		if merge_mode; then
-			if $base_present; then
+		if merge_mode
+		then
+			if $base_present
+			then
 				"$merge_tool_path" \
 					-f emerge-files-with-ancestor-command \
 					"$LOCAL" "$REMOTE" "$BASE" \
@@ -181,9 +198,11 @@ run_merge_tool () {
 		fi
 		;;
 	gvimdiff|vimdiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -f -d -c "wincmd J" \
 					"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
 			else
@@ -198,7 +217,8 @@ run_merge_tool () {
 		fi
 		;;
 	gvimdiff2|vimdiff2)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" -f -d -c "wincmd l" \
 				"$LOCAL" "$MERGED" "$REMOTE"
@@ -210,36 +230,39 @@ run_merge_tool () {
 		fi
 		;;
 	kdiff3)
-		if merge_mode; then
-			if $base_present; then
-				("$merge_tool_path" --auto \
+		if merge_mode
+		then
+			if $base_present
+			then
+				"$merge_tool_path" --auto \
 					--L1 "$MERGED (Base)" \
 					--L2 "$MERGED (Local)" \
 					--L3 "$MERGED (Remote)" \
 					-o "$MERGED" \
 					"$BASE" "$LOCAL" "$REMOTE" \
-				> /dev/null 2>&1)
+				>/dev/null 2>&1
 			else
-				("$merge_tool_path" --auto \
+				"$merge_tool_path" --auto \
 					--L1 "$MERGED (Local)" \
 					--L2 "$MERGED (Remote)" \
 					-o "$MERGED" \
 					"$LOCAL" "$REMOTE" \
-				> /dev/null 2>&1)
+				>/dev/null 2>&1
 			fi
 			status=$?
 		else
-			("$merge_tool_path" --auto \
+			"$merge_tool_path" --auto \
 				--L1 "$MERGED (A)" \
 				--L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
-			> /dev/null 2>&1)
+			>/dev/null 2>&1
 		fi
 		;;
 	kompare)
 		"$merge_tool_path" "$LOCAL" "$REMOTE"
 		;;
 	meld)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
 			check_unchanged
@@ -248,9 +271,11 @@ run_merge_tool () {
 		fi
 		;;
 	opendiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$LOCAL" "$REMOTE" \
 					-ancestor "$BASE" \
 					-merge "$MERGED" | cat
@@ -264,7 +289,8 @@ run_merge_tool () {
 		fi
 		;;
 	p4merge)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			$base_present || >"$BASE"
 			"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
@@ -274,8 +300,10 @@ run_merge_tool () {
 		fi
 		;;
 	tkdiff)
-		if merge_mode; then
-			if $base_present; then
+		if merge_mode
+		then
+			if $base_present
+			then
 				"$merge_tool_path" -a "$BASE" \
 					-o "$MERGED" "$LOCAL" "$REMOTE"
 			else
@@ -288,7 +316,8 @@ run_merge_tool () {
 		fi
 		;;
 	tortoisemerge)
-		if $base_present; then
+		if $base_present
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" \
 				-base:"$BASE" -mine:"$LOCAL" \
@@ -300,9 +329,11 @@ run_merge_tool () {
 		fi
 		;;
 	xxdiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -X --show-merged-pane \
 					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
 					-R 'Accel.Search: "Ctrl+F"' \
@@ -327,16 +358,20 @@ run_merge_tool () {
 		;;
 	*)
 		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-		if test -z "$merge_tool_cmd"; then
-			if merge_mode; then
+		if test -z "$merge_tool_cmd"
+		then
+			if merge_mode
+			then
 				status=1
 			fi
 			break
 		fi
-		if merge_mode; then
+		if merge_mode
+		then
 			trust_exit_code="$(git config --bool \
 				mergetool."$1".trustExitCode || echo false)"
-			if test "$trust_exit_code" = "false"; then
+			if test "$trust_exit_code" = "false"
+			then
 				touch "$BACKUP"
 				( eval $merge_tool_cmd )
 				check_unchanged
@@ -353,13 +388,16 @@ run_merge_tool () {
 }
 
 guess_merge_tool () {
-	if merge_mode; then
+	if merge_mode
+	then
 		tools="tortoisemerge"
 	else
 		tools="kompare"
 	fi
-	if test -n "$DISPLAY"; then
-		if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
+	if test -n "$DISPLAY"
+	then
+		if test -n "$GNOME_DESKTOP_SESSION_ID"
+		then
 			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
 		else
 			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
@@ -380,7 +418,8 @@ guess_merge_tool () {
 	for i in $tools
 	do
 		merge_tool_path="$(translate_merge_tool_path "$i")"
-		if type "$merge_tool_path" > /dev/null 2>&1; then
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
 			echo "$i"
 			return 0
 		fi
@@ -393,12 +432,14 @@ guess_merge_tool () {
 get_configured_merge_tool () {
 	# Diff mode first tries diff.tool and falls back to merge.tool.
 	# Merge mode only checks merge.tool
-	if diff_mode; then
+	if diff_mode
+	then
 		merge_tool=$(git config diff.tool || git config merge.tool)
 	else
 		merge_tool=$(git config merge.tool)
 	fi
-	if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
+	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
+	then
 		echo >&2 "git config option $TOOL_MODE.tool set to unknown tool: $merge_tool"
 		echo >&2 "Resetting to default..."
 		return 1
@@ -408,26 +449,31 @@ get_configured_merge_tool () {
 
 get_merge_tool_path () {
 	# A merge tool has been set, so verify that it's valid.
-	if test -n "$1"; then
+	if test -n "$1"
+	then
 		merge_tool="$1"
 	else
 		merge_tool="$(get_merge_tool)"
 	fi
-	if ! valid_tool "$merge_tool"; then
+	if ! valid_tool "$merge_tool"
+	then
 		echo >&2 "Unknown merge tool $merge_tool"
 		exit 1
 	fi
-	if diff_mode; then
+	if diff_mode
+	then
 		merge_tool_path=$(git config difftool."$merge_tool".path ||
 		                  git config mergetool."$merge_tool".path)
 	else
 		merge_tool_path=$(git config mergetool."$merge_tool".path)
 	fi
-	if test -z "$merge_tool_path"; then
+	if test -z "$merge_tool_path"
+	then
 		merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
 	fi
 	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
-	! type "$merge_tool_path" > /dev/null 2>&1; then
+		! type "$merge_tool_path" >/dev/null 2>&1
+	then
 		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
 		         "'$merge_tool_path'"
 		exit 1
@@ -439,7 +485,8 @@ get_merge_tool () {
 	# Check if a merge tool has been configured
 	merge_tool=$(get_configured_merge_tool)
 	# Try to guess an appropriate merge tool if no tool has been set.
-	if test -z "$merge_tool"; then
+	if test -z "$merge_tool"
+	then
 		merge_tool="$(guess_merge_tool)" || exit
 	fi
 	echo "$merge_tool"
-- 
1.7.6.476.g57292

^ permalink raw reply related

* [PATCH 1/4] difftool--helper: Make style consistent with git
From: David Aguilar @ 2011-08-18  7:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth
In-Reply-To: <1313652227-48545-1-git-send-email-davvid@gmail.com>

Use the predominant conditional style where "then" appears
alone on the line after the test expression.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool--helper.sh |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 0594bf7..8452890 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -13,7 +13,8 @@ TOOL_MODE=diff
 should_prompt () {
 	prompt_merge=$(git config --bool mergetool.prompt || echo true)
 	prompt=$(git config --bool difftool.prompt || echo $prompt_merge)
-	if test "$prompt" = true; then
+	if test "$prompt" = true
+	then
 		test -z "$GIT_DIFFTOOL_NO_PROMPT"
 	else
 		test -n "$GIT_DIFFTOOL_PROMPT"
@@ -37,9 +38,11 @@ launch_merge_tool () {
 
 	# $LOCAL and $REMOTE are temporary files so prompt
 	# the user with the real $MERGED name before launching $merge_tool.
-	if should_prompt; then
+	if should_prompt
+	then
 		printf "\nViewing: '$MERGED'\n"
-		if use_ext_cmd; then
+		if use_ext_cmd
+		then
 			printf "Hit return to launch '%s': " \
 				"$GIT_DIFFTOOL_EXTCMD"
 		else
@@ -48,7 +51,8 @@ launch_merge_tool () {
 		read ans
 	fi
 
-	if use_ext_cmd; then
+	if use_ext_cmd
+	then
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
@@ -56,8 +60,10 @@ launch_merge_tool () {
 	fi
 }
 
-if ! use_ext_cmd; then
-	if test -n "$GIT_DIFF_TOOL"; then
+if ! use_ext_cmd
+then
+	if test -n "$GIT_DIFF_TOOL"
+	then
 		merge_tool="$GIT_DIFF_TOOL"
 	else
 		merge_tool="$(get_merge_tool)" || exit
-- 
1.7.6.476.g57292

^ permalink raw reply related

* [PATCH 0/4] Refactor git-mergetool--lib.sh
From: David Aguilar @ 2011-08-18  7:23 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
	Sebastian Schuberth

This series splits the git-mergetool--lib.sh file into separate
tool scriptlets that define the commands for diff and merge.

David Aguilar (4):
  difftool--helper: Make style consistent with git
  mergetool--lib: Make style consistent with git
  mergetool--lib: Refactor tools into separate files
  mergetools/meld: Use '--output' when available

 Makefile                 |   11 ++
 git-difftool--helper.sh  |   18 ++-
 git-mergetool--lib.sh    |  394 ++++++++++------------------------------------
 mergetools/araxis        |   21 +++
 mergetools/bc3           |   20 +++
 mergetools/defaults      |   46 ++++++
 mergetools/diffuse       |   17 ++
 mergetools/ecmerge       |   16 ++
 mergetools/emerge        |   23 +++
 mergetools/kdiff3        |   24 +++
 mergetools/kompare       |    7 +
 mergetools/meld          |   40 +++++
 mergetools/opendiff      |   16 ++
 mergetools/p4merge       |   10 ++
 mergetools/tkdiff        |   12 ++
 mergetools/tortoisemerge |   17 ++
 mergetools/vim           |   44 +++++
 mergetools/xxdiff        |   25 +++
 18 files changed, 441 insertions(+), 320 deletions(-)
 create mode 100644 mergetools/araxis
 create mode 100644 mergetools/bc3
 create mode 100644 mergetools/defaults
 create mode 100644 mergetools/diffuse
 create mode 100644 mergetools/ecmerge
 create mode 100644 mergetools/emerge
 create mode 100644 mergetools/kdiff3
 create mode 100644 mergetools/kompare
 create mode 100644 mergetools/meld
 create mode 100644 mergetools/opendiff
 create mode 100644 mergetools/p4merge
 create mode 100644 mergetools/tkdiff
 create mode 100644 mergetools/tortoisemerge
 create mode 100644 mergetools/vim
 create mode 100644 mergetools/xxdiff

$ git describe --dirty
v1.7.6-476-g572925c
$ git version
git version 1.7.6.476.g57292
$ ./t7610-mergetool.sh 
ok 1 - setup
ok 2 - custom mergetool
ok 3 - mergetool crlf
ok 4 - mergetool in subdir
ok 5 - mergetool on file in parent dir
ok 6 - mergetool skips autoresolved
ok 7 - mergetool merges all from subdir
ok 8 - mergetool skips resolved paths when rerere is active
ok 9 - deleted vs modified submodule
ok 10 - file vs modified submodule
ok 11 - submodule in subdirectory
ok 12 - directory vs modified submodule
# passed all 12 test(s)
1..12
$ ./t7800-difftool.sh 
ok 1 - setup
ok 2 - custom commands
ok 3 - difftool ignores bad --tool values
ok 4 - difftool honors --gui
ok 5 - difftool --gui works without configured diff.guitool
ok 6 - GIT_DIFF_TOOL variable
ok 7 - GIT_DIFF_TOOL overrides
ok 8 - GIT_DIFFTOOL_NO_PROMPT variable
ok 9 - GIT_DIFFTOOL_PROMPT variable
ok 10 - difftool.prompt config variable is false
ok 11 - difftool merge.prompt = false
ok 12 - difftool.prompt can overridden with -y
ok 13 - difftool.prompt can overridden with --prompt
ok 14 - difftool last flag wins
ok 15 - difftool + mergetool config variables
ok 16 - difftool.<tool>.path
ok 17 - difftool --extcmd=cat
ok 18 - difftool --extcmd cat
ok 19 - difftool -x cat
ok 20 - difftool --extcmd echo arg1
ok 21 - difftool --extcmd cat arg1
ok 22 - difftool --extcmd cat arg2
# passed all 22 test(s)
1..22

-- 
1.7.6.476.g57292

^ permalink raw reply

* Re: [PATCH] rev-parse: Allow @{U} as a synonym for @{u}
From: Michael J Gruber @ 2011-08-18  7:15 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git, Conrad Irwin, Sitaram Chamarty
In-Reply-To: <CACsJy8CX7X3u4i_kXChVHkFK=Q--pRBrxmdvjxrF7wr5_SyakA@mail.gmail.com>

Nguyen Thai Ngoc Duy venit, vidit, dixit 18.08.2011 03:53:
> On Thu, Aug 18, 2011 at 5:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Letting u/upstream spelled case-insensitively does improve consistency
>> among the above, but at the same time if we ever wanted to enhance @{...}
>> notation even further in the future, we are restricted to a payload that
>> is case insensitive to retain the consistency.
>>
>> The only remotely semi-plausible enhancement I could think of is perhaps
>> to allow @{/regexp} to find a reflog entry that matches the given pattern,
>> and in such a use case we would certainly want to take the pattern in a
>> case sensitive way. This change closes the door to that, and that is the
>> only downside I can think of right now.
>>
>> I hate to declare that we will never support such a feature with this
>> change, but at the same time, I do not think I would need such a feature
>> that often. But for that matter, I do not think I would miss @{UpStREAM},
>> either, and that takes me back to "Meh" for this change.
>>
>> What do people think?
> 
> The itch is probably because people have to release shift, then press
> shift again to type "@{u}". How about allow case insensitive match if
> there's only _one_ letter inside @{}?
> 
> That would solve the shift problem while leaving door for @{...} extension.

Well, this patch is actually specific to a particular keyboard layout!
It improves things only for a layout where you get @ and { using a shift
key.

For a german keyboard layout, you get @{u} from

hold AltGr (Meta)
press q
press 7
release AltGr
press u
hold AltGr
press 0

For me, an improvement would be to have the alias

@{↓}

for @{u} so that I can hold AltGr throughout and press q7u0. Which would
be very nice, pictorially :)

In general git could benefit from a rev alias mechanism. This idea has
come up several times, but in order to be really useful, we would need
that to be more general than textual replacements and allow for macro
like replacements such as

git config revalias.new "\1@{1}..\1"

to be used as, say

git log @{new(origin/next)}

after a fetch. (No, I'm not suggesting that specific syntax here.)

As a simpler case, a user could tailor to her keyboard layout with

git config revalias.↓ u

but for obvious reasons

git config revalias.U u

won't work.

So, maybe we could make the key part of a special revspec case
insensitive (just like config variable names) but still have a possible
payload (parameter) be case sensitive (just like config variable values)?

Michael

^ permalink raw reply

* (unknown)
From: milki @ 2011-08-18  6:36 UTC (permalink / raw)
  To: git

subscribe git

^ permalink raw reply

* git-config: case insensitivity for subsections
From: milki @ 2011-08-18  6:35 UTC (permalink / raw)
  To: git

In git-config(1):

There is also a case insensitive alternative [section.subsection]
syntax. In this syntax, subsection names follow the same restrictions
as for section names.

If I define [section.SUBSECTION] (aka, not all lowercase), I cannot
use: git config section.SUBSECTION.option, but rather only git config
section.subsection.option. Furthermore, If I also define a [section
"SUBSECTION"], the two sections are not merged.

I believe this differs from the case insensitity that is used for
sections: [section] and [SECTION] would be considered the same section.

Is this the proper behaviour for the case insensititve alternative for
subsections?

Thanks.

-- 
milki

^ permalink raw reply

* Re: Branches & directories
From: Michael Witten @ 2011-08-18  5:52 UTC (permalink / raw)
  To: Hilco Wijbenga; +Cc: Junio C Hamano, Evan Shelhamer, Git Mailing List
In-Reply-To: <CAE1pOi3rqqcz_6QxB8=g2jWOF-4SRZee7t8NXN1md2C4DL7wug@mail.gmail.com>

On Wed, Aug 17, 2011 at 21:23, Hilco Wijbenga <hilco.wijbenga@gmail.com> wrote:
> It would be really nice, though, if Git could somehow
> "stash" such files when checking out a different branch. In general, I
> would prefer if uncommitted changes and untracked and/or ignored files
> stuck to the branch where they were created.

As an aside, the problem here is likely a manifestation of the fact
that nobody understands what a branch is; the word 'branch' is
TERRIBLE, as everyone has a different idea for what that should mean.
In my opinion, `git branch' should become `git ref' or the like.

One of git's worst faults is that a complicated and imprecise
interface has been draped over a very simple and precise underlying
structure.

^ permalink raw reply

* [PATCH 10/10] want_color: automatically fallback to color.ui
From: Jeff King @ 2011-08-18  5:05 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>

All of the "do we want color" flags default to -1 to
indicate that we don't have any color configured. This value
is handled in one of two ways:

  1. In porcelain, we check early on whether the value is
     still -1 after reading the config, and set it to the
     value of color.ui (which defaults to 0).

  2. In plumbing, it stays untouched as -1, and want_color
     defaults it to off.

This works fine, but means that every porcelain has to check
and reassign its color flag. Now that want_color gives us a
place to put this check in a single spot, we can do that,
simplifying the calling code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    3 ---
 builtin/commit.c      |   11 +----------
 builtin/diff.c        |    3 ---
 builtin/grep.c        |    2 --
 builtin/log.c         |   12 ------------
 builtin/merge.c       |    4 ----
 builtin/show-branch.c |    3 ---
 color.c               |    7 +++++--
 color.h               |    5 -----
 9 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index d6d3c7d..73d4170 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -673,9 +673,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	git_config(git_branch_config, NULL);
 
-	if (branch_use_color == -1)
-		branch_use_color = git_use_color_default;
-
 	track = git_branch_track;
 
 	head = resolve_ref("HEAD", head_sha1, 0, NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 295803a..9763146 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1237,10 +1237,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	if (s.relative_paths)
 		s.prefix = prefix;
-	if (s.use_color == -1)
-		s.use_color = git_use_color_default;
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
 
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
@@ -1394,15 +1390,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	git_config(git_commit_config, &s);
 	determine_whence(&s);
 
-	if (s.use_color == -1)
-		s.use_color = git_use_color_default;
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage,
 					  prefix, &s);
-	if (dry_run) {
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
+	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, &s);
-	}
 	index_file = prepare_index(argc, argv, prefix, 0);
 
 	/* Set up everything for writing the commit object.  This includes
diff --git a/builtin/diff.c b/builtin/diff.c
index 69cd5ee..1118689 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -277,9 +277,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_diff_ui_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 
 	/* If this is a no-index diff, just run it and exit there. */
diff --git a/builtin/grep.c b/builtin/grep.c
index d80db22..2cbf01f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -896,8 +896,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	strcpy(opt.color_sep, GIT_COLOR_CYAN);
 	opt.color = -1;
 	git_config(grep_config, &opt);
-	if (opt.color == -1)
-		opt.color = git_use_color_default;
 
 	/*
 	 * If there is no -- then the paths must exist in the working
diff --git a/builtin/log.c b/builtin/log.c
index 5c2af59..d760ee0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -359,9 +359,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -446,9 +443,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_pathspec(&match_all, NULL);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
@@ -524,9 +518,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.verbose_header = 1;
@@ -549,9 +540,6 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/merge.c b/builtin/merge.c
index 7209edf..b75ae01 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1031,10 +1031,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	git_config(git_merge_config, NULL);
 
-	/* for color.ui */
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	if (branch_mergeoptions)
 		parse_branch_merge_options(branch_mergeoptions);
 	argc = parse_options(argc, argv, prefix, builtin_merge_options,
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b726fa..4b480d7 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -685,9 +685,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	git_config(git_show_branch_config, NULL);
 
-	if (showbranch_use_color == -1)
-		showbranch_use_color = git_use_color_default;
-
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_num) {
 		ac = default_num;
diff --git a/color.c b/color.c
index ec96fe1..e8e2681 100644
--- a/color.c
+++ b/color.c
@@ -1,7 +1,7 @@
 #include "cache.h"
 #include "color.h"
 
-int git_use_color_default = 0;
+static int git_use_color_default = 0;
 int color_stdout_is_tty = -1;
 
 /*
@@ -196,12 +196,15 @@ int want_color(int var)
 {
 	static int want_auto = -1;
 
+	if (var < 0)
+		var = git_use_color_default;
+
 	if (var == GIT_COLOR_AUTO) {
 		if (want_auto < 0)
 			want_auto = check_auto_color();
 		return want_auto;
 	}
-	return var > 0;
+	return var;
 }
 
 int git_color_config(const char *var, const char *value, void *cb)
diff --git a/color.h b/color.h
index 5949bcd..3068a99 100644
--- a/color.h
+++ b/color.h
@@ -58,11 +58,6 @@ struct strbuf;
 #define GIT_COLOR_NEVER  1
 #define GIT_COLOR_AUTO   2
 
-/*
- * This variable stores the value of color.ui
- */
-extern int git_use_color_default;
-
 /* A default list of colors to use for commit graphs and show-branch output */
 extern const char *column_colors_ansi[];
 extern const int column_colors_ansi_max;
-- 
1.7.6.10.g62f04

^ permalink raw reply related

* [PATCH 09/10] diff: don't load color config in plumbing
From: Jeff King @ 2011-08-18  5:05 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>

The diff config callback is split into two functions: one
which loads "ui" config, and one which loads "basic" config.
The former chains to the latter, as the diff UI config is a
superset of the plumbing config.

The color.diff variable is only loaded in the UI config.
However, the basic config actually chains to
git_color_default_config, which loads color.ui. This doesn't
actually cause any bugs, because the plumbing diff code does
not actually look at the value of color.ui.

However, it is somewhat nonsensical, and it makes it
difficult to refactor the color code. It probably came about
because there is no git_color_config to load only color
config, but rather just git_color_default_config, which
loads color config and chains to git_default_config.

This patch splits out the color-specific portion of
git_color_default_config so that the diff UI config can call
it directly. This is perhaps better explained by the
chaining of callbacks. Before we had:

  git_diff_ui_config
    -> git_diff_basic_config
      -> git_color_default_config
        -> git_default_config

Now we have:

  git_diff_ui_config
    -> git_color_config
    -> git_diff_basic_config
      -> git_default_config

Signed-off-by: Jeff King <peff@peff.net>
---
 color.c |   10 +++++++++-
 color.h |    4 +++-
 diff.c  |    5 ++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/color.c b/color.c
index 8586417..ec96fe1 100644
--- a/color.c
+++ b/color.c
@@ -204,13 +204,21 @@ int want_color(int var)
 	return var > 0;
 }
 
-int git_color_default_config(const char *var, const char *value, void *cb)
+int git_color_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
 		git_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 
+	return 0;
+}
+
+int git_color_default_config(const char *var, const char *value, void *cb)
+{
+	if (git_color_config(var, value, cb) < 0)
+		return -1;
+
 	return git_default_config(var, value, cb);
 }
 
diff --git a/color.h b/color.h
index d715fd5..5949bcd 100644
--- a/color.h
+++ b/color.h
@@ -74,8 +74,10 @@ extern const int column_colors_ansi_max;
 extern int color_stdout_is_tty;
 
 /*
- * Use this instead of git_default_config if you need the value of color.ui.
+ * Use the first one if you need only color config; the second is a convenience
+ * if you are just going to change to git_default_config, too.
  */
+int git_color_config(const char *var, const char *value, void *cb);
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value);
diff --git a/diff.c b/diff.c
index 29cecf1..0a22320 100644
--- a/diff.c
+++ b/diff.c
@@ -164,6 +164,9 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
 
+	if (git_color_config(var, value, cb) < 0)
+		return -1;
+
 	return git_diff_basic_config(var, value, cb);
 }
 
@@ -212,7 +215,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
-	return git_color_default_config(var, value, cb);
+	return git_default_config(var, value, cb);
 }
 
 static char *quote_two(const char *one, const char *two)
-- 
1.7.6.10.g62f04

^ permalink raw reply related

* [PATCH 08/10] config: refactor get_colorbool function
From: Jeff King @ 2011-08-18  5:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>

For "git config --get-colorbool color.foo", we use a custom
callback that looks not only for the key that the user gave
us, but also for "diff.color" (for backwards compatibility)
and "color.ui" (as a fallback).

For the former, we use a custom variable to store the
diff.color value. For the latter, though, we store it in the
main "git_use_color_default" variable, turning on color.ui
for any other parts of git that respect this value.

In practice, this doesn't cause any bugs, because git-config
runs without caring about git_use_color_default, and then
exits. But it crosses module boundaries in an unusual and
confusing way, and it makes refactoring color handling
harder than it needs to be.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 3a09296..0b4ecac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -305,6 +305,7 @@ static void get_color(const char *def_color)
 
 static int get_colorbool_found;
 static int get_diff_color_found;
+static int get_color_ui_found;
 static int git_get_colorbool_config(const char *var, const char *value,
 		void *cb)
 {
@@ -313,7 +314,7 @@ static int git_get_colorbool_config(const char *var, const char *value,
 	else if (!strcmp(var, "diff.color"))
 		get_diff_color_found = git_config_colorbool(var, value);
 	else if (!strcmp(var, "color.ui"))
-		git_use_color_default = git_config_colorbool(var, value);
+		get_color_ui_found = git_config_colorbool(var, value);
 	return 0;
 }
 
@@ -327,7 +328,7 @@ static int get_colorbool(int print)
 		if (!strcmp(get_colorbool_slot, "color.diff"))
 			get_colorbool_found = get_diff_color_found;
 		if (get_colorbool_found < 0)
-			get_colorbool_found = git_use_color_default;
+			get_colorbool_found = get_color_ui_found;
 	}
 
 	get_colorbool_found = want_color(get_colorbool_found);
-- 
1.7.6.10.g62f04

^ permalink raw reply related

* [PATCH 07/10] color: delay auto-color decision until point of use
From: Jeff King @ 2011-08-18  5:04 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>

When we read a color value either from a config file or from
the command line, we use git_config_colorbool to convert it
from the tristate always/never/auto into a single yes/no
boolean value.

This has some timing implications with respect to starting
a pager.

If we start (or decide not to start) the pager before
checking the colorbool, everything is fine. Either isatty(1)
will give us the right information, or we will properly
check for pager_in_use().

However, if we decide to start a pager after we have checked
the colorbool, things are not so simple. If stdout is a tty,
then we will have already decided to use color. However, the
user may also have configured color.pager not to use color
with the pager. In this case, we need to actually turn off
color. Unfortunately, the pager code has no idea which color
variables were turned on (and there are many of them
throughout the code, and they may even have been manipulated
after the colorbool selection by something like "--color" on
the command line).

This bug can be seen any time a pager is started after
config and command line options are checked. This has
affected "git diff" since 89d07f7 (diff: don't run pager if
user asked for a diff style exit code, 2007-08-12). It has
also affect the log family since 1fda91b (Fix 'git log'
early pager startup error case, 2010-08-24).

This patch splits the notion of parsing a colorbool and
actually checking the configuration. The "use_color"
variables now have an additional possible value,
GIT_COLOR_AUTO. Users of the variable should use the new
"want_color()" wrapper, which will lazily determine and
cache the auto-color decision.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    2 +-
 builtin/config.c      |    2 ++
 builtin/show-branch.c |    4 ++--
 color.c               |   20 ++++++++++++++++++--
 color.h               |   11 +++++++++++
 diff.c                |   17 +++++++----------
 graph.c               |    2 +-
 grep.c                |    2 +-
 log-tree.c            |    2 +-
 t/t7006-pager.sh      |   12 ++++++++++++
 wt-status.c           |    4 +++-
 11 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b15fee5..d6d3c7d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -88,7 +88,7 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 
 static const char *branch_get_color(enum color_branch ix)
 {
-	if (branch_use_color > 0)
+	if (want_color(branch_use_color))
 		return branch_colors[ix];
 	return "";
 }
diff --git a/builtin/config.c b/builtin/config.c
index 5505ced..3a09296 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -330,6 +330,8 @@ static int get_colorbool(int print)
 			get_colorbool_found = git_use_color_default;
 	}
 
+	get_colorbool_found = want_color(get_colorbool_found);
+
 	if (print) {
 		printf("%s\n", get_colorbool_found ? "true" : "false");
 		return 0;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e6650b4..4b726fa 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -26,14 +26,14 @@ static const char **default_arg;
 
 static const char *get_color_code(int idx)
 {
-	if (showbranch_use_color)
+	if (want_color(showbranch_use_color))
 		return column_colors_ansi[idx % column_colors_ansi_max];
 	return "";
 }
 
 static const char *get_color_reset_code(void)
 {
-	if (showbranch_use_color)
+	if (want_color(showbranch_use_color))
 		return GIT_COLOR_RESET;
 	return "";
 }
diff --git a/color.c b/color.c
index 67affa4..8586417 100644
--- a/color.c
+++ b/color.c
@@ -166,7 +166,7 @@ int git_config_colorbool(const char *var, const char *value)
 		if (!strcasecmp(value, "always"))
 			return 1;
 		if (!strcasecmp(value, "auto"))
-			goto auto_color;
+			return GIT_COLOR_AUTO;
 	}
 
 	if (!var)
@@ -177,7 +177,11 @@ int git_config_colorbool(const char *var, const char *value)
 		return 0;
 
 	/* any normal truth value defaults to 'auto' */
- auto_color:
+	return GIT_COLOR_AUTO;
+}
+
+static int check_auto_color(void)
+{
 	if (color_stdout_is_tty < 0)
 		color_stdout_is_tty = isatty(1);
 	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
@@ -188,6 +192,18 @@ int git_config_colorbool(const char *var, const char *value)
 	return 0;
 }
 
+int want_color(int var)
+{
+	static int want_auto = -1;
+
+	if (var == GIT_COLOR_AUTO) {
+		if (want_auto < 0)
+			want_auto = check_auto_color();
+		return want_auto;
+	}
+	return var > 0;
+}
+
 int git_color_default_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
diff --git a/color.h b/color.h
index a190a25..d715fd5 100644
--- a/color.h
+++ b/color.h
@@ -49,6 +49,16 @@ struct strbuf;
 #define GIT_COLOR_NIL "NIL"
 
 /*
+ * The first three are chosen to match common usage in the code, and what is
+ * returned from git_config_colorbool. The "auto" value can be returned from
+ * config_colorbool, and will be converted by want_color() into either 0 or 1.
+ */
+#define GIT_COLOR_UNKNOWN -1
+#define GIT_COLOR_ALWAYS 0
+#define GIT_COLOR_NEVER  1
+#define GIT_COLOR_AUTO   2
+
+/*
  * This variable stores the value of color.ui
  */
 extern int git_use_color_default;
@@ -69,6 +79,7 @@ extern int color_stdout_is_tty;
 int git_color_default_config(const char *var, const char *value, void *cb);
 
 int git_config_colorbool(const char *var, const char *value);
+int want_color(int var);
 void color_parse(const char *value, const char *var, char *dst);
 void color_parse_mem(const char *value, int len, const char *var, char *dst);
 __attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index 2dfc359..29cecf1 100644
--- a/diff.c
+++ b/diff.c
@@ -622,7 +622,7 @@ static void emit_rewrite_diff(const char *name_a,
 	size_two = fill_textconv(textconv_two, two, &data_two);
 
 	memset(&ecbdata, 0, sizeof(ecbdata));
-	ecbdata.color_diff = o->use_color > 0;
+	ecbdata.color_diff = want_color(o->use_color);
 	ecbdata.found_changesp = &o->found_changes;
 	ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 	ecbdata.opt = o;
@@ -1003,7 +1003,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
 
 const char *diff_get_color(int diff_use_color, enum color_diff ix)
 {
-	if (diff_use_color > 0)
+	if (want_color(diff_use_color))
 		return diff_colors[ix];
 	return "";
 }
@@ -2155,7 +2155,7 @@ static void builtin_diff(const char *name_a,
 		memset(&xecfg, 0, sizeof(xecfg));
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
-		ecbdata.color_diff = o->use_color > 0;
+		ecbdata.color_diff = want_color(o->use_color);
 		ecbdata.found_changesp = &o->found_changes;
 		ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
 		if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
@@ -2203,7 +2203,7 @@ static void builtin_diff(const char *name_a,
 					break;
 				}
 			}
-			if (o->use_color > 0) {
+			if (want_color(o->use_color)) {
 				struct diff_words_style *st = ecbdata.diff_words->style;
 				st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
 				st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
@@ -2853,7 +2853,7 @@ static void run_diff_cmd(const char *pgm,
 		 */
 		fill_metainfo(msg, name, other, one, two, o, p,
 			      &must_show_header,
-			      o->use_color > 0 && !pgm);
+			      want_color(o->use_color) && !pgm);
 		xfrm_msg = msg->len ? msg->buf : NULL;
 	}
 
@@ -3410,12 +3410,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->use_color = 1;
 	else if (!prefixcmp(arg, "--color=")) {
 		int value = git_config_colorbool(NULL, arg+8);
-		if (value == 0)
-			options->use_color = 0;
-		else if (value > 0)
-			options->use_color = 1;
-		else
+		if (value < 0)
 			return error("option `color' expects \"always\", \"auto\", or \"never\"");
+		options->use_color = value;
 	}
 	else if (!strcmp(arg, "--no-color"))
 		options->use_color = 0;
diff --git a/graph.c b/graph.c
index 556834a..7358416 100644
--- a/graph.c
+++ b/graph.c
@@ -347,7 +347,7 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
 
 static unsigned short graph_get_current_column_color(const struct git_graph *graph)
 {
-	if (graph->revs->diffopt.use_color <= 0)
+	if (!want_color(graph->revs->diffopt.use_color))
 		return column_colors_max;
 	return graph->default_column_color;
 }
diff --git a/grep.c b/grep.c
index 04e9ba4..abf4288 100644
--- a/grep.c
+++ b/grep.c
@@ -430,7 +430,7 @@ static int word_char(char ch)
 static void output_color(struct grep_opt *opt, const void *data, size_t size,
 			 const char *color)
 {
-	if (opt->color && color && color[0]) {
+	if (want_color(opt->color) && color && color[0]) {
 		opt->output(opt, color, strlen(color));
 		opt->output(opt, data, size);
 		opt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));
diff --git a/log-tree.c b/log-tree.c
index 9ba8fb2..95d6d40 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -31,7 +31,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
 
 static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
 {
-	if (decorate_use_color > 0)
+	if (want_color(decorate_use_color))
 		return decoration_colors[ix];
 	return "";
 }
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4884e1b..4582336 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -167,6 +167,18 @@ test_expect_success TTY 'color when writing to a pager' '
 	colorful paginated.out
 '
 
+test_expect_success TTY 'colors are suppressed by color.pager' '
+	rm -f paginated.out &&
+	test_config color.ui auto &&
+	test_config color.pager false &&
+	(
+		TERM=vt100 &&
+		export TERM &&
+		test_terminal git log
+	) &&
+	! colorful paginated.out
+'
+
 test_expect_success 'color when writing to a file intended for a pager' '
 	rm -f colorful.log &&
 	test_config color.ui auto ||
diff --git a/wt-status.c b/wt-status.c
index ee03431..8836a52 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -26,7 +26,9 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 
 static const char *color(int slot, struct wt_status *s)
 {
-	const char *c = s->use_color > 0 ? s->color_palette[slot] : "";
+	const char *c = "";
+	if (want_color(s->use_color))
+		c = s->color_palette[slot];
 	if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
 		c = s->color_palette[WT_STATUS_HEADER];
 	return c;
-- 
1.7.6.10.g62f04

^ permalink raw reply related

* [PATCH 06/10] git_config_colorbool: refactor stdout_is_tty handling
From: Jeff King @ 2011-08-18  5:03 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>

Usually this function figures out for itself whether stdout
is a tty. However, it has an extra parameter just to allow
git-config to override the auto-detection for its
--get-colorbool option.

Instead of an extra parameter, let's just use a global
variable. This makes calling easier in the common case, and
will make refactoring the colorbool code much simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c      |    2 +-
 builtin/commit.c      |    2 +-
 builtin/config.c      |   23 +++++++----------------
 builtin/grep.c        |    2 +-
 builtin/show-branch.c |    2 +-
 color.c               |   11 ++++++-----
 color.h               |    8 +++++++-
 diff.c                |    4 ++--
 parse-options.c       |    2 +-
 9 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3142daa..b15fee5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -71,7 +71,7 @@ static int parse_branch_color_slot(const char *var, int ofs)
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.branch")) {
-		branch_use_color = git_config_colorbool(var, value, -1);
+		branch_use_color = git_config_colorbool(var, value);
 		return 0;
 	}
 	if (!prefixcmp(var, "color.branch.")) {
diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..295803a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1144,7 +1144,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
-		s->use_color = git_config_colorbool(k, v, -1);
+		s->use_color = git_config_colorbool(k, v);
 		return 0;
 	}
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
diff --git a/builtin/config.c b/builtin/config.c
index 211e118..5505ced 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -303,24 +303,17 @@ static void get_color(const char *def_color)
 	fputs(parsed_color, stdout);
 }
 
-static int stdout_is_tty;
 static int get_colorbool_found;
 static int get_diff_color_found;
 static int git_get_colorbool_config(const char *var, const char *value,
 		void *cb)
 {
-	if (!strcmp(var, get_colorbool_slot)) {
-		get_colorbool_found =
-			git_config_colorbool(var, value, stdout_is_tty);
-	}
-	if (!strcmp(var, "diff.color")) {
-		get_diff_color_found =
-			git_config_colorbool(var, value, stdout_is_tty);
-	}
-	if (!strcmp(var, "color.ui")) {
-		git_use_color_default = git_config_colorbool(var, value, stdout_is_tty);
-		return 0;
-	}
+	if (!strcmp(var, get_colorbool_slot))
+		get_colorbool_found = git_config_colorbool(var, value);
+	else if (!strcmp(var, "diff.color"))
+		get_diff_color_found = git_config_colorbool(var, value);
+	else if (!strcmp(var, "color.ui"))
+		git_use_color_default = git_config_colorbool(var, value);
 	return 0;
 }
 
@@ -510,9 +503,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_GET_COLORBOOL) {
 		if (argc == 1)
-			stdout_is_tty = git_config_bool("command line", argv[0]);
-		else if (argc == 0)
-			stdout_is_tty = isatty(1);
+			color_stdout_is_tty = git_config_bool("command line", argv[0]);
 		return get_colorbool(argc != 0);
 	}
 
diff --git a/builtin/grep.c b/builtin/grep.c
index cccf8da..d80db22 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -325,7 +325,7 @@ static int grep_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "color.grep"))
-		opt->color = git_config_colorbool(var, value, -1);
+		opt->color = git_config_colorbool(var, value);
 	else if (!strcmp(var, "color.grep.context"))
 		color = opt->color_context;
 	else if (!strcmp(var, "color.grep.filename"))
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index facc63a..e6650b4 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -573,7 +573,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "color.showbranch")) {
-		showbranch_use_color = git_config_colorbool(var, value, -1);
+		showbranch_use_color = git_config_colorbool(var, value);
 		return 0;
 	}
 
diff --git a/color.c b/color.c
index 3db214c..67affa4 100644
--- a/color.c
+++ b/color.c
@@ -2,6 +2,7 @@
 #include "color.h"
 
 int git_use_color_default = 0;
+int color_stdout_is_tty = -1;
 
 /*
  * The list of available column colors.
@@ -157,7 +158,7 @@ bad:
 	die("bad color value '%.*s' for variable '%s'", value_len, value, var);
 }
 
-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
+int git_config_colorbool(const char *var, const char *value)
 {
 	if (value) {
 		if (!strcasecmp(value, "never"))
@@ -177,9 +178,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 
 	/* any normal truth value defaults to 'auto' */
  auto_color:
-	if (stdout_is_tty < 0)
-		stdout_is_tty = isatty(1);
-	if (stdout_is_tty || (pager_in_use() && pager_use_color)) {
+	if (color_stdout_is_tty < 0)
+		color_stdout_is_tty = isatty(1);
+	if (color_stdout_is_tty || (pager_in_use() && pager_use_color)) {
 		char *term = getenv("TERM");
 		if (term && strcmp(term, "dumb"))
 			return 1;
@@ -190,7 +191,7 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 int git_color_default_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.ui")) {
-		git_use_color_default = git_config_colorbool(var, value, -1);
+		git_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 
diff --git a/color.h b/color.h
index 68a926a..a190a25 100644
--- a/color.h
+++ b/color.h
@@ -58,11 +58,17 @@ extern const char *column_colors_ansi[];
 extern const int column_colors_ansi_max;
 
 /*
+ * Generally the color code will lazily figure this out itself, but
+ * this provides a mechanism for callers to override autodetection.
+ */
+extern int color_stdout_is_tty;
+
+/*
  * Use this instead of git_default_config if you need the value of color.ui.
  */
 int git_color_default_config(const char *var, const char *value, void *cb);
 
-int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
+int git_config_colorbool(const char *var, const char *value);
 void color_parse(const char *value, const char *var, char *dst);
 void color_parse_mem(const char *value, int len, const char *var, char *dst);
 __attribute__((format (printf, 3, 4)))
diff --git a/diff.c b/diff.c
index 2d86abe..2dfc359 100644
--- a/diff.c
+++ b/diff.c
@@ -137,7 +137,7 @@ static int git_config_rename(const char *var, const char *value)
 int git_diff_ui_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
-		diff_use_color_default = git_config_colorbool(var, value, -1);
+		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {
@@ -3409,7 +3409,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--color"))
 		options->use_color = 1;
 	else if (!prefixcmp(arg, "--color=")) {
-		int value = git_config_colorbool(NULL, arg+8, -1);
+		int value = git_config_colorbool(NULL, arg+8);
 		if (value == 0)
 			options->use_color = 0;
 		else if (value > 0)
diff --git a/parse-options.c b/parse-options.c
index 879ea82..be4383e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -621,7 +621,7 @@ int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
 
 	if (!arg)
 		arg = unset ? "never" : (const char *)opt->defval;
-	value = git_config_colorbool(NULL, arg, -1);
+	value = git_config_colorbool(NULL, arg);
 	if (value < 0)
 		return opterror(opt,
 			"expects \"always\", \"auto\", or \"never\"", 0);
-- 
1.7.6.10.g62f04

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox