* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: John Keeping @ 2013-01-25 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vbocd2auo.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 01:47:59PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> > With the patch above, the block of code at the top becomes:
> >
> > test "$tool" = defaults && continue
> >
> > setup_tool "$tool" 2>/dev/null || continue
> > merge_tool_path=$(translate_merge_tool_path "$tool")
> >
> > which IMHO is pretty readable.
>
> Of course it is. The current callers of setup_tool may need some
> adjustments, but that should be fairly trivial, I hope.
There are only two and one of them already seems like it doesn't want
the command to cause the script to exit.
David, can you incorporate the following two patches when you re-roll?
Your original 7/7 with the change above will want to build on 8/7.
John
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: Junio C Hamano @ 2013-01-25 21:47 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130125211601.GD7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> > It doesn't - the "|| continue" is to catch errors from setup_tool.
>>
>> Ugh.
>
> Is that targeted at my suggestion at the top of this email or calling
> exit in setup_tool?
At the fact that you had to go a convoluted route because you cannot
just run setup_tool in subshell and do translate_merge_tool_path
after that, because the latter needs to look at the shell variable
the former sets.
> With the patch above, the block of code at the top becomes:
>
> test "$tool" = defaults && continue
>
> setup_tool "$tool" 2>/dev/null || continue
> merge_tool_path=$(translate_merge_tool_path "$tool")
>
> which IMHO is pretty readable.
Of course it is. The current callers of setup_tool may need some
adjustments, but that should be fairly trivial, I hope.
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: John Keeping @ 2013-01-25 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vlibh2d8a.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> >> John Keeping <john@keeping.me.uk> writes:
> >>
> >> > Actually, can we just change all of the above part of the loop to:
> >> >
> >> > test "$tool" = defaults && continue
> >> >
> >> > merge_tool_path=$(
> >> > setup_tool "$tool" >/dev/null 2>&1 &&
> >> > translate_merge_tool_path "$tool"
> >> > ) || continue
> >>
> >> Meaning "setup_tool ought to know which mode we are in and should
> >> fail if we are in merge mode and it does not support merging"? That
> >> line of reasoning makes tons of sense to me, compared to this script
> >> implementing that logic for these scriptlets.
> >
> > Yes, that's part of what setup_tool does. It actually calls "exit" if
> > the "mode? && can_mode" test fails, which is why we need to call it in
> > the subshell.
> >
> > I think this would get even better if we add a preparatory patch like
> > this, so we can just call setup_tool and then set merge_tool_path:
> >
> > -- >8 --
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 888ae3e..4644cbf 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -67,11 +67,11 @@ setup_tool () {
> > if merge_mode && ! can_merge
> > then
> > echo "error: '$tool' can not be used to resolve merges" >&2
> > - exit 1
> > + return 1
> > elif diff_mode && ! can_diff
> > then
> > echo "error: '$tool' can only be used to resolve merges" >&2
> > - exit 1
> > + return 1
> > fi
> > return 0
> > }
> > @@ -100,7 +100,7 @@ run_merge_tool () {
> > status=0
> >
> > # Bring tool-specific functions into scope
> > - setup_tool "$1"
> > + setup_tool "$1" || return
> >
> > if merge_mode
> > then
> >
> > -- 8< --
> >
> >> How/when does translate_merge_tool_path fail?
> >
> > It doesn't - the "|| continue" is to catch errors from setup_tool.
>
> Ugh.
Is that targeted at my suggestion at the top of this email or calling
exit in setup_tool?
With the patch above, the block of code at the top becomes:
test "$tool" = defaults && continue
setup_tool "$tool" 2>/dev/null || continue
merge_tool_path=$(translate_merge_tool_path "$tool")
which IMHO is pretty readable.
John
^ permalink raw reply
* Re: [PATCH 0/2] git-p4 support for older python
From: Junio C Hamano @ 2013-01-25 21:10 UTC (permalink / raw)
To: Brandon Casey, Pete Wyckoff; +Cc: git, esr, john
In-Reply-To: <1359146641-27810-1-git-send-email-drafnel@gmail.com>
Both patches look simple enough. Pete, what do you think?
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: Junio C Hamano @ 2013-01-25 20:56 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130125204619.GC7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> > Actually, can we just change all of the above part of the loop to:
>> >
>> > test "$tool" = defaults && continue
>> >
>> > merge_tool_path=$(
>> > setup_tool "$tool" >/dev/null 2>&1 &&
>> > translate_merge_tool_path "$tool"
>> > ) || continue
>>
>> Meaning "setup_tool ought to know which mode we are in and should
>> fail if we are in merge mode and it does not support merging"? That
>> line of reasoning makes tons of sense to me, compared to this script
>> implementing that logic for these scriptlets.
>
> Yes, that's part of what setup_tool does. It actually calls "exit" if
> the "mode? && can_mode" test fails, which is why we need to call it in
> the subshell.
>
> I think this would get even better if we add a preparatory patch like
> this, so we can just call setup_tool and then set merge_tool_path:
>
> -- >8 --
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 888ae3e..4644cbf 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -67,11 +67,11 @@ setup_tool () {
> if merge_mode && ! can_merge
> then
> echo "error: '$tool' can not be used to resolve merges" >&2
> - exit 1
> + return 1
> elif diff_mode && ! can_diff
> then
> echo "error: '$tool' can only be used to resolve merges" >&2
> - exit 1
> + return 1
> fi
> return 0
> }
> @@ -100,7 +100,7 @@ run_merge_tool () {
> status=0
>
> # Bring tool-specific functions into scope
> - setup_tool "$1"
> + setup_tool "$1" || return
>
> if merge_mode
> then
>
> -- 8< --
>
>> How/when does translate_merge_tool_path fail?
>
> It doesn't - the "|| continue" is to catch errors from setup_tool.
Ugh.
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: John Keeping @ 2013-01-25 20:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vpq0t2f2t.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > Actually, can we just change all of the above part of the loop to:
> >
> > test "$tool" = defaults && continue
> >
> > merge_tool_path=$(
> > setup_tool "$tool" >/dev/null 2>&1 &&
> > translate_merge_tool_path "$tool"
> > ) || continue
>
> Meaning "setup_tool ought to know which mode we are in and should
> fail if we are in merge mode and it does not support merging"? That
> line of reasoning makes tons of sense to me, compared to this script
> implementing that logic for these scriptlets.
Yes, that's part of what setup_tool does. It actually calls "exit" if
the "mode? && can_mode" test fails, which is why we need to call it in
the subshell.
I think this would get even better if we add a preparatory patch like
this, so we can just call setup_tool and then set merge_tool_path:
-- >8 --
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 888ae3e..4644cbf 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -67,11 +67,11 @@ setup_tool () {
if merge_mode && ! can_merge
then
echo "error: '$tool' can not be used to resolve merges" >&2
- exit 1
+ return 1
elif diff_mode && ! can_diff
then
echo "error: '$tool' can only be used to resolve merges" >&2
- exit 1
+ return 1
fi
return 0
}
@@ -100,7 +100,7 @@ run_merge_tool () {
status=0
# Bring tool-specific functions into scope
- setup_tool "$1"
+ setup_tool "$1" || return
if merge_mode
then
-- 8< --
> How/when does translate_merge_tool_path fail?
It doesn't - the "|| continue" is to catch errors from setup_tool.
John
^ permalink raw reply related
* [PATCH 2/2] git-p4.py: support Python 2.4
From: Brandon Casey @ 2013-01-25 20:44 UTC (permalink / raw)
To: git; +Cc: pw, esr, john, Brandon Casey, Brandon Casey
In-Reply-To: <1359146641-27810-1-git-send-email-drafnel@gmail.com>
Python 2.4 lacks the following features:
subprocess.check_call
struct.pack_into
Take a cue from 460d1026 and provide an implementation of the
CalledProcessError exception. Then replace the calls to
subproccess.check_call with calls to subprocess.call that check the return
status and raise a CalledProcessError exception if necessary.
The struct.pack_into in t/9802 can be converted into a single struct.pack
call which is available in Python 2.4.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
INSTALL | 2 +-
git-p4.py | 27 ++++++++++++++++++++++++---
t/t9802-git-p4-filetype.sh | 11 ++++++-----
3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/INSTALL b/INSTALL
index fc723b3..b96e16d 100644
--- a/INSTALL
+++ b/INSTALL
@@ -131,7 +131,7 @@ Issues of note:
use English. Under autoconf the configure script will do this
automatically if it can't find libintl on the system.
- - Python version 2.5 or later is needed to use the git-p4
+ - Python version 2.4 or later is needed to use the git-p4
interface to Perforce.
- Some platform specific issues are dealt with Makefile rules,
diff --git a/git-p4.py b/git-p4.py
index 4f95d7a..faec09d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -18,6 +18,21 @@ import optparse, os, marshal, subprocess, shelve
import tempfile, getopt, os.path, time, platform
import re, shutil
+try:
+ from subprocess import CalledProcessError
+except ImportError:
+ # from python2.7:subprocess.py
+ # Exception classes used by this module.
+ class CalledProcessError(Exception):
+ """This exception is raised when a process run by check_call() returns
+ a non-zero exit status. The exit status will be stored in the
+ returncode attribute."""
+ def __init__(self, returncode, cmd):
+ self.returncode = returncode
+ self.cmd = cmd
+ def __str__(self):
+ return "Command '%s' returned non-zero exit status %d" % (self.cmd, self.returncode)
+
verbose = False
# Only labels/tags matching this will be imported/exported
@@ -158,13 +173,17 @@ def system(cmd):
expand = isinstance(cmd,basestring)
if verbose:
sys.stderr.write("executing %s\n" % str(cmd))
- subprocess.check_call(cmd, shell=expand)
+ retcode = subprocess.call(cmd, shell=expand)
+ if retcode:
+ raise CalledProcessError(retcode, cmd)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
expand = isinstance(real_cmd, basestring)
- subprocess.check_call(real_cmd, shell=expand)
+ retcode = subprocess.call(real_cmd, shell=expand)
+ if retcode:
+ raise CalledProcessError(retcode, real_cmd)
def p4_integrate(src, dest):
p4_system(["integrate", "-Dt", wildcard_encode(src), wildcard_encode(dest)])
@@ -3174,7 +3193,9 @@ class P4Clone(P4Sync):
init_cmd = [ "git", "init" ]
if self.cloneBare:
init_cmd.append("--bare")
- subprocess.check_call(init_cmd)
+ retcode = subprocess.call(init_cmd)
+ if retcode:
+ raise CalledProcessError(retcode, init_cmd)
if not P4Sync.run(self, depotPaths):
return False
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 21924df..be299dc 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -105,12 +105,13 @@ build_gendouble() {
cat >gendouble.py <<-\EOF
import sys
import struct
- import array
- s = array.array("c", '\0' * 26)
- struct.pack_into(">L", s, 0, 0x00051607) # AppleDouble
- struct.pack_into(">L", s, 4, 0x00020000) # version 2
- s.tofile(sys.stdout)
+ s = struct.pack(">LL18s",
+ 0x00051607, # AppleDouble
+ 0x00020000, # version 2
+ "" # pad to 26 bytes
+ )
+ sys.stdout.write(s);
EOF
}
--
1.8.1.1.297.gad3d74e
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply related
* [PATCH 0/2] git-p4 support for older python
From: Brandon Casey @ 2013-01-25 20:43 UTC (permalink / raw)
To: git; +Cc: pw, esr, john, Brandon Casey
Offered for consideration.
These two patches allow git-p4 to run on python 2.4 which is shipped on
RHEL 5.X. The changes are minor and unintrusive in my opinion, but please
feel free to reject one or both of them for any reason (in which case the
version check at the top of git-p4.py should be updated).
Brandon Casey (2):
git-p4.py: support Python 2.5
git-p4.py: support Python 2.4
INSTALL | 2 +-
git-p4.py | 30 ++++++++++++++++++++++++++----
t/t9802-git-p4-filetype.sh | 11 ++++++-----
3 files changed, 33 insertions(+), 10 deletions(-)
--
1.8.1.1.297.gad3d74e
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply
* [PATCH 1/2] git-p4.py: support Python 2.5
From: Brandon Casey @ 2013-01-25 20:44 UTC (permalink / raw)
To: git; +Cc: pw, esr, john, Brandon Casey, Brandon Casey
In-Reply-To: <1359146641-27810-1-git-send-email-drafnel@gmail.com>
Python 2.5 and older do not accept None as the first argument to
translate() and complain with:
TypeError: expected a character buffer object
Satisfy this older python by calling maketrans() to generate an empty
translation table and supplying that to translate().
This allows git-p4 to be used with Python 2.5.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
INSTALL | 2 +-
git-p4.py | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/INSTALL b/INSTALL
index 28f34bd..fc723b3 100644
--- a/INSTALL
+++ b/INSTALL
@@ -131,7 +131,7 @@ Issues of note:
use English. Under autoconf the configure script will do this
automatically if it can't find libintl on the system.
- - Python version 2.6 or later is needed to use the git-p4
+ - Python version 2.5 or later is needed to use the git-p4
interface to Perforce.
- Some platform specific issues are dealt with Makefile rules,
diff --git a/git-p4.py b/git-p4.py
index 2da5649..4f95d7a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -768,7 +768,8 @@ def wildcard_encode(path):
return path
def wildcard_present(path):
- return path.translate(None, "*#@%") != path
+ from string import maketrans
+ return path.translate(maketrans("",""), "*#@%") != path
class Command:
def __init__(self):
--
1.8.1.1.297.gad3d74e
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply related
* Re: [PATCH v3 2/8] git_remote_helpers: fix input when running under Python 3
From: Brandon Casey @ 2013-01-25 20:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Sverre Rabbelier, Dennis Kaarsemaker, John Keeping, Git List
In-Reply-To: <7v622nhc0u.fsf@alter.siamese.dyndns.org>
On Wed, Jan 23, 2013 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
>
>> On Wed, Jan 23, 2013 at 11:47 AM, John Keeping <john@keeping.me.uk> wrote:
>>>> When did we last revisit what minimal python version we are ok with requiring?
>>>
>>> I was wondering if people would weigh in discussing that in response to
>>> [1] but no one has commented on that part of it. As another datapoint,
>>> Brandon Casey was suggesting patching git-p4.py to support Python 2.4
>>> [2].
>>>
>>> [1] http://article.gmane.org/gmane.comp.version-control.git/213920
>>> [2] http://article.gmane.org/gmane.comp.version-control.git/214048
>>
>> I for one would be happy to kill off support for anything older than
>> 2.6 (which had it's latest release on October 1st, 2008).
>>
>> Junio, how have we decided in the past which version of x to support?
>
> I do not think there was any conclusion. $gmane/212215 claiming 2.4
> support matters for RHEL 5.x users was the last on the topic as far
> as I can tell, so it boils down to another question: do users on
> RHEL 5.x matter?
>
> I can read from $gmane/212215 that users of the said platform can
> safely keep using Python 2.4 under their vendor support contract
> until 2017. But let's focus on what do these users expect of their
> system and software they run on it a bit.
>
> When they want to run a piece software that is not shipped with
> RHEL, either by writing their own or by importing from elsewhere,
> that needs 2.6 features, what are their options?
>
> (a) The platform vendor optionally supplies 2.6 with or without
> support;
>
> (b) The users can and do install 2.6 as /usr/local/bin/python2.6,
> which may even be community-supported, but the vendor does not
> support it; or
>
> (c) The vendor terminates the support contract for users who choose
> to go (b).
>
> I think we can safely discard (c); if that is the case, the users on
> the said platform will not choose to update Git either, so it does
> not matter where the future versions of Git sets the lower bound of
> Python version at.
>
> If we are not talking about the situation (c), then the users can
> choose to use 2.6, and more importantly, Python being a popular
> software, I would imagine that there are reputable sources of
> prepackaged RPMs for them to do so without going too much hassle of
> configuring, compiling and installing.
>
> Now how does the decision we make today for releases of Git that
> haven't yet happened will affect these users? As these versions of
> newer Git were not shipped with RHEL 5.x, and also I am assuming
> that Git is a more niche product than Python is, I would imagine
> that it is very unlikely that the vendor gives it the users as an
> optional package. The users will have to do the same thing to be
> able to use such versions of Git as whatever they do in order to use
> Python 2.6.
>
> Given that, what the vendor originally shipped and officially
> supports does not affect the choices we would make today for newer
> versions of Git. The users in a shop where additional third-party
> software in /usr/local/bin is strictly forbidden, they are stuck
> with the version of Git that the vendor shipped anyway, because they
> won't be able to install an updated Git in /usr/local/bin, either.
>
> That is, unless installing 2.6 as /usr/local/bin/python2.6 (or if
> you are really paranoid, /usr/local/only-for-git/bin/python2.6 where
> nobody's $PATH points at) is impossible.
>
> So personally I do not think dropping 2.4 is a huge problem for
> future versions of Git, but I'd like to hear from those working in
> IT support for large and slow-moving organizations (aka RHEL 5
> customers).
I'm not really in the demographic that you asked to hear from, but
I'll give my 2 cents anyway. :)
Firstly, I defer to those with more knowledge and experience with
python to decide which version should be the minimum version
supported. Python 2.6 seems to be the consensus and that's fine with
me.
With respect to older platforms like RHEL 5.X that don't ship with
Python 2.6 or later, I suspect most people who work in an organization
with a dedicated IT staff can request that a more recent version of
python be installed. So, I don't think a python 2.6 requirement (if
there was one) would be a blocker for them, and I don't think it would
be a major pain for the sysadmin to install.
My only opinion is that if we can avoid breaking older platforms
fairly easily, we should do so. If there is someone out there
building git packages (e.g. EPEL) for RHEL 5.X or anything else, I
imagine that one less dependency makes installing and supporting the
package that much easier.
So, my comments shouldn't be taken to suggest that git should support
any particular version of python. That decision should be made by
those who are willing to support whatever version they feel strongly
about.
-Brandon
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: Junio C Hamano @ 2013-01-25 20:16 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130125200807.GB7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> Actually, can we just change all of the above part of the loop to:
>
> test "$tool" = defaults && continue
>
> merge_tool_path=$(
> setup_tool "$tool" >/dev/null 2>&1 &&
> translate_merge_tool_path "$tool"
> ) || continue
Meaning "setup_tool ought to know which mode we are in and should
fail if we are in merge mode and it does not support merging"? That
line of reasoning makes tons of sense to me, compared to this script
implementing that logic for these scriptlets.
How/when does translate_merge_tool_path fail?
>
>> > if type "$merge_tool_path" >/dev/null 2>&1
>> > then
>> > - available="$available$i$LF"
>> > + available="$available$tool$LF"
>> > else
>> > - unavailable="$unavailable$i$LF"
>> > + unavailable="$unavailable$tool$LF"
>> > fi
>> > done
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: John Keeping @ 2013-01-25 20:08 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <20130125195446.GA7498@serenity.lan>
On Fri, Jan 25, 2013 at 07:54:46PM +0000, John Keeping wrote:
> On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> > Check the can_diff and can_merge functions before deciding whether to
> > add the tool to the available/unavailable lists. This makes --tool-help context-
> > sensitive so that "git mergetool --tool-help" displays merge tools only
> > and "git difftool --tool-help" displays diff tools only.
>
> This log message is misleading - the existing code in
> list_merge_tool_candidates already filters the tools like this, so the
> change is more:
>
> mergetool--lib: don't use a hardcoded list for "--tool-help"
>
> Instead of using a list of tools in list_merge_tool_candidates, list
> the available scriptlets and query each of those to know whether it
> applies to diff mode and/or merge mode.
>
> guess_merge_tool still relies on list_merge_tool_candidates so we
> can't remove that function now.
>
>
> The patch seems to do the right thing, although I have a couple of minor
> nits...
>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> > git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
> > 1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index db8218a..c547c59 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
> > }
> >
> > show_tool_help () {
> > - list_merge_tool_candidates
> > unavailable= available= LF='
> > '
> > - for i in $tools
> > +
> > + scriptlets="$(git --exec-path)"/mergetools
> > + for i in "$scriptlets"/*
> > do
> > - merge_tool_path=$(translate_merge_tool_path "$i")
> > + . "$scriptlets"/defaults
> > + . "$i"
> > +
> > + tool="$(basename "$i")"
>
> Quotes are unnecessary here.
>
> > + if test "$tool" = "defaults"
> > + then
> > + continue
> > + elif merge_mode && ! can_merge
> > + then
> > + continue
> > + elif diff_mode && ! can_diff
> > + then
> > + continue
> > + fi
>
> Would this be better as:
>
> test "$tool" = "defaults" && continue
>
> can_merge || ! merge_mode || continue
> can_diff || ! diff_mode || continue
>
> or is that a bit too concise?
>
> I'd prefer to see two separate if statements either way since the "test
> $tool = defaults" case is different from the "does it apply to the
> current mode?" case. The "$tool = defaults" case could even move to the
> top of the loop.
>
> > + merge_tool_path=$(translate_merge_tool_path "$tool")
Actually, can we just change all of the above part of the loop to:
test "$tool" = defaults && continue
merge_tool_path=$(
setup_tool "$tool" >/dev/null 2>&1 &&
translate_merge_tool_path "$tool"
) || continue
> > if type "$merge_tool_path" >/dev/null 2>&1
> > then
> > - available="$available$i$LF"
> > + available="$available$tool$LF"
> > else
> > - unavailable="$unavailable$i$LF"
> > + unavailable="$unavailable$tool$LF"
> > fi
> > done
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: Junio C Hamano @ 2013-01-25 20:06 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130125195446.GA7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> + tool="$(basename "$i")"
>
> Quotes are unnecessary here.
Yeah, the outer quotes aren't needed; the inner ones are.
>> + if test "$tool" = "defaults"
>> + then
>> + continue
>> + elif merge_mode && ! can_merge
>> + then
>> + continue
>> + elif diff_mode && ! can_diff
>> + then
>> + continue
>> + fi
>
> Would this be better as:
>
> test "$tool" = "defaults" && continue
>
> can_merge || ! merge_mode || continue
> can_diff || ! diff_mode || continue
>
> or is that a bit too concise?
It is beyond "too concise"; it is unreadable, and more importantly,
the latter two lines are illogical (why do you even ask if it can be
used for merging, before asking merge_mode to see if the answer to
that question matters to you?)
^ permalink raw reply
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
From: John Keeping @ 2013-01-25 19:54 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <1359107034-14606-8-git-send-email-davvid@gmail.com>
On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> Check the can_diff and can_merge functions before deciding whether to
> add the tool to the available/unavailable lists. This makes --tool-help context-
> sensitive so that "git mergetool --tool-help" displays merge tools only
> and "git difftool --tool-help" displays diff tools only.
This log message is misleading - the existing code in
list_merge_tool_candidates already filters the tools like this, so the
change is more:
mergetool--lib: don't use a hardcoded list for "--tool-help"
Instead of using a list of tools in list_merge_tool_candidates, list
the available scriptlets and query each of those to know whether it
applies to diff mode and/or merge mode.
guess_merge_tool still relies on list_merge_tool_candidates so we
can't remove that function now.
The patch seems to do the right thing, although I have a couple of minor
nits...
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db8218a..c547c59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
> }
>
> show_tool_help () {
> - list_merge_tool_candidates
> unavailable= available= LF='
> '
> - for i in $tools
> +
> + scriptlets="$(git --exec-path)"/mergetools
> + for i in "$scriptlets"/*
> do
> - merge_tool_path=$(translate_merge_tool_path "$i")
> + . "$scriptlets"/defaults
> + . "$i"
> +
> + tool="$(basename "$i")"
Quotes are unnecessary here.
> + if test "$tool" = "defaults"
> + then
> + continue
> + elif merge_mode && ! can_merge
> + then
> + continue
> + elif diff_mode && ! can_diff
> + then
> + continue
> + fi
Would this be better as:
test "$tool" = "defaults" && continue
can_merge || ! merge_mode || continue
can_diff || ! diff_mode || continue
or is that a bit too concise?
I'd prefer to see two separate if statements either way since the "test
$tool = defaults" case is different from the "does it apply to the
current mode?" case. The "$tool = defaults" case could even move to the
top of the loop.
> + merge_tool_path=$(translate_merge_tool_path "$tool")
> if type "$merge_tool_path" >/dev/null 2>&1
> then
> - available="$available$i$LF"
> + available="$available$tool$LF"
> else
> - unavailable="$unavailable$i$LF"
> + unavailable="$unavailable$tool$LF"
> fi
> done
^ permalink raw reply
* Re: [PATCH] git-web--browser: avoid errors in terminal when running Firefox on Windows
From: Junio C Hamano @ 2013-01-25 19:49 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git, Jeff King
In-Reply-To: <3eeabf4989f7f1b4593e89e4c6bcfa8710a7b793.1359125053.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <alex.crezoff@gmail.com> writes:
> Firefox on Windows by default is placed in "C:\Program Files\Mozilla Firefox"
> folder, i.e. its path contains spaces. Before running this browser "git-web--browse"
> tests version of Firefox to decide whether to use "-new-tab" option or not.
>
> Quote browser path to avoid error during this test.
>
> Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>
Thanks, both.
> ---
> git-web--browse.sh | 2 +-
> t/t9901-git-web--browse.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/git-web--browse.sh b/git-web--browse.sh
> index 1e82726..f96e5bd 100755
> --- a/git-web--browse.sh
> +++ b/git-web--browse.sh
> @@ -149,7 +149,7 @@ fi
> case "$browser" in
> firefox|iceweasel|seamonkey|iceape)
> # Check version because firefox < 2.0 does not support "-new-tab".
> - vers=$(expr "$($browser_path -version)" : '.* \([0-9][0-9]*\)\..*')
> + vers=$(expr "$("$browser_path" -version)" : '.* \([0-9][0-9]*\)\..*')
> NEWTAB='-new-tab'
> test "$vers" -lt 2 && NEWTAB=''
> "$browser_path" $NEWTAB "$@" &
> diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
> index b0a6bad..30d5294 100755
> --- a/t/t9901-git-web--browse.sh
> +++ b/t/t9901-git-web--browse.sh
> @@ -8,8 +8,21 @@ This test checks that git web--browse can handle various valid URLs.'
> . ./test-lib.sh
>
> test_web_browse () {
> - # browser=$1 url=$2
> + # browser=$1 url=$2 sleep_timeout=$3
> + sleep_timeout="$3"
> git web--browse --browser="$1" "$2" >actual &&
> + # if $3 is set
> + # as far as Firefox is run in background (it is run with &)
> + # we trying to avoid race condition
> + # by waiting for "$sleep_timeout" seconds of timeout for 'fake_browser_ran' file appearance
> + (test -z "$sleep_timeout" || (
> + for timeout in $(seq 1 $sleep_timeout); do
> + test -f fake_browser_ran && break
> + sleep 1
> + done
> + test $timeout -ne $sleep_timeout
> + )
> + ) &&
Style:
- do/then/else begin a new line (a good rule of thumb is remember
this rule is to write control structures without using
semicolon).
- do not use "seq"; it is not available in some places.
I do not think of a reason why you want ( nested (subshell) ), but
if you don't need them, perhaps I'd write the above this way:
if test -n $sleep_timeout
then
for timeout in $(test_seq $sleep_timeout)
do
test -f fake_browser_ran && break
sleep 1
done
test $timeout -ne $sleep_timeout
fi &&
> @@ -48,6 +61,48 @@ test_expect_success \
> '
>
> test_expect_success \
> + 'Firefox below v2.0 paths are properly quoted' '
-ECANNOTPARSE.
"Paths to firefox older than v2.0 are properly quoted" you mean,
perhaps? I dunno.
> + echo fake: http://example.com/foo >expect &&
> + rm -f fake_browser_ran &&
> + cat >"fake browser" <<-\EOF &&
> + #!/bin/sh
Consider using "write_script" helper so that you get the path to the
shell the user specified via $SHELL_PATH.
> +
> + : > fake_browser_ran
Style: no SP between redirection operator and filename, i.e.
: >fake_browser_ran
> + if test "$1" = "-version"; then
Style (see above).
> + echo Fake Firefox browser version 1.2.3
> + else
> + # Firefox (in contrast to w3m) is run in background (with &)
> + # so redirect output to "actual"
> + echo fake: "$@" > actual
Style (see above).
> + fi
> + EOF
> + chmod +x "fake browser" &&
> + git config browser.firefox.path "`pwd`/fake browser" &&
We tend to prefer $(pwd) over `pwd`.
> + test_web_browse firefox http://example.com/foo 5
> +'
> +
> +test_expect_success \
> + 'Firefox not lower v2.0 paths are properly quoted' '
s/not lower v2.0/v2.0 and above/, but again -ECANNOTPARSE.
> + echo fake: -new-tab http://example.com/foo >expect &&
I'd feel safer if you quoted the arguments to "echo", i.e.
echo "fake: -new-tab http://example.com/foo" >expect &&
The same style comments as above apply to the remainder of patch.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] add: warn when -u or -A is used without filepattern
From: Junio C Hamano @ 2013-01-25 19:27 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Jonathan Nieder, Robin Rosenberg, Piotr Krukowiecki,
Eric James Michael Ritz, Tomas Carnecky
In-Reply-To: <1359110978-20054-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> Most git commands that can be used with our without a filepattern are
> tree-wide by default, the filepattern being used to restrict their scope.
> A few exceptions are: 'git grep', 'git clean', 'git add -u' and 'git add -A'.
>
> The inconsistancy of 'git add -u' and 'git add -A' are particularly
s/consistan/consisten/;
> diff --git a/builtin/add.c b/builtin/add.c
> index e664100..8252d19 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -363,6 +363,33 @@ static int add_files(struct dir_struct *dir, int flags)
> return exit_status;
> }
>
> +static void warn_pathless_add(const char *option_name) {
> + /*
> + * To be consistant with "git add -p" and most Git
Likewise.
> + warning(_("The behavior of 'git add %s' with no path argument from a subdirectory of the\n"
> ...
> + option_name,
> + option_name,
> + option_name);
> +}
> +
> int cmd_add(int argc, const char **argv, const char *prefix)
> {
> int exit_status = 0;
> @@ -392,8 +420,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> die(_("-A and -u are mutually incompatible"));
> if (!show_only && ignore_missing)
> die(_("Option --ignore-missing can only be used together with --dry-run"));
> - if ((addremove || take_worktree_changes) && !argc) {
> + if (addremove)
> + option_with_implicit_dot = "--all";
> + if (take_worktree_changes)
> + option_with_implicit_dot = "--update";
I wonder if we want to say in the message
The behaviour of 'git add --all (or -A)'...
otherwise people who typed "git add -A" and got this message with
just "--all" may go "Huh?" for a brief moment. I however do not
think replacing these strings to
option_with_implicit_dot = "--all (-A)";
is a solution, given they are goven to _("l10n template %s").
Other than that the patch looks reasonable.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
From: Junio C Hamano @ 2013-01-25 19:11 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr5ZsqO+PFoUabsZObgvG8jUBfTKL1HmVsn77ZhzsRZk-Q@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
>>> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
>>> list "vim" as being an unavailable tool. This is because they attempt
>>> to find a tool named according to the mergetool scriptlet instead of the Git-
>>> recognized tool name.
>>
>> Actually, after my patches both git-difftool and git-mergetool get this
>> right since list_merge_tool_candidates lists vimdiff and gvimdiff.
>>
>>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
>>> scriptlet. This required git-mergetool--lib to special-case it when
>>> setting up the tool.
>>>
>>> Remove the exception for "vim" and allow the scriptlets to be found
>>> naturally by using symlinks to a single "vimdiff" scriptlet.
>>
>> I wonder if it would be better to make these single-line scripts instead
>> of symlinks:
>>
>> . "$MERGE_TOOLS_DIR"/vimdiff
>>
>> where we make git-mergetool--lib.sh export:
>>
>> MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>
> That sounds like the way to go.
Yup, I'll expect a reroll of this one and possibly the next one (I
haven't read yet).
1-5/7 looked all very sensible.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/3] branch: reject -D/-d without branch name
From: Junio C Hamano @ 2013-01-25 19:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Matthieu Moy
In-Reply-To: <1359118225-14356-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ---
> builtin/branch.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Forgot to sign-off?
Is this a real problem?
I do not see it particularly wrong to succeed after deleting 0 or
more given branch names.
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 873f624..50fcacc 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -837,9 +837,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> colopts = 0;
> }
>
> - if (delete)
> + if (delete) {
> + if (!argc)
> + die(_("branch name required"));
> return delete_branches(argc, argv, delete > 1, kinds, quiet);
> - else if (list) {
> + } else if (list) {
> int ret = print_ref_list(kinds, detached, verbose, abbrev,
> with_commit, argv);
> print_columns(&output, colopts, NULL);
^ permalink raw reply
* Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects
From: Junio C Hamano @ 2013-01-25 19:00 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Jens Lehmann
In-Reply-To: <CACsJy8C3tLOHCK4Qc--W630do0M=xLKSMoYUxxv2_GDaUXaRww@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> .... Even
> when cache-tree is not involved, I do not want the index to point to
> an non-existing SHA-1 ("git diff --cached" may fail next time, for
> example).
I think we have tests that explicitly add SHA-1 that names an object
that does not exist to the index and check failures; you may have to
think what to do with them.
>> This is a tangent, but in addition, you may want to add an even
>> narrower variant that checks the same but ignoring _all_ alternate
>> object databases, "external" or not:
>>
>> int has_sha1_file_local(const unsigned char *sha1);
>>
>> That may be useful if we want to later make the alternate safer to
>> use; instead of letting the codepath to create an object ask
>> has_sha1_file() to see if it already exists and refrain from writing
>> a new copy, we switch to ask has_sha1_file_locally() and even if an
>> alternate object database we borrow from has it, we keep our own
>> copy in our repository.
This is not a tangent, but if you want to go this "forbid making our
repository depend on objects we do not have but we know about after
we peek submodule odb" route [*1*], write_sha1_file() needs to be
told about has_sha1_file_proper(). We may "git add" a new blob in
the superproject, the blob may not yet exist in *our* repository,
but may happen to already exist in the submodue odb. In such a
case, write_sha1_file() has to write that blob in our repository,
without the existing has_sha1_file() check bypassing it. Otherwise
our attempt to create a tree that contains that blob will fail,
saying that the blob only seems to exist to us via submodule odb but
not in our repository.
[Footnote]
*1* which I do not necessarily agree with---I am in favor of getting
rid of add_submodule_odb() to fix these issues at the root cause of
them.
^ permalink raw reply
* Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails
From: Junio C Hamano @ 2013-01-25 18:46 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, Angus Hammond, git, Mihai Rusu
In-Reply-To: <20130125010559.GA27657@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> (with a proper commit message, of course).
Will queue this one, to be merged to 'maint' and 'master'.
-- >8 --
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 24 Jan 2013 15:21:46 -0800
Subject: [PATCH] ident: do not drop username when reading from /etc/mailname
An earlier conversion from fgets() to strbuf_getline() in the
codepath to read from /etc/mailname to learn the default host-part
of the ident e-mail address forgot that strbuf_getline() stores the
line at the beginning of the buffer just like fgets().
The "username@" the caller has prepared in the strbuf, expecting the
function to append the host-part to it, was lost because of this.
Reported-by: Mihai Rusu <dizzy@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
ident.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/ident.c b/ident.c
index 484e0a9..fb4cc72 100644
--- a/ident.c
+++ b/ident.c
@@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
static int add_mailname_host(struct strbuf *buf)
{
FILE *mailname;
+ struct strbuf mailnamebuf = STRBUF_INIT;
mailname = fopen("/etc/mailname", "r");
if (!mailname) {
@@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
strerror(errno));
return -1;
}
- if (strbuf_getline(buf, mailname, '\n') == EOF) {
+ if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
if (ferror(mailname))
warning("cannot read /etc/mailname: %s",
strerror(errno));
+ strbuf_release(&mailnamebuf);
fclose(mailname);
return -1;
}
/* success! */
+ strbuf_addbuf(buf, &mailnamebuf);
+ strbuf_release(&mailnamebuf);
fclose(mailname);
return 0;
}
--
1.8.1.1.525.gdace530
^ permalink raw reply related
* Re: [PATCH] t9902: protect test from stray build artifacts
From: Junio C Hamano @ 2013-01-25 18:34 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130125042326.GA31281@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:
>
>> > Ahh, ok, we show one element per line and just make sure "bundle"
>> > is there, and we do not care what other buns appear in the output.
>> >
>> Not so quick, though. The lower level "read from help -a" is only
>> run once and its output kept in a two-level cache hierarchy; we need
>> to reset both.
>
> Ugh, I didn't even think about that.
>
> I wonder if it would be simpler if the completion tests actually ran a
> new bash for each test. That would be slower, but it somehow seems
> cleaner.
I agree 100% with that. Let's leave this fix as-is, at least as a
tentative fix while "git check-ignore" graduates into the upcoming
release, and let somebody who is interested work on an update to
this test script to do so as an independent topic.
>
>> It starts to look a bit too intimately tied to the implementation of
>> what is being tested for my taste, though.
>> [...]
>> +test_expect_success 'help -a read correctly by command list generator' '
>> + __git_all_commands= &&
>> + __git_porcelain_commands= &&
>> + GIT_TESTING_COMMAND_COMPLETION= &&
>> + run_completion "git bun" &&
>> + grep "^bundle $" out
>> +'
>
> Agreed. I could take or leave it at this point. It's nice to check that
> changes to "help -a" will not break it, but ultimately it feels a bit
> too contrived to catch anything useful.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] mergetools: Enhance tortoisemerge to work with
From: Junio C Hamano @ 2013-01-25 18:28 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King
In-Reply-To: <5102837C.9000608@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> TortoiseGitMerge and filenames with spaces
??? ECANNOTPARSE.
... ah, wait. Is this a broken-off tail of your subject line?
It may be a sign that you are doing too many unrelated things in a
single patch when your subject does not fit on a single line.
Perhaps this is better done as a two-patch series?
* mergetools: fix tortoisemerge support for pathnames with SP
* mergetools: support tortoisegitmerge
> mergetools/tortoisemerge | 51 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
> index ed7db49..8ee99a5 100644
> --- a/mergetools/tortoisemerge
> +++ b/mergetools/tortoisemerge
> @@ -1,17 +1,34 @@
> -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
> -}
> +can_diff () {
> + return 1
> +}
> +
> +merge_cmd () {
> + if $base_present
> + then
> + touch "$BACKUP"
> + basename="$(basename "$merge_tool_path" .exe)"
> + if test "$basename" = "tortoisegitmerge"
> + then
> + "$merge_tool_path" \
> + -base "$BASE" -mine "$LOCAL" \
> + -theirs "$REMOTE" -merged "$MERGED"
> + else
> + "$merge_tool_path" \
> + -base:"$BASE" -mine:"$LOCAL" \
> + -theirs:"$REMOTE" -merged:"$MERGED"
Hmph.
How was the support for "names with spaces" added in this new code?
I do not spot what is different between this "else" clause and the
original body of the merge_cmd (which only supported tortoisemerge).
They seem to be doing exactly the same thing.
> + fi
> + check_unchanged
> + else
> + echo "$merge_tool_path cannot be used without a base" 1>&2
> + return 1
> + fi
> +}
> +
> +translate_merge_tool_path() {
> + if type tortoisegitmerge >/dev/null 2>/dev/null
> + then
> + echo tortoisegitmerge
> + else
> + echo tortoisemerge
> + fi
> +}
^ permalink raw reply
* Re: [PATCH resent] send-email: Honor multi-part email messages
From: Junio C Hamano @ 2013-01-25 18:14 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git, Krzysztof Mazur
In-Reply-To: <cover.1359126360.git.Alex.Crezoff@gmail.com>
Alexey Shumkin <Alex.Crezoff@gmail.com> writes:
> This function is used to determine "broken" (non-ASCII) headers (to be encode them)
> The problem is if "Subject" is not broken, but message body contains non-ASCII chars,
> subject is marked as broken and encoded again.
I think that is not a "problem" but is a mere symptom.
The remainder of the codeflow of send-email, AFAICS (it's not my
code), is not prepared to deal with multipart messages at all. In
order to handle multi-part properly, you may still have to fix
broken Subject: of the whole thing, and you may also want to fix
broken headers inside one part while keeping correctly formatted
part intact.
Your patch just stops an early error checking that is meant for a
non multi-part message that happens to trigger on a multi-part
message in your test case from triggering (i.e. masking a symptom)
and let the remaining lines of the multi-part message to codepath
that does not do anything special to handle multi-part messages
correctly, letting it do whatever it happens to do to a message
assuming it is not a multi-part message, no?
In other words, making send-email capable of handling a multi-part
might be a worthy thing to do, but I do not think your patch is a
good first step for doing so.
^ permalink raw reply
* Re: git merge error question: The following untracked working tree files would be overwritten by merge
From: Junio C Hamano @ 2013-01-25 18:07 UTC (permalink / raw)
To: Carsten Fuchs; +Cc: git
In-Reply-To: <5102607E.2070106@cafu.de>
Carsten Fuchs <carsten.fuchs@cafu.de> writes:
> Hi all,
>
> in my repo, I'm doing this:
>
>> $ git status
>> # On branch master
>> # Your branch is behind 'origin/master' by 2 commits, and can be fast-forwarded.
>> #
>> # Untracked files:
>> # (use "git add <file>..." to include in what will be committed)
>> #
>> # obsolete/
>> nothing added to commit but untracked files present (use "git add" to track)
>>
>> $ git merge origin/master --ff-only
>> Updating f419d57..2da6052
>> error: The following untracked working tree files would be overwritten by merge:
>> obsolete/e107/Readme.txt
>> obsolete/e107/article.php
>> obsolete/e107/backend.php
>> [...]
> ...
> Compare with what Subversion did in an analogous case: When I ran "svn
> update" and the update brought new files for which there already was
> an untracked copy in the working directory, Subversion:
> - started to consider the file as tracked,
> - but left the file in the working-copy alone.
>
> As a result, a subsequent "svn status" might
> a) no longer show the file at all, if the foreign copy in the
> working directory happened to be the same as the one brought by the
> "svn update", or
> b) flag the file as modified, if different from the one that "svn
> update" would have created in its place.
Interesting. So before running "status", the merge is recorded (in this
particular case you are doing ff-only so there is nothing new to
record, but if the rest of the tree merges cleanly, the new tree
that contains "obsolete" from the other branch you just merged will
be the contents you record in the merge commit), and working tree is
immediately dirty? It makes sense, even though I haven't tried to
think things through to see if there are tricky corner cases.
> So my real question is, why does Git not do something analogous?
The answer to that question is "because nobody thought that doing so
would help users very much and bothered to implement it"; it is not
"people thought about doing so and found reasons why that would not
help users".
^ permalink raw reply
* Re: [PATCH] send-email: Honor multi-part email messages
From: Krzysztof Mazur @ 2013-01-25 17:47 UTC (permalink / raw)
To: Alexey Shumkin; +Cc: git, Junio C Hamano
In-Reply-To: <4de442db9fd0896f78166e6038b6ea35ed5ab266.1359126360.git.Alex.Crezoff@gmail.com>
On Fri, Jan 25, 2013 at 07:28:54PM +0400, Alexey Shumkin wrote:
> "git format-patch --attach/--inline" generates multi-part messages.
> Every part of such messages can contain non-ASCII characters with its own
> "Content-Type" and "Content-Transfer-Encoding" headers.
> But git-send-mail script interprets a patch-file as one-part message
> and does not recognize multi-part messages.
> So already quoted printable email subject may be encoded as quoted printable
> again. Due to this bug email subject looks corrupted in email clients.
I don't think that the problem with the Subject is multi-part message
specific. The real problem with the Subject is probably that
is_rfc2047_quoted() does not detect that the Subject is already quoted.
Of course we still need that explicit multi-part message support to
avoid "Which 8bit encoding should I declare [UTF-8]? " message.
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 94c7f76..d49befe 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1499,12 +1499,17 @@ sub file_has_nonascii {
>
> sub body_or_subject_has_nonascii {
> my $fn = shift;
> + my $multipart = 0;
> open(my $fh, '<', $fn)
> or die "unable to open $fn: $!\n";
> while (my $line = <$fh>) {
> last if $line =~ /^$/;
> + if ($line =~ /^Content-Type:\s*multipart\/mixed.*$/) {
> + $multipart = 1;
> + }
> return 1 if $line =~ /^Subject.*[^[:ascii:]]/;
> }
> + return 0 if $multipart;
> while (my $line = <$fh>) {
> return 1 if $line =~ /[^[:ascii:]]/;
> }
After this change the function name is no longer appropriate.
Maybe we should join body_or_subject_has_nonascii()
and file_declares_8bit_cte() because in case of multi-part messages
"next unless (body_or_subject_has_nonascii($f)
&& !file_declares_8bit_cte($f));"
is not valid anymore. We could also check for broken_encoding
in single pass.
Thanks,
Krzysiek
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox