* [PATCH] color_parse_mem: allow empty color spec
From: Jeff King @ 2017-02-01 0:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqqpoj2q25n.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 31, 2017 at 02:45:40PM -0800, Junio C Hamano wrote:
> * nd/log-graph-configurable-colors (2017-01-23) 3 commits
> (merged to 'next' on 2017-01-23 at c369982ad8)
> + log --graph: customize the graph lines with config log.graphColors
> + color.c: trim leading spaces in color_parse_mem()
> + color.c: fix color_parse_mem() with value_len == 0
>
> Some people feel the default set of colors used by "git log --graph"
> rather limiting. A mechanism to customize the set of colors has
> been introduced.
>
> Reported to break "add -p".
> cf. <20170128040709.tqx4u45ktgpkbfm4@sigill.intra.peff.net>
I hadn't heard anything back, so I wrapped up my fix with a commit
message and tests (it needs to go on top anyway, since the breakage is
in 'next').
-- >8 --
Subject: [PATCH] color_parse_mem: allow empty color spec
Prior to c2f41bf52 (color.c: fix color_parse_mem() with
value_len == 0, 2017-01-19), the empty string was
interpreted as a color "reset". This was an accidental
outcome, and that commit turned it into an error.
However, scripts may pass the empty string as a default
value to "git config --get-color" to disable color when the
value is not defined. The git-add--interactive script does
this. As a result, the script is unusable since c2f41bf52
unless you have color.diff.plain defined (if it is defined,
then we don't parse the empty default at all).
Our test scripts didn't notice the recent breakage because
they run without a terminal, and thus without color. They
never hit this code path at all. And nobody noticed the
original buggy "reset" behavior, because it was effectively
a noop.
Let's fix the code to have an empty color name produce an
empty sequence of color codes. The tests need a few fixups:
- we'll add a new test in t4026 to cover this case. But
note that we need to tweak the color() helper. While
we're there, let's factor out the literal ANSI ESC
character. Otherwise it makes the diff quite hard to
read.
- we'll add a basic sanity-check in t4026 that "git add
-p" works at all when color is enabled. That would have
caught this bug, as well as any others that are specific
to the color code paths.
- 73c727d69 (log --graph: customize the graph lines with
config log.graphColors, 2017-01-19) added a test to
t4202 that checks some "invalid" graph color config.
Since ",, blue" before yielded only "blue" as valid, and
now yields "empty, empty, blue", we don't match the
expected output.
One way to fix this would be to change the expectation
to the empty color strings. But that makes the test much
less interesting, since we show only two graph lines,
both of which would be colorless.
Since the empty-string case is now covered by t4026,
let's remove them entirely here. They're just in the way
of the primary thing the test is supposed to be
checking.
Signed-off-by: Jeff King <peff@peff.net>
---
color.c | 6 ++++--
t/t3701-add-interactive.sh | 14 ++++++++++++++
t/t4026-color.sh | 7 ++++++-
t/t4202-log.sh | 2 +-
4 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/color.c b/color.c
index 7bb4a96f8..2925a819b 100644
--- a/color.c
+++ b/color.c
@@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)
len--;
}
- if (!len)
- return -1;
+ if (!len) {
+ dst[0] = '\0';
+ return 0;
+ }
if (!strncasecmp(ptr, "reset", len)) {
xsnprintf(dst, end - dst, GIT_COLOR_RESET);
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index deae948c7..5ffe78e92 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -380,4 +380,18 @@ test_expect_success 'patch mode ignores unmerged entries' '
test_cmp expected diff
'
+test_expect_success 'diffs can be colorized' '
+ git reset --hard &&
+
+ # force color even though the test script has no terminal
+ test_config color.ui always &&
+
+ echo content >test &&
+ printf y | git add -p >output 2>&1 &&
+
+ # We do not want to depend on the exact coloring scheme
+ # git uses for diffs, so just check that we saw some kind of color.
+ grep "$(printf "\\033")" output
+'
+
test_done
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index ec78c5e3a..671e951ee 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -6,10 +6,11 @@
test_description='Test diff/status color escape codes'
. ./test-lib.sh
+ESC=$(printf '\033')
color()
{
actual=$(git config --get-color no.such.slot "$1") &&
- test "$actual" = "^[$2"
+ test "$actual" = "${2:+$ESC}$2"
}
invalid_color()
@@ -21,6 +22,10 @@ test_expect_success 'reset' '
color "reset" "[m"
'
+test_expect_success 'empty color is empty' '
+ color "" ""
+'
+
test_expect_success 'attribute before color name' '
color "bold red" "[1;31m"
'
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 0aeabed96..1edbb1e7f 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -329,7 +329,7 @@ cat > expect.colors <<\EOF
EOF
test_expect_success 'log --graph with merge with log.graphColors' '
- test_config log.graphColors ",, blue,invalid-color, cyan, red , " &&
+ test_config log.graphColors " blue,invalid-color, cyan, red , " &&
git log --color=always --graph --date-order --pretty=tformat:%s |
test_decode_color | sed "s/ *\$//" >actual &&
test_cmp expect.colors actual
--
2.11.0.948.gca97da533
^ permalink raw reply related
* Re: [PATCH 1/5] add SWAP macro
From: Ramsay Jones @ 2017-02-01 0:44 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <676ed19c-0c4e-341e-ba30-1f4a23440088@web.de>
On 31/01/17 21:02, René Scharfe wrote:
[snip]
>> It would be trivially "optimized" out of the box, even when compiling with
>> Tiny C or in debug mode.
>
> Such a compiler is already slowed down by memset(3) calls for initializing objects and lack of other optimizations. I doubt a few more memcpy(3) calls would make that much of a difference.
>
> NB: git as compiled with TCC fails several tests, alas. Builds wickedly fast, though.
Hmm, a couple of years ago, I used tcc to build git and it worked
just fine (and it passed all tests). Prompted by this note, I just
built tcc from the current mob branch (@5420bb8) and built git OK,
but, yes, lots of tests now fail. :(
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] color_parse_mem: allow empty color spec
From: Jacob Keller @ 2017-02-01 1:19 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Git mailing list
In-Reply-To: <20170201002129.xb62hmxwrzwgp6vg@sigill.intra.peff.net>
On Tue, Jan 31, 2017 at 4:21 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 31, 2017 at 02:45:40PM -0800, Junio C Hamano wrote:
>
>> * nd/log-graph-configurable-colors (2017-01-23) 3 commits
>> (merged to 'next' on 2017-01-23 at c369982ad8)
>> + log --graph: customize the graph lines with config log.graphColors
>> + color.c: trim leading spaces in color_parse_mem()
>> + color.c: fix color_parse_mem() with value_len == 0
>>
>> Some people feel the default set of colors used by "git log --graph"
>> rather limiting. A mechanism to customize the set of colors has
>> been introduced.
>>
>> Reported to break "add -p".
>> cf. <20170128040709.tqx4u45ktgpkbfm4@sigill.intra.peff.net>
>
> I hadn't heard anything back, so I wrapped up my fix with a commit
> message and tests (it needs to go on top anyway, since the breakage is
> in 'next').
>
I was just about to report this breakage... It definitely breaks usage
of git-add-interactive..
> -- >8 --
> Subject: [PATCH] color_parse_mem: allow empty color spec
>
> Prior to c2f41bf52 (color.c: fix color_parse_mem() with
> value_len == 0, 2017-01-19), the empty string was
> interpreted as a color "reset". This was an accidental
> outcome, and that commit turned it into an error.
>
> However, scripts may pass the empty string as a default
> value to "git config --get-color" to disable color when the
> value is not defined. The git-add--interactive script does
> this. As a result, the script is unusable since c2f41bf52
> unless you have color.diff.plain defined (if it is defined,
> then we don't parse the empty default at all).
>
> Our test scripts didn't notice the recent breakage because
> they run without a terminal, and thus without color. They
> never hit this code path at all. And nobody noticed the
> original buggy "reset" behavior, because it was effectively
> a noop.
>
> Let's fix the code to have an empty color name produce an
> empty sequence of color codes. The tests need a few fixups:
>
> - we'll add a new test in t4026 to cover this case. But
> note that we need to tweak the color() helper. While
> we're there, let's factor out the literal ANSI ESC
> character. Otherwise it makes the diff quite hard to
> read.
>
> - we'll add a basic sanity-check in t4026 that "git add
> -p" works at all when color is enabled. That would have
> caught this bug, as well as any others that are specific
> to the color code paths.
>
> - 73c727d69 (log --graph: customize the graph lines with
> config log.graphColors, 2017-01-19) added a test to
> t4202 that checks some "invalid" graph color config.
> Since ",, blue" before yielded only "blue" as valid, and
> now yields "empty, empty, blue", we don't match the
> expected output.
>
> One way to fix this would be to change the expectation
> to the empty color strings. But that makes the test much
> less interesting, since we show only two graph lines,
> both of which would be colorless.
>
> Since the empty-string case is now covered by t4026,
> let's remove them entirely here. They're just in the way
> of the primary thing the test is supposed to be
> checking.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
The patch looks good to me. Very nice to have a detailed explanation
of why we didn't catch it before, and how to ensure we don't have this
happen again.
Thanks,
Jake
> color.c | 6 ++++--
> t/t3701-add-interactive.sh | 14 ++++++++++++++
> t/t4026-color.sh | 7 ++++++-
> t/t4202-log.sh | 2 +-
> 4 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/color.c b/color.c
> index 7bb4a96f8..2925a819b 100644
> --- a/color.c
> +++ b/color.c
> @@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)
> len--;
> }
>
> - if (!len)
> - return -1;
> + if (!len) {
> + dst[0] = '\0';
> + return 0;
> + }
>
> if (!strncasecmp(ptr, "reset", len)) {
> xsnprintf(dst, end - dst, GIT_COLOR_RESET);
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index deae948c7..5ffe78e92 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -380,4 +380,18 @@ test_expect_success 'patch mode ignores unmerged entries' '
> test_cmp expected diff
> '
>
> +test_expect_success 'diffs can be colorized' '
> + git reset --hard &&
> +
> + # force color even though the test script has no terminal
> + test_config color.ui always &&
> +
> + echo content >test &&
> + printf y | git add -p >output 2>&1 &&
> +
> + # We do not want to depend on the exact coloring scheme
> + # git uses for diffs, so just check that we saw some kind of color.
> + grep "$(printf "\\033")" output
> +'
> +
> test_done
> diff --git a/t/t4026-color.sh b/t/t4026-color.sh
> index ec78c5e3a..671e951ee 100755
> --- a/t/t4026-color.sh
> +++ b/t/t4026-color.sh
> @@ -6,10 +6,11 @@
> test_description='Test diff/status color escape codes'
> . ./test-lib.sh
>
> +ESC=$(printf '\033')
> color()
> {
> actual=$(git config --get-color no.such.slot "$1") &&
> - test "$actual" = " $2"
> + test "$actual" = "${2:+$ESC}$2"
> }
>
> invalid_color()
> @@ -21,6 +22,10 @@ test_expect_success 'reset' '
> color "reset" "[m"
> '
>
> +test_expect_success 'empty color is empty' '
> + color "" ""
> +'
> +
> test_expect_success 'attribute before color name' '
> color "bold red" "[1;31m"
> '
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 0aeabed96..1edbb1e7f 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -329,7 +329,7 @@ cat > expect.colors <<\EOF
> EOF
>
> test_expect_success 'log --graph with merge with log.graphColors' '
> - test_config log.graphColors ",, blue,invalid-color, cyan, red , " &&
> + test_config log.graphColors " blue,invalid-color, cyan, red , " &&
> git log --color=always --graph --date-order --pretty=tformat:%s |
> test_decode_color | sed "s/ *\$//" >actual &&
> test_cmp expect.colors actual
> --
> 2.11.0.948.gca97da533
>
^ permalink raw reply
* Re: git push failing when push.recurseSubmodules on-demand and git commit --amend was used in submodule.
From: Carlo Wood @ 2017-02-01 1:10 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Heiko Voigt, git@vger.kernel.org
In-Reply-To: <CAGZ79kbCfKVDq+9Pr5LmOtT=+uB+u+EMQg1=FUNS2umCvtvHhg@mail.gmail.com>
On Tue, 31 Jan 2017 14:08:41 -0800
Stefan Beller <sbeller@google.com> wrote:
> On Sun, Jan 29, 2017 at 5:00 PM, Junio C Hamano <gitster@pobox.com>
> wrote:
> > 2. If the amend is good and ready to go, "git add" to update the
> > superproject to make that amended result the one that is needed
> > in the submodule.
>
> yup.
But that is what I am doing. The amended commit IS already
added to the superproject (and pushed to the remote).
Please have a look at my script, this happens here:
# Correct that in the parent too:
pushd parent
git add subm
git commit -m 'Updated subm.'
popd
The commit from before the amend was added to the super
project (but never pushed) but has now been completely
replaced. I still think this is a flaw in git. It shouldn't
not complain and simply push.
--
Carlo Wood <carlo@alinoe.com>
^ permalink raw reply
* Re: git push failing when push.recurseSubmodules on-demand and git commit --amend was used in submodule.
From: Carlo Wood @ 2017-02-01 1:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Heiko Voigt, Stefan Beller, git
In-Reply-To: <xmqqvasxwee1.fsf@gitster.mtv.corp.google.com>
On Sun, 29 Jan 2017 17:00:22 -0800
Junio C Hamano <gitster@pobox.com> wrote:
> I suspect the submodule folks would say it is working as intended,
> if \
>
> - you made a commit in the submodule;
> - recorded the resulting commit in the superproject;
> - you amended the commit in the submodule; and then
> - you did "push, while pushing out in the submodule as needed" from
> the superproject.
This is not what I'm doing.
This is what I'm doing (see my script):
- you made a commit in the submodule;
- recorded the resulting commit in the superproject;
- you amended the commit in the submodule;
- you record the amended commit in the superproject; <=== !
- you push the submodule out (or not, the on-demand does that
anyway)
- you try to push the superproject, but that fails,
as long as you use recurseSubmodules=on-demand.
>
> There are two commits in the submodule that are involved in the
> above scenario, and the first one before amending is needed by the
> other participants of the project in order for them to check out
> what you are trying to push in the superproject, because that is
> what the superproject's tree records.
I never pushed anything of that, so the other participants don't
know, nor have, the pre-amended commit.
It is true that the superproject THINKS that the pre-amended commit
is a normal commit though: the last recorded (amended) commit is
internally listed as being on top of the amended commit (which is
incorrect). This is why the superproject assumes that the current
add commit of the submodule needs the pre-amended commit to be
available too. This is not correct however, it is not needed to
be available to others and does not need to be pushed to a remote.
> I think you have two options.
>
> 1. If the amend was done to improve things in submodule but is not
> quite ready, then get rid of that amended commit and restore the
> branch in the submodule to the state before you amended, i.e.
> the tip of the branch will become the same commit as the one
> that is recorded in the superproject. Then push the submodule
> and the superproject out. After that, move the submodule branch
> to point at the amended commit (or record the amended commit as
> a child of the commit you pushed out).
That would work, but would be a horrible workaround for an existing
bug :p
> 2. If the amend is good and ready to go, "git add" to update the
> superproject to make that amended result the one that is needed
> in the submodule.
This was already done, also in the script that I provided.
Yet, the push in the superproject is still rejected.
--
Carlo Wood <carlo@alinoe.com>
^ permalink raw reply
* Re: [git-users] More on that "merge branch checkout" problem -- cannot abort/go back?
From: Michael @ 2017-02-01 6:36 UTC (permalink / raw)
To: git; +Cc: git-users, Konstantin Khomoutov
In-Reply-To: <B621FC58-AA73-4D88-B5E5-575BC1A6F0EE@gmail.com>
This is going to the main git list. I had an issue with a git gui checkin, where I did a checkin of selected lines (no problem), then tried to switch branches and check in the rest.
Things broke.
I attempted to get help on the git users list, but was unable to.
To recap what happened:
1. I started on develop.
2. I made a feature branch.
3. I did some stuff. About half would be checked in on the feature branch, and about half on a "code cleanup" branch.
4. One hunk in the diff was a close comment (for the code cleanup) and then a bunch of new stuff (For the feature).
5. I used git gui to select what portions I wanted to check in.
6. In the past, using git gui to check in some lines of a hunk in one commit, and the other lines in a succeeding commit _on the same branch_ did not give me trouble. But this time, I was switching branches in between.
7. Git checkout complained about my changes being overwritten. git checkout -m did "work", but complained about a merge conflict.
8. There was no way to switch back -- there was no merge to abort, and no way to recover the previous state.
What I don't understand is:
1. Why is there no way to abort a merge checkout, and
2. Why was it that I could check in a portion of a hunk in git gui, but then not switch without a conflict? I'm not asking why there was a conflict as reported by diff -- I checked in the bottom half of a hunk, and kept "not checked in" the top half, so the context lines would not match. I'm asking more like, why wasn't git able to tell that the other part of the hunk was already checked in, and what I wanted to keep here was the stuff that was not checked in -- the "live git diff" which only showed the close comment in that part of the diff.
To clarify #2: The three versions in the conflict file were "nothing" (the old develop that had none of these changes), "what was checked in" (the feature branch), and "everything"; what I wanted was "everything - what was checked in" (which is what git diff reported before I switched branches).
On 2017-01-30, at 5:45 PM, Michael <keybounce@gmail.com> wrote:
>
> On 2017-01-29, at 11:32 PM, Konstantin Khomoutov <flatworm@users.sourceforge.net> wrote:
>
>> On Sun, 29 Jan 2017 11:07:34 -0800
>> Michael <keybounce@gmail.com> wrote:
>>
>>> So since my attempt to switch branches with the "merge" flag (-m)
>>> gave me an error, I thought I'd try to go back.
>>>
>>> keybounceMBP:Finite-Fluids michael$ git merge --abort
>>> fatal: There is no merge to abort (MERGE_HEAD missing).
>>> keybounceMBP:Finite-Fluids michael$ git status
>>> On branch cleanup
>>> Unmerged paths:
>>> (use "git reset HEAD <file>..." to unstage)
>>> (use "git add <file>..." to mark resolution)
>>>
>>> both modified:
>>> src/main/java/com/mcfht/realisticfluids/fluids/BlockFiniteFluid.java
>>>
>>> Changes not staged for commit:
>>> (use "git add <file>..." to update what will be committed)
>>> (use "git checkout -- <file>..." to discard changes in working
>>> directory)
>>>
>>> modified:
>>> src/main/java/com/mcfht/realisticfluids/FluidData.java modified:
>>> src/main/java/com/mcfht/realisticfluids/commands/CommandEnableFlow.java
>>> modified:
>>> src/main/java/com/mcfht/realisticfluids/fluids/BlockFiniteLava.java
>>>
>>> no changes added to commit (use "git add" and/or "git commit -a")
>>>
>>> I'm not saying this is necessarily wrong (the other branch does have
>>> everything, and I'm on a branch where I could just commit all these
>>> changes now, and worry about fixing it later; no data would be lost),
>>> but I do think that it breaks the user's reasonable beliefs about
>>> what the system does -- including the ability of a version control
>>> system to roll back to a prior state (which in this case it cannot --
>>> or if it can, I don't know how.)
>>
>> To cite the documentation:
>>
>> | -m, --merge
>> | When switching branches, if you have local modifications to
>> | one or more files that are different between the current branch and
>> | the branch to which you are switching, the command refuses to switch
>> | branches in order to preserve your modifications in context. However,
>> | with this option, a three-way merge between the current branch, your
>> | working tree contents, and the new branch is done, and you will be on
>> | the new branch.
>> |
>> | When a merge conflict happens, the index entries for conflicting
>> | paths are left unmerged, and you need to resolve the conflicts and
>> | mark the resolved paths with git add (or git rm if the merge should
>> | result in deletion of the path).
>> |
>> | When checking out paths from the index, this option lets you recreate
>> | the conflicted merge in the specified paths.
>>
>> So I'd say your best bet is to run
>>
>> git checkout --ours path/in/conflict
>>
>> on conflicting paths.
>>
>> As to your idea, I find it to be sensible but in fact you did not
>> attempt true merge, and hence asking for aborting it is dubious.
>>
>> Maybe `git checkout --abort` would have more sense? But then the
>> question is: what state should it roll your back to? Should it revert
>> switching on a new branch as well?
>>
>> Please consider bringing this question on the main Git list to solicit
>> insight of those with mad Git skillz. ;-)
>
> I still don't understand "--ours" or "--theirs" on checkout.
>
> I also am not sure that any of them are what I want. The three "versions" at the merge hunk were:
> 1. Nothing (this was the previous 'develop' branch tip)
> 2. The stuff that was added in the checkin on the other branch (this was the previous 'feature' branch tip)
> 3. Everything, the close comment AND the stuff added (this was the whole conflicted hunk)
>
> What I needed was basically "whole hunk - stuff checked in on feature". And since the file had other things that merged in just fine (an open comment earlier up), neither version (ours or theirs) could be the right version, even if I could restrict it to just the conflicted hunk.
>
> Where should it roll back to? Where I was before. There's a ref (I think it's MERGE_HEAD) created to allow you to abort a merge back to where you were, so why not a CHECKOUT_HEAD (or, if it's doing a 3-way merge, why not record the full merge information?)
---
Entertaining minecraft videos
http://YouTube.com/keybounce
^ permalink raw reply
* Re: [ANNOUNCE] Git Merge Contributor Summit topic planning
From: Erik van Zijst @ 2017-02-01 9:32 UTC (permalink / raw)
To: peff; +Cc: git, ssaasen, mheemskerk
In-Reply-To: <20170131004804.p5sule4rh2xrgtwe@sigill.intra.peff.net>
On Tue, Jan 31, 2017 at 01:48:05AM +0100, Jeff King wrote:
> The list of topics is totally open. If you're coming and have something
> you'd like to present or discuss, then propose it here. If you're _not_
> coming, you may still chime in with input on topics, but please don't
> suggest a topic unless somebody who is there will agree to lead the
> discussion.
I would like to talk about the possibility of CDN-aided cloning
operations as mentioned on this list earlier this week:
http://public-inbox.org/git/CADoxLGPFgF7W4XJzt0X+xFJDoN6RmfFGx_96MO9GPSSOjDK0EQ@mail.gmail.com/
At Bitbucket we have recently rolled out so-called clonebundle support
for Mercurial repositories.
Full clone operations are rather expensive on the server and are
responsible for a substantial part of our CPU and IO load. CDN-based
clonebundles have allowed us to eliminate most of this load for
Mercurial repos and we've since built a clonebundle spike for Git.
Clients performing a full clone get redirected to a CDN where they seed
their new local repo from a pre-built bundle file, and then pull/fetch
any remaining changes. Mercurial has had native, built-in support for
this for a while now.
I imagine other large code hosts could benefit from this as well and
I'd love to gauge the group's interest for this. Could this make sense
for Git? Would it have a chance of landing?
Our spike implements it as an optional capability during ref
advertisement. What are your thoughts on this?
Cheers,
Erik
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #06; Tue, 31)
From: Patrick Steinhardt @ 2017-02-01 11:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, psteinhardt
In-Reply-To: <xmqqpoj2q25n.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]
On Tue, Jan 31, 2017 at 02:45:40PM -0800, Junio C Hamano wrote:
> * ps/urlmatch-wildcard (2017-01-31) 5 commits
> . urlmatch: allow globbing for the URL host part
> . urlmatch: include host in urlmatch ranking
> . urlmatch: split host and port fields in `struct url_info`
> . urlmatch: enable normalization of URLs with globs
> . mailmap: add Patrick Steinhardt's work address
>
> The <url> part in "http.<url>.<variable>" configuration variable
> can now be spelled with '*' that serves as wildcard.
> E.g. "http.https://*.example.com.proxy" can be used to specify the
> proxy used for https://a.example.com, https://b.example.com, etc.,
> i.e. any host in the example.com domain.
>
> With the update it still seems to fail the same t5551#31
> cf. <cover.1485853153.git.ps@pks.im>
Yeah, you're right. Sorry about this one.
I finally fixed the problem to get t5551 working again, see the
attached patch below. The problem was that I did not honor the
case where multiple configuration entries exist with the same
key. In some cases, this has to lead to the value being
accumulated multiple times, as for example with http.extraheaders
used in t5551. So the fixup below fixes this.
I just tried to write additional tests to exercise this in our
config-tests by using `git config --get-urlmatch` with multiple
`http.extraheader`s set. I've been stumped that it still didn't
work correctly with my patch, only the last value was actually
ever returned. But in fact, current Git (tested with v2.11.0)
also fails to do so correctly and only lists the last value.
As such, I'd argue this is a separate issue. If you're not
opposed, I'd like to fix this in a separate patch series later on
(probably after Git Merge).
--- >8 ---
Subject: [PATCH] fixup! urlmatch: include host in urlmatch ranking
---
urlmatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/urlmatch.c b/urlmatch.c
index 6c12f1a48..739a1a558 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -587,7 +587,7 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
if (!item->util) {
item->util = xcalloc(1, sizeof(matched));
} else {
- if (cmp_matches(&matched, item->util) <= 0)
+ if (cmp_matches(&matched, item->util) < 0)
/*
* Our match is worse than the old one,
* we cannot use it.
--
2.11.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-02-01 11:28 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Brandon Williams, Johannes Sixt, Git List,
Junio C Hamano
In-Reply-To: <20170131213507.uiwmkkcg7umvd3f4@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]
Hi Peff,
On Tue, 31 Jan 2017, Jeff King wrote:
> On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote:
>
> > > Perhaps we could disallow a side-effect operator in the macro. By
> > > disallow I mean place a comment at the definition to the macro and
> > > hopefully catch something like that in code-review. We have the
> > > same issue with the `ALLOC_GROW()` macro.
> >
> > SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.
> > Technically that should be enough. :) A comment wouldn't hurt, of
> > course.
>
> One funny thing is that your macro takes address-of itself, behind the
> scenes. I wonder if it would be more natural for it to take
> pointers-to-objects, making it look more like a real function (i.e.,
> SWAP(&a, &b) instead of SWAP(a, b)". And then these funny corner cases
> become quite obvious in the caller, because the caller is the one who
> has to type "&".
But forcing SWAP(&a, &b) would make it even more cumbersome to use, and it
would also make it harder to optimize, say, by using registers instead of
addressable memory (think swapping two 32-bit integers: there is *no* need
to write them into memory just to swap them).
And I think I should repeat my point that this discussion veers towards
making simple swaps *more* complicated, rather than less complicated. Bad
direction.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-02-01 11:39 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <676ed19c-0c4e-341e-ba30-1f4a23440088@web.de>
[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]
Hi René,
On Tue, 31 Jan 2017, René Scharfe wrote:
> Am 31.01.2017 um 13:13 schrieb Johannes Schindelin:
>
> > #define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)
> > ...
> > uint32_t large;
> > char nybble;
> >
> > ...
> >
> > if (!(large & ~0xf)) {
> > SIMPLE_SWAP(char, nybble, large);
> > ...
> > }
> >
> > i.e. mixing types, when possible.
> >
> > And while I do not necessarily expect that we need anything like this
> > anytime soon, merely the fact that it allows for this flexibility, while
> > being very readable at the same time, would make it a pretty good design
> > in my book.
>
> Such a skinny macro which only hides repetition is kind of attractive due to
> its simplicity; I can't say the same about the mixed type example above,
> though.
>
> The fat version isn't that bad either even without inlining, includes a few
> safety checks and doesn't require us to tell the compiler something it
> already knows very well. I'd rather let the machine do the work.
I am a big fan of letting the machine do the work. But I am not a big fan
of *creating* work for the machine.
So if you asked me what I would think of a script that, given a patch "in
mbox format", automatically fixes all formatting issues that typically
take up a sizable chunk of review time, I would say: yeah, let's get this
done! It would probably take away some fun because then reviewers could
bike-shed less, but I'd think that is a good thing.
If you asked me what my opinion is about a program you wrote that gathers
all the threads and sub threads of code^Wpatch reviews on the Git mailing
list, and cross-references them with public Git branches, and with Junio's
What's Cooking mail, and with the SHA-1s in `pu`: Great! That would
relieve me of a ton of really boring and grueling work, if the machine can
do it, all the better.
And if you ask me about adding a complex macro that adds a bunch of work
for the C compiler just to produce the same assembler code as a one-liner
macro would have produced much easier, I will reply that we could maybe
instead spend that time on letting the machine perform tasks that already
need to be done... :-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Jeff King @ 2017-02-01 11:47 UTC (permalink / raw)
To: Johannes Schindelin
Cc: René Scharfe, Brandon Williams, Johannes Sixt, Git List,
Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1702011225250.3469@virtualbox>
On Wed, Feb 01, 2017 at 12:28:13PM +0100, Johannes Schindelin wrote:
> > One funny thing is that your macro takes address-of itself, behind the
> > scenes. I wonder if it would be more natural for it to take
> > pointers-to-objects, making it look more like a real function (i.e.,
> > SWAP(&a, &b) instead of SWAP(a, b)". And then these funny corner cases
> > become quite obvious in the caller, because the caller is the one who
> > has to type "&".
>
> But forcing SWAP(&a, &b) would make it even more cumbersome to use, and it
> would also make it harder to optimize, say, by using registers instead of
> addressable memory (think swapping two 32-bit integers: there is *no* need
> to write them into memory just to swap them).
I don't find the register thing compelling at all. I'd expect modern
compilers to optimize *(&a) into just "a" and keep using a register. I'm
sure there are compilers that don't, but I'm also not sure how much we
_care_. If your compiler doesn't do basic micro-optimizations, then you
live with it or get a better compiler.
I'm much more interested in what's readable and maintainable, versus
trying to micro-optimize something that hasn't even been shown to be
measurably interesting.
That said...
> And I think I should repeat my point that this discussion veers towards
> making simple swaps *more* complicated, rather than less complicated. Bad
> direction.
I'm not altogether convinced that SWAP() is an improvement in
readability. I really like that it's shorter than the code it replaces,
but there is a danger with introducing opaque constructs. It's one more
thing for somebody familiar with C but new to the project to learn or
get wrong.
IMHO the main maintenance gain from René's patch is that you don't have
to specify the type, which means you can never have a memory-overflow
bug due to incorrect types. If we lose that, then I don't see all that
much value in the whole thing.
-Peff
^ permalink raw reply
* [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-02-01 11:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1485442231.git.johannes.schindelin@gmx.de>
We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.
For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.
This is error-prone and a bad user experience.
To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.
Changes relative to v2:
- touched up the documentation for ssh.variant to make it even easier to
understand
- free()d the config variable
- completely refactored the code to fulfil Junio's burning desire to
avoid split_cmdline() when unnecessary
It is quite preposterous to call this an "iteration" of the patch
series, because the code is so different now. I say this because I want
to caution that this code has not been tested as thoroughly, by far, as
the first iteration.
The primary purpose of code review is correctness, everything else is
either a consequence of it, or a means to make reviewing easier.
That means that I highly encourage those who pushed for these extensive
changes that make the patch series a lot less robust to balance things
out by at least *rudimentary* testing.
Johannes Schindelin (1):
git_connect(): factor out SSH variant handling
Junio C Hamano (1):
connect: rename tortoiseplink and putty variables
Segev Finer (2):
connect: handle putty/plink also in GIT_SSH_COMMAND
connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
Documentation/config.txt | 11 +++++++
Documentation/git.txt | 6 ++++
connect.c | 75 ++++++++++++++++++++++++++++++++++++------------
t/t5601-clone.sh | 41 ++++++++++++++++++++++++++
4 files changed, 114 insertions(+), 19 deletions(-)
base-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3
Interdiff vs v2:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f2c210f0a0..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
ssh.variant::
- Override the autodetection of plink/tortoiseplink in the SSH
- command that 'git fetch' and 'git push' use. It can be set to
- either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
- value will be treated as normal ssh. This is useful in case
- that Git gets this wrong.
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c322558aa7..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation
for further details.
`GIT_SSH_VARIANT`::
- If this environment variable is set, it overrides the autodetection
- of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
- push' use. It can be set to either `ssh`, `plink`, `putty` or
- `tortoiseplink`. Any other value will be treated as normal ssh. This
- is useful in case that Git gets this wrong.
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
diff --git a/connect.c b/connect.c
index 7b4437578b..7f1f802396 100644
--- a/connect.c
+++ b/connect.c
@@ -691,20 +691,45 @@ static const char *get_ssh_command(void)
return NULL;
}
-static int handle_ssh_variant(int *port_option, int *needs_batch)
+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
{
- const char *variant;
+ const char *variant = getenv("GIT_SSH_VARIANT");
+ char *p = NULL;
+
+ if (variant)
+ ; /* okay, fall through */
+ else if (!git_config_get_string("ssh.variant", &p))
+ variant = p;
+ else if (!is_cmdline) {
+ p = xstrdup(ssh_command);
+ variant = basename(p);
+ } else {
+ const char **ssh_argv;
- if (!(variant = getenv("GIT_SSH_VARIANT")) &&
- git_config_get_string_const("ssh.variant", &variant))
- return 0;
+ p = xstrdup(ssh_command);
+ if (split_cmdline(p, &ssh_argv)) {
+ variant = basename((char *)ssh_argv[0]);
+ /*
+ * At this point, variant points into the buffer
+ * referenced by p, hence we do not need ssh_argv
+ * any longer.
+ */
+ free(ssh_argv);
+ } else
+ return 0;
+ }
- if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+ if (!strcasecmp(variant, "plink") ||
+ !strcasecmp(variant, "plink.exe") ||
+ !strcasecmp(variant, "putty"))
*port_option = 'P';
- else if (!strcmp(variant, "tortoiseplink")) {
+ else if (!strcasecmp(variant, "tortoiseplink") ||
+ !strcasecmp(variant, "tortoiseplink.exe")) {
*port_option = 'P';
*needs_batch = 1;
}
+ free(p);
return 1;
}
@@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url,
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
- char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (ssh) {
- char *split_ssh = xstrdup(ssh);
- const char **ssh_argv;
-
- if (split_cmdline(split_ssh, &ssh_argv))
- ssh_argv0 = xstrdup(ssh_argv[0]);
- free(split_ssh);
- free((void *)ssh_argv);
- } else {
+ if (ssh)
+ handle_ssh_variant(ssh, 1, &port_option,
+ &needs_batch);
+ else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh = getenv("GIT_SSH");
if (!ssh)
ssh = "ssh";
-
- ssh_argv0 = xstrdup(ssh);
+ else
+ handle_ssh_variant(ssh, 0,
+ &port_option,
+ &needs_batch);
}
- if (!handle_ssh_variant(&port_option, &needs_batch) &&
- ssh_argv0) {
- const char *base = basename(ssh_argv0);
-
- if (!strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe")) {
- port_option = 'P';
- needs_batch = 1;
- } else if (!strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe")) {
- port_option = 'P';
- }
- }
-
- free(ssh_argv0);
-
argv_array_push(&conn->args, ssh);
if (flags & CONNECT_IPV4)
argv_array_push(&conn->args, "-4");
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply
* [PATCH v3 1/4] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>
From: Segev Finer <segev208@gmail.com>
Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.
However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).
When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.
This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.
Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 ++++++++++++++++-------
t/t5601-clone.sh | 15 +++++++++++++++
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,
int putty = 0, tortoiseplink = 0;
char *ssh_host = hostandport;
const char *port = NULL;
+ char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (!ssh) {
- const char *base;
- char *ssh_dup;
-
+ if (ssh) {
+ char *split_ssh = xstrdup(ssh);
+ const char **ssh_argv;
+
+ if (split_cmdline(split_ssh, &ssh_argv))
+ ssh_argv0 = xstrdup(ssh_argv[0]);
+ free(split_ssh);
+ free((void *)ssh_argv);
+ } else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,
if (!ssh)
ssh = "ssh";
- ssh_dup = xstrdup(ssh);
- base = basename(ssh_dup);
+ ssh_argv0 = xstrdup(ssh);
+ }
+
+ if (ssh_argv0) {
+ const char *base = basename(ssh_argv0);
tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
!strcasecmp(base, "tortoiseplink.exe");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,
!strcasecmp(base, "plink") ||
!strcasecmp(base, "plink.exe");
- free(ssh_dup);
+ free(ssh_argv0);
}
argv_array_push(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '
expect_ssh "-batch -P 123" myhost src
'
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+ GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+ git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+ expect_ssh "-v -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v3 2/4] connect: rename tortoiseplink and putty variables
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>
From: Junio C Hamano <gitster@pobox.com>
One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY". It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.
Rename them after what effect is desired. The current "putty"
option is about using "-P <port>" when OpenSSH would use "-p <port>",
so rename it to port_option whose value is either 'p' or 'P". The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.
[jes: wrapped overly-long line]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char *url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
- int putty = 0, tortoiseplink = 0;
+ int needs_batch = 0;
+ int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
- tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe");
- putty = tortoiseplink ||
- !strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe");
-
+ if (!strcasecmp(base, "tortoiseplink") ||
+ !strcasecmp(base, "tortoiseplink.exe")) {
+ port_option = 'P';
+ needs_batch = 1;
+ } else if (!strcasecmp(base, "plink") ||
+ !strcasecmp(base, "plink.exe")) {
+ port_option = 'P';
+ }
free(ssh_argv0);
}
@@ -833,11 +836,11 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
argv_array_push(&conn->args, "-batch");
if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(&conn->args, putty ? "-P" : "-p");
+ argv_array_pushf(&conn->args,
+ "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v3 3/4] git_connect(): factor out SSH variant handling
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>
We handle plink and tortoiseplink as OpenSSH replacements, by passing
the correct command-line options when detecting that they are used.
To let users override that auto-detection (in case Git gets it wrong),
we need to introduce new code to that end.
In preparation for this code, let's factor out the SSH variant handling
into its own function, handle_ssh_variant().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
connect.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/connect.c b/connect.c
index 9f750eacb6..2734b9a1ca 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,44 @@ static const char *get_ssh_command(void)
return NULL;
}
+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
+{
+ const char *variant;
+ char *p = NULL;
+
+ if (!is_cmdline) {
+ p = xstrdup(ssh_command);
+ variant = basename(p);
+ } else {
+ const char **ssh_argv;
+
+ p = xstrdup(ssh_command);
+ if (split_cmdline(p, &ssh_argv)) {
+ variant = basename((char *)ssh_argv[0]);
+ /*
+ * At this point, variant points into the buffer
+ * referenced by p, hence we do not need ssh_argv
+ * any longer.
+ */
+ free(ssh_argv);
+ } else
+ return 0;
+ }
+
+ if (!strcasecmp(variant, "plink") ||
+ !strcasecmp(variant, "plink.exe"))
+ *port_option = 'P';
+ else if (!strcasecmp(variant, "tortoiseplink") ||
+ !strcasecmp(variant, "tortoiseplink.exe")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ }
+ free(p);
+
+ return 1;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -773,7 +811,6 @@ struct child_process *git_connect(int fd[2], const char *url,
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
- char *ssh_argv0 = NULL;
transport_check_allowed("ssh");
get_host_and_port(&ssh_host, &port);
@@ -794,15 +831,10 @@ struct child_process *git_connect(int fd[2], const char *url,
}
ssh = get_ssh_command();
- if (ssh) {
- char *split_ssh = xstrdup(ssh);
- const char **ssh_argv;
-
- if (split_cmdline(split_ssh, &ssh_argv))
- ssh_argv0 = xstrdup(ssh_argv[0]);
- free(split_ssh);
- free((void *)ssh_argv);
- } else {
+ if (ssh)
+ handle_ssh_variant(ssh, 1, &port_option,
+ &needs_batch);
+ else {
/*
* GIT_SSH is the no-shell version of
* GIT_SSH_COMMAND (and must remain so for
@@ -813,22 +845,10 @@ struct child_process *git_connect(int fd[2], const char *url,
ssh = getenv("GIT_SSH");
if (!ssh)
ssh = "ssh";
-
- ssh_argv0 = xstrdup(ssh);
- }
-
- if (ssh_argv0) {
- const char *base = basename(ssh_argv0);
-
- if (!strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe")) {
- port_option = 'P';
- needs_batch = 1;
- } else if (!strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe")) {
- port_option = 'P';
- }
- free(ssh_argv0);
+ else
+ handle_ssh_variant(ssh, 0,
+ &port_option,
+ &needs_batch);
}
argv_array_push(&conn->args, ssh);
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: git; +Cc: Segev Finer, Junio C Hamano, Jeff King
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>
From: Segev Finer <segev208@gmail.com>
This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.
[jes: wrapped overly-long lines, factored out and changed
get_ssh_variant() to handle_ssh_variant() to accomodate the
change from the putty/tortoiseplink variables to
port_option/needs_batch, adjusted the documentation, free()d
value obtained from the config.]
Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/config.txt | 11 +++++++++++
Documentation/git.txt | 6 ++++++
connect.c | 11 ++++++++---
t/t5601-clone.sh | 26 ++++++++++++++++++++++++++
4 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,17 @@ Environment variable settings always override any matches. The URLs that are
matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
+ssh.variant::
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
+
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,12 @@ Usually it is easier to configure any desired options through your
personal `.ssh/config` file. Please consult your ssh documentation
for further details.
+`GIT_SSH_VARIANT`::
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
+
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 2734b9a1ca..7f1f802396 100644
--- a/connect.c
+++ b/connect.c
@@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
int *port_option, int *needs_batch)
{
- const char *variant;
+ const char *variant = getenv("GIT_SSH_VARIANT");
char *p = NULL;
- if (!is_cmdline) {
+ if (variant)
+ ; /* okay, fall through */
+ else if (!git_config_get_string("ssh.variant", &p))
+ variant = p;
+ else if (!is_cmdline) {
p = xstrdup(ssh_command);
variant = basename(p);
} else {
@@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
}
if (!strcasecmp(variant, "plink") ||
- !strcasecmp(variant, "plink.exe"))
+ !strcasecmp(variant, "plink.exe") ||
+ !strcasecmp(variant, "putty"))
*port_option = 'P';
else if (!strcasecmp(variant, "tortoiseplink") ||
!strcasecmp(variant, "tortoiseplink.exe")) {
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
expect_ssh "-v -P 123" myhost src
'
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ GIT_SSH_VARIANT=ssh \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+ copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+ git -c ssh.variant=ssh \
+ clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+ expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+ GIT_SSH_VARIANT=plink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+ expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+ GIT_SSH_VARIANT=tortoiseplink \
+ git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+ expect_ssh "-batch -P 123" myhost src
+'
+
# Reset the GIT_SSH environment variable for clone tests.
setup_ssh_wrapper
--
2.11.1.windows.prerelease.2.9.g3014b57
^ permalink raw reply related
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Johannes Schindelin @ 2017-02-01 12:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqqpoj8z7su.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 27 Jan 2017, Junio C Hamano wrote:
> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
> tokens before we realize that the user used the escape hatch and the
> splitting was a wasted effort. This is exactly because this thing
> is an escape hatch that is expected to be rarely used. Of course,
> if the "wasted effort" can be eliminated without sacrificing the
> simplicity of the code, that is fine as well.
Simplicity is retained. Battle-readiness was sacrificed on the way: the
new code is not tested well enough, and `next` will not help one bit.
Ciao,
Johannes
^ permalink raw reply
* Re: [ANNOUNCE] Git Merge Contributor Summit topic planning
From: Jeff King @ 2017-02-01 14:53 UTC (permalink / raw)
To: Erik van Zijst; +Cc: git, ssaasen, mheemskerk
In-Reply-To: <1485941532-47993-1-git-send-email-erik.van.zijst@gmail.com>
On Wed, Feb 01, 2017 at 10:32:12AM +0100, Erik van Zijst wrote:
> Clients performing a full clone get redirected to a CDN where they seed
> their new local repo from a pre-built bundle file, and then pull/fetch
> any remaining changes. Mercurial has had native, built-in support for
> this for a while now.
>
> I imagine other large code hosts could benefit from this as well and
> I'd love to gauge the group's interest for this. Could this make sense
> for Git? Would it have a chance of landing?
>
> Our spike implements it as an optional capability during ref
> advertisement. What are your thoughts on this?
I think this is definitely an interesting topic to discuss tomorrow.
Here are a few observations from my past thinking on the issue. I
haven't read the proposal from earlier this week yet, so some of them
may be obsolete.
Seeding from a bundle CDN generally solves two problems: getting the
bulk of the data from someplace with higher bandwidth (the CDN), and
getting the bulk of the data over a protocol that can be resumed (the
bundle).
But we don't necessarily have to solve both problems simultaneously.
And you might not want to. Storing a separate bundle on another server
is complicated to configure, and doubles the amount of disk space you
need (just half of it is on the CDN). Using a bundle means you can't
seed from a non-bundle source.
So for any solution, I'd want to consider how you can put together the
pieces. Can you seed from a non-bundle? Can you seed from yourself and
just get resumability? If so, how hard is it to serve a pseudo-bundle
based on the packfiles you have on disk (i.e., getting resumability
at least in the common cases without paying the disk cost). I.e., saving
enough data that you could reconstruct the bundle byte-for-byte when you
need to.
If you _can_ do that latter part, and you take "I only care about
resumability" to the simplest extreme, you'd probably end up with a
protocol more like:
Client: I need a packfile with this want/have
Server: OK, here it is; its opaque id is XYZ.
... connection interrupted ...
Client: It's me again. I have up to byte N of pack XYZ
Server: OK, resuming
[or: I don't have XYZ anymore; start from scratch]
Then generating XYZ and generating that bundle are basically the same
task.
All just food for thought. I look forward to digging into it more on the
list and in the in-person discussion.
-Peff
^ permalink raw reply
* Re: [PATCH v2 7/7] completion: recognize more long-options
From: Cornelius Weig @ 2017-02-01 16:49 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: j6t, spearce, git
In-Reply-To: <CAM0VKj=Ein4yrKG2aZnN7JU80ctZBQromcR6BEu-TyMLenLFCg@mail.gmail.com>
Hi Gabor,
thanks for taking a look at these commits.
On 01/31/2017 11:17 PM, SZEDER Gábor wrote:
> On Fri, Jan 27, 2017 at 10:17 PM, <cornelius.weig@tngtech.com> wrote:
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>>
>> Recognize several new long-options for bash completion in the following
>> commands:
>
> Adding more long options that git commands learn along the way is
> always an improvement. However, seeing "_several_ new long options"
> (or "some long options" in one of the other patches in the series)
> makes the reader wonder: are these the only new long options missing
> or are there more? If there are more, why only these are added? If
> there aren't any more missing long options left, then please say so,
> e.g. "Add all missing long options, except the potentially
> desctructive ones, for the following commands: ...."
Personally, I agree with you that
> Adding more long options that git commands learn along the way is
> always an improvement.
However, people may start complaining that their terminal becomes too
cluttered when doing a double-Tab. In my cover letter, I go to length
about this. My assumption was that all options that are mentioned in the
introduction of the command man-page should be important enough to have
them in the completion list. I'll change my commit message accordingly.
>> - rm: --force
>
> '--force' is a potentially destructive option, too.
Thanks for spotting this.
Btw, I haven't found that non-destructive options should not be eligible
for completion. To avoid confusion about this in the future, I suggest
to also change the documentation:
index 933bb6e..96f1c7f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -13,7 +13,7 @@
# *) git email aliases for git-send-email
# *) tree paths within 'ref:path/to/file' expressions
# *) file paths within current working directory and index
-# *) common --long-options
+# *) common non-destructive --long-options
#
# To use these routines:
#
I take it you have also looked at the code itself? Then I would gladly
mention you as reviewer in my sign-off.
^ permalink raw reply related
* Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-02-01 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1702011216130.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Fri, 27 Jan 2017, Junio C Hamano wrote:
>
>> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
>> tokens before we realize that the user used the escape hatch and the
>> splitting was a wasted effort. This is exactly because this thing
>> is an escape hatch that is expected to be rarely used. Of course,
>> if the "wasted effort" can be eliminated without sacrificing the
>> simplicity of the code, that is fine as well.
>
> Simplicity is retained. Battle-readiness was sacrificed on the way: the
> new code is not tested well enough, and `next` will not help one bit.
Let me make it clear that there is no burning desire to sacrifice
battle-readiness in the above. If we expect that auto-detection
would be minority, then it makes sense to get the configured value
first and then spend cycles to split and guess only when detection
is needed.
In this case, because we expect that auto-detection will be used
most of the time, it is good enough to always split first, get the
configured value, and spend cycles to guess, or for that matter it
is perfectly fine to always split and guess first and then override
with the configured value.
If your attempt to optimize for a wrong case ended up causing new
unnecessary bugs, don't blame me.
^ permalink raw reply
* Re: [ANNOUNCE] Git Merge Contributor Summit topic planning
From: Junio C Hamano @ 2017-02-01 18:06 UTC (permalink / raw)
To: Jeff King; +Cc: Erik van Zijst, git, ssaasen, mheemskerk
In-Reply-To: <20170201145300.4pn3faodhdb72jly@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If you _can_ do that latter part, and you take "I only care about
> resumability" to the simplest extreme, you'd probably end up with a
> protocol more like:
>
> Client: I need a packfile with this want/have
> Server: OK, here it is; its opaque id is XYZ.
> ... connection interrupted ...
> Client: It's me again. I have up to byte N of pack XYZ
> Server: OK, resuming
> [or: I don't have XYZ anymore; start from scratch]
>
> Then generating XYZ and generating that bundle are basically the same
> task.
The above allows a simple and naive implementation of generating a
packstream and "tee"ing it to a spool file to be kept while sending
to the first client that asks XYZ.
The story I heard from folks who run git servers at work for Android
and other projects, however, is that they rarely see two requests
with want/have that result in an identical XYZ, unless "have" is an
empty set (aka "clone"). In a busy repository, between two clone
requests relatively close together, somebody would be pushing, so
you'd need many XYZs in your spool even if you want to support only
the "clone" case.
So in the real life, I think that the exchange needs to be more
like this:
C: I need a packfile with this want/have
... C/S negotiate what "have"s are common ...
S: Sorry, but our negitiation indicates that you are way too
behind. I'll send you a packfile that brings you up to a
slightly older set of "want", so pretend that you asked for
these slightly older "want"s instead. The opaque id of that
packfile is XYZ. After getting XYZ, come back to me with
your original set of "want"s. You would give me more recent
"have" in that request.
... connection interrupted ...
C: It's me again. I have up to byte N of pack XYZ
S: OK, resuming (or: I do not have it anymore, start from scratch)
... after 0 or more iterations C fully receives and digests XYZ ...
and then the above will iterate until the server does not have to
say "Sorry but you are way too behind" and returns a packfile
without having to tweak the "want".
That way, you can limit the number of XYZ you would need to keep to
a reasonable number.
The recent proposal by Jonathan Tan also allows the server side to
tweak the final tips the client receives after the protocol exchange
started. I suspect the above two will become related.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-02-01 18:06 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin
Cc: Brandon Williams, Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <20170201114750.r5xdy6emdczmnh4j@sigill.intra.peff.net>
Am 01.02.2017 um 12:47 schrieb Jeff King:
> I'm not altogether convinced that SWAP() is an improvement in
> readability. I really like that it's shorter than the code it replaces,
> but there is a danger with introducing opaque constructs. It's one more
> thing for somebody familiar with C but new to the project to learn or
> get wrong.
I'm biased, of course, but SIMPLE_SWAP and SWAP exchange the values of
two variables -- how could someone get that wrong? Would it help to
name the macro SWAP_VALUES? Or XCHG? ;)
> IMHO the main maintenance gain from René's patch is that you don't have
> to specify the type, which means you can never have a memory-overflow
> bug due to incorrect types. If we lose that, then I don't see all that
> much value in the whole thing.
Size checks could be added to SIMPLE_SWAP as well.
The main benefit of a swap macro is reduced repetition IMHO: Users
specify the variables to swap once instead of twice in a special order,
and with SWAP they don't need to declare their type again. Squeezing
out redundancy makes the code easier to read and modify.
René
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Junio C Hamano @ 2017-02-01 18:33 UTC (permalink / raw)
To: René Scharfe
Cc: Jeff King, Johannes Schindelin, Brandon Williams, Johannes Sixt,
Git List
In-Reply-To: <bfd0d758-a9c8-9792-6294-9f9ed632cc98@web.de>
René Scharfe <l.s.r@web.de> writes:
> Size checks could be added to SIMPLE_SWAP as well.
Between SWAP() and SIMPLE_SWAP() I am undecided.
If the compiler can infer the type and the size of the two
"locations" given to the macro, there is no technical reason to
require the caller to specify the type as an extra argument, so
SIMPLE_SWAP() may not necessarily an improvement over SWAP() from
that point of view. If the redundancy is used as a sanity check,
I'd be in favor of SIMPLE_SWAP(), though.
If the goal of SIMPLE_SWAP(), on the other hand, were to support the
"only swap char with int for small value" example earlier in the
thread, it means you cannot sanity check the type of things being
swapped in the macro, and the readers of the code need to know about
the details of what variables are being swapped. It looks to me
that it defeats the main benefit of using a macro.
> The main benefit of a swap macro is reduced repetition IMHO: Users
> specify the variables to swap once instead of twice in a special
> order, and with SWAP they don't need to declare their type again.
> Squeezing out redundancy makes the code easier to read and modify.
Yes.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #06; Tue, 31)
From: Junio C Hamano @ 2017-02-01 18:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, psteinhardt
In-Reply-To: <20170201110851.GA475@pks-pc>
Patrick Steinhardt <ps@pks.im> writes:
> I just tried to write additional tests to exercise this in our
> config-tests by using `git config --get-urlmatch` with multiple
> `http.extraheader`s set. I've been stumped that it still didn't
> work correctly with my patch, only the last value was actually
> ever returned.
I think that is very much in line with "git config --get" works, and
"--get-urlmatch" being an enhancement of "--get", I would expect
that "--get-urlmatch" to follow the usual "the last one wins" rule.
If you want to see _all_ values for a multi-valued variable, you
would say "git config --get-all". IIUC, there currently is no
"--get-all-urlmatch", but there may need one. I dunno.
^ permalink raw reply
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-02-01 19:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <9780d67c9f11c056202987377c542d0313772ba2.1485950225.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> index 2734b9a1ca..7f1f802396 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
> static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> int *port_option, int *needs_batch)
> {
> - const char *variant;
> + const char *variant = getenv("GIT_SSH_VARIANT");
> char *p = NULL;
>
> - if (!is_cmdline) {
> + if (variant)
> + ; /* okay, fall through */
> + else if (!git_config_get_string("ssh.variant", &p))
> + variant = p;
> + else if (!is_cmdline) {
> p = xstrdup(ssh_command);
> variant = basename(p);
> } else {
> @@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> }
>
> if (!strcasecmp(variant, "plink") ||
> - !strcasecmp(variant, "plink.exe"))
> + !strcasecmp(variant, "plink.exe") ||
> + !strcasecmp(variant, "putty"))
This means that "putty" that appear as the first word of the command
line, not the value configured for ssh.variant or GIT_SSH_VARIANT,
will also trigger the option for "plink", no?
Worse, "plink.exe" configured as the value of "ssh.variant", is
anything other than 'ssh', 'plink', 'putty', or 'tortoiseplink', but
it is not treated as normal ssh, contrary to the documentation.
> +ssh.variant::
> + Depending on the value of the environment variables `GIT_SSH` or
> + `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
> + auto-detects whether to adjust its command-line parameters for use
> + with plink or tortoiseplink, as opposed to the default (OpenSSH).
> ++
> +The config variable `ssh.variant` can be set to override this auto-detection;
> +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
> +will be treated as normal ssh. This setting can be overridden via the
> +environment variable `GIT_SSH_VARIANT`.
I was hoping that the "rewrite that retains simplicity" was also
correct, but let's not go in this direction. The more lines you
touch in a hurry, the worse the code will get, and that is to be
expected.
I'd suggest taking the documentation updates from this series, and
then minimally plug the leak introduced by the previous round,
perhaps like the attached SQUASH. As I said, GIT_SSH_VARIANT and
ssh.variant are expected to be rare cases, and the case in which
they are set does not have to be optimized if it makes the necessary
change smaller and more likely to be correct.
I think restructuring along the line of 3/4 of this round is very
sensible in the longer term (if anything, handle_ssh_variant() now
really handles ssh variants in all cases, which makes it worthy of
its name, as opposed to the one in the previous round that only
reacts to the overrides). But it seems that it will take longer to
get such a rewrite right, and my priority is seeing this topic to
add autodetection to GIT_SSH_COMMAND and other smaller topics in the
upcoming -rc0 in a serviceable and correct shape.
The restructuring done by 3/4 can come later after the dust settles,
if somebody cares deeply enough about performance in the rare cases.
Documentation/config.txt | 14 +++++++++-----
Documentation/git.txt | 9 ++++-----
connect.c | 9 +++++----
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f2c210f0a0..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs
visited as a result of a redirection do not participate in matching.
ssh.variant::
- Override the autodetection of plink/tortoiseplink in the SSH
- command that 'git fetch' and 'git push' use. It can be set to
- either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
- value will be treated as normal ssh. This is useful in case
- that Git gets this wrong.
+ Depending on the value of the environment variables `GIT_SSH` or
+ `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+ auto-detects whether to adjust its command-line parameters for use
+ with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c322558aa7..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation
for further details.
`GIT_SSH_VARIANT`::
- If this environment variable is set, it overrides the autodetection
- of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
- push' use. It can be set to either `ssh`, `plink`, `putty` or
- `tortoiseplink`. Any other value will be treated as normal ssh. This
- is useful in case that Git gets this wrong.
+ If this environment variable is set, it overrides Git's autodetection
+ whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+ plink or tortoiseplink. This variable overrides the config setting
+ `ssh.variant` that serves the same purpose.
`GIT_ASKPASS`::
If this environment variable is set, then Git commands which need to
diff --git a/connect.c b/connect.c
index 7b4437578b..da51fef9ee 100644
--- a/connect.c
+++ b/connect.c
@@ -693,10 +693,11 @@ static const char *get_ssh_command(void)
static int handle_ssh_variant(int *port_option, int *needs_batch)
{
- const char *variant;
+ char *variant;
- if (!(variant = getenv("GIT_SSH_VARIANT")) &&
- git_config_get_string_const("ssh.variant", &variant))
+ variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+ if (!variant &&
+ git_config_get_string("ssh.variant", &variant))
return 0;
if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
@@ -705,7 +706,7 @@ static int handle_ssh_variant(int *port_option, int *needs_batch)
*port_option = 'P';
*needs_batch = 1;
}
-
+ free(variant);
return 1;
}
^ permalink raw reply related
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