* 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
* Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config
From: Junio C Hamano @ 2017-02-01 19:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer, Jeff King
In-Reply-To: <xmqq1svhpvm0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
Having said all that, because I like the patch 3/4 so much, here is
another way to fix this, and I think (of course I am biased, but...)
the result is easier to grok. Not only it makes it clear that the
namespace for the actual command names on the command line and the
namespace for the supported values of the configuration are distinct,
it makes it clear that we do not do anything extra when the user
explicitly tells us which variant to use.
This is designed to be squashed into [4/4] of this latest round (as
opposed to the one in the message I am responding to, which is to be
squashed into the previous round).
connect.c | 42 +++++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/connect.c b/connect.c
index 7f1f802396..12ad8d4c8b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,17 +691,39 @@ 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)
+static int override_ssh_variant(int *port_option, int *needs_batch)
{
- const char *variant = getenv("GIT_SSH_VARIANT");
+ char *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")) {
+ *port_option = 'P';
+ *needs_batch = 0;
+ } else if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ } else {
+ *port_option = 'p';
+ *needs_batch = 0;
+ }
+ free(variant);
+ return 1;
+}
+
+static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
+ int *port_option, int *needs_batch)
+{
+ const char *variant;
char *p = NULL;
- if (variant)
- ; /* okay, fall through */
- else if (!git_config_get_string("ssh.variant", &p))
- variant = p;
- else if (!is_cmdline) {
+ if (override_ssh_variant(port_option, needs_batch))
+ return;
+
+ if (!is_cmdline) {
p = xstrdup(ssh_command);
variant = basename(p);
} else {
@@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*/
free(ssh_argv);
} else
- return 0;
+ return;
}
if (!strcasecmp(variant, "plink") ||
@@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
*needs_batch = 1;
}
free(p);
-
- return 1;
}
/*
^ permalink raw reply related
* Re: [ANNOUNCE] Git Merge Contributor Summit topic planning
From: Christian Couder @ 2017-02-01 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170131005900.fo5qnbsb67rzgnjt@sigill.intra.peff.net>
On Tue, Jan 31, 2017 at 1:59 AM, Jeff King <peff@peff.net> wrote:
> 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.
>
> Here are the two topics I plan on bringing:
>
> - Git / Software Freedom Conservancy yearly report. I'll plan to give
> a rundown of the past year's activities and financials, along with
> some open questions that could benefit from community input.
>
> - The git-scm.com website: who runs that thing, anyway? An overview
> of the site, how it's managed, and what it needs.
>
> I plan to send out detailed emails on both topics to the list on
> Wednesday, and then follow-up with a summary of any useful in-person
> discussion (since obviously not everybody will be at the summit).
>
> I'd encourage anybody with a topic to present to consider doing
> something similar.
GitLab people at the Summit (this includes me) would like to spend a
few minutes to introduce https://gitlab.com/gitlab-org/gitaly/ and
answer any questions.
^ permalink raw reply
* Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-02-01 20:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 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.
You are utterly wrong here.
The primary purpose of code review is to spot and correct the
problems the submitter has missed. The problems can span from
stupid bugs that affect correctness to maintainability, to design
mistakes, to clarity of explanation for both end users and future
developers.
Among them, correctness problems are, as long as the problem to be
solved is specified clearly enough, the easiest to spot by the
submitter before the patch is sent out. The submitter is focused on
solving one problem, and if the updated code does not even work as
the submitter advertises it would, that can be caught by the
submitter before the patch is even sent out.
Of course, humans are not perfect, and catching correctness problems
is important, but that is not the only (let alone primary) thing.
When a submitter has been focusing on solving one problem, it is
easy to lose the larger picture and to end up adding something that
may be "correct" (in the sense of "works as advertised by the
submitter") but does not fit well with the rest of the system, or
covers some use cases but misses other important and related use
cases. If the "does not fit well" surfaces to the end user level,
that would become a design problem. If it affects the future
developers, that would become a maintainability problem.
Compared to the correctness issue, these are much harder to spot by
the submitter alone, who focused so intensely to get his code
"correct". The review process is of greater value to spot these
issues.
I've already read and commented on the series; as I said, I think
the rewrite in 3/4 makes the resulting code much easier to read, and
with the fix-up I just sent out, I think the end result is correct,
works as advertised, the way it is advertised is clear to end users,
and is maintainable.
But I do share your uneasiness about the "new-ness" of the code.
^ permalink raw reply
* Re: [ANNOUNCE] Git Merge Contributor Summit topic planning
From: Stefan Beller @ 2017-02-01 20:37 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170131004804.p5sule4rh2xrgtwe@sigill.intra.peff.net>
On Mon, Jan 30, 2017 at 4:48 PM, Jeff King <peff@peff.net> wrote:
> The Contributor Summit is only a few days away; I'd like to work out a
> few bits of logistics ahead of time.
>
> We're up to 26 attendees. The room layout will probably be three big
> round-tables, with a central projector. We should be able to have
> everybody pay attention to a single speaker, or break into 3 separate
> conversations.
>
> 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.
submodules and X (How do submodules and worktrees interact,
should they?, Which functions need support for submodules, e.g. checkout,
branch, grep, etc...? Are we interested in keeping a submodule its own
logical unit? Do we want to have dedicated plumbing commands for
submodules?)
... would be my line of talk,
Stefan
^ permalink raw reply
* Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Junio C Hamano @ 2017-02-01 20:56 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <xmqqvaswrv5q.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Matt McCutchen <matt@mattmccutchen.net> writes:
> ...
>> Please check that my refactoring is indeed correct! All the existing tests pass
>> for me, but the existing test coverage of these conflict messages looks poor.
>
> This unfortunately is doing a bit too many things at once from that
> point of view. I need to reserve a solid quiet 20-minutes without
> distraction to check it, which I am hoping to do tonight.
Let me make sure if I understood your changes correctly:
* conflict_rename_delete() knew which one is renamed and which one
is deleted (even though the deleted one was called "other"), but
because in the original code handle_change_delete() wants to
always see tree A first and then tree B in its parameter list,
the original code swapped a/b before calling it. In the original
code, handle_change_delete() needed to figure out which one is
the deleted one by looking at a_oid or b_oid.
* In the updated code, the knowledge of which branch survives and
which branch is deleted is passed from the caller to
handle_change_delete(), which no longer needs to figure out by
looking at a_oid/b_oid. The updated API only passes the oid for
surviving branch (as deleted one would have been 0{40} anyway).
* In the updated code, handle_change_delete() is told the names of
both branches (the one that survives and the other that was
deleted). It no longer has to switch between o->branch[12]
depending on the NULLness of a_oid/b_oid; it knows both names and
which one is which.
* handle_modify_delete() also calls handle_change_delete(). Unlike
conflict_rename_delete(), it is not told by its caller which
branch keeps the path and which branch deletes the path, and
instead relies on handle_change_delete() to figure it out.
Because of the above change to the API, now it needs to sort it
out before calling handle_change_delete().
It all makes sense to me.
The single call to update_file() that appears near the end of
handle_change_delete() in the updated code corresponds to calls to
the same function in 3 among 4 codepaths in the function in the
original code. It is a bit tricky to reason about, though.
In the original code, update_file() was omitted when we didn't have
to come up with a unique alternate filename and the one that is left
is a_oid (i.e. our side). The way to tell if we did not come up
with a unique alternate filename used to be to see the "renamed"
variable but now it is the NULL-ness of "alt_path". And the way to
tell if the side that is left is ours, we check to see o->branch1
is the change_branch, not delete_branch.
So the condition to guard the call to update_file() also looks
correct to me.
Thanks.
>> - char *renamed = NULL;
>> + char *alt_path = NULL;
>> + const char *update_path = path;
>> int ret = 0;
>> +
>> if (dir_in_way(path, !o->call_depth, 0)) {
>> - renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
>> + update_path = alt_path = unique_path(o, path, change_branch);
>> }
>>
>> if (o->call_depth) {
>> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,
>> */
>> ret = remove_file_from_cache(path);
>> if (!ret)
>> - ret = update_file(o, 0, o_oid, o_mode,
>> - renamed ? renamed : path);
>> - } else if (!a_oid) {
>> - if (!renamed) {
>> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> - "and %s in %s. Version %s of %s left in tree."),
>> - change, path, o->branch1, change_past,
>> - o->branch2, o->branch2, path);
>> - ret = update_file(o, 0, b_oid, b_mode, path);
>> - } else {
>> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> - "and %s in %s. Version %s of %s left in tree at %s."),
>> - change, path, o->branch1, change_past,
>> - o->branch2, o->branch2, path, renamed);
>> - ret = update_file(o, 0, b_oid, b_mode, renamed);
>> - }
>> + ret = update_file(o, 0, o_oid, o_mode, update_path);
>> } else {
>> - if (!renamed) {
>> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> - "and %s in %s. Version %s of %s left in tree."),
>> - change, path, o->branch2, change_past,
>> - o->branch1, o->branch1, path);
>> + if (!alt_path) {
>> + if (!old_path) {
>> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> + "and %s in %s. Version %s of %s left in tree."),
>> + change, path, delete_branch, change_past,
>> + change_branch, change_branch, path);
>> + } else {
>> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> + "and %s to %s in %s. Version %s of %s left in tree."),
>> + change, old_path, delete_branch, change_past, path,
>> + change_branch, change_branch, path);
>> + }
>> } else {
>> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> - "and %s in %s. Version %s of %s left in tree at %s."),
>> - change, path, o->branch2, change_past,
>> - o->branch1, o->branch1, path, renamed);
>> - ret = update_file(o, 0, a_oid, a_mode, renamed);
>> + if (!old_path) {
>> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> + "and %s in %s. Version %s of %s left in tree at %s."),
>> + change, path, delete_branch, change_past,
>> + change_branch, change_branch, path, alt_path);
>> + } else {
>> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
>> + "and %s to %s in %s. Version %s of %s left in tree at %s."),
>> + change, old_path, delete_branch, change_past, path,
>> + change_branch, change_branch, path, alt_path);
>> + }
>> }
>> /*
>> - * No need to call update_file() on path when !renamed, since
>> - * that would needlessly touch path. We could call
>> - * update_file_flags() with update_cache=0 and update_wd=0,
>> - * but that's a no-op.
>> + * No need to call update_file() on path when change_branch ==
>> + * o->branch1 && !alt_path, since that would needlessly touch
>> + * path. We could call update_file_flags() with update_cache=0
>> + * and update_wd=0, but that's a no-op.
>> */
>> + if (change_branch != o->branch1 || alt_path)
>> + ret = update_file(o, 0, changed_oid, changed_mode, update_path);
>> }
>> - free(renamed);
>> + free(alt_path);
>>
>> return ret;
>> }
>> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,
>> static int conflict_rename_delete(struct merge_options *o,
>> struct diff_filepair *pair,
>> const char *rename_branch,
>> - const char *other_branch)
>> + const char *delete_branch)
>> {
>> const struct diff_filespec *orig = pair->one;
>> const struct diff_filespec *dest = pair->two;
>> - const struct object_id *a_oid = NULL;
>> - const struct object_id *b_oid = NULL;
>> - int a_mode = 0;
>> - int b_mode = 0;
>> -
>> - if (rename_branch == o->branch1) {
>> - a_oid = &dest->oid;
>> - a_mode = dest->mode;
>> - } else {
>> - b_oid = &dest->oid;
>> - b_mode = dest->mode;
>> - }
>>
>> if (handle_change_delete(o,
>> o->call_depth ? orig->path : dest->path,
>> + o->call_depth ? NULL : orig->path,
>> &orig->oid, orig->mode,
>> - a_oid, a_mode,
>> - b_oid, b_mode,
>> + &dest->oid, dest->mode,
>> + rename_branch, delete_branch,
>> _("rename"), _("renamed")))
>> return -1;
>>
>> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,
>> struct object_id *a_oid, int a_mode,
>> struct object_id *b_oid, int b_mode)
>> {
>> + const char *modify_branch, *delete_branch;
>> + struct object_id *changed_oid;
>> + int changed_mode;
>> +
>> + if (a_oid) {
>> + modify_branch = o->branch1;
>> + delete_branch = o->branch2;
>> + changed_oid = a_oid;
>> + changed_mode = a_mode;
>> + } else {
>> + modify_branch = o->branch2;
>> + delete_branch = o->branch1;
>> + changed_oid = b_oid;
>> + changed_mode = b_mode;
>> + }
>> +
>> return handle_change_delete(o,
>> - path,
>> + path, NULL,
>> o_oid, o_mode,
>> - a_oid, a_mode,
>> - b_oid, b_mode,
>> + changed_oid, changed_mode,
>> + modify_branch, delete_branch,
>> _("modify"), _("modified"));
>> }
>>
>> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh
>> new file mode 100755
>> index 0000000..8f33244
>> --- /dev/null
>> +++ b/t/t6045-merge-rename-delete.sh
>> @@ -0,0 +1,23 @@
>> +#!/bin/sh
>> +
>> +test_description='Merge-recursive rename/delete conflict message'
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'rename/delete' '
>> +echo foo >A &&
>> +git add A &&
>> +git commit -m "initial" &&
>> +
>> +git checkout -b rename &&
>> +git mv A B &&
>> +git commit -m "rename" &&
>> +
>> +git checkout master &&
>> +git rm A &&
>> +git commit -m "delete" &&
>> +
>> +test_must_fail git merge --strategy=recursive rename >output &&
>> +test_i18ngrep "CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree." output
>> +'
>> +
>> +test_done
^ 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