* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-15 6:22 UTC (permalink / raw)
To: Jardel Weyrich; +Cc: Michael J Gruber, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <1D472234-A0A5-4F02-878D-D05DEE995FCD@gmail.com>
Jardel Weyrich <jweyrich@gmail.com> writes:
> If you allow me, I'd like you to forget about the concepts for a minute, and focus on the user experience.
> Imagine a simple hypothetical scenario in which the user wants to push to 2 distinct repositories. He already has cloned the repo from the 1st repository, thus (theoretically) all he needs to do, is to add a new repository for push. He then uses `remote set-url --add --push <2nd-repo>` (which I personally thought would suffice). However, if he tries to push a new commit to this remote, it would be pushed _only_ to the 2nd-repo.
The primary reason behind push-url was that
(1) usually you push to and fetch from the same, so no pushUrl is
ever needed, just a single Url will do (this is often true for
cvs/svn style shared repository workflow); and
(2) sometimes you want to fetch from one place and push to another
(this is often true for "fetch from upstream, push to my own
and ask upstream to pull from it" workflow), and in that case
you want pushUrl in addition to Url. Most importantly, in this
case, you do *NOT* want to push to Url. You only push to
pushUrl.
Setting *one* pushURL is a way to say "That URL I fetch from is
*not* the place I want to push (I may not even be able to push
there); when I say 'push', push there instead". Your proposed
semantics will make it impossible to arrange such an asymmetric
setting.
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-15 6:39 UTC (permalink / raw)
To: Jardel Weyrich; +Cc: Michael J Gruber, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <7vpq1755jb.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jardel Weyrich <jweyrich@gmail.com> writes:
>
>> If you allow me, I'd like you to forget about the concepts for a minute, and focus on the user experience.
>> Imagine a simple hypothetical scenario in which the user wants to push to 2 distinct repositories. He already has cloned the repo from the 1st repository, thus (theoretically) all he needs to do, is to add a new repository for push. He then uses `remote set-url --add --push <2nd-repo>` (which I personally thought would suffice). However, if he tries to push a new commit to this remote, it would be pushed _only_ to the 2nd-repo.
>
> The primary reason behind push-url was that
>
> (1) usually you push to and fetch from the same, so no pushUrl is
> ever needed, just a single Url will do (this is often true for
> cvs/svn style shared repository workflow); and
>
> (2) sometimes you want to fetch from one place and push to another
> (this is often true for "fetch from upstream, push to my own
> and ask upstream to pull from it" workflow), and in that case
> you want pushUrl in addition to Url. Most importantly, in this
> case, you do *NOT* want to push to Url. You only push to
> pushUrl.
>
> Setting *one* pushURL is a way to say "That URL I fetch from is
> *not* the place I want to push (I may not even be able to push
> there); when I say 'push', push there instead". Your proposed
> semantics will make it impossible to arrange such an asymmetric
> setting.
Now I think I finally see where that misunderstanding comes from.
It is "remote -v" that is misdesigned.
$ git clone ../there here
$ cd here
$ git remote -v
origin /var/tmp/there (fetch)
origin /var/tmp/there (push)
This is totally bogus. It should report something like this:
$ git remote -v
origin /var/tmp/there (fetch/push)
Then after running "git remote set-url --push origin ../another" we
should see
$ git remote -v
origin /var/tmp/there (fetch)
origin /var/tmp/another (push)
which would make it clear that the original fetch/push came from the
(1) usuall you push and fetch from the same place so there is only
one setting, and the two lines came from the (2) sometimes you need
a separate places to fetch from and push to.
At this point, if you say "set-url --push origin ../third", then
"another" will disappear and gets replaced by "third"; if you
instead say "set-url --add --push origin ../third", then we will see
two (push) lines, in addition to one (fetch), making it clear that
you are still in (2) above, fetching from and pushing to different
places, and having two places to push to.
I misread your response
From: Jardel Weyrich <jweyrich@gmail.com>
Subject: Re: [BUG] Possible bug in `remote set-url --add --push`
Date: Sat, 12 Jan 2013 06:09:35 -0200
Message-ID: <CAN8TAOvP_HX6BEK86aYoX-kVqWDmsbyptxTT2nk+fx+Ut1Tojg@mail.gmail.com>
where you showed that there was only remote.origin.url (and no
pushurl) in the first step, and somehow thought you had a
remote.origin.pushurl in the first place.
^ permalink raw reply
* Re: [PATCH 3/3] cvsimport: start adding cvsps 3.x support
From: Junio C Hamano @ 2013-01-15 6:44 UTC (permalink / raw)
To: Chris Rorvick; +Cc: git, jrnieder, mhagger, esr
In-Reply-To: <CAEUsAPZV6rdFz5R6NN55qYr5se4bFJftE0xGSPAtXLp8jcO0vw@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
[jc: please elide parts you are not responding to, leaving enough
lines to understand the context]
>> + def command(self):
>> + "Emit the command implied by all previous options."
>> + return self.cvsps + "--fast-export " + self.opts
>
> "--fast-export" string is missing a leading space. With this fix and
> the latest cvsps build I'm seeing 6 of 15 failures for t9650 which is
> what I was getting out of the patched t9600.
Thanks; I'll amend it and push the result out, but I think we will
need to rip this part out and form the command string in a saner way
(e.g. if the path needs to have SP in it, e.g. "/Program Files/cvsps3",
the above would not work correctly anyway).
^ permalink raw reply
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Johannes Sixt @ 2013-01-15 7:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robin Rosenberg, git
In-Reply-To: <7vy5fv71ad.fsf@alter.siamese.dyndns.org>
Am 1/15/2013 1:11, schrieb Junio C Hamano:
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.
It was my suggestion to have a list of names to ignore because the answer
to this question
> Is this "the user edits in eclipse and then runs 'git status' from the
> terminal" problem?
was "It is purely for performance in some situations" back then. But
today, the answer is "Yes". With this new background, your suggestion to
have just a single option that contains the token "jgit" may make more
sense. (core.ignoreCygwinFSTricks may serve as a precedent.) The original
patch was along this way, and the name contained "minimal", which I
objected to.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Robin Rosenberg @ 2013-01-15 7:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, j sixt, Shawn Pearce
In-Reply-To: <7vy5fv71ad.fsf@alter.siamese.dyndns.org>
----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>
> > Semantically they're somewhat different. My flags are for ignoring
> > a value when it's not used as indicated by the value zero, while
> > trustctime is for ignoring untrustworthy, non-zero, values.
>
> Yeah, I realized that after writing that message.
>
> > Another thing that I noticed, is that I probably wanto to be able
> > to filter on the precision
> > of timestamps. Again, this i JGit-related. Current JGit has
> > milliseconds precision (max), whereas
> > Git has down to nanosecond precision in timestamps. Newer JGits may
> > get nanoseconds timestamps too,
> > but on current Linux versions JGit gets only integral seconds
> > regardless of file system.
> >
> > Would the names, milli, micro, nano be good for ignoring the tail
> > when zero, or n1..n9 (obviously
> > n2 would be ok too). nN = ignore all but first N nsec digits if
> > they are zero)?
>
> It somehow starts to sound like over-engineering to solve a wrong
> problem.
>
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.
My first patch was something like that, just not using the word jgit. As
for what fields to ignore, it's something that can be configured by EGit
and documented on the EGit/JGit wiki.
-- robin
^ permalink raw reply
* Re: git grep performance regression
From: Ross Lagerwall @ 2013-01-15 7:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git, Jean-Noël AVILA, Junio C Hamano
In-Reply-To: <CACsJy8AnABCisgSVL7Qh_-uejAJA3x1kzy=4i+uXm3+90m5tuA@mail.gmail.com>
On Tue, Jan 15, 2013 at 11:38:32AM +0700, Duy Nguyen wrote:
> dirlen is not expected to include the trailing slash, but
> find_basename() does that. It messes up with the path filters for
> push/pop in the next code. This brings grep performance closely back
> to before for me. Ross, can you check (patch could be whitespace
> damaged by gmail)?
>
Yes, that did seem to bring back the performance to the old level.
I noticed that before the patch, the strace output was something like
this:
open("tools/vm/.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("tools/vm//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr/.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
open("usr//.gitattributes", O_RDONLY) = -1 ENOENT
(and yes, the whitespace was damaged by Gmail!)
Regards
--
Ross Lagerwall
^ permalink raw reply
* [PATCH v2 00/14] Remove unused code from imap-send.c
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
This is a re-roll, incorporating the feedback of Jonathan Nieder
(thanks!).
Differences from v1:
* Added comments to get_cmd_result() at the place where the
"NAMESPACE" response is skipped over.
* Added some comments to lf_to_crlf(), simplified the code a bit
further, and expanded the commit message.
* Replaced erroneously-deleted space in "APPEND" command in
imap_store_msg().
I also moved the patch rewriting lf_to_crlf() to the end of the
series, because it is not just dead-code elimination like the others.
Michael Haggerty (14):
imap-send.c: remove msg_data::flags, which was always zero
imap-send.c: remove struct msg_data
iamp-send.c: remove unused struct imap_store_conf
imap-send.c: remove struct store_conf
imap-send.c: remove struct message
imap-send.c: remove some unused fields from struct store
imap-send.c: inline imap_parse_list() in imap_list()
imap-send.c: remove struct imap argument to parse_imap_list_l()
imap-send.c: remove namespace fields from struct imap
imap-send.c: remove unused field imap_store::trashnc
imap-send.c: use struct imap_store instead of struct store
imap-send.c: remove unused field imap_store::uidvalidity
imap-send.c: fold struct store into struct imap_store
imap-send.c: simplify logic in lf_to_crlf()
imap-send.c | 308 +++++++++++-------------------------------------------------
1 file changed, 55 insertions(+), 253 deletions(-)
--
1.8.0.3
^ permalink raw reply
* [PATCH v2 03/14] iamp-send.c: remove unused struct imap_store_conf
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 29c10a4..dbe0546 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -130,11 +130,6 @@ static struct imap_server_conf server = {
NULL, /* auth_method */
};
-struct imap_store_conf {
- struct store_conf gen;
- struct imap_server_conf *server;
-};
-
#define NIL (void *)0x1
#define LIST (void *)0x2
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 04/14] imap-send.c: remove struct store_conf
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was never used.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index dbe0546..b8a7ff9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,15 +33,6 @@ typedef void *SSL;
#include <openssl/hmac.h>
#endif
-struct store_conf {
- char *name;
- const char *path; /* should this be here? its interpretation is driver-specific */
- char *map_inbox;
- char *trash;
- unsigned max_size; /* off_t is overkill */
- unsigned trash_remote_new:1, trash_only_new:1;
-};
-
/* For message->status */
#define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
#define M_DEAD (1<<1) /* expunged */
@@ -55,8 +46,6 @@ struct message {
};
struct store {
- struct store_conf *conf; /* foreign */
-
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
char *path; /* own */
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 05/14] imap-send.c: remove struct message
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was never used.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index b8a7ff9..9e181e0 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,23 +33,10 @@ typedef void *SSL;
#include <openssl/hmac.h>
#endif
-/* For message->status */
-#define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
-#define M_DEAD (1<<1) /* expunged */
-#define M_FLAGS (1<<2) /* flags fetched */
-
-struct message {
- struct message *next;
- size_t size; /* zero implies "not fetched" */
- int uid;
- unsigned char flags, status;
-};
-
struct store {
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
char *path; /* own */
- struct message *msgs; /* own */
int uidvalidity;
unsigned char opts; /* maybe preset? */
/* note that the following do _not_ reflect stats from msgs, but mailbox totals */
@@ -74,8 +61,6 @@ static void imap_warn(const char *, ...);
static char *next_arg(char **);
-static void free_generic_messages(struct message *);
-
__attribute__((format (printf, 3, 4)))
static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
@@ -447,16 +432,6 @@ static char *next_arg(char **s)
return ret;
}
-static void free_generic_messages(struct message *msgs)
-{
- struct message *tmsg;
-
- for (; msgs; msgs = tmsg) {
- tmsg = msgs->next;
- free(msgs);
- }
-}
-
static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
{
int ret;
@@ -914,7 +889,6 @@ static void imap_close_server(struct imap_store *ictx)
static void imap_close_store(struct store *ctx)
{
imap_close_server((struct imap_store *)ctx);
- free_generic_messages(ctx->msgs);
free(ctx);
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 9e181e0..f193211 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -36,12 +36,7 @@ typedef void *SSL;
struct store {
/* currently open mailbox */
const char *name; /* foreign! maybe preset? */
- char *path; /* own */
int uidvalidity;
- unsigned char opts; /* maybe preset? */
- /* note that the following do _not_ reflect stats from msgs, but mailbox totals */
- int count; /* # of messages */
- int recent; /* # of recent messages - don't trust this beyond the initial read */
};
static const char imap_send_usage[] = "git imap-send < <mbox>";
@@ -772,13 +767,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
!strcmp("NO", arg) || !strcmp("BYE", arg)) {
if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
return resp;
- } else if (!strcmp("CAPABILITY", arg))
+ } else if (!strcmp("CAPABILITY", arg)) {
parse_capability(imap, cmd);
- else if ((arg1 = next_arg(&cmd))) {
- if (!strcmp("EXISTS", arg1))
- ctx->gen.count = atoi(arg);
- else if (!strcmp("RECENT", arg1))
- ctx->gen.recent = atoi(arg);
+ } else if ((arg1 = next_arg(&cmd))) {
+ /* unused */
} else {
fprintf(stderr, "IMAP error: unable to parse untagged response\n");
return RESP_BAD;
@@ -1254,7 +1246,6 @@ static int imap_store_msg(struct store *gctx, struct strbuf *msg)
imap->caps = imap->rcaps;
if (ret != DRV_OK)
return ret;
- gctx->count++;
return DRV_OK;
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 08/14] imap-send.c: remove struct imap argument to parse_imap_list_l()
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
It was always set to NULL.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 39 +++------------------------------------
1 file changed, 3 insertions(+), 36 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index cbbf845..29e4037 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -578,11 +578,10 @@ static void free_list(struct imap_list *list)
}
}
-static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **curp, int level)
+static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
{
struct imap_list *cur;
char *s = *sp, *p;
- int n, bytes;
for (;;) {
while (isspace((unsigned char)*s))
@@ -598,39 +597,7 @@ static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **cu
/* sublist */
s++;
cur->val = LIST;
- if (parse_imap_list_l(imap, &s, &cur->child, level + 1))
- goto bail;
- } else if (imap && *s == '{') {
- /* literal */
- bytes = cur->len = strtol(s + 1, &s, 10);
- if (*s != '}')
- goto bail;
-
- s = cur->val = xmalloc(cur->len);
-
- /* dump whats left over in the input buffer */
- n = imap->buf.bytes - imap->buf.offset;
-
- if (n > bytes)
- /* the entire message fit in the buffer */
- n = bytes;
-
- memcpy(s, imap->buf.buf + imap->buf.offset, n);
- s += n;
- bytes -= n;
-
- /* mark that we used part of the buffer */
- imap->buf.offset += n;
-
- /* now read the rest of the message */
- while (bytes > 0) {
- if ((n = socket_read(&imap->buf.sock, s, bytes)) <= 0)
- goto bail;
- s += n;
- bytes -= n;
- }
-
- if (buffer_gets(&imap->buf, &s))
+ if (parse_imap_list_l(&s, &cur->child, level + 1))
goto bail;
} else if (*s == '"') {
/* quoted string */
@@ -673,7 +640,7 @@ static struct imap_list *parse_list(char **sp)
{
struct imap_list *head;
- if (!parse_imap_list_l(NULL, sp, &head, 0))
+ if (!parse_imap_list_l(sp, &head, 0))
return head;
free_list(head);
return NULL;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 09/14] imap-send.c: remove namespace fields from struct imap
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
They are unused, and their removal means that a bunch of list-related
infrastructure can be disposed of.
It might be that the "NAMESPACE" response that is now skipped over in
get_cmd_result() should never be sent by the server. But somebody
would have to check the IMAP protocol and how we interact with the
server to be sure. So for now I am leaving that branch of the "if"
statement there.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 75 ++++++++-----------------------------------------------------
1 file changed, 9 insertions(+), 66 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 29e4037..ff44013 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -99,15 +99,6 @@ static struct imap_server_conf server = {
NULL, /* auth_method */
};
-#define NIL (void *)0x1
-#define LIST (void *)0x2
-
-struct imap_list {
- struct imap_list *next, *child;
- char *val;
- int len;
-};
-
struct imap_socket {
int fd[2];
SSL *ssl;
@@ -124,7 +115,6 @@ struct imap_cmd;
struct imap {
int uidnext; /* from SELECT responses */
- struct imap_list *ns_personal, *ns_other, *ns_shared; /* NAMESPACE info */
unsigned caps, rcaps; /* CAPABILITY results */
/* command queue */
int nexttag, num_in_progress, literal_pending;
@@ -554,34 +544,9 @@ static int imap_exec_m(struct imap_store *ctx, struct imap_cmd_cb *cb,
}
}
-static int is_atom(struct imap_list *list)
-{
- return list && list->val && list->val != NIL && list->val != LIST;
-}
-
-static int is_list(struct imap_list *list)
-{
- return list && list->val == LIST;
-}
-
-static void free_list(struct imap_list *list)
-{
- struct imap_list *tmp;
-
- for (; list; list = tmp) {
- tmp = list->next;
- if (is_list(list))
- free_list(list->child);
- else if (is_atom(list))
- free(list->val);
- free(list);
- }
-}
-
-static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
+static int skip_imap_list_l(char **sp, int level)
{
- struct imap_list *cur;
- char *s = *sp, *p;
+ char *s = *sp;
for (;;) {
while (isspace((unsigned char)*s))
@@ -590,36 +555,23 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
s++;
break;
}
- *curp = cur = xmalloc(sizeof(*cur));
- curp = &cur->next;
- cur->val = NULL; /* for clean bail */
if (*s == '(') {
/* sublist */
s++;
- cur->val = LIST;
- if (parse_imap_list_l(&s, &cur->child, level + 1))
+ if (skip_imap_list_l(&s, level + 1))
goto bail;
} else if (*s == '"') {
/* quoted string */
s++;
- p = s;
for (; *s != '"'; s++)
if (!*s)
goto bail;
- cur->len = s - p;
s++;
- cur->val = xmemdupz(p, cur->len);
} else {
/* atom */
- p = s;
for (; *s && !isspace((unsigned char)*s); s++)
if (level && *s == ')')
break;
- cur->len = s - p;
- if (cur->len == 3 && !memcmp("NIL", p, 3))
- cur->val = NIL;
- else
- cur->val = xmemdupz(p, cur->len);
}
if (!level)
@@ -628,22 +580,15 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
goto bail;
}
*sp = s;
- *curp = NULL;
return 0;
bail:
- *curp = NULL;
return -1;
}
-static struct imap_list *parse_list(char **sp)
+static void skip_list(char **sp)
{
- struct imap_list *head;
-
- if (!parse_imap_list_l(sp, &head, 0))
- return head;
- free_list(head);
- return NULL;
+ skip_imap_list_l(sp, 0);
}
static void parse_capability(struct imap *imap, char *cmd)
@@ -722,9 +667,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
}
if (!strcmp("NAMESPACE", arg)) {
- imap->ns_personal = parse_list(&cmd);
- imap->ns_other = parse_list(&cmd);
- imap->ns_shared = parse_list(&cmd);
+ /* rfc2342 NAMESPACE response. */
+ skip_list(&cmd); /* Personal mailboxes */
+ skip_list(&cmd); /* Others' mailboxes */
+ skip_list(&cmd); /* Shared mailboxes */
} else if (!strcmp("OK", arg) || !strcmp("BAD", arg) ||
!strcmp("NO", arg) || !strcmp("BYE", arg)) {
if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
@@ -834,9 +780,6 @@ static void imap_close_server(struct imap_store *ictx)
imap_exec(ictx, NULL, "LOGOUT");
socket_shutdown(&imap->buf.sock);
}
- free_list(imap->ns_personal);
- free_list(imap->ns_other);
- free_list(imap->ns_shared);
free(imap);
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 10/14] imap-send.c: remove unused field imap_store::trashnc
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index ff44013..909e4db 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -127,7 +127,6 @@ struct imap_store {
int uidvalidity;
struct imap *imap;
const char *prefix;
- unsigned /*currentnc:1,*/ trashnc:1;
};
struct imap_cmd_cb {
@@ -1080,7 +1079,6 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
} /* !preauth */
ctx->prefix = "";
- ctx->trashnc = 1;
return (struct store *)ctx;
bail:
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 12/14] imap-send.c: remove unused field imap_store::uidvalidity
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
I suspect that the existence of both imap_store::uidvalidity and
store::uidvalidity was an accident.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/imap-send.c b/imap-send.c
index 48c646c..a0f42bb 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -124,7 +124,6 @@ struct imap {
struct imap_store {
struct store gen;
- int uidvalidity;
struct imap *imap;
const char *prefix;
};
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 13/14] imap-send.c: fold struct store into struct imap_store
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index a0f42bb..f2933e9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,12 +33,6 @@ typedef void *SSL;
#include <openssl/hmac.h>
#endif
-struct store {
- /* currently open mailbox */
- const char *name; /* foreign! maybe preset? */
- int uidvalidity;
-};
-
static const char imap_send_usage[] = "git imap-send < <mbox>";
#undef DRV_OK
@@ -123,7 +117,9 @@ struct imap {
};
struct imap_store {
- struct store gen;
+ /* currently open mailbox */
+ const char *name; /* foreign! maybe preset? */
+ int uidvalidity;
struct imap *imap;
const char *prefix;
};
@@ -618,7 +614,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
*p++ = 0;
arg = next_arg(&s);
if (!strcmp("UIDVALIDITY", arg)) {
- if (!(arg = next_arg(&s)) || !(ctx->gen.uidvalidity = atoi(arg))) {
+ if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
return RESP_BAD;
}
@@ -636,7 +632,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
for (; isspace((unsigned char)*p); p++);
fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
} else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
- if (!(arg = next_arg(&s)) || !(ctx->gen.uidvalidity = atoi(arg)) ||
+ if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
!(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
return RESP_BAD;
@@ -1140,7 +1136,7 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)
cb.dlen = msg->len;
cb.data = strbuf_detach(msg, NULL);
- box = ctx->gen.name;
+ box = ctx->name;
prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
cb.create = 0;
ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
@@ -1352,7 +1348,7 @@ int main(int argc, char **argv)
}
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
- ctx->gen.name = imap_folder;
+ ctx->name = imap_folder;
while (1) {
unsigned percent = n * 100 / total;
--
1.8.0.3
^ permalink raw reply related
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Junio C Hamano @ 2013-01-15 8:12 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, j sixt, Shawn Pearce
In-Reply-To: <1119893992.2134035.1358233781666.JavaMail.root@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>> I'd say a simplistic "ignore if zero is stored" or even "ignore this
>> as one of the systems that shares this file writes crap in it" may
>> be sufficient, and if this is a jGit specific issue, it might even
>> make sense to introduce a single configuration variable with string
>> "jgit" somewhere in its name and bypass the stat field comparison
>> for known-problematic fields, instead of having the user know and
>> list what stat fields need special attention.
>
> My first patch was something like that, just not using the word jgit. As
> for what fields to ignore, it's something that can be configured by EGit
> and documented on the EGit/JGit wiki.
That configurability is a slipperly slope to drag us into giving users
more complexity that does not help them very much, I suspect.
Earlier somebody mentioned "size and mtime is often enough", so I
think a single option core.looseStatInfo (substitute "loose" with
short, minimum or whatever adjective that is more appropriate---I am
not good at picking phrases, it sounds to me a way to more loosely
define stat info cleanliness than we usually do) that makes us
ignore all fields (regardless of their zero-ness) other than those
two fields might not be a bad way to go.
I do not offhand know if such a loose mode is too simple and make it
excessively risky, though.
^ permalink raw reply
* [PATCH v2 01/14] imap-send.c: remove msg_data::flags, which was always zero
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
This removes the need for function imap_make_flags(), so delete it,
too.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 40 +++-------------------------------------
1 file changed, 3 insertions(+), 37 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index e521e2f..f1c8f5a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -70,7 +70,6 @@ struct store {
struct msg_data {
struct strbuf data;
- unsigned char flags;
};
static const char imap_send_usage[] = "git imap-send < <mbox>";
@@ -225,14 +224,6 @@ static const char *cap_list[] = {
static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd);
-static const char *Flags[] = {
- "Draft",
- "Flagged",
- "Answered",
- "Seen",
- "Deleted",
-};
-
#ifndef NO_OPENSSL
static void ssl_socket_perror(const char *func)
{
@@ -1246,23 +1237,6 @@ bail:
return NULL;
}
-static int imap_make_flags(int flags, char *buf)
-{
- const char *s;
- unsigned i, d;
-
- for (i = d = 0; i < ARRAY_SIZE(Flags); i++)
- if (flags & (1 << i)) {
- buf[d++] = ' ';
- buf[d++] = '\\';
- for (s = Flags[i]; *s; s++)
- buf[d++] = *s;
- }
- buf[0] = '(';
- buf[d++] = ')';
- return d;
-}
-
static void lf_to_crlf(struct strbuf *msg)
{
size_t new_len;
@@ -1311,8 +1285,7 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
struct imap *imap = ctx->imap;
struct imap_cmd_cb cb;
const char *prefix, *box;
- int ret, d;
- char flagstr[128];
+ int ret;
lf_to_crlf(&msg->data);
memset(&cb, 0, sizeof(cb));
@@ -1320,17 +1293,10 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
cb.dlen = msg->data.len;
cb.data = strbuf_detach(&msg->data, NULL);
- d = 0;
- if (msg->flags) {
- d = imap_make_flags(msg->flags, flagstr);
- flagstr[d++] = ' ';
- }
- flagstr[d] = 0;
-
box = gctx->name;
prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
cb.create = 0;
- ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" %s", prefix, box, flagstr);
+ ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
imap->caps = imap->rcaps;
if (ret != DRV_OK)
return ret;
@@ -1483,7 +1449,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)
int main(int argc, char **argv)
{
struct strbuf all_msgs = STRBUF_INIT;
- struct msg_data msg = {STRBUF_INIT, 0};
+ struct msg_data msg = {STRBUF_INIT};
struct store *ctx = NULL;
int ofs = 0;
int r;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
The function is only called from here.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index f193211..cbbf845 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -669,21 +669,16 @@ bail:
return -1;
}
-static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
+static struct imap_list *parse_list(char **sp)
{
struct imap_list *head;
- if (!parse_imap_list_l(imap, sp, &head, 0))
+ if (!parse_imap_list_l(NULL, sp, &head, 0))
return head;
free_list(head);
return NULL;
}
-static struct imap_list *parse_list(char **sp)
-{
- return parse_imap_list(NULL, sp);
-}
-
static void parse_capability(struct imap *imap, char *cmd)
{
char *arg;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 02/14] imap-send.c: remove struct msg_data
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Now that its flags member has been deleted, all that is left is a
strbuf. So use a strbuf directly.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index f1c8f5a..29c10a4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -68,10 +68,6 @@ struct store {
int recent; /* # of recent messages - don't trust this beyond the initial read */
};
-struct msg_data {
- struct strbuf data;
-};
-
static const char imap_send_usage[] = "git imap-send < <mbox>";
#undef DRV_OK
@@ -1279,7 +1275,7 @@ static void lf_to_crlf(struct strbuf *msg)
* Store msg to IMAP. Also detach and free the data from msg->data,
* leaving msg->data empty.
*/
-static int imap_store_msg(struct store *gctx, struct msg_data *msg)
+static int imap_store_msg(struct store *gctx, struct strbuf *msg)
{
struct imap_store *ctx = (struct imap_store *)gctx;
struct imap *imap = ctx->imap;
@@ -1287,11 +1283,11 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
const char *prefix, *box;
int ret;
- lf_to_crlf(&msg->data);
+ lf_to_crlf(msg);
memset(&cb, 0, sizeof(cb));
- cb.dlen = msg->data.len;
- cb.data = strbuf_detach(&msg->data, NULL);
+ cb.dlen = msg->len;
+ cb.data = strbuf_detach(msg, NULL);
box = gctx->name;
prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
@@ -1449,7 +1445,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)
int main(int argc, char **argv)
{
struct strbuf all_msgs = STRBUF_INIT;
- struct msg_data msg = {STRBUF_INIT};
+ struct strbuf msg = STRBUF_INIT;
struct store *ctx = NULL;
int ofs = 0;
int r;
@@ -1511,10 +1507,10 @@ int main(int argc, char **argv)
unsigned percent = n * 100 / total;
fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
- if (!split_msg(&all_msgs, &msg.data, &ofs))
+ if (!split_msg(&all_msgs, &msg, &ofs))
break;
if (server.use_html)
- wrap_in_html(&msg.data);
+ wrap_in_html(&msg);
r = imap_store_msg(ctx, &msg);
if (r != DRV_OK)
break;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 11/14] imap-send.c: use struct imap_store instead of struct store
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
In fact, all struct store instances are upcasts of struct imap_store
anyway, so stop making the distinction.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 909e4db..48c646c 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -782,9 +782,9 @@ static void imap_close_server(struct imap_store *ictx)
free(imap);
}
-static void imap_close_store(struct store *ctx)
+static void imap_close_store(struct imap_store *ctx)
{
- imap_close_server((struct imap_store *)ctx);
+ imap_close_server(ctx);
free(ctx);
}
@@ -869,7 +869,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
return 0;
}
-static struct store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
{
struct imap_store *ctx;
struct imap *imap;
@@ -1079,10 +1079,10 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
} /* !preauth */
ctx->prefix = "";
- return (struct store *)ctx;
+ return ctx;
bail:
- imap_close_store(&ctx->gen);
+ imap_close_store(ctx);
return NULL;
}
@@ -1128,9 +1128,8 @@ static void lf_to_crlf(struct strbuf *msg)
* Store msg to IMAP. Also detach and free the data from msg->data,
* leaving msg->data empty.
*/
-static int imap_store_msg(struct store *gctx, struct strbuf *msg)
+static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)
{
- struct imap_store *ctx = (struct imap_store *)gctx;
struct imap *imap = ctx->imap;
struct imap_cmd_cb cb;
const char *prefix, *box;
@@ -1142,7 +1141,7 @@ static int imap_store_msg(struct store *gctx, struct strbuf *msg)
cb.dlen = msg->len;
cb.data = strbuf_detach(msg, NULL);
- box = gctx->name;
+ box = ctx->gen.name;
prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
cb.create = 0;
ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
@@ -1298,7 +1297,7 @@ int main(int argc, char **argv)
{
struct strbuf all_msgs = STRBUF_INIT;
struct strbuf msg = STRBUF_INIT;
- struct store *ctx = NULL;
+ struct imap_store *ctx = NULL;
int ofs = 0;
int r;
int total, n = 0;
@@ -1354,7 +1353,7 @@ int main(int argc, char **argv)
}
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
- ctx->name = imap_folder;
+ ctx->gen.name = imap_folder;
while (1) {
unsigned percent = n * 100 / total;
--
1.8.0.3
^ permalink raw reply related
* [PATCH v2 14/14] imap-send.c: simplify logic in lf_to_crlf()
From: Michael Haggerty @ 2013-01-15 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
* The first character in the string used to be special-cased to get
around the fact that msg->buf[i - 1] is not defined for i == 0.
Instead, keep track of the previous character in a separate
variable, "lastc", initialized in such a way to let the loop handle
i == 0 correctly.
* Make the two loops over the string look as similar as possible to
make it more obvious that the count computed in the first pass
agrees with the true length of the new string written in the second
pass. As a side effect, this makes it possible to use the "j"
counter in place of lfnum and new_len.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
imap-send.c | 52 +++++++++++++++++++++++-----------------------------
1 file changed, 23 insertions(+), 29 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index f2933e9..1d40207 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1081,42 +1081,36 @@ bail:
return NULL;
}
+/*
+ * Insert CR characters as necessary in *msg to ensure that every LF
+ * character in *msg is preceded by a CR.
+ */
static void lf_to_crlf(struct strbuf *msg)
{
- size_t new_len;
char *new;
- int i, j, lfnum = 0;
-
- if (msg->buf[0] == '\n')
- lfnum++;
- for (i = 1; i < msg->len; i++) {
- if (msg->buf[i - 1] != '\r' && msg->buf[i] == '\n')
- lfnum++;
+ size_t i, j;
+ char lastc;
+
+ /* First pass: tally, in j, the size of the new string: */
+ for (i = j = 0, lastc = '\0'; i < msg->len; i++) {
+ if (msg->buf[i] == '\n' && lastc != '\r')
+ j++; /* a CR will need to be added here */
+ lastc = msg->buf[i];
+ j++;
}
- new_len = msg->len + lfnum;
- new = xmalloc(new_len + 1);
- if (msg->buf[0] == '\n') {
- new[0] = '\r';
- new[1] = '\n';
- i = 1;
- j = 2;
- } else {
- new[0] = msg->buf[0];
- i = 1;
- j = 1;
- }
- for ( ; i < msg->len; i++) {
- if (msg->buf[i] != '\n') {
- new[j++] = msg->buf[i];
- continue;
- }
- if (msg->buf[i - 1] != '\r')
+ new = xmalloc(j + 1);
+
+ /*
+ * Second pass: write the new string. Note that this loop is
+ * otherwise identical to the first pass.
+ */
+ for (i = j = 0, lastc = '\0'; i < msg->len; i++) {
+ if (msg->buf[i] == '\n' && lastc != '\r')
new[j++] = '\r';
- /* otherwise it already had CR before */
- new[j++] = '\n';
+ lastc = new[j++] = msg->buf[i];
}
- strbuf_attach(msg, new, new_len, new_len + 1);
+ strbuf_attach(msg, new, j, j + 1);
}
/*
--
1.8.0.3
^ permalink raw reply related
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Michael J Gruber @ 2013-01-15 9:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jardel Weyrich, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <7vip6z54rh.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 15.01.2013 07:39:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jardel Weyrich <jweyrich@gmail.com> writes:
>>
>>> If you allow me, I'd like you to forget about the concepts for a minute, and focus on the user experience.
>>> Imagine a simple hypothetical scenario in which the user wants to push to 2 distinct repositories. He already has cloned the repo from the 1st repository, thus (theoretically) all he needs to do, is to add a new repository for push. He then uses `remote set-url --add --push <2nd-repo>` (which I personally thought would suffice). However, if he tries to push a new commit to this remote, it would be pushed _only_ to the 2nd-repo.
>>
>> The primary reason behind push-url was that
>>
>> (1) usually you push to and fetch from the same, so no pushUrl is
>> ever needed, just a single Url will do (this is often true for
>> cvs/svn style shared repository workflow); and
>>
>> (2) sometimes you want to fetch from one place and push to another
>> (this is often true for "fetch from upstream, push to my own
>> and ask upstream to pull from it" workflow), and in that case
>> you want pushUrl in addition to Url. Most importantly, in this
>> case, you do *NOT* want to push to Url. You only push to
>> pushUrl.
>>
>> Setting *one* pushURL is a way to say "That URL I fetch from is
>> *not* the place I want to push (I may not even be able to push
>> there); when I say 'push', push there instead". Your proposed
>> semantics will make it impossible to arrange such an asymmetric
>> setting.
>
> Now I think I finally see where that misunderstanding comes from.
> It is "remote -v" that is misdesigned.
>
> $ git clone ../there here
> $ cd here
> $ git remote -v
> origin /var/tmp/there (fetch)
> origin /var/tmp/there (push)
>
> This is totally bogus. It should report something like this:
>
> $ git remote -v
> origin /var/tmp/there (fetch/push)
>
> Then after running "git remote set-url --push origin ../another" we
> should see
>
> $ git remote -v
> origin /var/tmp/there (fetch)
> origin /var/tmp/another (push)
>
> which would make it clear that the original fetch/push came from the
> (1) usuall you push and fetch from the same place so there is only
> one setting, and the two lines came from the (2) sometimes you need
> a separate places to fetch from and push to.
Yes, that is one big source of misunderstanding. Cleaning up remote -v
would help, along with the man page.
Also there is a conceptual confusion: pushurl is meant to push to the
same repo using a different url, e.g. something authenticated
(https/ssh) for push and something faster/easier for fetch.
It never was meant to push to several repos. That is what "remotes" are
for, and it would help if we could push to a remote group (which is
difficult because of refspecs etc.) easily.
That being said, I don't mind changing the behaviour of set-url.
> At this point, if you say "set-url --push origin ../third", then
> "another" will disappear and gets replaced by "third"; if you
> instead say "set-url --add --push origin ../third", then we will see
> two (push) lines, in addition to one (fetch), making it clear that
> you are still in (2) above, fetching from and pushing to different
> places, and having two places to push to.
>
> I misread your response
>
> From: Jardel Weyrich <jweyrich@gmail.com>
> Subject: Re: [BUG] Possible bug in `remote set-url --add --push`
> Date: Sat, 12 Jan 2013 06:09:35 -0200
> Message-ID: <CAN8TAOvP_HX6BEK86aYoX-kVqWDmsbyptxTT2nk+fx+Ut1Tojg@mail.gmail.com>
>
> where you showed that there was only remote.origin.url (and no
> pushurl) in the first step, and somehow thought you had a
> remote.origin.pushurl in the first place.
>
^ permalink raw reply
* Re: [PATCH v2 3/3] diff: Introduce --diff-algorithm command line option
From: Michal Privoznik @ 2013-01-15 10:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, trast
In-Reply-To: <7vvcaz8of4.fsf@alter.siamese.dyndns.org>
On 14.01.2013 22:06, Junio C Hamano wrote:
> Michal Privoznik <mprivozn@redhat.com> writes:
>
>> +--diff-algorithm={patience|minimal|histogram|myers}::
>> + Choose a diff algorithm. The variants are as follows:
>> ++
>> +--
>> +`myers`;;
>> + The basic greedy diff algorithm.
>> +`minimal`;;
>> + Spend extra time to make sure the smallest possible diff is
>> + produced.
>> +`patience`;;
>> + Use "patience diff" algorithm when generating patches.
>> +`histogram`;;
>> + This algorithm extends the patience algorithm to "support
>> + low-occurrence common elements".
>> +--
>> ++
>> +For instance, if you configured diff.algorithm variable to a
>> +non-default value and want to use the default one, then you
>> +have to use `--diff-algorithm=myers` option.
>> +
>> +You should prefer this option over the `--minimal`, `--patience` and
>> +`--histogram` which are kept just for backwards compatibility.
>
> Much better; I'd drop the last paragraph, though.
>
> I also think we really should consider "default" synonym for
> whichever happens to be the built-in default (currently myers).
>
>> diff --git a/diff.c b/diff.c
>> index e9a7e4d..3e021d5 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -144,7 +144,7 @@ static int git_config_rename(const char *var, const char *value)
>> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
>> }
>>
>> -static long parse_algorithm_value(const char *value)
>> +long parse_algorithm_value(const char *value)
>> {
>> if (!value || !strcasecmp(value, "myers"))
>> return 0;
>> @@ -3633,6 +3633,16 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>> options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
>> else if (!strcmp(arg, "--histogram"))
>> options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
>> + else if (!prefixcmp(arg, "--diff-algorithm=")) {
>> + long value = parse_algorithm_value(arg+17);
>> + if (value < 0)
>> + return error("option diff-algorithm accepts \"myers\", "
>> + "\"minimal\", \"patience\" and \"histogram\"");
>> + /* clear out previous settings */
>> + DIFF_XDL_CLR(options, NEED_MINIMAL);
>> + options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
>> + options->xdl_opts |= value;
>
> This makes me wonder if other places that use DIFF_WITH_ALG() also
> need to worry about clearing NEED_MINIMAL?
>
Not really. In my approach, --minimal looks at yet another algorithm.
However, current code sees it as orthogonal to the myers algorithm. The
flag doesn't have any effect on --patience or --histogram at all.
So I think we need to clear the flag only when using --diff-algorithm.
Michal
^ permalink raw reply
* Re: [PATCH] remote-hg: store converted URL
From: Max Horn @ 2013-01-15 11:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Felipe Contreras
In-Reply-To: <7vmwwbd43o.fsf@alter.siamese.dyndns.org>
On 14.01.2013, at 19:14, Junio C Hamano wrote:
> Max Horn <max@quendi.de> writes:
>
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> Mercurial might convert the URL to something more appropriate, like an
>> absolute path.
>
> "What it is converted *TO*" is fairly clear with ", like an ...",
> but from the first reading it was unclear to me "what it is
> converted *FROM*" and "*WHEN* the conversion happens". Do you mean
> that the user gives "git clone" an URL "../hg-repo" via the command
> line (e.g. the argument to "git clone" is spelled something like
> "hg::../hg-repo"), and that "../hg-repo" is rewritten to something
> else (an absolute path, e.g. "/srv/project/hg-repo")?
Yes, that was meant.
>
>> Lets store that instead of the original URL, which won't
>> work from a different working directory if it's relative.
>
> What is lacking from this description is why it even needs to work
> from a different working directory. I am guessing that remote-hg
> later creates a hidden Hg repository or something in a different
> place and still tries to use the URL to interact with the upstream,
> and that is what breaks, but with only the above description without
> looking at your original report, people who will read the "git log"
> output and find this change will not be able to tell why this was
> needed, I am afraid.
>
> Of course, the above guess of mine may even be wrong, but then that
> is yet another reason that the log needs to explain the change
> better.
Fully agreed. How about this commit message:
-- >8 --
remote-hg: store converted URL of hg repo in git config
When remote-hg is invoked, read the remote repository URL from the git config,
give Mercurial a chance to expand it, and if changed, store it back into
the git config.
This fixes the following problem: Suppose you clone a local hg repository
using a relative path, e.g.
git clone hg::hgrepo gitrepo
This stores "hg::hgrepo" in gitrepo/.git/config. However, no information
about the PWD is stored, making it impossible to correctly interpret the
relative path later on. Thus when latter attempting to, say, "git pull"
from inside gitrepo, remote-hg cannot resolve the relative path correctly,
and the user sees an unexpected error.
With this commit, the URL "hg::hgrepo" gets expanded (during cloning,
but also during any other remote operation) and the resulting absolute
URL (e.g. "hg::/abspath/hgrepo") is stored in gitrepo/.git/config.
Thus the git clone of hgrepo becomes usable.
-- >8 --
>
>> Suggested-by: Max Horn <max@quendi.de>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> Signed-off-by: Max Horn <max@quendi.de>
>> ---
>> For a discussion of the problem, see also
>> http://article.gmane.org/gmane.comp.version-control.git/210250
>
> I do not see any discussion; only your problem report.
Aha, an english language issue on my side I guess: For me, a single person can "discuss" a problem (often, a research paper is said to be "discussing a problem"). Sorry for that. Anyway, the reason I gave that link was because it attempts explains the problem and one solution (which this patch ended up implementing), but also express that I feel a bit uncomfortable with this. Which I still do. Relying on the remote helper to invoke "git config" feels like a hack and I was wondering whether this is deemed an acceptable solution -- or whether one should instead extend the remote-helper protocol, allowing the remote helper to signal a rewritten remote URL (perhaps only directly after a clone?). As it is, the remote helper seems (?) to have no way to distinguish whether it is being called duri
ng a clone or a pull; hence it has to "expand" and rewrite the URL every time it is called, just in case.
Anyway, as long as this particular command works somehow, I am fine:
git clone hg::../relative/path/to/hg-repo git-repo
> Was this work done outside the list? I just want to make sure this
> patch is not something Felipe did not want to sign off for whatever
> reason but you are passing it to the list as a patch signed off by
> him.
The work was done by Felipe's and published in his github repository:
https://github.com/felipec/git/commit/605bad5b52d2fcf3d8f5fd782a87d7c97d1b040a
See also the discussion (yeah, this time a real one ;-) leading to this:
https://github.com/felipec/git/issues/2
I took his sign-off from there and interpreted it as saying that Felipe was OK with this being pushed to git.git. But perhaps this is not what I should have done? In that case I am very sorry :-(. It's just that I feel this patch is quite useful and important for daily use (which is why I suggested it in the first place ;-), so I was/am quite eager to see it in.
Cheers,
Max
PS: recently, yet another tool has (re)emerged for using hg repos from inside git:
https://github.com/buchuki/gitifyhg
This is partially based on Felipe's work, but has several bug fixes atop that. It is also seems to be a priority for its author, so it os more actively developed... anyway, that's now, what, "solution" #5 or #6? I really hope the dust on this will settle soon and we'll have just one (or maybe two) tools doing a decent job, instead of attention splitting over so many different ones...
^ 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