* [REROLL PATCH v2 2/8] Support mandatory capabilities
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Add support for marking capability as mandatory for hosting git version
to understand. This is useful for helpers which require various types
of assistance from main git binary.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
Documentation/git-remote-helpers.txt | 5 ++++-
transport-helper.c | 25 +++++++++++++++++++------
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 5cfdc0c..20a05fe 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -25,7 +25,10 @@ Commands are given by the caller on the helper's standard input, one per line.
'capabilities'::
Lists the capabilities of the helper, one per line, ending
- with a blank line.
+ with a blank line. Each capability may be preceeded with '*'.
+ This marks them mandatory for git version using the remote
+ helper to understand (unknown mandatory capability is fatal
+ error).
'list'::
Lists the refs, one per line, in the format "<value> <name>
diff --git a/transport-helper.c b/transport-helper.c
index a721dc2..4b17aaa 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -93,25 +93,38 @@ static struct child_process *get_helper(struct transport *transport)
data->out = xfdopen(helper->out, "r");
while (1) {
+ const char *capname;
+ int mandatory = 0;
recvline(data, &buf);
if (!*buf.buf)
break;
+
+ if (*buf.buf == '*') {
+ capname = buf.buf + 1;
+ mandatory = 1;
+ } else
+ capname = buf.buf;
+
if (debug)
- fprintf(stderr, "Debug: Got cap %s\n", buf.buf);
- if (!strcmp(buf.buf, "fetch"))
+ fprintf(stderr, "Debug: Got cap %s\n", capname);
+ if (!strcmp(capname, "fetch"))
data->fetch = 1;
- if (!strcmp(buf.buf, "option"))
+ else if (!strcmp(capname, "option"))
data->option = 1;
- if (!strcmp(buf.buf, "push"))
+ else if (!strcmp(capname, "push"))
data->push = 1;
- if (!strcmp(buf.buf, "import"))
+ else if (!strcmp(capname, "import"))
data->import = 1;
- if (!data->refspecs && !prefixcmp(buf.buf, "refspec ")) {
+ else if (!data->refspecs && !prefixcmp(capname, "refspec ")) {
ALLOC_GROW(refspecs,
refspec_nr + 1,
refspec_alloc);
refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
+ } else if (mandatory) {
+ die("Unknown madatory capability %s. This remote "
+ "helper probably needs newer version of Git.\n",
+ capname);
}
}
if (refspecs) {
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 7/8] Support remote archive from all smart transports
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Previously, remote archive required internal (non remote-helper)
smart transport. Extend the remote archive to also support smart
transports implemented by remote helpers.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
builtin-archive.c | 17 ++++++++++-------
transport-helper.c | 19 +++++++++++++++++++
transport.c | 21 +++++++++++++++++++++
transport.h | 5 +++++
4 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/builtin-archive.c b/builtin-archive.c
index 12351e9..d34b3fd 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -5,6 +5,7 @@
#include "cache.h"
#include "builtin.h"
#include "archive.h"
+#include "transport.h"
#include "parse-options.h"
#include "pkt-line.h"
#include "sideband.h"
@@ -25,12 +26,16 @@ static void create_output_file(const char *output_file)
static int run_remote_archiver(int argc, const char **argv,
const char *remote, const char *exec)
{
- char *url, buf[LARGE_PACKET_MAX];
+ char buf[LARGE_PACKET_MAX];
int fd[2], i, len, rv;
- struct child_process *conn;
+ struct transport *transport;
+ struct remote *_remote;
- url = xstrdup(remote);
- conn = git_connect(fd, url, exec, 0);
+ _remote = remote_get(remote);
+ if (!_remote->url[0])
+ die("git archive: Remote with no URL");
+ transport = transport_get(_remote, _remote->url[0]);
+ transport_connect(transport, "git-upload-archive", exec, fd);
for (i = 1; i < argc; i++)
packet_write(fd[1], "argument %s\n", argv[i]);
@@ -53,9 +58,7 @@ static int run_remote_archiver(int argc, const char **argv,
/* Now, start reading from fd[0] and spit it out to stdout */
rv = recv_sideband("archive", fd[0], 1);
- close(fd[0]);
- close(fd[1]);
- rv |= finish_connect(conn);
+ rv |= transport_disconnect(transport);
return !!rv;
}
diff --git a/transport-helper.c b/transport-helper.c
index 216af87..2ce4ef5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -469,6 +469,24 @@ static int process_connect(struct transport *transport,
return process_connect_service(transport, name, exec);
}
+static int connect_helper(struct transport *transport, const char *name,
+ const char *exec, int fd[2])
+{
+ struct helper_data *data = transport->data;
+
+ /* Get_helper so connect is inited. */
+ get_helper(transport);
+ if (!data->connect)
+ die("Operation not supported by protocol.");
+
+ if (!process_connect_service(transport, name, exec))
+ die("Can't connect to subservice %s.", name);
+
+ fd[0] = data->helper->out;
+ fd[1] = data->helper->in;
+ return 0;
+}
+
static int fetch(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
@@ -713,6 +731,7 @@ int transport_helper_init(struct transport *transport, const char *name)
transport->fetch = fetch;
transport->push_refs = push_refs;
transport->disconnect = release_helper;
+ transport->connect = connect_helper;
transport->smart_options = &(data->transport_options);
return 0;
}
diff --git a/transport.c b/transport.c
index ad25b98..a7d67eb 100644
--- a/transport.c
+++ b/transport.c
@@ -756,6 +756,17 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
return ret;
}
+static int connect_git(struct transport *transport, const char *name,
+ const char *executable, int fd[2])
+{
+ struct git_transport_data *data = transport->data;
+ data->conn = git_connect(data->fd, transport->url,
+ executable, 0);
+ fd[0] = data->fd[0];
+ fd[1] = data->fd[1];
+ return 0;
+}
+
static int disconnect_git(struct transport *transport)
{
struct git_transport_data *data = transport->data;
@@ -901,6 +912,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->get_refs_list = get_refs_via_connect;
ret->fetch = fetch_refs_via_pack;
ret->push_refs = git_transport_push;
+ ret->connect = connect_git;
ret->disconnect = disconnect_git;
ret->smart_options = &(data->options);
@@ -1061,6 +1073,15 @@ void transport_unlock_pack(struct transport *transport)
}
}
+int transport_connect(struct transport *transport, const char *name,
+ const char *exec, int fd[2])
+{
+ if (transport->connect)
+ return transport->connect(transport, name, exec, fd);
+ else
+ die("Operation not supported by protocol");
+}
+
int transport_disconnect(struct transport *transport)
{
int ret = 0;
diff --git a/transport.h b/transport.h
index 781db2e..97ba251 100644
--- a/transport.h
+++ b/transport.h
@@ -64,6 +64,8 @@ struct transport {
**/
int (*push_refs)(struct transport *transport, struct ref *refs, int flags);
int (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);
+ int (*connect)(struct transport *connection, const char *name,
+ const char *executable, int fd[2]);
/** get_refs_list(), fetch(), and push_refs() can keep
* resources (such as a connection) reserved for futher
@@ -133,6 +135,9 @@ char *transport_anonymize_url(const char *url);
void transport_take_over(struct transport *transport,
struct child_process *child);
+int transport_connect(struct transport *transport, const char *name,
+ const char *exec, int fd[2]);
+
/* Transport methods defined outside transport.c */
int transport_helper_init(struct transport *transport, const char *name);
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 4/8] Refactor git transport options parsing
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Refactor the transport options parsing so that protocols that aren't
directly smart transports (file://, git://, ssh:// & co) can record
the smart transport options for the case if it turns that transport
can actually be smart.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
transport.c | 78 +++++++++++++++++++++++++++++++++++-----------------------
transport.h | 15 +++++++++++
2 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/transport.c b/transport.c
index dea37d0..e6eb20e 100644
--- a/transport.c
+++ b/transport.c
@@ -395,41 +395,35 @@ static int close_bundle(struct transport *transport)
}
struct git_transport_data {
- unsigned thin : 1;
- unsigned keep : 1;
- unsigned followtags : 1;
- int depth;
+ struct git_transport_options options;
struct child_process *conn;
int fd[2];
- const char *uploadpack;
- const char *receivepack;
struct extra_have_objects extra_have;
};
-static int set_git_option(struct transport *connection,
+static int set_git_option(struct git_transport_options *opts,
const char *name, const char *value)
{
- struct git_transport_data *data = connection->data;
if (!strcmp(name, TRANS_OPT_UPLOADPACK)) {
- data->uploadpack = value;
+ opts->uploadpack = value;
return 0;
} else if (!strcmp(name, TRANS_OPT_RECEIVEPACK)) {
- data->receivepack = value;
+ opts->receivepack = value;
return 0;
} else if (!strcmp(name, TRANS_OPT_THIN)) {
- data->thin = !!value;
+ opts->thin = !!value;
return 0;
} else if (!strcmp(name, TRANS_OPT_FOLLOWTAGS)) {
- data->followtags = !!value;
+ opts->followtags = !!value;
return 0;
} else if (!strcmp(name, TRANS_OPT_KEEP)) {
- data->keep = !!value;
+ opts->keep = !!value;
return 0;
} else if (!strcmp(name, TRANS_OPT_DEPTH)) {
if (!value)
- data->depth = 0;
+ opts->depth = 0;
else
- data->depth = atoi(value);
+ opts->depth = atoi(value);
return 0;
}
return 1;
@@ -439,7 +433,8 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)
{
struct git_transport_data *data = transport->data;
data->conn = git_connect(data->fd, transport->url,
- for_push ? data->receivepack : data->uploadpack,
+ for_push ? data->options.receivepack :
+ data->options.uploadpack,
verbose ? CONNECT_VERBOSE : 0);
return 0;
}
@@ -469,15 +464,15 @@ static int fetch_refs_via_pack(struct transport *transport,
struct ref *refs_tmp = NULL;
memset(&args, 0, sizeof(args));
- args.uploadpack = data->uploadpack;
- args.keep_pack = data->keep;
+ args.uploadpack = data->options.uploadpack;
+ args.keep_pack = data->options.keep;
args.lock_pack = 1;
- args.use_thin_pack = data->thin;
- args.include_tag = data->followtags;
+ args.use_thin_pack = data->options.thin;
+ args.include_tag = data->options.followtags;
args.verbose = (transport->verbose > 0);
args.quiet = (transport->verbose < 0);
args.no_progress = args.quiet || (!transport->progress && !isatty(1));
- args.depth = data->depth;
+ args.depth = data->options.depth;
for (i = 0; i < nr_heads; i++)
origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
@@ -734,7 +729,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
memset(&args, 0, sizeof(args));
args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
args.force_update = !!(flags & TRANSPORT_PUSH_FORCE);
- args.use_thin_pack = data->thin;
+ args.use_thin_pack = data->options.thin;
args.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);
args.quiet = !!(flags & TRANSPORT_PUSH_QUIET);
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
@@ -847,12 +842,14 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->get_refs_list = get_refs_via_rsync;
ret->fetch = fetch_objs_via_rsync;
ret->push = rsync_transport_push;
+ ret->smart_options = NULL;
} else if (is_local(url) && is_file(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
ret->get_refs_list = get_refs_from_bundle;
ret->fetch = fetch_refs_from_bundle;
ret->disconnect = close_bundle;
+ ret->smart_options = NULL;
} else if (!is_url(url)
|| !prefixcmp(url, "file://")
|| !prefixcmp(url, "git://")
@@ -862,20 +859,14 @@ struct transport *transport_get(struct remote *remote, const char *url)
/* These are builtin smart transports. */
struct git_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
- ret->set_option = set_git_option;
+ ret->set_option = NULL;
ret->get_refs_list = get_refs_via_connect;
ret->fetch = fetch_refs_via_pack;
ret->push_refs = git_transport_push;
ret->disconnect = disconnect_git;
+ ret->smart_options = &(data->options);
- data->thin = 1;
data->conn = NULL;
- data->uploadpack = "git-upload-pack";
- if (remote->uploadpack)
- data->uploadpack = remote->uploadpack;
- data->receivepack = "git-receive-pack";
- if (remote->receivepack)
- data->receivepack = remote->receivepack;
} else if (!prefixcmp(url, "http://")
|| !prefixcmp(url, "https://")
|| !prefixcmp(url, "ftp://")) {
@@ -893,14 +884,39 @@ struct transport *transport_get(struct remote *remote, const char *url)
transport_helper_init(ret, handler);
}
+ if (ret->smart_options) {
+ ret->smart_options->thin = 1;
+ ret->smart_options->uploadpack = "git-upload-pack";
+ if (remote->uploadpack)
+ ret->smart_options->uploadpack = remote->uploadpack;
+ ret->smart_options->receivepack = "git-receive-pack";
+ if (remote->receivepack)
+ ret->smart_options->receivepack = remote->receivepack;
+ }
+
return ret;
}
int transport_set_option(struct transport *transport,
const char *name, const char *value)
{
+ int git_reports = 1, protocol_reports = 1;
+
+ if (transport->smart_options)
+ git_reports = set_git_option(transport->smart_options,
+ name, value);
+
if (transport->set_option)
- return transport->set_option(transport, name, value);
+ protocol_reports = transport->set_option(transport, name,
+ value);
+
+ /* If either report is 0, report 0 (success). */
+ if (!git_reports || !protocol_reports)
+ return 0;
+ /* If either reports -1 (invalid value), report -1. */
+ if ((git_reports == -1) || (protocol_reports == -1))
+ return -1;
+ /* Otherwise if both report unknown, report unknown. */
return 1;
}
diff --git a/transport.h b/transport.h
index 9e74406..e90c285 100644
--- a/transport.h
+++ b/transport.h
@@ -4,6 +4,15 @@
#include "cache.h"
#include "remote.h"
+struct git_transport_options {
+ unsigned thin : 1;
+ unsigned keep : 1;
+ unsigned followtags : 1;
+ int depth;
+ const char *uploadpack;
+ const char *receivepack;
+};
+
struct transport {
struct remote *remote;
const char *url;
@@ -65,6 +74,12 @@ struct transport {
signed verbose : 3;
/* Force progress even if the output is not a tty */
unsigned progress : 1;
+ /*
+ * If transport is at least potentially smart, this points to
+ * git_transport_options structure to use in case transport
+ * actually turns out to be smart.
+ */
+ struct git_transport_options *smart_options;
};
#define TRANSPORT_PUSH_ALL 1
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 5/8] Support taking over transports
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Add support for taking over transports that turn out to be smart.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
transport-helper.c | 19 ++++++++++++++++++-
transport.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
transport.h | 2 ++
3 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 271af34..97eed6c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -22,6 +22,10 @@ struct helper_data
/* These go from remote name (as in "list") to private name */
struct refspec *refspecs;
int refspec_nr;
+ /* Transport options for fetch-pack/send-pack (should one of
+ * those be invoked).
+ */
+ struct git_transport_options transport_options;
};
static void sendline(struct helper_data *helper, struct strbuf *buffer)
@@ -81,6 +85,7 @@ static struct child_process *get_helper(struct transport *transport)
const char **refspecs = NULL;
int refspec_nr = 0;
int refspec_alloc = 0;
+ int duped;
if (data->helper)
return data->helper;
@@ -99,9 +104,19 @@ static struct child_process *get_helper(struct transport *transport)
die("Unable to run helper: git %s", helper->argv[0]);
data->helper = helper;
+ /*
+ * Open the output as FILE* so strbuf_getline() can be used.
+ * Do this with duped fd because fclose() will close the fd,
+ * and stuff like taking over will require the fd to remain.
+ *
+ */
+ duped = dup(helper->out);
+ if (duped < 0)
+ die_errno("Can't dup helper output fd");
+ data->out = xfdopen(duped, "r");
+
write_constant(helper->in, "capabilities\n");
- data->out = xfdopen(helper->out, "r");
while (1) {
const char *capname;
int mandatory = 0;
@@ -163,6 +178,7 @@ static int disconnect_helper(struct transport *transport)
strbuf_addf(&buf, "\n");
sendline(data, &buf);
close(data->helper->in);
+ close(data->helper->out);
fclose(data->out);
finish_command(data->helper);
free((char *)data->helper->argv[0]);
@@ -583,5 +599,6 @@ int transport_helper_init(struct transport *transport, const char *name)
transport->fetch = fetch;
transport->push_refs = push_refs;
transport->disconnect = release_helper;
+ transport->smart_options = &(data->transport_options);
return 0;
}
diff --git a/transport.c b/transport.c
index e6eb20e..ad25b98 100644
--- a/transport.c
+++ b/transport.c
@@ -398,6 +398,7 @@ struct git_transport_data {
struct git_transport_options options;
struct child_process *conn;
int fd[2];
+ unsigned got_remote_heads : 1;
struct extra_have_objects extra_have;
};
@@ -432,10 +433,15 @@ static int set_git_option(struct git_transport_options *opts,
static int connect_setup(struct transport *transport, int for_push, int verbose)
{
struct git_transport_data *data = transport->data;
+
+ if (data->conn)
+ return 0;
+
data->conn = git_connect(data->fd, transport->url,
for_push ? data->options.receivepack :
data->options.uploadpack,
verbose ? CONNECT_VERBOSE : 0);
+
return 0;
}
@@ -447,6 +453,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
connect_setup(transport, for_push, 0);
get_remote_heads(data->fd[0], &refs, 0, NULL,
for_push ? REF_NORMAL : 0, &data->extra_have);
+ data->got_remote_heads = 1;
return refs;
}
@@ -477,9 +484,10 @@ static int fetch_refs_via_pack(struct transport *transport,
for (i = 0; i < nr_heads; i++)
origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
- if (!data->conn) {
+ if (!data->got_remote_heads) {
connect_setup(transport, 0, 0);
get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
+ data->got_remote_heads = 1;
}
refs = fetch_pack(&args, data->fd, data->conn,
@@ -490,6 +498,7 @@ static int fetch_refs_via_pack(struct transport *transport,
if (finish_connect(data->conn))
refs = NULL;
data->conn = NULL;
+ data->got_remote_heads = 0;
free_refs(refs_tmp);
@@ -718,12 +727,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
struct send_pack_args args;
int ret;
- if (!data->conn) {
+ if (!data->got_remote_heads) {
struct ref *tmp_refs;
connect_setup(transport, 1, 0);
get_remote_heads(data->fd[0], &tmp_refs, 0, NULL, REF_NORMAL,
NULL);
+ data->got_remote_heads = 1;
}
memset(&args, 0, sizeof(args));
@@ -741,6 +751,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
close(data->fd[0]);
ret |= finish_connect(data->conn);
data->conn = NULL;
+ data->got_remote_heads = 0;
return ret;
}
@@ -749,7 +760,8 @@ static int disconnect_git(struct transport *transport)
{
struct git_transport_data *data = transport->data;
if (data->conn) {
- packet_flush(data->fd[1]);
+ if (data->got_remote_heads)
+ packet_flush(data->fd[1]);
close(data->fd[0]);
close(data->fd[1]);
finish_connect(data->conn);
@@ -759,6 +771,32 @@ static int disconnect_git(struct transport *transport)
return 0;
}
+void transport_take_over(struct transport *transport,
+ struct child_process *child)
+{
+ struct git_transport_data *data;
+
+ if (!transport->smart_options)
+ die("Bug detected: Taking over transport requires non-NULL "
+ "smart_options field.");
+
+ data = xcalloc(1, sizeof(*data));
+ data->options = *transport->smart_options;
+ data->conn = child;
+ data->fd[0] = data->conn->out;
+ data->fd[1] = data->conn->in;
+ data->got_remote_heads = 0;
+ transport->data = data;
+
+ transport->set_option = NULL;
+ transport->get_refs_list = get_refs_via_connect;
+ transport->fetch = fetch_refs_via_pack;
+ transport->push = NULL;
+ transport->push_refs = git_transport_push;
+ transport->disconnect = disconnect_git;
+ transport->smart_options = &(data->options);
+}
+
static int is_local(const char *url)
{
const char *colon = strchr(url, ':');
@@ -867,6 +905,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->smart_options = &(data->options);
data->conn = NULL;
+ data->got_remote_heads = 0;
} else if (!prefixcmp(url, "http://")
|| !prefixcmp(url, "https://")
|| !prefixcmp(url, "ftp://")) {
@@ -927,9 +966,9 @@ int transport_push(struct transport *transport,
*nonfastforward = 0;
verify_remote_names(refspec_nr, refspec);
- if (transport->push)
+ if (transport->push) {
return transport->push(transport, refspec_nr, refspec, flags);
- if (transport->push_refs) {
+ } else if (transport->push_refs) {
struct ref *remote_refs =
transport->get_refs_list(transport, 1);
struct ref *local_refs = get_local_heads();
@@ -973,6 +1012,7 @@ const struct ref *transport_get_remote_refs(struct transport *transport)
{
if (!transport->remote_refs)
transport->remote_refs = transport->get_refs_list(transport, 0);
+
return transport->remote_refs;
}
@@ -1007,6 +1047,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
}
rc = transport->fetch(transport, nr_heads, heads);
+
free(heads);
return rc;
}
diff --git a/transport.h b/transport.h
index e90c285..781db2e 100644
--- a/transport.h
+++ b/transport.h
@@ -130,6 +130,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);
void transport_unlock_pack(struct transport *transport);
int transport_disconnect(struct transport *transport);
char *transport_anonymize_url(const char *url);
+void transport_take_over(struct transport *transport,
+ struct child_process *child);
/* Transport methods defined outside transport.c */
int transport_helper_init(struct transport *transport, const char *name);
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 3/8] Pass unknown protocols to external protocol handlers
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Change URL handling to allow external protocol handlers to implement
new protocols without the '::' syntax if helper name does not conflict
with any built-in protocol.
foo:// now invokes git-remote-foo with foo:// as the URL.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
transport-helper.c | 12 +++++++-
transport.c | 76 +++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 4b17aaa..271af34 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -63,6 +63,16 @@ static void write_constant(int fd, const char *str)
die_errno("Full write to remote helper failed");
}
+const char *remove_ext_force(const char *url)
+{
+ if (url) {
+ const char *colon = strchr(url, ':');
+ if (colon && colon[1] == ':')
+ return colon + 2;
+ }
+ return url;
+}
+
static struct child_process *get_helper(struct transport *transport)
{
struct helper_data *data = transport->data;
@@ -83,7 +93,7 @@ static struct child_process *get_helper(struct transport *transport)
strbuf_addf(&buf, "remote-%s", data->name);
helper->argv[0] = strbuf_detach(&buf, NULL);
helper->argv[1] = transport->remote->name;
- helper->argv[2] = transport->url;
+ helper->argv[2] = remove_ext_force(transport->url);
helper->git_cmd = 1;
if (start_command(helper))
die("Unable to run helper: git %s", helper->argv[0]);
diff --git a/transport.c b/transport.c
index 3eea836..dea37d0 100644
--- a/transport.c
+++ b/transport.c
@@ -780,6 +780,44 @@ static int is_file(const char *url)
return S_ISREG(buf.st_mode);
}
+static int is_url(const char *url)
+{
+ const char *url2, *first_slash;
+
+ if (!url)
+ return 0;
+ url2 = url;
+ first_slash = strchr(url, '/');
+
+ /* Input with no slash at all or slash first can't be URL. */
+ if (!first_slash || first_slash == url)
+ return 0;
+ /* Character before must be : and next must be /. */
+ if (first_slash[-1] != ':' || first_slash[1] != '/')
+ return 0;
+ /* There must be something before the :// */
+ if (first_slash == url + 1)
+ return 0;
+ /*
+ * Check all characters up to first slash - 1. Only alphanum
+ * is allowed.
+ */
+ url2 = url;
+ while (url2 < first_slash - 1) {
+ if (!isalnum((unsigned char)*url2))
+ return 0;
+ url2++;
+ }
+
+ /* Valid enough. */
+ return 1;
+}
+
+static int external_specification_len(const char *url)
+{
+ return strchr(url, ':') - url;
+}
+
struct transport *transport_get(struct remote *remote, const char *url)
{
struct transport *ret = xcalloc(1, sizeof(*ret));
@@ -805,30 +843,23 @@ struct transport *transport_get(struct remote *remote, const char *url)
if (remote && remote->foreign_vcs) {
transport_helper_init(ret, remote->foreign_vcs);
- return ret;
- }
-
- if (!prefixcmp(url, "rsync:")) {
+ } else if (!prefixcmp(url, "rsync:")) {
ret->get_refs_list = get_refs_via_rsync;
ret->fetch = fetch_objs_via_rsync;
ret->push = rsync_transport_push;
-
- } else if (!prefixcmp(url, "http://")
- || !prefixcmp(url, "https://")
- || !prefixcmp(url, "ftp://")) {
- transport_helper_init(ret, "curl");
-#ifdef NO_CURL
- error("git was compiled without libcurl support.");
-#endif
-
} else if (is_local(url) && is_file(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
ret->get_refs_list = get_refs_from_bundle;
ret->fetch = fetch_refs_from_bundle;
ret->disconnect = close_bundle;
-
- } else {
+ } else if (!is_url(url)
+ || !prefixcmp(url, "file://")
+ || !prefixcmp(url, "git://")
+ || !prefixcmp(url, "ssh://")
+ || !prefixcmp(url, "git+ssh://")
+ || !prefixcmp(url, "ssh+git://")) {
+ /* These are builtin smart transports. */
struct git_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
ret->set_option = set_git_option;
@@ -845,6 +876,21 @@ struct transport *transport_get(struct remote *remote, const char *url)
data->receivepack = "git-receive-pack";
if (remote->receivepack)
data->receivepack = remote->receivepack;
+ } else if (!prefixcmp(url, "http://")
+ || !prefixcmp(url, "https://")
+ || !prefixcmp(url, "ftp://")) {
+ /* These three are just plain special. */
+ transport_helper_init(ret, "curl");
+#ifdef NO_CURL
+ error("git was compiled without libcurl support.");
+#endif
+ } else {
+ /* Unknown protocol in URL. Pass to external handler. */
+ int len = external_specification_len(url);
+ char *handler = xmalloc(len + 1);
+ handler[len] = 0;
+ strncpy(handler, url, len);
+ transport_helper_init(ret, handler);
}
return ret;
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 1/8] Add remote helper debug mode
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
In-Reply-To: <1260372394-16427-1-git-send-email-ilari.liusvaara@elisanet.fi>
Remote helpers deadlock easily, so support debug mode which shows the
interaction steps.
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
transport-helper.c | 94 ++++++++++++++++++++++++++++++++++++++--------------
1 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..a721dc2 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -8,6 +8,8 @@
#include "quote.h"
#include "remote.h"
+static int debug;
+
struct helper_data
{
const char *name;
@@ -22,6 +24,45 @@ struct helper_data
int refspec_nr;
};
+static void sendline(struct helper_data *helper, struct strbuf *buffer)
+{
+ if (debug)
+ fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf);
+ if (write_in_full(helper->helper->in, buffer->buf, buffer->len)
+ != buffer->len)
+ die_errno("Full write to remote helper failed");
+}
+
+static int recvline(struct helper_data *helper, struct strbuf *buffer)
+{
+ strbuf_reset(buffer);
+ if (debug)
+ fprintf(stderr, "Debug: Remote helper: Waiting...\n");
+ if (strbuf_getline(buffer, helper->out, '\n') == EOF) {
+ if (debug)
+ fprintf(stderr, "Debug: Remote helper quit.\n");
+ exit(128);
+ }
+
+ if (debug)
+ fprintf(stderr, "Debug: Remote helper: <- %s\n", buffer->buf);
+ return 0;
+}
+
+static void xchgline(struct helper_data *helper, struct strbuf *buffer)
+{
+ sendline(helper, buffer);
+ recvline(helper, buffer);
+}
+
+static void write_constant(int fd, const char *str)
+{
+ if (debug)
+ fprintf(stderr, "Debug: Remote helper: -> %s", str);
+ if (write_in_full(fd, str, strlen(str)) != strlen(str))
+ die_errno("Full write to remote helper failed");
+}
+
static struct child_process *get_helper(struct transport *transport)
{
struct helper_data *data = transport->data;
@@ -48,15 +89,16 @@ static struct child_process *get_helper(struct transport *transport)
die("Unable to run helper: git %s", helper->argv[0]);
data->helper = helper;
- write_str_in_full(helper->in, "capabilities\n");
+ write_constant(helper->in, "capabilities\n");
data->out = xfdopen(helper->out, "r");
while (1) {
- if (strbuf_getline(&buf, data->out, '\n') == EOF)
- exit(128); /* child died, message supplied already */
+ recvline(data, &buf);
if (!*buf.buf)
break;
+ if (debug)
+ fprintf(stderr, "Debug: Got cap %s\n", buf.buf);
if (!strcmp(buf.buf, "fetch"))
data->fetch = 1;
if (!strcmp(buf.buf, "option"))
@@ -82,14 +124,21 @@ static struct child_process *get_helper(struct transport *transport)
free(refspecs);
}
strbuf_release(&buf);
+ if (debug)
+ fprintf(stderr, "Debug: Capabilities complete.\n");
return data->helper;
}
static int disconnect_helper(struct transport *transport)
{
struct helper_data *data = transport->data;
+ struct strbuf buf = STRBUF_INIT;
+
if (data->helper) {
- write_str_in_full(data->helper->in, "\n");
+ if (debug)
+ fprintf(stderr, "Debug: Disconnecting.\n");
+ strbuf_addf(&buf, "\n");
+ sendline(data, &buf);
close(data->helper->in);
fclose(data->out);
finish_command(data->helper);
@@ -117,10 +166,11 @@ static int set_helper_option(struct transport *transport,
const char *name, const char *value)
{
struct helper_data *data = transport->data;
- struct child_process *helper = get_helper(transport);
struct strbuf buf = STRBUF_INIT;
int i, ret, is_bool = 0;
+ get_helper(transport);
+
if (!data->option)
return 1;
@@ -143,12 +193,7 @@ static int set_helper_option(struct transport *transport,
quote_c_style(value, &buf, NULL, 0);
strbuf_addch(&buf, '\n');
- if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
- die_errno("cannot send option to %s", data->name);
-
- strbuf_reset(&buf);
- if (strbuf_getline(&buf, data->out, '\n') == EOF)
- exit(128); /* child died, message supplied already */
+ xchgline(data, &buf);
if (!strcmp(buf.buf, "ok"))
ret = 0;
@@ -208,13 +253,10 @@ static int fetch_with_fetch(struct transport *transport,
}
strbuf_addch(&buf, '\n');
- if (write_in_full(data->helper->in, buf.buf, buf.len) != buf.len)
- die_errno("cannot send fetch to %s", data->name);
+ sendline(data, &buf);
while (1) {
- strbuf_reset(&buf);
- if (strbuf_getline(&buf, data->out, '\n') == EOF)
- exit(128); /* child died, message supplied already */
+ recvline(data, &buf);
if (!prefixcmp(buf.buf, "lock ")) {
const char *name = buf.buf + 5;
@@ -249,12 +291,13 @@ static int fetch_with_import(struct transport *transport,
int nr_heads, struct ref **to_fetch)
{
struct child_process fastimport;
- struct child_process *helper = get_helper(transport);
struct helper_data *data = transport->data;
int i;
struct ref *posn;
struct strbuf buf = STRBUF_INIT;
+ get_helper(transport);
+
if (get_importer(transport, &fastimport))
die("Couldn't run fast-import");
@@ -264,7 +307,7 @@ static int fetch_with_import(struct transport *transport,
continue;
strbuf_addf(&buf, "import %s\n", posn->name);
- write_in_full(helper->in, buf.buf, buf.len);
+ sendline(data, &buf);
strbuf_reset(&buf);
}
disconnect_helper(transport);
@@ -369,17 +412,14 @@ static int push_refs(struct transport *transport,
}
strbuf_addch(&buf, '\n');
- if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
- exit(128);
+ sendline(data, &buf);
ref = remote_refs;
while (1) {
char *refname, *msg;
int status;
- strbuf_reset(&buf);
- if (strbuf_getline(&buf, data->out, '\n') == EOF)
- exit(128); /* child died, message supplied already */
+ recvline(data, &buf);
if (!buf.len)
break;
@@ -471,8 +511,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
while (1) {
char *eov, *eon;
- if (strbuf_getline(&buf, data->out, '\n') == EOF)
- exit(128); /* child died, message supplied already */
+ recvline(data, &buf);
if (!*buf.buf)
break;
@@ -497,6 +536,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
}
tail = &((*tail)->next);
}
+ if (debug)
+ fprintf(stderr, "Debug: Read ref listing.\n");
strbuf_release(&buf);
for (posn = ret; posn; posn = posn->next)
@@ -510,6 +551,9 @@ int transport_helper_init(struct transport *transport, const char *name)
struct helper_data *data = xcalloc(sizeof(*data), 1);
data->name = name;
+ if (getenv("GIT_TRANSPORT_HELPER_DEBUG"))
+ debug = 1;
+
transport->data = data;
transport->set_option = set_helper_option;
transport->get_refs_list = get_refs_list;
--
1.6.6.rc1.300.gfbc27
^ permalink raw reply related
* [REROLL PATCH v2 0/8] Remote helpers smart transport extensions
From: Ilari Liusvaara @ 2009-12-09 15:26 UTC (permalink / raw)
To: git
Changes from reroll v1:
- Wording fix in pass unknown protocols patch commit message
- Fix NO_CURL and hopefully make remote-http building bit clearer
- Remove extraneous fflush(stderr)
- Simplify remove_ext_force().
- Use strchr() instead of strchrc(). strchr() not const-correct, but...
- Simplify URL checking in is_url()
- Expand stdin and stdout in remote helper documentation
- _recvline -> recvline_fh
- _process_connect -> process_connect_service
- Reworded remote archive patch message
- gitoptions -> transport_options.
Ilari Liusvaara (8):
Add remote helper debug mode
Support mandatory capabilities
Pass unknown protocols to external protocol handlers
Refactor git transport options parsing
Support taking over transports
Support remote helpers implementing smart transports
Support remote archive from all smart transports
Remove special casing of http, https and ftp
.gitignore | 4 +
Documentation/git-remote-helpers.txt | 30 ++++-
Makefile | 27 +++-
builtin-archive.c | 17 ++-
transport-helper.c | 283 ++++++++++++++++++++++++++++++----
transport.c | 214 ++++++++++++++++++++------
transport.h | 22 +++
7 files changed, 504 insertions(+), 93 deletions(-)
^ permalink raw reply
* Re: [REROLL PATCH 5/8] Support taking over transports
From: Ilari Liusvaara @ 2009-12-09 15:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vljhd597h.fsf@alter.siamese.dyndns.org>
On Tue, Dec 08, 2009 at 03:37:06PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
>
> Will we ever have another 'xxxoptions' field in this structure that is not
> so git-ish? 'gitoptions' somehow doesn't feel right, unless you plan to
> later add scm specific options like 'hgoptions', 'bzroptions' in this
> field you need to distinguish "git" one from them.
>
> I know you needed to name the new field to store the transport option
> under a different name from the existing 'option' field, but for that
> purpose, 'transport_options' might be a more appropriate name.
Changed.
> > @@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)
> > die("Unable to run helper: git %s", helper->argv[0]);
> > data->helper = helper;
> >
> > + /*
> > + * Open the output as FILE* so strbuf_getline() can be used.
> > + * Do this with duped fd because fclose() will close the fd,
> > + * and stuff like taking over will require the fd to remain.
> > + *
> > + */
> > + duped = dup(helper->out);
> > + if (duped < 0)
> > + die_errno("Can't dup helper output fd");
> > + data->out = xfdopen(duped, "r");
> > +
>
> Neat hack (the kind I often love), but it makes something deep inside me
> cringe. This looks fragile and possibly wrong.
>
> Who guarantees that reading from the (FILE *)data->out by strbuf_getline()
> that eventually calls into fgetc() does not cause more data be read in the
> buffer assiciated with the (FILE *) than we will consume? The other FILE*
> you will make out of helper->out won't be able to read what data->out has
> already slurped in to its stdio buffer.
Causality impiles this can happen only if buffered version gets its buffer
filled after sending connect command. And looking at stdio operations that
can occur after sending the command:
- fprintfs on stderr (debug mode only).
- fgetc()s on unbuffered version.
-Ilari
^ permalink raw reply
* Re: [REROLL PATCH 7/8] Support remote archive from external protocol helpers
From: Ilari Liusvaara @ 2009-12-09 15:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7hsx5935.fsf@alter.siamese.dyndns.org>
On Tue, Dec 08, 2009 at 03:39:42PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
>
> > Helpers which support connect also should support remote archive
> > snapshot (or at least there's only one way to attempt it). So support
> > remote snapshotting for protocol helpers.
>
> Or "Because the transport layer has been restructured cleanly enough to
> allow passing general payload, there is no reason not to do this change to
> pass 'archive' output in addition to the 'git smart fetch/push protocol'
> payload, and this allows the archive command to take advantage of the
> helper based transports"???
This one is the correct interpretation.
Changed to:
-----------
Support remote archive from all smart transports
Previously, remote archive required internal (non remote-helper)
smart transport. Extend the remote archive to also support smart
transports implemented by remote helpers.
------------
-Ilari
^ permalink raw reply
* Re: [REROLL PATCH 6/8] Support remote helpers implementing smart transports
From: Ilari Liusvaara @ 2009-12-09 15:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vein5594u.fsf@alter.siamese.dyndns.org>
On Tue, Dec 08, 2009 at 03:38:41PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> >
> > +'connect' <service>::
> > + Connects to given service. Stdin and stdout of helper are
>
> A minor point, but in prose, unless it explains how to use "stdin" and
> "stdout" as a variable, keyword, etc. in code, I'd prefer to see these
> spelled out, like so:
>
> The standard input and output of helpers are connected to ...
Changed.
> > -static int recvline(struct helper_data *helper, struct strbuf *buffer)
> > +static int _recvline(FILE *helper, struct strbuf *buffer)
> > {
>
> recvline_fh() vs revline() might be better as most of the interaction in
> this layer are done on helper_data, which makes the name recvline() pair
> nicely with sendline that also takes helper_data; and the oddball one that
> is _not_ an implementation detail (i.e. you have calls outside recvline()
> implementation that call _recvline()) hints that it takes filehandle
> instead.
- _recvline -> recvline_fh
- _process_connect -> process_connect_service
-Ilari
^ permalink raw reply
* Re: [REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers
From: Ilari Liusvaara @ 2009-12-09 15:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vskbl59ai.fsf@alter.siamese.dyndns.org>
On Tue, Dec 08, 2009 at 03:35:17PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
>
> > +const char *remove_ext_force(const char *url)
> > +{
> > + const char *url2 = url;
> > + const char *first_colon = NULL;
> > +
> > + if (!url)
> > + return NULL;
> > +
> > + while (*url2 && !first_colon)
> > + if (*url2 == ':')
> > + first_colon = url2;
> > + else
> > + url2++;
> > +
> > + if (first_colon && first_colon[1] == ':')
> > + return first_colon + 2;
> > + else
> > + return url;
> > +}
>
> Sorry, I am slow today, but is this any different from:
>
> if (url) {
> const char *colon = strchr(url, ':');
> if (colon && colon[1] == ':')
> return url2 + 2;
> }
> return url;
Undefined variable url2 (you mean colon?). Changed.
> > diff --git a/transport.c b/transport.c
> > index 3eea836..e42a82b 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -780,6 +780,58 @@ static int is_file(const char *url)
> > return S_ISREG(buf.st_mode);
> > }
> >
> > +static const char *strchrc(const char *str, int c)
> > +{
> > + while (*str)
> > + if (*str == c)
> > + return str;
> > + else
> > + str++;
> > + return NULL;
> > +}
>
> I cannot spot how this is different from strchr()...
Return type. Not that useful. Removed.
> > +static int is_url(const char *url)
> > +{
> > + const char *url2, *first_slash;
> > +
> > + if (!url)
> > + return 0;
> > + url2 = url;
> > + first_slash = strchrc(url, '/');
> > +
> > + /* Input with no slash at all or slash first can't be URL. */
> > + if (!first_slash || first_slash == url)
> > + return 0;
> > + /* Character before must be : and next must be /. */
> > + if (first_slash[-1] != ':' || first_slash[1] != '/')
> > + return 0;
> > + /* There must be something before the :// */
> > + if (first_slash == url + 1)
> > + return 0;
> > + /*
> > + * Check all characters up to first slash. Only alpha, num and
> > + * colon (":") are allowed. ":" must be followed by ":" or "/".
> > + */
I tightened this a bit, only checking (exclusive) character before first
"/" and requiring all-alphanum. The previous rules were apparently
leftover from times the "specify remote helper by url" patch didn't
exist.
Considering context at call site, it is equivalent, since any ':' must be
followed by '/' or is_url will never be called, and first ':/' must be
exactly at last_slash - 1 (by earlier checks).
> Hmm, so "a::::bc:://" is ok?
is_url never gets such thing. Due to "::" it bypasses URL validation. And
yes, it is ok.
> Is the reason this does not to check the string up to (first_slash-1) for
> a valid syntax for <scheme> (as in "<scheme>://") because this potentially
> has "<helper>::" in front of it?
It doesn't have '<helper>::' in front of it.
> I cannot tell if you want to allow "<helper1>::<helper2>::<scheme>://..."
> by reading this code.
Allowing or denying that is not up to this function. And one can't chain
helpers that way. It invokes <helper1>
> > +static int external_specification_len(const char *url)
> > +{
> > + return strchrc(url, ':') - url;
> > +}
>
> Does the caller guarantee url has at least one colon in it?
Anything passing is_url() does have ':' in it.
> > + } else if (!is_url(url)
> > + || !prefixcmp(url, "file://")
> > + || !prefixcmp(url, "git://")
> > + || !prefixcmp(url, "ssh://")
> > + || !prefixcmp(url, "git+ssh://")
> > + || !prefixcmp(url, "ssh+git://")) {
> > + /* These are builtin smart transports. */
>
> Hmm, what is !is_url(url) at the beginning for, if this lists "builtin"
> smart transports?
It catches the scp and path syntaxes as not URLs.
> > + } else {
> > + /* Unknown protocol in URL. Pass to external handler. */
> > + int len = external_specification_len(url);
> > + char *handler = xmalloc(len + 1);
> > + handler[len] = 0;
> > + strncpy(handler, url, len);
> > + transport_helper_init(ret, handler);
>
> Hmph, external_specification_len() may get passed a colon-less string
> after all, I think.
Nope, it can't. Else if above snarfs everything that doesn't pass
is_url, and string can't pass it without having ':' in it.
-Ilari
^ permalink raw reply
* Re: [REROLL PATCH 2/8] Support mandatory capabilities
From: Ilari Liusvaara @ 2009-12-09 15:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzl5t59bp.fsf@alter.siamese.dyndns.org>
On Tue, Dec 08, 2009 at 03:34:34PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> > + fflush(stderr);
> > + die("Unknown madatory capability %s. This remote "
> > + "helper probably needs newer version of Git.\n",
> > + capname);
>
> Why fflush() here? Is the reason for needing to flush stderr before
> letting die() to write into it very specific to this codepath, or shared
> among other callers of die()? I am wondering if we should add this
> fflush() to report() in usage.c instead.
No idea why its there (anymore). Die will flush stderr anyway via exit.
Removed.
-Ilari
^ permalink raw reply
* Re: Octopus merge
From: Claudia.Ignat @ 2009-12-09 15:10 UTC (permalink / raw)
To: Michael J Gruber, Thomas Rast; +Cc: git
In-Reply-To: <4B0EB257.8080002@drmicha.warpmail.net>
Michael and Thomas, thank you for your explanations. The fact that in
some cases octopus merge fails, but changes of the first merges subparts
of octopus merge are included in the worktree rests a dilemma.
Further, I have noticed that for the same conflicting changes the
conflict messages (referring to conflict types) received in the case of
an octopus merge are not the same as the conflict messages received for
the corresponding sequential merges.
I slightly changed the scenario I described in my previous message.
Workspaces ws2, ws3 and ws4 are clones of the workspace ws1. Suppose the
current workspace is ws3 and some changes are done in parallel in all
these 3 workspaces. Changes of ws2 and ws3 are conflicting: both modify
the same line of file file1.txt to different values and they rename the
same file file2.txt to fileWS2.txt and fileWS3.txt respectively. Changes
in ws4 are not conflicting with changes in ws2 and ws3.
While I am in the current workspace ws3 I perform the merge
git merge ws2
and content conflict in file1.txt as well as rename/rename conflict are
signaled.
If instead I perform the merge
git merge ws4 ws2
only the content conflict in file1.txt is raised, while the conflict
rename/rename of file2.txt is not mentioned.
The script for the second mentioned scenario is given below.
Do you think this should be the normal behavior of octopus merge?
Thanks.
Cheers,
Claudia
# !/bin/bash
TEST_NAME="TP1" # name of the working directory
rm -rf ${TEST_NAME} # cleaning working directory
mkdir -p ${TEST_NAME}
cd ${TEST_NAME}
# initialising initial git workspace
mkdir ws1
cd ws1
git init
# adding 2 files to ws1
# commit changes
echo -e -n "1\n2\n3\n4\n5\n" > file1.txt
echo -e -n "a\nb\nc\nd\ne\n" > file2.txt
git add file1.txt file2.txt
git commit -m "ws1 | add 12345 in file1.txt; add abcde in file2.txt"
cd ..
# cloning three times ws1 (as ws2, ws3 and ws4)
git clone ws1 ws2
git clone ws1 ws3
git clone ws1 ws4
# updating file1.txt in ws2 (insert X at line 3, then write and quit 'ed')
# rename file2.txt to fileWS2.txt
# commit changes
cd ws2
echo -e "3i\nX\n.\nw\nq\n" | ed file1.txt
git add file1.txt
git mv file2.txt fileWS2.txt
git commit -m "ws2 | insert 12X345 in file1.txt; rename file2.txt to
fileWS2.txt"
cd ..
# updating file1.txt in ws3 (insert Y at line 3, then write and quit 'ed')
# rename file2.txt to fileWS3.txt
# commit changes
cd ws3
echo -e "3i\nY\n.\nw\nq\n" | ed file1.txt
git add file1.txt
git mv file2.txt fileWS3.txt
git commit -m "ws3 | insert 12Y345 in file1.txt; rename file2.txt to
fileWS3.txt"
cd ..
# add file u.txt in ws4
cd ws4
echo -e -n "U1\n2\n3\n4\n5\n" > u.txt
git add u.txt
git commit -m "ws4 | add u.txt"
cd ..
# ws3 pull from ws2 ws4
cd ws3
git remote add bws2 ../ws2
git remote add bws4 ../ws4
git fetch bws2
git fetch bws4
git merge bws4/master bws2/master
cd ..
Michael J Gruber wrote:
> Claudia.Ignat@loria.fr venit, vidit, dixit 26.11.2009 16:56:
>> # !/bin/bash
>> TEST_NAME="TP1" # name of the working directory
>> rm -rf ${TEST_NAME} # cleaning working directory
>> mkdir -p ${TEST_NAME}
>> cd ${TEST_NAME}
>>
>> # initialising initial git workspace
>> mkdir ws1
>> cd ws1
>> git init
>>
>> # adding a file to ws1
>> # commit changes
>> echo -e -n "1\n2\n3\n4\n5\n" > file.txt
>> git add file.txt
>> git commit -m "ws1 | add 12345"
>> cd ..
>>
>> # cloning three times ws1 (as ws2, ws3 and ws4)
>> git clone ws1 ws2
>> git clone ws1 ws3
>> git clone ws1 ws4
>> git clone ws1 ws5
>>
>> # updating file.txt in ws2 (insert X at line 3, then write and quit 'ed')
>> # commit changes
>> cd ws2
>> echo -e "3i\nX\n.\nw\nq\n" | ed file.txt
>> git add file.txt
>> git commit -m "ws2 | insert 12X345"
>> cd ..
>>
>>
>> # updating file.txt in ws3 (insert Y at line 3, then write and quit 'ed')
>> # commit changes
>> cd ws3
>> echo -e "3i\nY\n.\nw\nq\n" | ed file.txt
>> git add file.txt
>> git commit -m "ws3 | insert 12Y345"
>> cd ..
>>
>> cd ws4
>> echo -e -n "U1\n2\n3\n4\n5\n" > u.txt
>> git add u.txt
>> git commit -m "ws4 | add u.txt"
>> cd ..
>>
>> cd ws5
>> echo -e -n "W1\n2\n3\n4\n5\n" > w.txt
>> git add w.txt
>> git commit -m "ws5 | add w.txt"
>> cd ..
>>
>> # ws3 pull from ws2 ws4 ws5
>> cd ws3
>> git remote add bws2 ../ws2
>> git remote add bws4 ../ws4
>> git remote add bws5 ../ws5
>> git fetch bws2
>> git fetch bws4
>> git fetch bws5
>> git merge bws4/master bws2/master bws5/master
>> cd ..
>>
>> # resolve conflict in ws3
>> cd ws3
>
> First of all, thanks for the clear description and test case!
>
> The octopus strategy cannot do merges which need manual resolution. Or
> so the doc says. After trying the merge with 4 2 5, Git tells you:
>
> Trying simple merge with 7ff9b5bd514cb600bac935ebd40eae366bba7d19
> Trying simple merge with 6872cd350154743d59cb4d313cbdb122ac43e537
> Simple merge did not work, trying automatic merge.
> Auto-merging file.txt
> ERROR: content conflict in file.txt
> fatal: merge program failed
> Automated merge did not work.
> Should not be doing an Octopus.
> Merge with strategy octopus failed.
>
> That is, it aborts the merge completely. If you "resolve" it and commit
> it's simply a commit that you make.
>
> If, instead, you merge 4 5 2, Git tells you:
>
> Trying simple merge with e4f78f6905bed39bcd96790a4f63e138a455a445
> Trying simple merge with 14c1f2a70767334df5d6d3120631752564094699
> Trying simple merge with 8540a039d3fc964d097d4f037357668441d1d4f5
> Simple merge did not work, trying automatic merge.
> Auto-merging file.txt
> ERROR: content conflict in file.txt
> fatal: merge program failed
> Automatic merge failed; fix conflicts and then commit the result.
>
> Admittedly this looks fatal also, but the last line tells you that the
> actual merge process is not aborted yet. If you resolve the conflict and
> commit without -m you even see the prepared commit message.
>
> So, octopus can deal with manual conflict resolution if the conflicts
> appear in the last step only. That is the difference between the two cases.
>
> Now, in the first case the aborted merge leaves some traces in the index
> as well as in the worktree. I'm not sure that is how it's supposed to be.
>
> Cheers,
> Michael
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Michael S. Tsirkin @ 2009-12-09 14:12 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: Matthieu Moy, Junio C Hamano, git
In-Reply-To: <20091209140145.GA31130@atjola.homenet>
On Wed, Dec 09, 2009 at 03:01:45PM +0100, Björn Steinbrink wrote:
> On 2009.12.09 15:45:36 +0200, Michael S. Tsirkin wrote:
> > On Wed, Dec 09, 2009 at 02:30:06PM +0100, Matthieu Moy wrote:
> > > Junio C Hamano <gitster@pobox.com> writes:
> > >
> > > > So perhaps a good way to move forward is to teach "git cherry-pick A..B"
> > > > to be a thin wrapper that invokes a new hidden mode of operation added to
> > > > "rebase" that is not advertised to the end user.
> > > >
> > > > I would suggest calling the option to invoke that hidden mode not
> > > > "--revisions", but "--reverse" or "--opposite" or something of that
> > > > nature, though. It makes "rebase" work in different direction.
> > >
> > > Intuitively,
> > >
> > > git rebase --reverse A..B
> > >
> > > would mean "take the range A..B, and start applying the patches from
> > > B, going in reverse order up to A", like "git log --reverse". So, I'd
> > > find it misleading.
> > >
> > > Perhaps "git rebase --cherry-pick A..B" would be a better name. No
> > > objection for --opposite either.
> >
> > I relly like --cherry-pick. Junio, objections to that one?
>
> Hm, there's also (the probably not so well known)
> "git rev-list --cherry-pick A...B", which excludes commits that appear
> on both A and B and have the same patch id. I'd rather call the rev-list
> option a misnomer than the suggested hidden option for rebase, but I'd
> call it --cherry-pick-mode or --cherry-picking (like am's hidden "git am
> --rebasing"), just to make sure... Of course, it's not _that_ important,
> as it's going to be a hidden option, so user confusion is probably not
> that much of a concern.
>
> Björn
OK, --cherry-picking looks fine as well.
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Michael S. Tsirkin @ 2009-12-09 14:02 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: git, Junio C Hamano
In-Reply-To: <20091209131945.GB30218@atjola.homenet>
On Wed, Dec 09, 2009 at 02:19:45PM +0100, Björn Steinbrink wrote:
> On 2009.12.08 22:00:17 +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 08, 2009 at 08:11:07PM +0100, Björn Steinbrink wrote:
> > > So you can already do what you want to do, but wrapping it in a single
> > > porcelain might still be useful because it's obviously a lot easier and
> > > safer that way. That said, I wonder what kind of workflow you're using
> > > though, and why you require that feature. I've never needed something
> > > like that.
> >
> > I need this often for many reasons:
> > - Imagine developing a patchset with a complex bugfix on master branch.
> > Then I decide to also apply (backport) this patchset to stable branch.
>
> Hm, I'd also imagine that you want a separate branch then, and not
> directly mess up the stable branch,
Well, directly working with a stable branch is easier, so yes,
I want to mess it up: this is just my local tree, if anything
goes wrong I just don't push or "reset --hard origin/stable".
> so I'd do:
> git branch foo-stable foo # Create a branch for the backport
> git rebase --onto stable master foo-stable # Backport
>
> Now you got your backported version and can merge it to "stable".
The annoying thing is that merge step. I can create a merge
commit if I mistype things, and I do not want any
merge commits, I want to create linear history.
> Common wisdom is do things the other way around though. Create the
> bugfix for the oldest branch that it applies to, then merge it forward,
> either doing:
>
> "bugfix -> stable" and "stable -> master" merges, or
> "bugfix -> stable" and "bugfix -> master" merges.
>
> That approach has the advantage that you don't get multiple commits
> doing the same thing, which you get with rebasing/cherry-picking.
>
> IIRC the gitworkflows manpage describe that in some more detail.
I know. The advantage of making all changes to master first
is that this way a change gets more review and testing before
being applied to stable. Further, often different people
maintain master and stable branches.
> > - Imagine developing a bugfix/feature patchset on master branch.
> > Then I decide the patchset is too large/unsafe and want to
> > switch it to staging branch.
>
> Hm, so you have a topic branch "foo" based upon master, but it's too
> experimental so you don't want to merge it to master, but "staging". I
> don't see why you even have to rebase it then. "staging" is likely ahead
> of master, so the merge base of "foo" and "master" is also reachable
> through "staging", and simply merging "foo" to "staging" should work
> without any ill-effects.
Yes but I want my development history to be linear,
so that format patch rebase -i etc work well.
> > - I have a large queue of patches on staging branch, I decide that
> > a range of patches is mature enough for master.
>
> Basically, same deal as with the first two cases. If the series is
> directly on "staging" (i.e. you didn't create a topic branch), you can
> create one now:
> git branch foo $last_commit_for_foo
> git rebase --onto master $first_commit_for_foo^ foo
>
> And you got your backported topic branch for "foo".
>
> Or you already have a topic branch "foo-staging", but it's based upon
> some commit only in "staging" but not in "master", so a plain merge
> would mess things up. Same deal as with backporting from "master" to
> "stable"
Yes, I understand that creating a temporary branch and checking it out
then merging it will make rebase do what I want.
The only disadvantage is that I need to remember where I am in the
process, while an "atomic" command does this for me.
> And in this case it's also true that basing the topic branches on
> "master" instead of "staging" makes things easier. That way, you can
> merge to either "staging" or "master" without any ill-effects.
>
> Björn
As above, I do not want merges.
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-09 14:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Matthieu Moy, Junio C Hamano, git
In-Reply-To: <20091209134535.GK2977@redhat.com>
On 2009.12.09 15:45:36 +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 09, 2009 at 02:30:06PM +0100, Matthieu Moy wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > So perhaps a good way to move forward is to teach "git cherry-pick A..B"
> > > to be a thin wrapper that invokes a new hidden mode of operation added to
> > > "rebase" that is not advertised to the end user.
> > >
> > > I would suggest calling the option to invoke that hidden mode not
> > > "--revisions", but "--reverse" or "--opposite" or something of that
> > > nature, though. It makes "rebase" work in different direction.
> >
> > Intuitively,
> >
> > git rebase --reverse A..B
> >
> > would mean "take the range A..B, and start applying the patches from
> > B, going in reverse order up to A", like "git log --reverse". So, I'd
> > find it misleading.
> >
> > Perhaps "git rebase --cherry-pick A..B" would be a better name. No
> > objection for --opposite either.
>
> I relly like --cherry-pick. Junio, objections to that one?
Hm, there's also (the probably not so well known)
"git rev-list --cherry-pick A...B", which excludes commits that appear
on both A and B and have the same patch id. I'd rather call the rev-list
option a misnomer than the suggested hidden option for rebase, but I'd
call it --cherry-pick-mode or --cherry-picking (like am's hidden "git am
--rebasing"), just to make sure... Of course, it's not _that_ important,
as it's going to be a hidden option, so user confusion is probably not
that much of a concern.
Björn
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Michael S. Tsirkin @ 2009-12-09 13:45 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, git
In-Reply-To: <vpqiqcgp95t.fsf@bauges.imag.fr>
On Wed, Dec 09, 2009 at 02:30:06PM +0100, Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > So perhaps a good way to move forward is to teach "git cherry-pick A..B"
> > to be a thin wrapper that invokes a new hidden mode of operation added to
> > "rebase" that is not advertised to the end user.
> >
> > I would suggest calling the option to invoke that hidden mode not
> > "--revisions", but "--reverse" or "--opposite" or something of that
> > nature, though. It makes "rebase" work in different direction.
>
> Intuitively,
>
> git rebase --reverse A..B
>
> would mean "take the range A..B, and start applying the patches from
> B, going in reverse order up to A", like "git log --reverse". So, I'd
> find it misleading.
>
> Perhaps "git rebase --cherry-pick A..B" would be a better name. No
> objection for --opposite either.
I relly like --cherry-pick. Junio, objections to that one?
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-09 13:41 UTC (permalink / raw)
To: Peter Krefting; +Cc: Michael S. Tsirkin, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.00.0912091414460.470@ds9.cixit.se>
On 2009.12.09 14:20:23 +0100, Peter Krefting wrote:
> Björn Steinbrink:
>
> >Err, no. "git merge --squash foo" merges all changes from the
> >merge base of HEAD and foo up to foo. "git cherry-pick foo" takes
> >just the changes
> > from foo^ to foo.
>
> Exactly!
>
> What I meant to say in the original message was that conceptually,
> the difference between a "git rebase --reverse A..B", a "git
> cherry-pick A..B" and a "git merge --squash A..B" is minute.
>
> And when continuing the thought experiment, the step from "git merge
> --squash A..B" to "git merge A..B" is not very large either, just (a
> lot) more difficult to implement.
"git merge" is about merging histories. --squash and the A..B you
suggest make it degenerate into merging changes (and you can't record
that using the commit DAG). So that messes things up conceptually.
Implementing probably wouldn't be that hard, IIRC it should be a matter
of messing with the fake merge base that cherry-pick uses to get its job
done. While "git cherry-pick foo" uses foo^ as the merge base, you'd
just make "git merge --squash A..B" work like "git cherry-pick B" but
use A as the fake merge base. It's been a while since I looked at the
cherry-pick code though, so I might be off here.
(Kind of ironic though that I didn't think of that when I said that
"cherry-pick --squash" would be hard to do...)
Anyway, "git merge" with a range simply makes no sense at all given how
git's merge works (opposed to svn's idea of merging, which is about
changes, not about histories). If you want a squash flag, tell
cherry-pick to handle ranges and teach such a flag to it.
Björn
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Matthieu Moy @ 2009-12-09 13:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael S. Tsirkin, git
In-Reply-To: <7vfx7lcj18.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> So perhaps a good way to move forward is to teach "git cherry-pick A..B"
> to be a thin wrapper that invokes a new hidden mode of operation added to
> "rebase" that is not advertised to the end user.
>
> I would suggest calling the option to invoke that hidden mode not
> "--revisions", but "--reverse" or "--opposite" or something of that
> nature, though. It makes "rebase" work in different direction.
Intuitively,
git rebase --reverse A..B
would mean "take the range A..B, and start applying the patches from
B, going in reverse order up to A", like "git log --reverse". So, I'd
find it misleading.
Perhaps "git rebase --cherry-pick A..B" would be a better name. No
objection for --opposite either.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Peter Krefting @ 2009-12-09 13:20 UTC (permalink / raw)
To: Björn Steinbrink
Cc: Michael S. Tsirkin, Junio C Hamano, Git Mailing List
In-Reply-To: <20091209112237.GA27740@atjola.homenet>
Björn Steinbrink:
> Err, no. "git merge --squash foo" merges all changes from the merge base
> of HEAD and foo up to foo. "git cherry-pick foo" takes just the changes
> from foo^ to foo.
Exactly!
What I meant to say in the original message was that conceptually, the
difference between a "git rebase --reverse A..B", a "git cherry-pick A..B"
and a "git merge --squash A..B" is minute.
And when continuing the thought experiment, the step from "git merge
--squash A..B" to "git merge A..B" is not very large either, just (a
lot) more difficult to implement.
--
\\// Peter - http://www.softwolves.pp.se/
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-09 13:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: git, Junio C Hamano
In-Reply-To: <20091208200017.GA827@redhat.com>
On 2009.12.08 22:00:17 +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 08, 2009 at 08:11:07PM +0100, Björn Steinbrink wrote:
> > So you can already do what you want to do, but wrapping it in a single
> > porcelain might still be useful because it's obviously a lot easier and
> > safer that way. That said, I wonder what kind of workflow you're using
> > though, and why you require that feature. I've never needed something
> > like that.
>
> I need this often for many reasons:
> - Imagine developing a patchset with a complex bugfix on master branch.
> Then I decide to also apply (backport) this patchset to stable branch.
Hm, I'd also imagine that you want a separate branch then, and not
directly mess up the stable branch, so I'd do:
git branch foo-stable foo # Create a branch for the backport
git rebase --onto stable master foo-stable # Backport
Now you got your backported version and can merge it to "stable".
Common wisdom is do things the other way around though. Create the
bugfix for the oldest branch that it applies to, then merge it forward,
either doing:
"bugfix -> stable" and "stable -> master" merges, or
"bugfix -> stable" and "bugfix -> master" merges.
That approach has the advantage that you don't get multiple commits
doing the same thing, which you get with rebasing/cherry-picking.
IIRC the gitworkflows manpage describe that in some more detail.
> - Imagine developing a bugfix/feature patchset on master branch.
> Then I decide the patchset is too large/unsafe and want to
> switch it to staging branch.
Hm, so you have a topic branch "foo" based upon master, but it's too
experimental so you don't want to merge it to master, but "staging". I
don't see why you even have to rebase it then. "staging" is likely ahead
of master, so the merge base of "foo" and "master" is also reachable
through "staging", and simply merging "foo" to "staging" should work
without any ill-effects.
> - I have a large queue of patches on staging branch, I decide that
> a range of patches is mature enough for master.
Basically, same deal as with the first two cases. If the series is
directly on "staging" (i.e. you didn't create a topic branch), you can
create one now:
git branch foo $last_commit_for_foo
git rebase --onto master $first_commit_for_foo^ foo
And you got your backported topic branch for "foo".
Or you already have a topic branch "foo-staging", but it's based upon
some commit only in "staging" but not in "master", so a plain merge
would mess things up. Same deal as with backporting from "master" to
"stable"
And in this case it's also true that basing the topic branches on
"master" instead of "staging" makes things easier. That way, you can
merge to either "staging" or "master" without any ill-effects.
Björn
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-09 13:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andreas Schwab, Peter Krefting, Junio C Hamano, Git Mailing List
In-Reply-To: <20091209120748.GI2977@redhat.com>
On 2009.12.09 14:07:48 +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 09, 2009 at 01:06:10PM +0100, Björn Steinbrink wrote:
> > On 2009.12.09 12:48:24 +0100, Andreas Schwab wrote:
> > > Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> > >
> > > > Err, no. "git merge --squash foo" merges all changes from the merge base
> > > > of HEAD and foo up to foo. "git cherry-pick foo" takes just the changes
> > > > from foo^ to foo. For example:
> > > >
> > > > A---B---C (master)
> > > > \
> > > > D---E---F (foo)
> > > >
> > > > git cherry-pick foo # Tries to create a new commit with the changes from
> > > > # "git diff D F"
> > >
> > > Did you mean "git diff E F"?
> >
> > Ugh, yes, of course. Thanks.
>
> So this will be best written as
> git cherry-pick ..foo
No, "git cherry-pick ..foo" should pick the individual commits, and not
create a single big commit like "git merge --squash". So such a command
should make you end up with:
A---B---C---D'--E'--F' (master)
\
D---E---F
Not:
A---B---C---M (master)
\
D---E---F (foo)
[M being the "sqash-merge"]
"merge --squash" is one of the things I really dislike, because it turns
off the "history" part of the merge. You can say "Merging in git is about
histories, merging in svn is about changes only" to describe the major
difference for the merge commands in the two systems... "But then
there's --squash which turns git into svn".
I think a "cherry-pick --squash <range>" command would be nicer from a
conceptual point of view, but it's way too late for merge --squash to be
dropped. And I guess it wouldn't be trivial to add such a flag, and not
worth the effort, as you could as well use the interactive mode and
replace "pick" with "squash" manually. (An el cheapo implementation that
automatically replaces it would likely confuse the user, because he
asked for a single commit, but might get to fix conflicts for all the
individual commits).
Björn
^ permalink raw reply
* Re: help: bisect single file from repos
From: walter harms @ 2009-12-09 12:12 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Nanako Shiraishi, Junio C Hamano, git, Christian Couder,
Michael J Gruber
In-Reply-To: <20091209094532.GS18686@neumann>
SZEDER Gábor schrieb:
> Hi,
>
>
> On Wed, Dec 09, 2009 at 05:27:37PM +0900, Nanako Shiraishi wrote:
>> Quoting SZEDER Gábor <szeder@ira.uka.de>
>>
>>> [1] - 'git cherry-pick' doc says the following:
>>>
>>> <commit>
>>> Commit to cherry-pick. For a more complete list of ways to spell
>>> commits, see the "SPECIFYING REVISIONS" section in git-rev-parse(1).
>>>
>>> What? "A _more_ complete list"!? Well, it's not very hard to be more
>>> complete than this, there is not a single way described here (;
>
>> I agree that "more" shouldn't be in that sentence, and I understand
>> your hesitation to read plumbing manual pages, but I don't think it
>> is a sane solution to the issue to repeat how to name a commit in
>> manual pages for every single command to bloat the two line
>> description you quoted into a half-page paragraph. Even within that
>> two lines, the real information that should be in the manual for
>> cherry-pick is only three words "Commit to cherry-pick" and the rest
>> is to help people who don't know.
>
> I agree, that's why I proposed "a _section_ about specifying these
> commits" in the more relevant part of my previous email you did not
> quote.
>
> The description of the "<commit>" option would remain almost the same,
> but it will now refer to a dedicated section about specifying commits
> below, but still in the same manpage. This new dedicated section
> would contain the list of three, five, N most common ways to specify a
> commit, avoiding the bloatage in the options section. And for those
> who really want to dig deep, this dedicated section will refer to 'git
> rev-parse' for the complete list.
>
> And this would not be the first time we document something in many
> places, think of '--pretty' and diff options, for example.
>
>
It would be no problem when you have the description multiple times.
Important is that they use the same words for the same things
and add examples. Most people that use git have a fair idea what they
want but not how to do it. git is new an you can not assume that
even basic principles are known to the general (programmer) community.
So you need to make extra effort to explain it all over again.
re,
wh
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Michael S. Tsirkin @ 2009-12-09 12:07 UTC (permalink / raw)
To: Björn Steinbrink
Cc: Andreas Schwab, Peter Krefting, Junio C Hamano, Git Mailing List
In-Reply-To: <20091209120610.GA29430@atjola.homenet>
On Wed, Dec 09, 2009 at 01:06:10PM +0100, Björn Steinbrink wrote:
> On 2009.12.09 12:48:24 +0100, Andreas Schwab wrote:
> > Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> >
> > > Err, no. "git merge --squash foo" merges all changes from the merge base
> > > of HEAD and foo up to foo. "git cherry-pick foo" takes just the changes
> > > from foo^ to foo. For example:
> > >
> > > A---B---C (master)
> > > \
> > > D---E---F (foo)
> > >
> > > git cherry-pick foo # Tries to create a new commit with the changes from
> > > # "git diff D F"
> >
> > Did you mean "git diff E F"?
>
> Ugh, yes, of course. Thanks.
>
> Björn
So this will be best written as
git cherry-pick ..foo
--
MST
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-09 12:06 UTC (permalink / raw)
To: Andreas Schwab
Cc: Peter Krefting, Michael S. Tsirkin, Junio C Hamano,
Git Mailing List
In-Reply-To: <m2pr6ocqrb.fsf@igel.home>
On 2009.12.09 12:48:24 +0100, Andreas Schwab wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
>
> > Err, no. "git merge --squash foo" merges all changes from the merge base
> > of HEAD and foo up to foo. "git cherry-pick foo" takes just the changes
> > from foo^ to foo. For example:
> >
> > A---B---C (master)
> > \
> > D---E---F (foo)
> >
> > git cherry-pick foo # Tries to create a new commit with the changes from
> > # "git diff D F"
>
> Did you mean "git diff E F"?
Ugh, yes, of course. Thanks.
Björn
^ 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