* 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
* [PATCH 5/5] Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.
From: Johannes Sixt @ 2007-09-30 20:10 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1191183001-5368-5-git-send-email-johannes.sixt@telecom.at>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
builtin-fetch-pack.c | 55 ++++++++++++++-----------------------------------
1 files changed, 16 insertions(+), 39 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 19c144d..d0eca2d 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -7,6 +7,7 @@
#include "pack.h"
#include "sideband.h"
#include "fetch-pack.h"
+#include "run-command.h"
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
@@ -499,11 +500,13 @@ static int get_pack(int xd[2], char **pack_lockfile)
char hdr_arg[256];
const char **av;
int do_keep = args.keep_pack;
- int keep_pipe[2];
+ struct child_process cmd;
side_pid = setup_sideband(fd, xd);
- av = argv;
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.argv = av = argv;
+
*hdr_arg = 0;
if (!args.keep_pack && unpack_limit) {
struct pack_header header;
@@ -519,8 +522,8 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
if (do_keep) {
- if (pack_lockfile && pipe(keep_pipe))
- die("fetch-pack: pipe setup failure: %s", strerror(errno));
+ if (pack_lockfile)
+ cmd.out = -1;
*av++ = "index-pack";
*av++ = "--stdin";
if (!args.quiet && !args.no_progress)
@@ -544,43 +547,17 @@ static int get_pack(int xd[2], char **pack_lockfile)
*av++ = hdr_arg;
*av++ = NULL;
- pid = fork();
- if (pid < 0)
+ cmd.in = fd[0];
+ cmd.git_cmd = 1;
+ if (start_command(&cmd))
die("fetch-pack: unable to fork off %s", argv[0]);
- if (!pid) {
- dup2(fd[0], 0);
- if (do_keep && pack_lockfile) {
- dup2(keep_pipe[1], 1);
- close(keep_pipe[0]);
- close(keep_pipe[1]);
- }
- close(fd[0]);
- close(fd[1]);
- execv_git_cmd(argv);
- die("%s exec failed", argv[0]);
- }
- close(fd[0]);
close(fd[1]);
- if (do_keep && pack_lockfile) {
- close(keep_pipe[1]);
- *pack_lockfile = index_pack_lockfile(keep_pipe[0]);
- close(keep_pipe[0]);
- }
- while (waitpid(pid, &status, 0) < 0) {
- if (errno != EINTR)
- die("waiting for %s: %s", argv[0], strerror(errno));
- }
- if (WIFEXITED(status)) {
- int code = WEXITSTATUS(status);
- if (code)
- die("%s died with error code %d", argv[0], code);
- return 0;
- }
- if (WIFSIGNALED(status)) {
- int sig = WTERMSIG(status);
- die("%s died of signal %d", argv[0], sig);
- }
- die("%s died of unnatural causes %d", argv[0], status);
+ if (do_keep && pack_lockfile)
+ *pack_lockfile = index_pack_lockfile(cmd.out);
+
+ if (finish_command(&cmd))
+ die("%s failed", argv[0]);
+ return 0;
}
static struct ref *do_fetch_pack(int fd[2],
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
From: Johannes Sixt @ 2007-09-30 20:09 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1191183001-5368-1-git-send-email-johannes.sixt@telecom.at>
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.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
builtin-archive.c | 8 +++-----
builtin-fetch-pack.c | 8 +++-----
cache.h | 4 ++--
connect.c | 28 +++++++++++++++-------------
peek-remote.c | 8 +++-----
send-pack.c | 8 +++-----
transport.c | 9 ++-------
7 files changed, 31 insertions(+), 42 deletions(-)
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;
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);
for (i = 1; i < argc; i++) {
if (i == exec_at)
@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,
rv = recv_sideband("archive", fd[0], 1, 2);
close(fd[0]);
close(fd[1]);
- rv |= finish_connect(pid);
+ rv |= finish_connect(chld);
return !!rv;
}
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 8f25d50..19c144d 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -762,7 +762,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
{
int i, ret;
int fd[2];
- pid_t pid;
+ struct child_process *chld;
struct ref *ref;
struct stat st;
@@ -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);
close(fd[0]);
close(fd[1]);
- ret = finish_connect(pid);
+ ret = finish_connect(chld);
if (!ret && nr_heads) {
/* If the heads to pull were given, we should have
diff --git a/cache.h b/cache.h
index 27485d3..32ce8a7 100644
--- a/cache.h
+++ b/cache.h
@@ -503,8 +503,8 @@ struct ref {
#define REF_TAGS (1u << 2)
#define CONNECT_VERBOSE (1u << 0)
-extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
-extern int finish_connect(pid_t pid);
+extern struct child_process *git_connect(int fd[2], char *url, const char *prog, int flags);
+extern int finish_connect(struct child_process *chld);
extern int path_match(const char *path, int nr, char **match);
extern int get_ack(int fd, unsigned char *result_sha1);
extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags);
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
* 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)
{
char *host, *path = url;
char *end;
int c;
int pipefd[2][2];
- pid_t pid;
+ struct child_process *chld;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
@@ -568,15 +569,16 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
free(target_host);
if (free_path)
free(path);
- return 0;
+ return NULL;
}
if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
die("unable to create pipe pair for communication");
- pid = fork();
- if (pid < 0)
+ chld = xcalloc(1, sizeof(*chld));
+ chld->pid = fork();
+ if (chld->pid < 0)
die("unable to fork");
- if (!pid) {
+ if (!chld->pid) {
struct strbuf cmd;
strbuf_init(&cmd, MAX_CMD_LEN);
@@ -625,15 +627,15 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
close(pipefd[1][0]);
if (free_path)
free(path);
- return pid;
+ return chld;
}
-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;
}
diff --git a/peek-remote.c b/peek-remote.c
index ceb7871..59cc8eb 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -25,7 +25,7 @@ int main(int argc, char **argv)
int i, ret;
char *dest = NULL;
int fd[2];
- pid_t pid;
+ struct child_process *chld;
int nongit = 0;
unsigned flags = 0;
@@ -64,12 +64,10 @@ int main(int argc, char **argv)
if (!dest || i != argc - 1)
usage(peek_remote_usage);
- pid = git_connect(fd, dest, uploadpack, 0);
- if (pid < 0)
- return 1;
+ chld = git_connect(fd, dest, uploadpack, 0);
ret = peek_remote(fd, flags);
close(fd[0]);
close(fd[1]);
- ret |= finish_connect(pid);
+ ret |= finish_connect(chld);
return !!ret;
}
diff --git a/send-pack.c b/send-pack.c
index 4533d1b..e9257fd 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -362,7 +362,7 @@ int main(int argc, char **argv)
char *dest = NULL;
char **heads = NULL;
int fd[2], ret;
- pid_t pid;
+ struct child_process *chld;
char *remote_name = NULL;
struct remote *remote = NULL;
@@ -426,12 +426,10 @@ int main(int argc, char **argv)
}
}
- pid = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
- if (pid < 0)
- return 1;
+ chld = git_connect(fd, dest, receivepack, verbose ? CONNECT_VERBOSE : 0);
ret = send_pack(fd[0], fd[1], remote, nr_heads, heads);
close(fd[0]);
close(fd[1]);
- ret |= finish_connect(pid);
+ ret |= finish_connect(chld);
return !!ret;
}
diff --git a/transport.c b/transport.c
index 3475cca..54ed9bc 100644
--- a/transport.c
+++ b/transport.c
@@ -279,18 +279,13 @@ static struct ref *get_refs_via_connect(const struct transport *transport)
struct git_transport_data *data = transport->data;
struct ref *refs;
int fd[2];
- pid_t pid;
char *dest = xstrdup(transport->url);
-
- pid = git_connect(fd, dest, data->uploadpack, 0);
-
- if (pid < 0)
- die("Failed to connect to \"%s\"", transport->url);
+ struct child_process *chld = git_connect(fd, dest, data->uploadpack, 0);
get_remote_heads(fd[0], &refs, 0, NULL, 0);
packet_flush(fd[1]);
- finish_connect(pid);
+ finish_connect(chld);
free(dest);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 4/5] Use run_command() to spawn external diff programs instead of fork/exec.
From: Johannes Sixt @ 2007-09-30 20:10 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1191183001-5368-4-git-send-email-johannes.sixt@telecom.at>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
diff.c | 38 +++-----------------------------------
1 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/diff.c b/diff.c
index 0bd7e24..29dfa82 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,7 @@
#include "xdiff-interface.h"
#include "color.h"
#include "attr.h"
+#include "run-command.h"
#ifdef NO_FAST_WORKING_DIRECTORY
#define FAST_WORKING_DIRECTORY 0
@@ -1748,40 +1749,6 @@ static void remove_tempfile_on_signal(int signo)
raise(signo);
}
-static int spawn_prog(const char *pgm, const char **arg)
-{
- pid_t pid;
- int status;
-
- fflush(NULL);
- pid = fork();
- if (pid < 0)
- die("unable to fork");
- if (!pid) {
- execvp(pgm, (char *const*) arg);
- exit(255);
- }
-
- while (waitpid(pid, &status, 0) < 0) {
- if (errno == EINTR)
- continue;
- return -1;
- }
-
- /* Earlier we did not check the exit status because
- * diff exits non-zero if files are different, and
- * we are not interested in knowing that. It was a
- * mistake which made it harder to quit a diff-*
- * session that uses the git-apply-patch-script as
- * the GIT_EXTERNAL_DIFF. A custom GIT_EXTERNAL_DIFF
- * should also exit non-zero only when it wants to
- * abort the entire diff-* session.
- */
- if (WIFEXITED(status) && !WEXITSTATUS(status))
- return 0;
- return -1;
-}
-
/* An external diff command takes:
*
* diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -1834,7 +1801,8 @@ static void run_external_diff(const char *pgm,
*arg++ = name;
}
*arg = NULL;
- retval = spawn_prog(pgm, spawn_arg);
+ fflush(NULL);
+ retval = run_command_v_opt(spawn_arg, 0);
remove_tempfile();
if (retval) {
fprintf(stderr, "external diff died, stopping at %s.\n", name);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 2/5] Use start_command() in git_connect() instead of explicit fork/exec.
From: Johannes Sixt @ 2007-09-30 20:09 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1191183001-5368-2-git-send-email-johannes.sixt@telecom.at>
The child process handling is delegated to start_command() and
finish_command().
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
connect.c | 104 ++++++++++++++++++++++++++++--------------------------------
1 files changed, 49 insertions(+), 55 deletions(-)
diff --git a/connect.c b/connect.c
index f6dcab9..29fd431 100644
--- a/connect.c
+++ b/connect.c
@@ -482,11 +482,12 @@ struct child_process *git_connect(int fd[2], char *url,
char *host, *path = url;
char *end;
int c;
- int pipefd[2][2];
struct child_process *chld;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
+ const char **arg;
+ struct strbuf cmd;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
@@ -572,72 +573,65 @@ struct child_process *git_connect(int fd[2], char *url,
return NULL;
}
- if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
- die("unable to create pipe pair for communication");
chld = xcalloc(1, sizeof(*chld));
- chld->pid = fork();
- if (chld->pid < 0)
- die("unable to fork");
- if (!chld->pid) {
- struct strbuf cmd;
-
- strbuf_init(&cmd, MAX_CMD_LEN);
- strbuf_addstr(&cmd, prog);
- strbuf_addch(&cmd, ' ');
- sq_quote_buf(&cmd, path);
- if (cmd.len >= MAX_CMD_LEN)
- die("command line too long");
-
- dup2(pipefd[1][0], 0);
- dup2(pipefd[0][1], 1);
- close(pipefd[0][0]);
- close(pipefd[0][1]);
- close(pipefd[1][0]);
- close(pipefd[1][1]);
- if (protocol == PROTO_SSH) {
- const char *ssh, *ssh_basename;
- ssh = getenv("GIT_SSH");
- if (!ssh) ssh = "ssh";
- ssh_basename = strrchr(ssh, '/');
- if (!ssh_basename)
- ssh_basename = ssh;
- else
- ssh_basename++;
- if (!port)
- execlp(ssh, ssh_basename, host, cmd.buf, NULL);
- else
- execlp(ssh, ssh_basename, "-p", port, host,
- cmd.buf, NULL);
+ strbuf_init(&cmd, MAX_CMD_LEN);
+ strbuf_addstr(&cmd, prog);
+ strbuf_addch(&cmd, ' ');
+ sq_quote_buf(&cmd, path);
+ if (cmd.len >= MAX_CMD_LEN)
+ die("command line too long");
+
+ chld->in = chld->out = -1;
+ chld->argv = arg = xcalloc(6, sizeof(*arg));
+ if (protocol == PROTO_SSH) {
+ const char *ssh = getenv("GIT_SSH");
+ if (!ssh) ssh = "ssh";
+
+ *arg++ = ssh;
+ if (port) {
+ *arg++ = "-p";
+ *arg++ = port;
}
- else {
- unsetenv(ALTERNATE_DB_ENVIRONMENT);
- unsetenv(DB_ENVIRONMENT);
- unsetenv(GIT_DIR_ENVIRONMENT);
- unsetenv(GIT_WORK_TREE_ENVIRONMENT);
- unsetenv(GRAFT_ENVIRONMENT);
- unsetenv(INDEX_ENVIRONMENT);
- execlp("sh", "sh", "-c", cmd.buf, NULL);
- }
- die("exec failed");
+ *arg++ = host;
+ }
+ else {
+ /* remove these from the environment */
+ const char *env[] = {
+ ALTERNATE_DB_ENVIRONMENT,
+ DB_ENVIRONMENT,
+ GIT_DIR_ENVIRONMENT,
+ GIT_WORK_TREE_ENVIRONMENT,
+ GRAFT_ENVIRONMENT,
+ INDEX_ENVIRONMENT,
+ NULL
+ };
+ chld->env = env;
+ *arg++ = "sh";
+ *arg++ = "-c";
}
- fd[0] = pipefd[0][0];
- fd[1] = pipefd[1][1];
- close(pipefd[0][1]);
- close(pipefd[1][0]);
+ *arg++ = cmd.buf;
+ *arg = NULL;
+
+ if (start_command(chld))
+ die("unable to fork");
+
+ fd[0] = chld->out; /* read from child's stdout */
+ fd[1] = chld->in; /* write to child's stdin */
if (free_path)
free(path);
+ strbuf_release(&cmd);
return chld;
}
int finish_connect(struct child_process *chld)
{
+ int code;
if (chld == NULL)
return 0;
- while (waitpid(chld->pid, NULL, 0) < 0) {
- if (errno != EINTR)
- return -1;
- }
- return 0;
+ code = finish_command(chld);
+ free(chld->argv);
+ free(chld);
+ return code;
}
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 3/5] Use start_command() to run the filter instead of explicit fork/exec.
From: Johannes Sixt @ 2007-09-30 20:09 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
In-Reply-To: <1191183001-5368-3-git-send-email-johannes.sixt@telecom.at>
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
convert.c | 36 ++++++++++--------------------------
1 files changed, 10 insertions(+), 26 deletions(-)
diff --git a/convert.c b/convert.c
index 0d5e909..560679d 100644
--- a/convert.c
+++ b/convert.c
@@ -196,40 +196,24 @@ static int filter_buffer(const char *path, const char *src,
/*
* Spawn cmd and feed the buffer contents through its stdin.
*/
- struct child_process child_process;
- int pipe_feed[2];
+ struct child_process chld;
int write_err, status;
+ const char *argv[] = { "sh", "-c", cmd, NULL };
- memset(&child_process, 0, sizeof(child_process));
+ memset(&chld, 0, sizeof(chld));
+ chld.argv = argv;
+ chld.in = -1;
- if (pipe(pipe_feed) < 0) {
- error("cannot create pipe to run external filter %s", cmd);
- return 1;
- }
+ if (start_command(&chld))
+ return error("cannot fork to run external filter %s", cmd);
- child_process.pid = fork();
- if (child_process.pid < 0) {
- error("cannot fork to run external filter %s", cmd);
- close(pipe_feed[0]);
- close(pipe_feed[1]);
- return 1;
- }
- if (!child_process.pid) {
- dup2(pipe_feed[0], 0);
- close(pipe_feed[0]);
- close(pipe_feed[1]);
- execlp("sh", "sh", "-c", cmd, NULL);
- return 1;
- }
- close(pipe_feed[0]);
-
- write_err = (write_in_full(pipe_feed[1], src, size) < 0);
- if (close(pipe_feed[1]))
+ write_err = (write_in_full(chld.in, src, size) < 0);
+ if (close(chld.in))
write_err = 1;
if (write_err)
error("cannot feed the input to external filter %s", cmd);
- status = finish_command(&child_process);
+ status = finish_command(&chld);
if (status)
error("external filter %s failed %d", cmd, -status);
return (write_err || status);
--
1.5.3.3.1134.gee562
^ permalink raw reply related
* [PATCH 0/5] fork/exec removal series
From: Johannes Sixt @ 2007-09-30 20:09 UTC (permalink / raw)
To: gitster; +Cc: git, Johannes Sixt
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.
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 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.)
- The upload-pack case calls for an entirely different solution.
[*] Just this weekend Mike Pape ported git-daemon, but I didn't find the
time to have it integrated in this series - if that were possible at all.
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).
builtin-archive.c | 8 +--
builtin-fetch-pack.c | 63 ++++++++-----------------
cache.h | 4 +-
connect.c | 126 ++++++++++++++++++++++++--------------------------
convert.c | 36 ++++----------
diff.c | 38 +--------------
peek-remote.c | 8 +--
send-pack.c | 8 +--
transport.c | 9 +---
9 files changed, 106 insertions(+), 194 deletions(-)
-- Hannes
^ permalink raw reply
* Re: suggestion for git stash
From: Junio C Hamano @ 2007-09-30 19:59 UTC (permalink / raw)
To: Bruno Haible; +Cc: git
In-Reply-To: <200709302050.41273.bruno@clisp.org>
Isn't "stash apply --index" what you talk about?
^ permalink raw reply
* Re: GIT_EXTERNAL_DIFF invoked with undocumented calling convention after unstashing conflicts
From: Junio C Hamano @ 2007-09-30 19:59 UTC (permalink / raw)
To: Bruno Haible; +Cc: git
In-Reply-To: <200709302117.37422.bruno@clisp.org>
> "git diff --uncached" invokes the GIT_EXTERNAL_DIFF variable with just one
> argument (instead of 7 arguments, as documented) in a particular situation:
You are looking at an unmerged path.
'GIT_EXTERNAL_DIFF'::
When the environment variable 'GIT_EXTERNAL_DIFF' is set, the
program named by it is called, instead of the diff invocation
described above. For a path that is added, removed, or modified,
'GIT_EXTERNAL_DIFF' is called with 7 parameters:
...
+
For a path that is unmerged, 'GIT_EXTERNAL_DIFF' is called with 1
parameter, <path>.
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".
^ 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