* Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
From: Johannes Sixt @ 2007-10-01 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <7vbqbjg9zz.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
>
>> This prepares the API of git_connect() and finish_connect() to operate on
>> a struct child_process. Currently, we just use that object as a placeholder
>> for the pid that we used to return. A follow-up patch will change the
>> implementation of git_connect() and finish_connect() to make full use
>> of the object.
>
> Good description, except removal of checks for negative return
> of the calling sites raised my eyebrow and had me spend a few
> more minutes to review than necessary (see below).
I've thought about this issue a bit more.
Letting git_connect() die on error unconditionally is poison for any
libification efforts. So here's a plan:
1. Let git_connect() return a struct child_process even for the
non-forking case. This way a return value of NULL can uniquely
identify a failure.
2. Keep the error checks in the callers (adjust to test for NULL).
3. Change the die() calls to return failure.
4. Note that the int fd[2] parameter to git_connect() is really an
output: Remove it and use .in and .out of the returned struct
child_process instead.
And maybe:
5. Reuse somehow the struct child_process that git_proxy_connect()
already fills in.
Since my patch doesn't do (1), it can't do (2), i.e. keep the error checks -
they must be removed because no unique failure value exists. So I could
complete (1) in a new version of this patch, in order to also do (2). What
is your preference?
-- Hannes
PS: I've postponed the completion of this plan - in favor of the MinGW port
integration - because it only helps libification.
^ permalink raw reply
* Re: [PATCH 0/5] fork/exec removal series
From: Johannes Sixt @ 2007-10-01 7:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, gitster
In-Reply-To: <Pine.LNX.4.64.0709302242100.28395@racer.site>
Johannes Schindelin schrieb:
> On Sun, 30 Sep 2007, Johannes Sixt wrote:
>> These cases I hope to be able to treat as "coroutine":
>>
>> - sideband demultiplexer in builtin-fetch-pack.c
>> - internal rev-list in upload-pack
>> - the two-way pipe handling in convert.c and builtin-upload-archive.c
>>
>> There are probably more in daemon.c and imap-send.c.
>>
>> BTW, the convert.c case (apply_filter) is most interesting for me, since I
>> have a real-world use-case for a clean-filter.
>
> Calling it coroutine is interesting... But yes, I agree that these three
> cases cannot be handled otherwise.
Suggestions for a better name are appreciated!
-- Hannes
^ permalink raw reply
* Re: [PATCH] rebase: add --signoff option
From: Steffen Prohaska @ 2007-10-01 4:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0709302240450.28395@racer.site>
On Sep 30, 2007, at 11:41 PM, Johannes Schindelin wrote:
> On Sun, 30 Sep 2007, Steffen Prohaska wrote:
>
>> On Sep 30, 2007, at 11:30 PM, Johannes Schindelin wrote:
>>
>>> On Sun, 30 Sep 2007, Steffen Prohaska wrote:
>>>
>>>> When preparing a series of commits for upstream you may need to
>>>> signoff commits if you forgot to do so earlier.
>>>
>>> Why not use format-patch's --signoff option for that?
>>
>> format-patch is fine for mail. But if I either push the commits to a
>> shared repo (like msysgit's mob) or ask upstream to pull from my
>> public
>> repo format-patch doesn't help directly.
>
> Fair enough. But maybe "rebase --interactive" is not too difficult,
> either?
But not too easy either because "rebase --interactive" is based on
"git merge" and not "git am". You can just use a two step process
instead and first "rebase --interactive" followed by
"rebase --signoff".
Steffen
^ permalink raw reply
* Referring a commit-id remote repo.
From: David Brown @ 2007-10-01 4:16 UTC (permalink / raw)
To: Git
Say someone has given me a url to a git-web view of a commit:
<http://www.linux-arm.org/git?p=linux-2.6.git;a=commitdiff_plain;h=c1a54638653ef81187309624940bfa1537aa0fab>
I managed to mangle this around to a repo name that I can clone from, so I
try:
% git clone git://linux-arm.org/linux-2.6.git play
Now, from within my new directory, I try looking at this commit:
% git show c1a54638653ef81187309624940bfa1537aa0fab
fatal: bad object c1a54638653ef81187309624940bfa1537aa0fab
Ok, so they repo appears to have an object that none of their refs can
find, so I didn't pull it over with the clone.
Looking at the patch, I was able to find other commits with the same
content, so I suspect they were doing some rebasing on their repo, after
sending someone a commit ID via email.
The question I have: is there any way I can look at this particular commit
ID on the remote repo? I couldn't come up with any way to get git fetch to
retrieve it.
Thanks,
David Brown
^ permalink raw reply
* Re: git-svn merge helper
From: Björn Steinbrink @ 2007-10-01 2:50 UTC (permalink / raw)
To: Benoit SIGOURE; +Cc: git
In-Reply-To: <1EF130A4-3CC7-4A42-9166-3539D9A38828@lrde.epita.fr>
On 2007.09.30 16:15:49 +0200, Benoit SIGOURE wrote:
> On Sep 30, 2007, at 1:05 PM, Björn Steinbrink wrote:
>
>> Hi,
>>
>> I recently discovered git-svn and absolutey love it. One thing that I'm
>> missing though, is an equivalent of "svn merge" for merging between svn
>> remotes, to support the SVN way of using "squashed" merges, where you
>> just note the merge meta-data in the commit message. "git merge" didn't
>> work for me (and probably isn't expected to work) to merge between two
>> svn branches, so I've resorted to cherry-picking the required commits
>> one by one into a temporary branch and then squashing them together by
>> doing a --squash merge with a second temporary branch (as in [1]).
>
> I fail to see why you'd want to reproduce the broken behavior of svn merge.
> Anyways, git-svn is a great way to merge SVN branches, unfortunately it
> can't detect when merges happened on the SVN side. I think you can use it
> nevertheless by manually adding a graft at the last merge point, which
> would help you merging the right revisions without having to specify what
> needs to be merged (unless someone made another merge on the SVN side, in
> which case you need to update your graft).
Then how does that work? The manpage explicitly says that I should not
use git-{pull,merge} on branches I want to dcommit from. And a trivial
test immediately got the expected effect of git-svn trying to commit to
trunk instead of the branch.
Thanks,
Björn
^ permalink raw reply
* [PATCH v4 2/2] fetch/push: readd rsync support
From: Johannes Schindelin @ 2007-09-30 23:59 UTC (permalink / raw)
To: gitster, spearce, git
We lost rsync support when transitioning from shell to C. Support it
again (even if the transport is technically deprecated, some people just
do not have any chance to use anything else).
Also, add a test to t5510. Since rsync transport is not configured by
default on most machines, and especially not such that you can write to
rsync://127.0.0.1$(pwd)/, it is disabled by default; you can enable it by
setting the environment variable TEST_RSYNC.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Seems that mkdtemp() is BSD, not POSIX, and even then Windows
lacks it ;-)
So I'll provide a NEEDS_MKDTEMP patch tomorrow, I hope...
t/t5510-fetch.sh | 35 ++++++
transport.c | 328 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 362 insertions(+), 1 deletions(-)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 439430f..73a4e3c 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -153,4 +153,39 @@ test_expect_success 'bundle should be able to create a full history' '
'
+test "$TEST_RSYNC" && {
+test_expect_success 'fetch via rsync' '
+ git pack-refs &&
+ mkdir rsynced &&
+ cd rsynced &&
+ git init &&
+ git fetch rsync://127.0.0.1$(pwd)/../.git master:refs/heads/master &&
+ git gc --prune &&
+ test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+ git fsck --full
+'
+
+test_expect_success 'push via rsync' '
+ mkdir ../rsynced2 &&
+ (cd ../rsynced2 &&
+ git init) &&
+ git push rsync://127.0.0.1$(pwd)/../rsynced2/.git master &&
+ cd ../rsynced2 &&
+ git gc --prune &&
+ test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+ git fsck --full
+'
+
+test_expect_success 'push via rsync' '
+ cd .. &&
+ mkdir rsynced3 &&
+ (cd rsynced3 &&
+ git init) &&
+ git push --all rsync://127.0.0.1$(pwd)/rsynced3/.git &&
+ cd rsynced3 &&
+ test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
+ git fsck --full
+'
+}
+
test_done
diff --git a/transport.c b/transport.c
index 3475cca..7266fd3 100644
--- a/transport.c
+++ b/transport.c
@@ -6,6 +6,330 @@
#include "fetch-pack.h"
#include "walker.h"
#include "bundle.h"
+#include "dir.h"
+#include "refs.h"
+
+/* rsync support */
+
+/*
+ * We copy packed-refs and refs/ into a temporary file, then read the
+ * loose refs recursively (sorting whenever possible), and then inserting
+ * those packed refs that are not yet in the list (not validating, but
+ * assuming that the file is sorted).
+ *
+ * Appears refactoring this from refs.c is too cumbersome.
+ */
+
+static int str_cmp(const void *a, const void *b)
+{
+ const char *s1 = a;
+ const char *s2 = b;
+
+ return strcmp(s1, s2);
+}
+
+/* path->buf + name_offset is expected to point to "refs/" */
+
+static int read_loose_refs(struct strbuf *path, int name_offset,
+ struct ref **tail)
+{
+ DIR *dir = opendir(path->buf);
+ struct dirent *de;
+ struct {
+ char **entries;
+ int nr, alloc;
+ } list;
+ int i, pathlen;
+
+ if (!dir)
+ return -1;
+
+ memset (&list, 0, sizeof(list));
+
+ while ((de = readdir(dir))) {
+ if (de->d_name[0] == '.' && (de->d_name[1] == '\0' ||
+ (de->d_name[1] == '.' &&
+ de->d_name[2] == '\0')))
+ continue;
+ ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
+ list.entries[list.nr++] = xstrdup(de->d_name);
+ }
+ closedir(dir);
+
+ /* sort the list */
+
+ qsort(list.entries, list.nr, sizeof(char *), str_cmp);
+
+ pathlen = path->len;
+ strbuf_addch(path, '/');
+
+ for (i = 0; i < list.nr; i++, strbuf_setlen(path, pathlen + 1)) {
+ strbuf_addstr(path, list.entries[i]);
+ if (read_loose_refs(path, name_offset, tail)) {
+ int fd = open(path->buf, O_RDONLY);
+ char buffer[40];
+ struct ref *next;
+
+ if (fd < 0)
+ continue;
+ next = alloc_ref(path->len - name_offset + 1);
+ if (read_in_full(fd, buffer, 40) != 40 ||
+ get_sha1_hex(buffer, next->old_sha1)) {
+ close(fd);
+ free(next);
+ continue;
+ }
+ close(fd);
+ strcpy(next->name, path->buf + name_offset);
+ (*tail)->next = next;
+ *tail = next;
+ }
+ }
+ strbuf_setlen(path, pathlen);
+
+ for (i = 0; i < list.nr; i++)
+ free(list.entries[i]);
+ free(list.entries);
+
+ return 0;
+}
+
+/* insert the packed refs for which no loose refs were found */
+
+static void insert_packed_refs(const char *packed_refs, struct ref **list)
+{
+ FILE *f = fopen(packed_refs, "r");
+ static char buffer[PATH_MAX];
+
+ if (!f)
+ return;
+
+ for (;;) {
+ int cmp, len;
+
+ if (!fgets(buffer, sizeof(buffer), f)) {
+ fclose(f);
+ return;
+ }
+
+ if (hexval(buffer[0]) > 0xf)
+ continue;
+ len = strlen(buffer);
+ if (buffer[len - 1] == '\n')
+ buffer[--len] = '\0';
+ if (len < 41)
+ continue;
+ while ((*list)->next &&
+ (cmp = strcmp(buffer + 41,
+ (*list)->next->name)) > 0)
+ list = &(*list)->next;
+ if (!(*list)->next || cmp < 0) {
+ struct ref *next = alloc_ref(len - 40);
+ buffer[40] = '\0';
+ if (get_sha1_hex(buffer, next->old_sha1)) {
+ warning ("invalid SHA-1: %s", buffer);
+ free(next);
+ continue;
+ }
+ strcpy(next->name, buffer + 41);
+ next->next = (*list)->next;
+ (*list)->next = next;
+ list = &(*list)->next;
+ }
+ }
+}
+
+static struct ref *get_refs_via_rsync(const struct transport *transport)
+{
+ struct strbuf buf = STRBUF_INIT, temp_dir = STRBUF_INIT;
+ struct ref dummy, *tail = &dummy;
+ struct child_process rsync;
+ const char *args[5];
+ int temp_dir_len;
+
+ /* copy the refs to the temporary directory */
+
+ strbuf_addstr(&temp_dir, git_path("rsync-refs-XXXXXX"));
+ if (!mkdtemp(temp_dir.buf))
+ die ("Could not make temporary directory");
+ temp_dir_len = temp_dir.len;
+
+ strbuf_addstr(&buf, transport->url);
+ strbuf_addstr(&buf, "/refs");
+
+ memset(&rsync, 0, sizeof(rsync));
+ rsync.argv = args;
+ rsync.stdout_to_stderr = 1;
+ args[0] = "rsync";
+ args[1] = transport->verbose ? "-rv" : "-r";
+ args[2] = buf.buf;
+ args[3] = temp_dir.buf;
+ args[4] = NULL;
+
+ if (run_command(&rsync))
+ die ("Could not run rsync to get refs");
+
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, transport->url);
+ strbuf_addstr(&buf, "/packed-refs");
+
+ args[2] = buf.buf;
+
+ if (run_command(&rsync))
+ die ("Could not run rsync to get refs");
+
+ /* read the copied refs */
+
+ strbuf_addstr(&temp_dir, "/refs");
+ read_loose_refs(&temp_dir, temp_dir_len + 1, &tail);
+ strbuf_setlen(&temp_dir, temp_dir_len);
+
+ tail = &dummy;
+ strbuf_addstr(&temp_dir, "/packed-refs");
+ insert_packed_refs(temp_dir.buf, &tail);
+ strbuf_setlen(&temp_dir, temp_dir_len);
+
+ if (remove_dir_recursively(&temp_dir, 0))
+ warning ("Error removing temporary directory %s.",
+ temp_dir.buf);
+
+ strbuf_release(&buf);
+ strbuf_release(&temp_dir);
+
+ return dummy.next;
+}
+
+static int fetch_objs_via_rsync(struct transport *transport,
+ int nr_objs, struct ref **to_fetch)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process rsync;
+ const char *args[8];
+ int result;
+
+ strbuf_addstr(&buf, transport->url);
+ strbuf_addstr(&buf, "/objects/");
+
+ memset(&rsync, 0, sizeof(rsync));
+ rsync.argv = args;
+ rsync.stdout_to_stderr = 1;
+ args[0] = "rsync";
+ args[1] = transport->verbose ? "-rv" : "-r";
+ args[2] = "--ignore-existing";
+ args[3] = "--exclude";
+ args[4] = "info";
+ args[5] = buf.buf;
+ args[6] = get_object_directory();
+ args[7] = NULL;
+
+ /* NEEDSWORK: handle one level of alternates */
+ result = run_command(&rsync);
+
+ strbuf_release(&buf);
+
+ return result;
+}
+
+static int write_one_ref(const char *name, const unsigned char *sha1,
+ int flags, void *data)
+{
+ struct strbuf *buf = data;
+ int len = buf->len;
+ FILE *f;
+
+ /* when called via for_each_ref(), flags is non-zero */
+ if (flags && prefixcmp(name, "refs/heads/") &&
+ prefixcmp(name, "refs/tags/"))
+ return 0;
+
+ strbuf_addstr(buf, name);
+ if (safe_create_leading_directories(buf->buf) ||
+ !(f = fopen(buf->buf, "w")) ||
+ fprintf(f, "%s\n", sha1_to_hex(sha1)) < 0 ||
+ fclose(f))
+ return error("problems writing temporary file %s", buf->buf);
+ strbuf_setlen(buf, len);
+ return 0;
+}
+
+static int write_refs_to_temp_dir(struct strbuf *temp_dir,
+ int refspec_nr, const char **refspec)
+{
+ int i;
+
+ for (i = 0; i < refspec_nr; i++) {
+ unsigned char sha1[20];
+ char *ref;
+
+ if (dwim_ref(refspec[i], strlen(refspec[i]), sha1, &ref) != 1)
+ return error("Could not get ref %s", refspec[i]);
+
+ if (write_one_ref(ref, sha1, 0, temp_dir)) {
+ free(ref);
+ return -1;
+ }
+ free(ref);
+ }
+ return 0;
+}
+
+static int rsync_transport_push(struct transport *transport,
+ int refspec_nr, const char **refspec, int flags)
+{
+ struct strbuf buf = STRBUF_INIT, temp_dir = STRBUF_INIT;
+ int result = 0, i;
+ struct child_process rsync;
+ const char *args[8];
+
+ /* first push the objects */
+
+ strbuf_addstr(&buf, transport->url);
+ strbuf_addch(&buf, '/');
+
+ memset(&rsync, 0, sizeof(rsync));
+ rsync.argv = args;
+ rsync.stdout_to_stderr = 1;
+ args[0] = "rsync";
+ args[1] = transport->verbose ? "-av" : "-a";
+ args[2] = "--ignore-existing";
+ args[3] = "--exclude";
+ args[4] = "info";
+ args[5] = get_object_directory();;
+ args[6] = buf.buf;
+ args[7] = NULL;
+
+ if (run_command(&rsync))
+ return error("Could not push objects to %s", transport->url);
+
+ /* copy the refs to the temporary directory; they could be packed. */
+
+ strbuf_addstr(&temp_dir, git_path("rsync-refs-XXXXXX"));
+ if (!mkdtemp(temp_dir.buf))
+ die ("Could not make temporary directory");
+ strbuf_addch(&temp_dir, '/');
+
+ if (flags & TRANSPORT_PUSH_ALL) {
+ if (for_each_ref(write_one_ref, &temp_dir))
+ return -1;
+ } else if (write_refs_to_temp_dir(&temp_dir, refspec_nr, refspec))
+ return -1;
+
+ i = (flags & TRANSPORT_PUSH_FORCE) ? 2 : 3;
+ args[i++] = temp_dir.buf;
+ args[i++] = transport->url;
+ args[i++] = NULL;
+ if (run_command(&rsync))
+ result = error("Could not push to %s", transport->url);
+
+ if (remove_dir_recursively(&temp_dir, 0))
+ warning ("Could not remove temporary directory %s.",
+ temp_dir.buf);
+
+ strbuf_release(&buf);
+ strbuf_release(&temp_dir);
+
+ return result;
+}
/* Generic functions for using commit walkers */
@@ -402,7 +726,9 @@ struct transport *transport_get(struct remote *remote, const char *url)
ret->url = url;
if (!prefixcmp(url, "rsync://")) {
- /* not supported; don't populate any ops */
+ 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://")
--
1.5.3.2.1102.g9487
^ permalink raw reply related
* [PATCH] git stash: document apply's --index switch
From: Miklos Vajna @ 2007-09-30 22:30 UTC (permalink / raw)
To: git
In-Reply-To: <Pine.LNX.4.64.0709302226270.28395@racer.site>
---
On Sun, Sep 30, 2007 at 10:29:05PM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Now that you know what --index is supposed to do, maybe you are nice
> enough to extend the documentation and post a patch?
something like this?
VMiklos
Documentation/git-stash.txt | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 05f40cf..5723bb0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -63,7 +63,7 @@ show [<stash>]::
it will accept any format known to `git-diff` (e.g., `git-stash show
-p stash@\{1}` to view the second most recent stash in patch form).
-apply [<stash>]::
+apply [--index] [<stash>]::
Restore the changes recorded in the stash on top of the current
working tree state. When no `<stash>` is given, applies the latest
@@ -71,6 +71,11 @@ apply [<stash>]::
+
This operation can fail with conflicts; you need to resolve them
by hand in the working tree.
++
+If the `--index` option is used, then tries to reinstate not only the working
+tree's changes, but also the index's ones. However, this can fail, when you
+have conflicts (which are stored in the index, where you therefore can no
+longer apply the changes as they were originally).
clear::
Remove all the stashed states. Note that those states will then
--
1.5.3.2.111.g5166-dirty
^ permalink raw reply related
* Re: [PATCH 0/5] fork/exec removal series
From: Johannes Schindelin @ 2007-09-30 21:43 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster
In-Reply-To: <200709302334.37129.johannes.sixt@telecom.at>
Hi,
On Sun, 30 Sep 2007, Johannes Sixt wrote:
> On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:
>
> > Is there more than the case I introduced with shallow clones?
>
> I don't know which one you are refering to.
The rev-list one in upload-pack.
> These cases I hope to be able to treat as "coroutine":
>
> - sideband demultiplexer in builtin-fetch-pack.c
> - internal rev-list in upload-pack
> - the two-way pipe handling in convert.c and builtin-upload-archive.c
>
> There are probably more in daemon.c and imap-send.c.
>
> BTW, the convert.c case (apply_filter) is most interesting for me, since I
> have a real-world use-case for a clean-filter.
Calling it coroutine is interesting... But yes, I agree that these three
cases cannot be handled otherwise.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] rebase: add --signoff option
From: Johannes Schindelin @ 2007-09-30 21:41 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: git
In-Reply-To: <C94CC989-096D-43B5-BA16-DBD4D84038C0@zib.de>
Hi,
On Sun, 30 Sep 2007, Steffen Prohaska wrote:
> On Sep 30, 2007, at 11:30 PM, Johannes Schindelin wrote:
>
> > On Sun, 30 Sep 2007, Steffen Prohaska wrote:
> >
> > > When preparing a series of commits for upstream you may need to
> > > signoff commits if you forgot to do so earlier.
> >
> > Why not use format-patch's --signoff option for that?
>
> format-patch is fine for mail. But if I either push the commits to a
> shared repo (like msysgit's mob) or ask upstream to pull from my public
> repo format-patch doesn't help directly.
Fair enough. But maybe "rebase --interactive" is not too difficult,
either?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
From: Johannes Sixt @ 2007-09-30 21:40 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <7vbqbjg9zz.fsf@gitster.siamese.dyndns.org>
On Sunday 30 September 2007 22:43, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > + struct child_process *chld;
>
> Is "child" a reserved keyword or something that we need to avoid
> its use as an identifier?
Allow me to call it "conn": We are using this as a "connection object".
Will integrate your and Dscho's suggestions and resend when the strbuf series
appears in master.
-- Hannes
^ permalink raw reply
* Re: [PATCH] rebase: add --signoff option
From: Steffen Prohaska @ 2007-09-30 21:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0709302229320.28395@racer.site>
On Sep 30, 2007, at 11:30 PM, Johannes Schindelin wrote:
> On Sun, 30 Sep 2007, Steffen Prohaska wrote:
>
>> When preparing a series of commits for upstream you may
>> need to signoff commits if you forgot to do so earlier.
>
> Why not use format-patch's --signoff option for that?
format-patch is fine for mail. But if I either push the commits
to a shared repo (like msysgit's mob) or ask upstream to pull
from my public repo format-patch doesn't help directly.
Steffen
^ permalink raw reply
* Re: [PATCH 0/5] fork/exec removal series
From: Johannes Sixt @ 2007-09-30 21:34 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, gitster
In-Reply-To: <Pine.LNX.4.64.0709302212160.28395@racer.site>
On Sunday 30 September 2007 23:14, Johannes Schindelin wrote:
> On Sun, 30 Sep 2007, Johannes Sixt wrote:
> > - The fork()s are not followed by exec(). These need a different
> > implementation. I am thinking of a start_coroutine()/finish_coroutine()
> > API that is implemented with threads in MinGW. (Suggestions of a better
> > as well as implementations are welcome.)
>
> Is there more than the case I introduced with shallow clones?
I don't know which one you are refering to.
These cases I hope to be able to treat as "coroutine":
- sideband demultiplexer in builtin-fetch-pack.c
- internal rev-list in upload-pack
- the two-way pipe handling in convert.c and builtin-upload-archive.c
There are probably more in daemon.c and imap-send.c.
BTW, the convert.c case (apply_filter) is most interesting for me, since I
have a real-world use-case for a clean-filter.
-- Hannes
^ permalink raw reply
* Re: [PATCH] rebase: add --signoff option
From: Johannes Schindelin @ 2007-09-30 21:30 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: git
In-Reply-To: <11911689111797-git-send-email-prohaska@zib.de>
Hi,
On Sun, 30 Sep 2007, Steffen Prohaska wrote:
> When preparing a series of commits for upstream you may
> need to signoff commits if you forgot to do so earlier.
Why not use format-patch's --signoff option for that?
Ciao,
Dscho
^ permalink raw reply
* Re: suggestion for git stash
From: Johannes Schindelin @ 2007-09-30 21:29 UTC (permalink / raw)
To: Bruno Haible; +Cc: Junio C Hamano, git
In-Reply-To: <200709302259.11731.bruno@clisp.org>
Hi,
On Sun, 30 Sep 2007, Bruno Haible wrote:
> I don't know what "git stash apply --index" does, since it's not
> documented.
It's documented in code ;-)
No, really, what it does is trying to reinstate not only the working
tree's changes, but also the index' ones. However, this can fail, when
you have conflicts (which are stored in the index, where you therefore can
no longer apply the changes as they were originally).
Now that you know what --index is supposed to do, maybe you are nice
enough to extend the documentation and post a patch?
Thanks,
Dscho
^ permalink raw reply
* Re: Stashing untracked files
From: Johannes Schindelin @ 2007-09-30 21:25 UTC (permalink / raw)
To: Tom Tobin; +Cc: Neil Macneale, git
In-Reply-To: <bf0b20a90709301344j1ac8f538u616fb4ba3fc47efe@mail.gmail.com>
Hi,
On Sun, 30 Sep 2007, Tom Tobin wrote:
> On 9/29/07, Neil Macneale <mac4-git@theory.org> wrote:
> > When using "git stash," in some cases I'd like to stash away files
> > that are currently untracked. It seems to me like there should be a
> > way to stash everything in a working directory so that the end result
> > is a pristine tree. Then applying the stash will reinstate those file
> > as untracked.
>
> Since this is an itch of my own, I have a local copy of git-stash that
> does this (stashes away the untracked -- but not ignored -- files, and
> restores them as untracked upon "stash apply"); unfortunately, I'm
> pretty new to git, so I'm certain my code is *quite* unoptimized and
> ugly. As soon as I feel comfortable with it (which should include
> making the new behavior optional), I'll drop a line here with some code.
> :-)
We are known on this list for never biting someone's head off, especially
not when posting patches.
Why not just post it, see what comments come back, adapt it, and repost?
More often than not, people learn by getting their code reviewed, and
faster so when the code is reviewed early.
Ciao,
Dscho
^ permalink raw reply
* Re: GIT_EXTERNAL_DIFF invoked with undocumented calling convention after unstashing conflicts
From: Bruno Haible @ 2007-09-30 21:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk5q7hqld.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> The script needs to decide how it wants to present an unmerged
> path; the information on each unmerged stages can be read from
> the output of "ls-files -u $thatpath".
Thanks for this info. This, and "git cat-file", did the trick.
Bruno
^ permalink raw reply
* Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
From: Junio C Hamano @ 2007-09-30 20:43 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-2-git-send-email-johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
> This prepares the API of git_connect() and finish_connect() to operate on
> a struct child_process. Currently, we just use that object as a placeholder
> for the pid that we used to return. A follow-up patch will change the
> implementation of git_connect() and finish_connect() to make full use
> of the object.
Good description, except removal of checks for negative return
of the calling sites raised my eyebrow and had me spend a few
more minutes to review than necessary (see below).
> diff --git a/builtin-archive.c b/builtin-archive.c
> index 04385de..76db6cf 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
> {
> char *url, buf[LARGE_PACKET_MAX];
> int fd[2], i, len, rv;
> - pid_t pid;
> + struct child_process *chld;
Is "child" a reserved keyword or something that we need to avoid
its use as an identifier?
> const char *exec = "git-upload-archive";
> int exec_at = 0;
>
> @@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
> }
>
> url = xstrdup(remote);
> - pid = git_connect(fd, url, exec, 0);
> - if (pid < 0)
> - return pid;
> + chld = git_connect(fd, url, exec, 0);
>
Interesting. This and other callers of git_connect() were
prepared to see git_connect() to report errors with negative PID
but the callee simply died --- so this change is not regressing
by removing an early error return. It would be better to have
something like this in the commit log message:
Old code had early-return-on-error checks at the calling
sites of git_connect(), but the callee simply died on
errors without returning negative values. This patch
removes such bogosity.
> diff --git a/connect.c b/connect.c
> index 3d5c4ab..f6dcab9 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -468,21 +468,22 @@ char *get_port(char *host)
> }
>
> /*
> - * This returns 0 if the transport protocol does not need fork(2),
> + * This returns NULL if the transport protocol does not need fork(2),
> * or a process id if it does. Once done, finish the connection
It does not return a process ID anymore, so this comment needs
to be updated. Instead, you now return a struct child_process
that is newly allocated, and needs to be deallocated somehow.
At the end of finish_connect() might be a good place to do so.
> * with finish_connect() with the value returned from this function
> - * (it is safe to call finish_connect() with 0 to support the former
> + * (it is safe to call finish_connect() with NULL to support the former
> * case).
> *
> - * Does not return a negative value on error; it just dies.
> + * If it returns, the connect is successful; it just dies on errors.
> */
> -pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
> +struct child_process *git_connect(int fd[2], char *url,
> + const char *prog, int flags)
> {
> ...
> -int finish_connect(pid_t pid)
> +int finish_connect(struct child_process *chld)
> {
> - if (pid == 0)
> + if (chld == NULL)
> return 0;
>
> - while (waitpid(pid, NULL, 0) < 0) {
> + while (waitpid(chld->pid, NULL, 0) < 0) {
> if (errno != EINTR)
> return -1;
> }
But it seems you don't, leaking the struct. I see this is fixed
in the next patch in the series, but it would be nicer to have
the free from the very beginning.
^ permalink raw reply
* Re: [PATCH 0/5] fork/exec removal series
From: Johannes Schindelin @ 2007-09-30 21:14 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-1-git-send-email-johannes.sixt@telecom.at>
Hi,
On Sun, 30 Sep 2007, Johannes Sixt wrote:
> You can regard this as the beginning of the MinGW port integration.
Thank you very much! This effort cannot be praised enough.
> There still remain a few forks, which fall into these categories:
>
> - They are in tools or code that are not (yet) ported to MinGW.[*]
The nice thing about the integration effort: It does not need to be done
in one go.
> - The fork()s are not followed by exec(). These need a different
> implementation. I am thinking of a start_coroutine()/finish_coroutine()
> API that is implemented with threads in MinGW. (Suggestions of a better
> as well as implementations are welcome.)
Is there more than the case I introduced with shallow clones?
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.
From: Johannes Schindelin @ 2007-09-30 21:10 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-5-git-send-email-johannes.sixt@telecom.at>
Hi,
On Sun, 30 Sep 2007, Johannes Sixt wrote:
> diff.c | 38 +++-----------------------------------
> 1 files changed, 3 insertions(+), 35 deletions(-)
_Nice_!
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.
From: Johannes Schindelin @ 2007-09-30 21:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-4-git-send-email-johannes.sixt@telecom.at>
Hi,
On Sun, 30 Sep 2007, Johannes Sixt wrote:
> - struct child_process child_process;
> + struct child_process chld;
Why? This patch is less readable because of that rename.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
From: Johannes Schindelin @ 2007-09-30 21:04 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-2-git-send-email-johannes.sixt@telecom.at>
Hi,
On Sun, 30 Sep 2007, Johannes Sixt wrote:
> - pid_t pid;
> + struct child_process *chld;
Why not call it "child"? It's only one letter more, and much less
confusing.
> - pid = git_connect(fd, url, exec, 0);
> - if (pid < 0)
> - return pid;
> + chld = git_connect(fd, url, exec, 0);
chld never gets free()d. (I'm was about to suggest doing that in
finish_command(), but I'm not quite sure what this would break.)
> @@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
> st.st_mtime = 0;
> }
>
> - pid = git_connect(fd, (char *)dest, uploadpack,
> + chld = git_connect(fd, (char *)dest, uploadpack,
> args.verbose ? CONNECT_VERBOSE : 0);
> - if (pid < 0)
> - return NULL;
> if (heads && nr_heads)
> nr_heads = remove_duplicates(nr_heads, heads);
> ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);
In general, I would not lightly do away with the "return NULL" on error,
since it is really hard to assess what do_fetch_pack() or packet_write()
do (or don't) when git_connect() failed.
> - while (waitpid(pid, NULL, 0) < 0) {
> + while (waitpid(chld->pid, NULL, 0) < 0) {
Shouldn't this be converted to finish_command() already?
Ciao,
Dscho
^ permalink raw reply
* Re: suggestion for git stash
From: Bruno Haible @ 2007-09-30 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfy0vhqkl.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Isn't "stash apply --index" what you talk about?
Not really:
$ git status
# On branch master
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# modified: README
#
# Changed but not updated:
# (use "git add <file>..." to update what will be committed)
#
# modified: NEWS
#
$ git stash
Saved "WIP on master: 61135ee... Use check_PROGRAMS instead of noinst_PROGRAMS."
HEAD is now at 61135ee... Use check_PROGRAMS instead of noinst_PROGRAMS.
$ git stash apply --index
error: No changes
Conflicts in index. Try without --index.
(This is git version 1.5.3.1.)
I don't know what "git stash apply --index" does, since it's not documented.
Bruno
^ permalink raw reply
* Re: Stashing untracked files
From: Tom Tobin @ 2007-09-30 20:44 UTC (permalink / raw)
To: Neil Macneale; +Cc: git
In-Reply-To: <46FE9924.7080006@theory.org>
On 9/29/07, Neil Macneale <mac4-git@theory.org> wrote:
> When using "git stash," in some cases I'd like to stash away files that
> are currently untracked. It seems to me like there should be a way to
> stash everything in a working directory so that the end result is a
> pristine tree. Then applying the stash will reinstate those file as
> untracked.
Since this is an itch of my own, I have a local copy of git-stash that
does this (stashes away the untracked -- but not ignored -- files, and
restores them as untracked upon "stash apply"); unfortunately, I'm
pretty new to git, so I'm certain my code is *quite* unoptimized and
ugly. As soon as I feel comfortable with it (which should include
making the new behavior optional), I'll drop a line here with some
code. :-)
^ permalink raw reply
* Re: suggestion for git stash
From: Matthieu Moy @ 2007-09-30 20:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bruno Haible, git
In-Reply-To: <7vfy0vhqkl.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Isn't "stash apply --index" what you talk about?
It doesn't seem to be documented in Documentation/git-stash.txt, but
seems to be the answer.
--
Matthieu
^ permalink raw reply
* Re: [PATCH 0/5] fork/exec removal series
From: Junio C Hamano @ 2007-09-30 20:20 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
In-Reply-To: <1191183001-5368-1-git-send-email-johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
> Here is a series of patches that removes a number fork/exec pairs.
> They are replaced by delegating to start_command/finish_command/run_command.
> You can regard this as the beginning of the MinGW port integration.
Yay!
> Patch 2 depends on Patch 1; otherwise, there are no dependencies.
> The series goes on top of next (it touches str_buf stuff in connect.c).
Thanks for an advance warning.
I'll keep the series in my mailbox (promise) and queue it for
'pu' for now (but only if I have time), as I intend to merge the
strbuf stuff to 'master' soon.
^ 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