* [PATCH] User Manual: add a chapter for submodules
From: Miklos Vajna @ 2007-09-20 0:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
In-Reply-To: <7vbqbyphil.fsf@gitster.siamese.dyndns.org>
Signed-off-by: Michael Smith <msmith@cbnco.com>
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
[ 2nd version ]
On Wed, Sep 19, 2007 at 12:44:18PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > +To see how submodule support works, create (for example) four example
> > +repository that can be used later as a submodule:
>
> s/repository/repositories/;
fixed.
> > +$ for i in a b c d
> > +do
> > + mkdir $i
> > + cd $i
> > + git init
> > + echo "module $i" > $i.txt
> > + git add $i.txt
> > + git commit -m "Initial commit, submodule $mod"
>
> s/\$mod/$i/;
fixed
> > +$ echo hi > super.txt
> > +$ git add super.txt
> > +$ git commit -m "Initial commit of empty superproject"
>
> This is not *empty*; do you even need this step?
in fact, no :) removed.
> > +-------------------------------------------------
> > +$ git commit -m "Add submodules a, b, c, d."
> > +-------------------------------------------------
>
> s/c, d./c and d./;
fixed.
> > +$ ls -a a
> > +. ..
> > +$ git submodule status
> > +-d266b9873ad50488163457f025db7cdd9683d88b a
> > +-e81d457da15309b4fef4249aba9b50187999670d b
> > +-c1536a972b9affea0f16e0680ba87332dc059146 c
> > +-d96249ff5d57de5de093e6baff9e0aafa5276a74 d
> > +-------------------------------------------------
>
> You might want to mention...
>
> Note: the commit object names shown above would be different for
> you, but they should match the HEAD commit object names of your
> repositories. You can check it by doing:
>
> $ git ls-remote ../a
i've added such a note.
> > +If you want to make a change within a submodule, you should first check out a
> > +branch, make your changes, publish the change within the submodule, and then
> > +update the superproject to reference the new commit:
> > +
> > +-------------------------------------------------
> > +$ git branch
> > +* (no branch)
> > + master
> > +$ git checkout master
>
> I am not so sure about this advice. Don't you want to see how
> the detached HEAD and 'master' (or any other branches) are
> related before doing this? You might even want to create a
> "fix-up" branch that is rooted at the detached HEAD if the
> change you are making is to fix minor details of the submodule
> to suit what superproject wants (i.e. "little feature that is
> applicable to the submodule as a standalone project, meant to
> be pushed back to the submodule upstream").
right, changed it to two examples, one about "git checkout master", and
the other about "git checkout -b fix-up"
> > +$ git push
> > +$ cd ..
> > +$ git add a
>
> Before doing this "git add", it would be educational to have the
> user do
>
> $ git diff
>
> which would say that now the submodule is being updated.
added.
> > +$ git commit -m "Updated submodule a."
> > +$ git push
> > +-------------------------------------------------
> > +
> > +NOTE: This means that you have to run `git submodule update` after `git pull`
> > +if you want to update the subprojects, too.
>
> I do not quite understand this note. I do understand the part
> after "that you have to", but I do not know how the above
> example relates to that --- iow, the above example does not
> seem to "mean" such thing to me.
>
> Also "submodule" is used consistently in the rest of the
> document but this sentence talks about "subproject".
yes, it was illogical to use a 'note' here as it's not related. i've
just added a separte sentence about this without the 'this means'.
> > +Problems with submodules
> > +------------------------
>
> The list is good, but are they Problems or pitfalls to watch out
> for?
the later :) title changed.
> > +Always publish the submodule change before publishing the change to the
> > +superproject that references it. If you forget to publish the submodule change,
> > +others won't be able to clone the repository:
>
> The same caution applies not to rewind branches in submodule
> beyond commits that were ever recorded in any superproject.
i've added a paragraph about this issue.
> > +-------------------------------------------------
> > +$ echo i added another line to this file >> a.txt
> > +$ git commit -a -m "doing it wrong this time"
>
> It is not clear in which repository you are supposed to try this
> command in the example sequence, as we crossed the section
> boundary. I am _guessing_ that the above two commands are to be
> run inside ~/git/cloned-2/a after redoing ~/git/cloned-2 the
> same way as you did ~/git/cloned, but you may want to make it
> clear.
right, i forgot the 'cd ~/git/super/a'. provided the repos are
symmetric, i think it would just make the situation unnecessary
complicated to clone a third repo.
> > +$ git submodule update
> > +error: pathspec '261dfac35cb99d380eb966e102c1197139f7fa24' did not match any file(s) known to git.
> > +Did you forget to 'git add'?
>
> I sense there is a bug in "git submodule update" here. I do not
> have time to look at what it's doing right now, but this is a
> symptom of running:
>
> $ git checkout 261dfac35...
>
> without checking if that is a valid commit object name, and git
> checkout cannot decide if the request is about detaching the
> head at commit 261dfac35 or checking the path 261dfac35 out of
> the current index. Porcelains should check with "cat-file -t"
> first.
hm yes. i think as long as this behaviour is not changed, it would not
be good to remove the "Did you forget to 'git add'?" part from the
example
> > +Unable to checkout '261dfac35cb99d380eb966e102c1197139f7fa24' in submodule path 'a'
> > +-------------------------------------------------
> > +
> > +It's not safe to run `git submodule update` if you've made changes within a
> > +submodule. They will be silently overwritten:
>
> You want to distinguish two cases here. Local uncommitted
> changes, and committed changes. The latter case is switched
> away without too much noise, which is what you are demonstrating
> here. I _think_ local uncommitted changes will not be
> overwritten at all, unless "submodule update" does "reset
> --hard", which I suspect it doesn't.
i've just checked that it does a git checkout, so uncommitted changes
are preserved. i've updated this part to mention this fact.
i've also added the suggestions by skimo.
VMiklos
Documentation/user-manual.txt | 175 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 175 insertions(+), 0 deletions(-)
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index ecb2bf9..ce0cf38 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -3155,6 +3155,181 @@ a tree which you are in the process of working on.
If you blow the index away entirely, you generally haven't lost any
information as long as you have the name of the tree that it described.
+[[submodules]]
+Submodules
+==========
+
+This tutorial explains how to create and publish a repository with submodules
+using the gitlink:git-submodule[1] command.
+
+Submodules maintain their own identity; the submodule support just stores the
+submodule repository location and commit ID, so other developers who clone the
+superproject can easily clone all the submodules at the same revision.
+
+To see how submodule support works, create (for example) four example
+repository that can be used later as a submodule:
+
+-------------------------------------------------
+$ mkdir ~/git
+$ cd ~/git
+$ for i in a b c d
+do
+ mkdir $i
+ cd $i
+ git init
+ echo "module $i" > $i.txt
+ git add $i.txt
+ git commit -m "Initial commit, submodule $mod"
+ cd ..
+done
+-------------------------------------------------
+
+Now create the superproject and add all the submodules:
+
+-------------------------------------------------
+$ mkdir super
+$ cd super
+$ git init
+$ echo hi > super.txt
+$ git add super.txt
+$ git commit -m "Initial commit of empty superproject"
+$ for i in a b c d
+do
+ git submodule add ~/git/$i
+done
+-------------------------------------------------
+
+See what files `git submodule` created:
+
+-------------------------------------------------
+$ ls -a
+. .. .git .gitmodules a b c d super.txt
+-------------------------------------------------
+
+The `git submodule add` command does a couple of things:
+
+- It clones the submodule under the current directory and by default checks out
+ the master branch.
+- It adds the submodule's clone path to the `.gitmodules` file and adds this
+ file to the index, ready to be committed.
+- It adds the submodule's current commit ID to the index, ready to be
+ committed.
+
+Commit the superproject:
+
+-------------------------------------------------
+$ git commit -m "Add submodules a, b, c, d."
+-------------------------------------------------
+
+Now clone the superproject:
+
+-------------------------------------------------
+$ cd ..
+$ git clone super cloned
+$ cd cloned
+-------------------------------------------------
+
+The submodule directories are there, but they're empty:
+
+-------------------------------------------------
+$ ls -a a
+. ..
+$ git submodule status
+-d266b9873ad50488163457f025db7cdd9683d88b a
+-e81d457da15309b4fef4249aba9b50187999670d b
+-c1536a972b9affea0f16e0680ba87332dc059146 c
+-d96249ff5d57de5de093e6baff9e0aafa5276a74 d
+-------------------------------------------------
+
+Pulling down the submodules is a two-step process. First run `git submodule
+init` to add the submodule repository URLs to `.git/config`:
+
+-------------------------------------------------
+$ git submodule init
+-------------------------------------------------
+
+Now use `git submodule update` to clone the repositories and check out the
+commits specified in the superproject:
+
+-------------------------------------------------
+$ git submodule update
+$ cd a
+$ ls -a
+. .. .git a.txt
+-------------------------------------------------
+
+One major difference between `git submodule update` and `git submodule add` is
+that `git submodule update` checks out a specific commit, rather than the tip
+of a branch. It's like checking out a tag: the head is detached, so you're not
+working on a branch.
+
+-------------------------------------------------
+$ git branch
+* (no branch)
+ master
+-------------------------------------------------
+
+If you want to make a change within a submodule, you should first check out a
+branch, make your changes, publish the change within the submodule, and then
+update the superproject to reference the new commit:
+
+-------------------------------------------------
+$ git branch
+* (no branch)
+ master
+$ git checkout master
+$ echo "adding a line again" >> a.txt
+$ git commit -a -m "Updated the submodule from within the superproject."
+$ git push
+$ cd ..
+$ git add a
+$ git commit -m "Updated submodule a."
+$ git push
+-------------------------------------------------
+
+NOTE: This means that you have to run `git submodule update` after `git pull`
+if you want to update the subprojects, too.
+
+Problems with submodules
+------------------------
+
+Always publish the submodule change before publishing the change to the
+superproject that references it. If you forget to publish the submodule change,
+others won't be able to clone the repository:
+
+-------------------------------------------------
+$ echo i added another line to this file >> a.txt
+$ git commit -a -m "doing it wrong this time"
+$ cd ..
+$ git add a
+$ git commit -m "Updated submodule a again."
+$ git push
+$ cd ~/git/cloned
+$ git pull
+$ git submodule update
+error: pathspec '261dfac35cb99d380eb966e102c1197139f7fa24' did not match any file(s) known to git.
+Did you forget to 'git add'?
+Unable to checkout '261dfac35cb99d380eb966e102c1197139f7fa24' in submodule path 'a'
+-------------------------------------------------
+
+It's not safe to run `git submodule update` if you've made changes within a
+submodule. They will be silently overwritten:
+
+-------------------------------------------------
+$ cat a.txt
+module a
+$ echo line added from private2 >> a.txt
+$ git commit -a -m "line added inside private2"
+$ cd ..
+$ git submodule update
+Submodule path 'a': checked out 'd266b9873ad50488163457f025db7cdd9683d88b'
+$ cd a
+$ cat a.txt
+module a
+-------------------------------------------------
+
+NOTE: The changes are still visible in the submodule's reflog.
+
[[low-level-operations]]
Low-level git operations
========================
--
1.5.3.1.1.g1e61-dirty
^ permalink raw reply related
* [PATCH] User Manual: add a chapter for submodules
From: Miklos Vajna @ 2007-09-20 0:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
In-Reply-To: <7vbqbyphil.fsf@gitster.siamese.dyndns.org>
Signed-off-by: Michael Smith <msmith@cbnco.com>
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
Sorry, I sent the original patch again. So here is the updated second version.
Documentation/user-manual.txt | 202 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 202 insertions(+), 0 deletions(-)
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index ecb2bf9..aee654c 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -3155,6 +3155,208 @@ a tree which you are in the process of working on.
If you blow the index away entirely, you generally haven't lost any
information as long as you have the name of the tree that it described.
+[[submodules]]
+Submodules
+==========
+
+This tutorial explains how to create and publish a repository with submodules
+using the gitlink:git-submodule[1] command.
+
+Submodules maintain their own identity; the submodule support just stores the
+submodule repository location and commit ID, so other developers who clone the
+superproject can easily clone all the submodules at the same revision.
+
+To see how submodule support works, create (for example) four example
+repositories that can be used later as a submodule:
+
+-------------------------------------------------
+$ mkdir ~/git
+$ cd ~/git
+$ for i in a b c d
+do
+ mkdir $i
+ cd $i
+ git init
+ echo "module $i" > $i.txt
+ git add $i.txt
+ git commit -m "Initial commit, submodule $i"
+ cd ..
+done
+-------------------------------------------------
+
+Now create the superproject and add all the submodules:
+
+-------------------------------------------------
+$ mkdir super
+$ cd super
+$ git init
+$ for i in a b c d
+do
+ git submodule add ~/git/$i
+done
+-------------------------------------------------
+
+NOTE: Do not use local URLs here if you plan to publish your superproject!
+
+See what files `git submodule` created:
+
+-------------------------------------------------
+$ ls -a
+. .. .git .gitmodules a b c d
+-------------------------------------------------
+
+The `git submodule add` command does a couple of things:
+
+- It clones the submodule under the current directory and by default checks out
+ the master branch.
+- It adds the submodule's clone path to the `.gitmodules` file and adds this
+ file to the index, ready to be committed.
+- It adds the submodule's current commit ID to the index, ready to be
+ committed.
+
+Commit the superproject:
+
+-------------------------------------------------
+$ git commit -m "Add submodules a, b, c and d."
+-------------------------------------------------
+
+Now clone the superproject:
+
+-------------------------------------------------
+$ cd ..
+$ git clone super cloned
+$ cd cloned
+-------------------------------------------------
+
+The submodule directories are there, but they're empty:
+
+-------------------------------------------------
+$ ls -a a
+. ..
+$ git submodule status
+-d266b9873ad50488163457f025db7cdd9683d88b a
+-e81d457da15309b4fef4249aba9b50187999670d b
+-c1536a972b9affea0f16e0680ba87332dc059146 c
+-d96249ff5d57de5de093e6baff9e0aafa5276a74 d
+-------------------------------------------------
+
+NOTE: The commit object names shown above would be different for you, but they
+should match the HEAD commit object names of your repositories. You can check
+it by running `git ls-remote ../a`.
+
+Pulling down the submodules is a two-step process. First run `git submodule
+init` to add the submodule repository URLs to `.git/config`:
+
+-------------------------------------------------
+$ git submodule init
+-------------------------------------------------
+
+Now use `git submodule update` to clone the repositories and check out the
+commits specified in the superproject:
+
+-------------------------------------------------
+$ git submodule update
+$ cd a
+$ ls -a
+. .. .git a.txt
+-------------------------------------------------
+
+One major difference between `git submodule update` and `git submodule add` is
+that `git submodule update` checks out a specific commit, rather than the tip
+of a branch. It's like checking out a tag: the head is detached, so you're not
+working on a branch.
+
+-------------------------------------------------
+$ git branch
+* (no branch)
+ master
+-------------------------------------------------
+
+If you want to make a change within a submodule and you have a detached head,
+then you should create or checkout a branch, make your changes, publish the
+change within the submodule, and then update the superproject to reference the
+new commit:
+
+-------------------------------------------------
+$ git checkout master
+-------------------------------------------------
+
+or
+
+-------------------------------------------------
+$ git checkout -b fix-up
+-------------------------------------------------
+
+then
+
+-------------------------------------------------
+$ echo "adding a line again" >> a.txt
+$ git commit -a -m "Updated the submodule from within the superproject."
+$ git push
+$ cd ..
+$ git diff
+diff --git a/a b/a
+index d266b98..261dfac 160000
+--- a/a
++++ b/a
+@@ -1 +1 @@
+-Subproject commit d266b9873ad50488163457f025db7cdd9683d88b
++Subproject commit 261dfac35cb99d380eb966e102c1197139f7fa24
+$ git add a
+$ git commit -m "Updated submodule a."
+$ git push
+-------------------------------------------------
+
+You have to run `git submodule update` after `git pull` if you want to update
+submodules, too.
+
+Pitfalls with submodules
+------------------------
+
+Always publish the submodule change before publishing the change to the
+superproject that references it. If you forget to publish the submodule change,
+others won't be able to clone the repository:
+
+-------------------------------------------------
+$ cd ~/git/super/a
+$ echo i added another line to this file >> a.txt
+$ git commit -a -m "doing it wrong this time"
+$ cd ..
+$ git add a
+$ git commit -m "Updated submodule a again."
+$ git push
+$ cd ~/git/cloned
+$ git pull
+$ git submodule update
+error: pathspec '261dfac35cb99d380eb966e102c1197139f7fa24' did not match any file(s) known to git.
+Did you forget to 'git add'?
+Unable to checkout '261dfac35cb99d380eb966e102c1197139f7fa24' in submodule path 'a'
+-------------------------------------------------
+
+You also should not rewind branches in a submodule beyond commits that were
+ever recorded in any superproject.
+
+It's not safe to run `git submodule update` if you've made and committed
+changes within a submodule without checking out a branch first. They will be
+silently overwritten:
+
+-------------------------------------------------
+$ cat a.txt
+module a
+$ echo line added from private2 >> a.txt
+$ git commit -a -m "line added inside private2"
+$ cd ..
+$ git submodule update
+Submodule path 'a': checked out 'd266b9873ad50488163457f025db7cdd9683d88b'
+$ cd a
+$ cat a.txt
+module a
+-------------------------------------------------
+
+NOTE: The changes are still visible in the submodule's reflog.
+
+This is not the case if you did not commit your changes.
+
[[low-level-operations]]
Low-level git operations
========================
--
1.5.3.1.1.g1e61-dirty
^ permalink raw reply related
* Re: [PATCH 7/7] Implement git commit as a builtin command.
From: Junio C Hamano @ 2007-09-20 1:27 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
In-Reply-To: <11900740163661-git-send-email-krh@redhat.com>
Kristian Høgsberg <krh@redhat.com> writes:
> diff --git a/builtin-commit.c b/builtin-commit.c
> new file mode 100644
> index 0000000..ee98de9
> --- /dev/null
> +++ b/builtin-commit.c
> @@ -0,0 +1,740 @@
> +/*
> + * Builtin "git commit"
> + *
> + * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
> + * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
With 85023577a8f4b540aa64aa37f6f44578c0c305a3 (simplify
inclusion of system header files.), we introduced a rule against
these lines. We probably would want a Coding Guideline (aka
"Hacking Git") document somewhere.
> +#include "cache.h"
> +#include "cache-tree.h"
> +#include "builtin.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +#include "commit.h"
> +#include "revision.h"
> +#include "wt-status.h"
> +#include "run-command.h"
> +#include "refs.h"
> +#include "log-tree.h"
> +#include "strbuf.h"
> +#include "utf8.h"
> +
> +static const char builtin_commit_usage[] =
> + "[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [--template <file>] [[-i | -o] <path>...]";
> +
> +static unsigned char head_sha1[20], merge_head_sha1[20];
> +static char *use_message_buffer;
> +static const char commit_editmsg[] = "COMMIT_EDITMSG";
> +static struct lock_file lock_file;
> +
> +enum option_type {
> + OPTION_NONE,
> + OPTION_STRING,
> + OPTION_INTEGER,
> + OPTION_LAST,
> +};
> +
> +struct option {
> + enum option_type type;
> + const char *long_name;
> + char short_name;
> + void *value;
> +};
> +
> +static int scan_options(const char ***argv, struct option *options)
> +{
> + const char *value, *eq;
> + int i;
> +
> + if (**argv == NULL)
> + return 0;
> + if ((**argv)[0] != '-')
> + return 0;
> + if (!strcmp(**argv, "--"))
> + return 0;
> +
> + value = NULL;
> + for (i = 0; options[i].type != OPTION_LAST; i++) {
> + if ((**argv)[1] == '-') {
> + if (!prefixcmp(options[i].long_name, **argv + 2)) {
> + if (options[i].type != OPTION_NONE)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + eq = strchr(**argv + 2, '=');
> + if (eq && options[i].type != OPTION_NONE &&
> + !strncmp(**argv + 2,
> + options[i].long_name, eq - **argv - 2)) {
> + value = eq + 1;
> + goto match;
> + }
> + }
> +
> + if ((**argv)[1] == options[i].short_name) {
> + if ((**argv)[2] == '\0') {
> + if (options[i].type != OPTION_NONE)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + if (options[i].type != OPTION_NONE) {
> + value = **argv + 2;
> + goto match;
> + }
> + }
> + }
How do you disambiguate between "--author <me>" and "--amend"?
"The order in *options list matters" is an acceptable answer but
it needs to be documented.
> +
> + usage(builtin_commit_usage);
> +
> + match:
> + switch (options[i].type) {
> + case OPTION_NONE:
> + *(int *)options[i].value = 1;
> + break;
> + case OPTION_STRING:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
> + *(const char **)options[i].value = value;
> + break;
> + case OPTION_INTEGER:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
> + *(int *)options[i].value = atoi(value);
> + break;
> + default:
> + assert(0);
> + }
> +
> + (*argv)++;
> +
> + return 1;
> +}
I do not particularly like this OPTION_LAST convention, but that
is a minor detail. I also suspect in the longer term we might
be better off using getopt() or popt(), but that would be a
larger project.
In any case, if you want to use this option parser, you would
need to add a new file, perhaps parse_options.c, and move this
part there, so that other parts of the system can reuse it.
And the same comment goes for the launch_editor still in
builtin-tag.c. We should move it to editor.c or something.
> +
> +static char *logfile, *force_author, *message, *template_file;
> +static char *edit_message, *use_message;
> +static int all, edit_flag, also, interactive, only, no_verify, amend, signoff;
> +static int quiet, verbose, untracked_files;
> +
> +static int no_edit, initial_commit, in_merge;
> +const char *only_include_assumed;
> +
> +static struct option commit_options[] = {
> + { OPTION_STRING, "file", 'F', (void *) &logfile },
> + { OPTION_NONE, "all", 'a', &all },
> + { OPTION_STRING, "author", 0, (void *) &force_author },
> + { OPTION_NONE, "edit", 'e', &edit_flag },
Why do some get casted to (void*) and others don't? It doesn't
seem to have any pattern. I am puzzled...
> + { OPTION_NONE, "include", 'i', &also },
> + { OPTION_NONE, "interactive", 0, &interactive },
> + { OPTION_NONE, "only", 'o', &only },
> + { OPTION_STRING, "message", 'm', &message },
> + { OPTION_NONE, "no-verify", 'n', &no_verify },
> + { OPTION_NONE, "amend", 0, &amend },
> + { OPTION_STRING, "reedit-message", 'c', &edit_message },
> + { OPTION_STRING, "reuse-message", 'C', &use_message },
> + { OPTION_NONE, "signoff", 's', &signoff },
> + { OPTION_NONE, "quiet", 'q', &quiet },
> + { OPTION_NONE, "verbose", 'v', &verbose },
> + { OPTION_NONE, "untracked-files", 0, &untracked_files },
> + { OPTION_STRING, "template", 't', &template_file },
> + { OPTION_LAST },
> +};
> +
> +/* FIXME: Taken from builtin-add, should be shared. */
You're darn right. I thought you have some other patch that
touches builtin-add already...
> +
> +static void update_callback(struct diff_queue_struct *q,
> + struct diff_options *opt, void *cbdata)
> +{
> + int i, verbose;
> +
> + verbose = *((int *)cbdata);
> + for (i = 0; i < q->nr; i++) {
> + struct diff_filepair *p = q->queue[i];
> + const char *path = p->one->path;
> + switch (p->status) {
> + default:
> + die("unexpacted diff status %c", p->status);
> + case DIFF_STATUS_UNMERGED:
> + case DIFF_STATUS_MODIFIED:
> + case DIFF_STATUS_TYPE_CHANGED:
> + add_file_to_cache(path, verbose);
> + break;
> + case DIFF_STATUS_DELETED:
> + remove_file_from_cache(path);
> + cache_tree_invalidate_path(active_cache_tree, path);
I've updated remove_file_from_cache() to invalidate the path in
cache-tree so this does not hurt but is no longer necessary. I
removed this line in the version I queued in 'pu'.
> + if (verbose)
> + printf("remove '%s'\n", path);
> + break;
> + }
> + }
> +}
> +
> +static void
> +add_files_to_cache(int fd, const char **files, const char *prefix)
> +{
> + struct rev_info rev;
> +
> + init_revisions(&rev, "");
> + setup_revisions(0, NULL, &rev, NULL);
> + rev.prune_data = get_pathspec(prefix, files);
> + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> + rev.diffopt.format_callback = update_callback;
> + rev.diffopt.format_callback_data = &verbose;
> +
> + run_diff_files(&rev, 0);
> + refresh_cache(REFRESH_QUIET);
Your update_callback() does add_file_to_cache() which picks up
the stat information from the filesystem already, so I do not
see much point calling refresh_cache() afterwards. On the other
hand, refreshing before running diff_files() might make sense,
as you can avoid a lot of unnecessary work when you are told to
do "git commit ."
Have you tested this with an unmerged index?
> +
> + if (write_cache(fd, active_cache, active_nr) || close(fd))
> + die("unable to write new index file");
> +}
> +
> +static char *
> +prepare_index(const char **files, const char *prefix)
> +{
> + int fd;
> + struct tree *tree;
> + struct lock_file *next_index_lock;
> +
> + fd = hold_locked_index(&lock_file, 1);
> + if (read_cache() < 0)
> + die("index file corrupt");
> +
> + if (all) {
> + add_files_to_cache(fd, files, NULL);
> + return lock_file.filename;
Should you be passing files, which is used as pathspec to decide
if your update_callback() should be called, under --all option?
> + } else if (also) {
> + add_files_to_cache(fd, files, prefix);
> + return lock_file.filename;
> + }
> +
> + if (interactive)
> + interactive_add();
Don't you need to
(1) abort if the user aborts out of interactive add with ^C?
(2) re-read the index after interactive_add() returns?
> + if (*files == NULL) {
> + /* Commit index as-is. */
> + rollback_lock_file(&lock_file);
> + return get_index_file();
> + }
> +
> + /*
> + * FIXME: Warn on unknown files. Shell script does
> + *
> + * commit_only=`git-ls-files --error-unmatch -- "$@"`
> + */
This should be much easier to do than from the shell script, as
you have active_cache[] (aka "the_index.cache[]") in-core.
> + /*
> + * FIXME: shell script does
> + *
> + * git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
> + *
> + * which warns about unmerged files in the index.
> + */
I think "unmerged warning" is an unintended side effect. The
point of the command is to have a copy of index, as there is no
way for shell script to work with more than one index at a time
without using a temporary file.
> +
> + /* update the user index file */
> + add_files_to_cache(fd, files, prefix);
> +
> + if (!initial_commit) {
> + tree = parse_tree_indirect(head_sha1);
> + if (!tree)
> + die("failed to unpack HEAD tree object");
> + if (read_tree(tree, 0, NULL))
> + die("failed to read HEAD tree object");
> + }
Huh? Doesn't this read_tree() defeat the add_files_to_cache()
you did earlier?
> +
> + /* Uh oh, abusing lock_file to create a garbage collected file */
> + next_index_lock = xmalloc(sizeof(*next_index_lock));
> + fd = hold_lock_file_for_update(next_index_lock,
> + git_path("next-index-%d", getpid()), 1);
That's not an abuse, but I wonder what happened to the fd you
got at the beginning of the function.
> + add_files_to_cache(fd, files, prefix);
and then this is puzzling to me.
I am starting to suspect that you might be better off if you do
not follow the use of temporary index file that was in the shell
script version to the letter. You can use more than one index
in the core at the same time, now you are built-in.
> +
> + return next_index_lock->filename;
> +}
... and if we were to go that route, wt_status structure would
have a pointer to the "struct index_state" instead of the
filename of the index... oops, wt_status_print() will discard
and re-read the cache from file, so that approach would not work
right now without fixing wt-status first. Hmm.
> +static int run_status(FILE *fp, const char *index_file)
> +{
> + struct wt_status s;
> +
> + wt_status_prepare(&s);
> +
> + if (amend) {
> + s.amend = 1;
> + s.reference = "HEAD^1";
> + }
> + s.verbose = verbose;
> + s.untracked = untracked_files;
> + s.index_file = index_file;
> + s.fp = fp;
> +
> + wt_status_print(&s);
> +
> + return s.commitable;
> +}
I did not look at the rest of the patch; it should be either
obviously correct or outright incorrect --- anybody would notice
the breakage immediately.
The prepare_index() part is the most (and only) "interesting"
part of the puzzle. I need to look at this again and think
about it a bit more. It needs to:
* save the current index and restore in case the whole
"git-commit" is aborted in any way (e.g. ^C, empty log
message from the editor); This is easy to do from C with
hold_locked_index().
* when doing a partial commit,
- read HEAD in a temporary index, if not initial commit;
- update the temporary index with the given paths;
- write out the temporary index as a tree to build a commit;
- update the real index the same way with the given paths;
- if all goes well, commit the updated real index; again,
this is easy in C with commit_locked_index();
* when not doing a partial commit,
- update the real index with given paths (if "--also") or all
what diff-files reports (if "--all");
- write out the real index as a tree to build a commit;
- if all goes well, commit the updated real index; again,
this is easy in C with commit_locked_index();
A thing to keep in mind is that you can write out the temporary
index as a tree from the core without writing it out first to a
temporary index file at all. Perhaps the code should be
structured like this.
- parse options and all the other necessary setup;
- hold_locked_index() to lock the "real" index;
- prepare_index() is responsibile for doing two things
1. build, write out and return the tree object to be
committed;
2. leave the_index[] in the state to become the "real"
index if this commit command is not aborted;
- take the tree object, put commit log message around it and
make a commit object;
- commit_locked_index() to write out the "real" index.
Now, the task #1 for prepare_index() is simpler for "also" and
"all" case. You do the equivalent of "git-add -u" or "git add
<path>" and run cache_tree_update() to write out the tree, and
when you are done, both the_index.cache and the_index.cache_tree
are up-to-date, ready to be written out by the caller at the
very end.
For a partial commit, the task is a bit more convoluted, as
writing out the tree needs to be done from outside the_index. I
would say something like this:
int prepare_index(unsigned char *sha1) {
if (partial commit) {
struct index_state tmp_index;
/* "temporary index" only in-core */
memset(&tmp_index, 0, sizeof(tmp_index));
read_index(&tmp_index);
add_files_to_index(files, prefix, &tmp_index);
write_index_as_tree(&tmp_index, sha1);
discard_index(&tmp_index);
/* update the index the same way */
add_files_to_index(files, prefix, &the_index);
return ok;
}
/* otherwise */
if (files && *files)
add_files_to_index(files, prefix, &the_index);
else if (all)
add_files_to_index(NULL, prefix, &the_index);
write_index_as_tree(&the_index, sha1);
return ok;
}
where
* add_files_to_index() is similar to your add_files_to_cache()
but takes struct index_state; the callback can still diff
with the_index.cache (aka active_cache) as that diff is what
we are interested in updating anyway;
* write_index_as_tree() would be a new function that takes
struct index_state and writes that as a tree. It would
essentially be the builtin-write-tree.c::write_tree()
function except the opening and reading the index (or
updating the cache-tree) part, something like:
write_index_as_tree(struct index_state *istate, unsigned char *sha1)
{
if (!cache_tree_fully_valid(istate->cache_tree))
cache_tree_update(istate->cache_tree,
istate->cache,
istate->cache_nr, 0, 0);
hashcpy(sha1, istate->cache_tree->sha1);
}
^ permalink raw reply
* Re: [PATCH 2/5] Refactor struct transport_ops inlined into struct transport
From: Daniel Barkalow @ 2007-09-20 2:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Shawn O. Pearce, Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0709191409590.28395@racer.site>
On Wed, 19 Sep 2007, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
>
> > diff --git a/transport.c b/transport.c
> > index cc76e3f..d8458dc 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)
> > return 0;
> > }
> >
> > -static const struct transport_ops rsync_transport;
> > -
> > static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {
> > const char **argv;
> > int argc;
> > @@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)
> > ret->url = url;
> >
> > if (!prefixcmp(url, "rsync://")) {
> > - ret->ops = &rsync_transport;
> > + /* not supported; don't populate any ops */
> > +
>
> That is sneaky. What are the reasons to remove rsync support? I know it
> is deprecated, but I'd still like to have it, especially for initial
> clones on small-RAMed machines.
It never got implemented in a way called from C. This is just removing the
pointer to the empty struct where support would go. If anybody knows
enough about interfacing with rsync to write the necessary functions, it
can be restored.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH 0/5] Yet another builtin-fetch round
From: Daniel Barkalow @ 2007-09-20 2:40 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20070919044923.GP3099@spearce.org>
On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
> Another short series for db/fetch-pack, still in pu. Aside from
> optimizing the pipeline on the native transport (so we only invoke
> the remote process we need once vs. twice) I'm actually now quite
> comfortable with this whole series and think it is ready for next.
While it's still in pu, should these series of corrections be amended into
the original series (for the ones that correct new code)? Most of the
before-fixing states aren't worth saving as project history.
> I'm certainly running it in production, and will be until it is
> merged. The performance difference is too big for me (and at least
> some of my coworkers) to not be doing so. If there are any specific
> reasons why this topic is not ready for next or is unsuitable for
> merging please let me know so I can take the time to correct it.
Good to hear that it's working for you. It's been working for me since
July, for my usage patterns, but they're not very extensive, and I was
mostly relying on the tests.
> 1/5 Rename remote.uri to remote.url within remote handling internals
> 2/5 Refactor struct transport_ops inlined into struct transport
> 3/5 Always obtain fetch-pack arguments from struct fetch_pack_args
>
> These three are basic code cleanups for small issues that
> bothered me about the original implementation of builtin-fetch.
> Now is just as good of a time as any to cleanup the code and make
> it more maintainable. I think the overall total line count is
> reduced by these three patches.
All of these look right.
> * Ensure builtin-fetch honors {fetch,transfer}.unpackLimit
> * Fix memory leaks when disconnecting transport instances
>
> Fixes two known (but minor) outstanding bugs. At this point
> I do not know of any other bugs in builtin-fetch so I would
> really appreciate testing reports from other people, especially
> those whose uses cases might stray outside of my workflow. Hah,
> I did not tell you my workflow. ;-)
Looks good, but I didn't really check over them.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* [PATCH] Add git-rev-list --invert-match
From: Bart Trojanowski @ 2007-09-20 2:52 UTC (permalink / raw)
To: git
In-Reply-To: <20070919202615.GK3076@jukie.net>
Example usage:
git log --invert-match --grep="uninteresting"
This command will prune out all commits that match the grep pattern.
How it works:
The --invert-match flag sets invert_match, in rev_info. This boolean
is later checked in commit_match() and if set it inverts the result of
grep_buffer().
Signed-off-by: Bart Trojanowski <bart@jukie.net>
---
This patch includes the Documentation update I missed in the first
patch.
Documentation/git-rev-list.txt | 6 ++++++
revision.c | 11 ++++++++++-
revision.h | 3 ++-
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 7cd0e89..f7a4891 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -29,6 +29,7 @@ SYNOPSIS
[ \--(author|committer|grep)=<pattern> ]
[ \--regexp-ignore-case | \-i ]
[ \--extended-regexp | \-E ]
+ [ \--invert-match ]
[ \--date={local|relative|default|iso|rfc|short} ]
[ [\--objects | \--objects-edge] [ \--unpacked ] ]
[ \--pretty | \--header ]
@@ -238,6 +239,11 @@ limiting may be applied.
Consider the limiting patterns to be extended regular expressions
instead of the default basic regular expressions.
+--invert-match::
+
+ Show those parts of history that do not match any of the regular
+ expression patterns.
+
--remove-empty::
Stop when a given path disappears from the tree.
diff --git a/revision.c b/revision.c
index 33d092c..57b2d0f 100644
--- a/revision.c
+++ b/revision.c
@@ -1182,6 +1182,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
regflags |= REG_ICASE;
continue;
}
+ if (!strcmp(arg, "--invert-match")) {
+ revs->invert_match = 1;
+ continue;
+ }
if (!strcmp(arg, "--all-match")) {
all_match = 1;
continue;
@@ -1383,11 +1387,16 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
+ int result;
+
if (!opt->grep_filter)
return 1;
- return grep_buffer(opt->grep_filter,
+
+ result = grep_buffer(opt->grep_filter,
NULL, /* we say nothing, not even filename */
commit->buffer, strlen(commit->buffer));
+
+ return opt->invert_match ? !result : result;
}
static struct commit *get_revision_1(struct rev_info *revs)
diff --git a/revision.h b/revision.h
index 98a0a8f..ead04a7 100644
--- a/revision.h
+++ b/revision.h
@@ -48,7 +48,8 @@ struct rev_info {
parents:1,
reverse:1,
cherry_pick:1,
- first_parent_only:1;
+ first_parent_only:1,
+ invert_match:1;
/* Diff flags */
unsigned int diff:1,
--
1.5.3.1.154.g734e65
^ permalink raw reply related
* Re: [PATCH] Add git-rev-list --invert-match
From: Junio C Hamano @ 2007-09-20 4:05 UTC (permalink / raw)
To: Bart Trojanowski; +Cc: git
In-Reply-To: <20070920025225.GA12076@jukie.net>
> diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
> index 7cd0e89..f7a4891 100644
> --- a/Documentation/git-rev-list.txt
> +++ b/Documentation/git-rev-list.txt
> @@ -29,6 +29,7 @@ SYNOPSIS
> [ \--(author|committer|grep)=<pattern> ]
> [ \--regexp-ignore-case | \-i ]
> [ \--extended-regexp | \-E ]
> + [ \--invert-match ]
> [ \--date={local|relative|default|iso|rfc|short} ]
> [ [\--objects | \--objects-edge] [ \--unpacked ] ]
> [ \--pretty | \--header ]
> @@ -238,6 +239,11 @@ limiting may be applied.
> Consider the limiting patterns to be extended regular expressions
> instead of the default basic regular expressions.
>
> +--invert-match::
> +
> + Show those parts of history that do not match any of the regular
> + expression patterns.
> +
Tabs and spaces?
I am afraid people may not understand what "any of the regular
expression patterns" means. How about being a bit more
explicit, like this?
When filtering the commits based on log message (`--grep`)
or people (`--author` and `--committer`), show commits
that do not match the criteria.
^ permalink raw reply
* Re: [PATCH 0/5] Yet another builtin-fetch round
From: Junio C Hamano @ 2007-09-20 4:08 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Shawn O. Pearce, git
In-Reply-To: <Pine.LNX.4.64.0709192233360.21941@iabervon.org>
Daniel Barkalow <barkalow@iabervon.org> writes:
> On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
>
>> Another short series for db/fetch-pack, still in pu. Aside from
>> optimizing the pipeline on the native transport (so we only invoke
>> the remote process we need once vs. twice) I'm actually now quite
>> comfortable with this whole series and think it is ready for next.
>
> While it's still in pu, should these series of corrections be amended into
> the original series (for the ones that correct new code)? Most of the
> before-fixing states aren't worth saving as project history.
Yeah, I was wondering if that is a sane thing to do. It is
merely additional work to arrive at the same tree state, but
might be a good investment in the longer term.
^ permalink raw reply
* Re: [PATCH] User Manual: add a chapter for submodules
From: Junio C Hamano @ 2007-09-20 4:15 UTC (permalink / raw)
To: Miklos Vajna; +Cc: git, J. Bruce Fields
In-Reply-To: <20070920003413.GJ16235@genesis.frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> writes:
> Signed-off-by: Michael Smith <msmith@cbnco.com>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> Sorry, I sent the original patch again. So here is the updated second version.
Looks Ok to me, although I didn't verify the examples by
actually running them myself this time (last round I did).
^ permalink raw reply
* Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.
From: Junio C Hamano @ 2007-09-20 4:27 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: gitster, git
In-Reply-To: <1190241736-30449-3-git-send-email-madcoder@debian.org>
Pierre Habouzit <madcoder@debian.org> writes:
> * drop nfasprintf.
> * move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,
> and die() in case of overflow. It should be enough for imap commands, if
> someone cares about imap-send, he's welcomed to fix it properly.
> * replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf
> logic, it's one place, we'll live with it.
> To ease the change, output_buffer string list is replaced with a strbuf ;)
While I'd agree with all of the above,
> * rework trace.c API's so that only one of the trace functions takes a
> vararg. It's used to format strerror()s and git command names, it should
> never be more than a few octets long, let it work on a 8k static buffer
> with vsnprintf or die loudly.
and I'd agree with this in principle, there is a minor nit with
the implementation and use in trace.c. E.g.
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..c0f954e 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)
> tmp = argv[0];
> argv[0] = git_command;
>
> - trace_argv_printf(argv, -1, "trace: exec:");
> + trace_printf("trace: exec:");
> + trace_argv(argv, -1);
This used to be a single call into trace.c which would format a
single string to write(2) out. Now these two messages go
through separate write(2) and can be broken up. I think the
atomicity of the log/trace message was the primary reason the
original had such a strange calling convention.
^ permalink raw reply
* Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.
From: Christian Couder @ 2007-09-20 4:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pierre Habouzit, git
In-Reply-To: <7vwsumkll8.fsf@gitster.siamese.dyndns.org>
Le jeudi 20 septembre 2007, Junio C Hamano a écrit :
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 9b74ed2..c0f954e 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)
> > tmp = argv[0];
> > argv[0] = git_command;
> >
> > - trace_argv_printf(argv, -1, "trace: exec:");
> > + trace_printf("trace: exec:");
> > + trace_argv(argv, -1);
>
> This used to be a single call into trace.c which would format a
> single string to write(2) out. Now these two messages go
> through separate write(2) and can be broken up. I think the
> atomicity of the log/trace message was the primary reason the
> original had such a strange calling convention.
That's right.
Christian.
^ permalink raw reply
* Re: [PATCH 0/5] Yet another builtin-fetch round
From: Junio C Hamano @ 2007-09-20 5:09 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Shawn O. Pearce, git
In-Reply-To: <7v6426m110.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Daniel Barkalow <barkalow@iabervon.org> writes:
>
>> On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
>>
>>> Another short series for db/fetch-pack, still in pu. Aside from
>>> optimizing the pipeline on the native transport (so we only invoke
>>> the remote process we need once vs. twice) I'm actually now quite
>>> comfortable with this whole series and think it is ready for next.
>>
>> While it's still in pu, should these series of corrections be amended into
>> the original series (for the ones that correct new code)? Most of the
>> before-fixing states aren't worth saving as project history.
>
> Yeah, I was wondering if that is a sane thing to do. It is
> merely additional work to arrive at the same tree state, but
> might be a good investment in the longer term.
Heh, I did not realize that they are now all part of 'next' so
that's moot.
^ permalink raw reply
* [PATCH] rev-list --bisect: Fix best == NULL case.
From: Christian Couder @ 2007-09-20 5:23 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-rev-list.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
Not sure if the "if (best)" check is really needed, but it looks safer
with this patch.
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 899a31d..3894633 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -436,10 +436,10 @@ static struct commit_list *find_bisection(struct commit_list *list,
/* Do the real work of finding bisection commit. */
best = do_find_bisection(list, nr, weights);
- if (best)
+ if (best) {
best->next = NULL;
-
- *reaches = weight(best);
+ *reaches = weight(best);
+ }
free(weights);
return best;
--
1.5.3.2.80.g077d6f
^ permalink raw reply related
* Re: State of Perforce importing.
From: Dmitry Kakurin @ 2007-09-20 6:12 UTC (permalink / raw)
To: David Brown; +Cc: Git
In-Reply-To: <20070918154918.GA19106@old.davidb.org>
[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]
I had to import quite a big Perforce depot too. And after some struggle it
went fine.
For case mismatch:
http://kb.perforce.com/AdminTasks/SuperuserTasks/CrossPlatfor..erMigration,
bullet 10. It requires server access.
For a@b and for excluding one p4 path during migration you could use my
quick-and-dirty fix (attached).
You can easily extend it to include and exclude multiple paths. Then it
becomes as flexible as p4 client mapping.
- Dmitry
----- Original Message -----
From: "David Brown" <git@davidb.org>
Newsgroups: gmane.comp.version-control.git
To: "Sam Vilain" <sam@vilain.net>
Cc: "Git" <git@vger.kernel.org>
Sent: Tuesday, 18 September 2007 8:49
Subject: Re: State of Perforce importing.
> On Tue, Sep 18, 2007 at 07:27:13PM +1200, Sam Vilain wrote:
>
>>I'm pretty close to giving a newer one a spin, that actually imports
>>from the raw perforce back-end files without needing the perforce
>>server. I am hoping that this should give a very clean import and will
>>be very fast and efficient, sending files that share ancestry to gfi in
>>sequence so that the on-the-fly delta system works.
>
> Unfortunately, this isn't something I'm going to be able to use. The
> Perforce server will remain live, and resides on a machine I don't have
> access to.
>
>>It could possibly be adapted to use the p4 client (though I'd expect
>>that to be relatively slow per-revision), and possibly be extended to be
>>bidirectional as all of the upstream change number information is
>>recorded, a la git-svn.
>
> I was able to get 'git-p4' to work a lot better by using @all, but it
> still
> has some problems, at least bad interactions with P4.
>
> - It doesn't use any client spec. Our P4 server space is a complete
> mismash and has to be fixed up to get a sane directory layout. For
> example, some revisions have hundred-MB tar files sitting in the root
> directory and I don't want that in the repo. I also need to exclude
> directories, and in some cases completely rearrange the directory
> layout.
>
> - Our P4 server is set to be case insensitive. 'git-p4' ignores paths
> that come back from the server that are specified using a different
> case. Unfortunately, this means that a handful of files just get
> randomly dropped from each revision.
>
> I tried importing a client path instead of a depot path, but the names
> that come back from 'p4 files' are depot based so none ever match. I
> end up with a nice revision history of entirely empty trees.
>
> I'm probably going to end up writing an importer that uses an actual
> client
> workspace to let Perforce do the client mapping. I'm also going to have
> to
> put some work into some code to clean up the log messages, since most of
> our changes have as a first line "New Features:", which makes for a rather
> uninformative shortlog.
>
> But, I did learn about 'p4 -G' from git-p4 so that will help in getting
> information from the repository.
>
> Thanks,
> David
[-- Attachment #2: 0001-git-p4-Added-exclude-option-and-use-P4-client-nam.patch --]
[-- Type: application/octet-stream, Size: 2664 bytes --]
>From a089b02239c3bc310956964d61075b093d26549f Mon Sep 17 00:00:00 2001
From: Dmitry Kakurin <Dmitry.Kakurin@gmail.com>
Date: Sun, 9 Sep 2007 13:58:12 -0700
Subject: [PATCH] git-p4: Added --exclude option and use P4 client name in commits
---
contrib/fast-import/git-p4 | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index adaaae6..337854f 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -769,6 +769,7 @@ class P4Sync(Command):
self.keepRepoPath = False
self.depotPaths = None
self.p4BranchesInGit = []
+ self.cloneExclude = None
if gitConfig("git-p4.syncFromOrigin") == "false":
self.syncWithOrigin = False
@@ -779,8 +780,12 @@ class P4Sync(Command):
while commit.has_key("depotFile%s" % fnum):
path = commit["depotFile%s" % fnum]
- found = [p for p in self.depotPaths
- if path.startswith (p)]
+ if self.cloneExclude and path.startswith( self.cloneExclude ):
+ found = False
+ else:
+ found = [p for p in self.depotPaths
+ if path.startswith (p)]
+
if not found:
fnum = fnum + 1
continue
@@ -905,6 +910,8 @@ class P4Sync(Command):
else:
committer = "%s <a@b> %s %s" % (author, epoch, self.tz)
+ committer = "%s <%s@%s> %s %s" % (author, details["user"], details["client"], epoch, self.tz)
+
self.gitStream.write("committer %s\n" % committer)
self.gitStream.write("data <<EOT\n")
@@ -1540,10 +1547,14 @@ class P4Clone(P4Sync):
P4Sync.__init__(self)
self.description = "Creates a new git repository and imports from Perforce into it"
self.usage = "usage: %prog [options] //depot/path[@revRange]"
- self.options.append(
+ self.options += [
optparse.make_option("--destination", dest="cloneDestination",
action='store', default=None,
- help="where to leave result of the clone"))
+ help="where to leave result of the clone"),
+ optparse.make_option("--exclude", dest="cloneExclude",
+ action='store', default=None,
+ help="exclude depot path")
+ ]
self.cloneDestination = None
self.needsGit = False
--
1.5.3.mingw.1.1.g01e3a1
^ permalink raw reply related
* Re: [PATCH 1/5] strbuf API additions and enhancements.
From: Johannes Sixt @ 2007-09-20 6:17 UTC (permalink / raw)
To: Edgar Toernig
Cc: Pierre Habouzit, Junio C Hamano, Shawn O. Pearce,
Git Mailing List
In-Reply-To: <20070919144604.7deca4f7.froese@gmx.de>
Edgar Toernig schrieb:
> Pierre Habouzit wrote:
>> +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
>> +{
>> + int len;
>> +
>> + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>> + if (len < 0) {
>> + len = 0;
>> + }
>> + if (len > strbuf_avail(sb)) {
>> + strbuf_grow(sb, len);
>> + len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>> + if (len > strbuf_avail(sb)) {
>> + die("this should not happen, your snprintf is broken");
>> + }
>> + }
>> + strbuf_setlen(sb, sb->len + len);
>> +}
>
> The second vsnprintf won't work as the first one consumed all args
> from va_list ap. You need to va_copy the ap.
Your analysis is not correct. The second vsnprintf receives the same
argument pointer as the first, and, hence, consumes the same set of arguments.
You have to use va_copy in a variadic function, ie. if you are using
va_start+va_end in the same function, but not in a function with a fixed
list of arguments like this one.
-- Hannes
^ permalink raw reply
* Re: [PATCH] rev-list --bisect: Fix best == NULL case.
From: Junio C Hamano @ 2007-09-20 6:46 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <20070920072301.689b96c9.chriscool@tuxfamily.org>
The patch makes sense. In fact you can crash any released
version like this:
$ git rev-list --bisect v1.5.2..v1.5.0
I'll need to apply a moral equivalent to 'maint'.
By the way, you may find the attached patch an interesting food
for thought, on top of your patch.
$ git rev-list --bisect-all --pretty=oneline v1.5.2.5..v1.5.3.2
---
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 3894633..e4ef172 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -9,6 +9,7 @@
#include "revision.h"
#include "list-objects.h"
#include "builtin.h"
+#include "log-tree.h"
/* bits #0-15 in revision.h */
@@ -74,6 +75,7 @@ static void show_commit(struct commit *commit)
parents = parents->next;
}
}
+ show_decorations(commit);
if (revs.commit_format == CMIT_FMT_ONELINE)
putchar(' ');
else
@@ -278,6 +280,57 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
return best;
}
+struct commit_dist {
+ struct commit *commit;
+ int distance;
+};
+
+static int compare_commit_dist(const void *a_, const void *b_)
+{
+ struct commit_dist *a, *b;
+
+ a = (struct commit_dist *)a_;
+ b = (struct commit_dist *)b_;
+ if (a->distance != b->distance)
+ return b->distance - a->distance; /* desc sort */
+ return hashcmp(a->commit->object.sha1, b->commit->object.sha1);
+}
+
+static struct commit_list *best_bisection_sorted(struct commit_list *list, int nr)
+{
+ struct commit_list *p;
+ struct commit_dist *array = xcalloc(nr, sizeof(*array));
+ int cnt, i;
+
+ for (p = list, cnt = 0; p; p = p->next) {
+ int distance;
+ unsigned flags = p->item->object.flags;
+
+ if (revs.prune_fn && !(flags & TREECHANGE))
+ continue;
+ distance = weight(p);
+ if (nr - distance < distance)
+ distance = nr - distance;
+ array[cnt].commit = p->item;
+ array[cnt].distance = distance;
+ cnt++;
+ }
+ qsort(array, cnt, sizeof(*array), compare_commit_dist);
+ for (p = list, i = 0; i < cnt; i++) {
+ struct name_decoration *r = xmalloc(sizeof(*r) + 100);
+ struct object *obj = &(array[i].commit->object);
+
+ sprintf(r->name, "dist=%d", array[i].distance);
+ r->next = add_decoration(&name_decoration, obj, r);
+ p->item = array[i].commit;
+ p = p->next;
+ }
+ if (p)
+ p->next = NULL;
+ free(array);
+ return list;
+}
+
/*
* zero or positive weight is the number of interesting commits it can
* reach, including itself. Especially, weight = 0 means it does not
@@ -292,7 +345,8 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
* or positive distance.
*/
static struct commit_list *do_find_bisection(struct commit_list *list,
- int nr, int *weights)
+ int nr, int *weights,
+ int find_all)
{
int n, counted;
struct commit_list *p;
@@ -351,7 +405,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
clear_distance(list);
/* Does it happen to be at exactly half-way? */
- if (halfway(p, nr))
+ if (!find_all && halfway(p, nr))
return p;
counted++;
}
@@ -389,19 +443,22 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
weight_set(p, weight(q));
/* Does it happen to be at exactly half-way? */
- if (halfway(p, nr))
+ if (!find_all && halfway(p, nr))
return p;
}
}
show_list("bisection 2 counted all", counted, nr, list);
- /* Then find the best one */
- return best_bisection(list, nr);
+ if (!find_all)
+ return best_bisection(list, nr);
+ else
+ return best_bisection_sorted(list, nr);
}
static struct commit_list *find_bisection(struct commit_list *list,
- int *reaches, int *all)
+ int *reaches, int *all,
+ int find_all)
{
int nr, on_list;
struct commit_list *p, *best, *next, *last;
@@ -434,14 +491,12 @@ static struct commit_list *find_bisection(struct commit_list *list,
weights = xcalloc(on_list, sizeof(*weights));
/* Do the real work of finding bisection commit. */
- best = do_find_bisection(list, nr, weights);
-
- if (best) {
+ best = do_find_bisection(list, nr, weights, find_all);
+ if (!find_all && best) {
best->next = NULL;
*reaches = weight(best);
}
free(weights);
-
return best;
}
@@ -468,6 +523,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
int i;
int read_from_stdin = 0;
int bisect_show_vars = 0;
+ int bisect_find_all = 0;
git_config(git_default_config);
init_revisions(&revs, prefix);
@@ -490,6 +546,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
bisect_list = 1;
continue;
}
+ if (!strcmp(arg, "--bisect-all")) {
+ bisect_list = 1;
+ bisect_find_all = 1;
+ continue;
+ }
if (!strcmp(arg, "--bisect-vars")) {
bisect_list = 1;
bisect_show_vars = 1;
@@ -536,7 +597,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if (bisect_list) {
int reaches = reaches, all = all;
- revs.commits = find_bisection(revs.commits, &reaches, &all);
+ revs.commits = find_bisection(revs.commits, &reaches, &all,
+ bisect_find_all);
if (bisect_show_vars) {
int cnt;
if (!revs.commits)
diff --git a/log-tree.c b/log-tree.c
index a642371..32e360b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -15,7 +15,7 @@ static void show_parents(struct commit *commit, int abbrev)
}
}
-static void show_decorations(struct commit *commit)
+void show_decorations(struct commit *commit)
{
const char *prefix;
struct name_decoration *decoration;
diff --git a/log-tree.h b/log-tree.h
index e82b56a..b33f7cd 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -12,5 +12,6 @@ int log_tree_diff_flush(struct rev_info *);
int log_tree_commit(struct rev_info *, struct commit *);
int log_tree_opt_parse(struct rev_info *, const char **, int);
void show_log(struct rev_info *opt, const char *sep);
+void show_decorations(struct commit *commit);
#endif
^ permalink raw reply related
* Re: [PATCH] rev-list --bisect: Fix best == NULL case.
From: Junio C Hamano @ 2007-09-20 7:08 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <7vmyvhltpy.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The patch makes sense. In fact you can crash any released
> version like this:
>
> $ git rev-list --bisect v1.5.2..v1.5.0
>
> I'll need to apply a moral equivalent to 'maint'.
I spoke too fast. This is not broken in any released version.
Only with the patch from somebody that broke it with commit
ce0cbad7727457854d631a6314d9aee7c837cb65 (rev-list --bisect:
Move finding bisection into do_find_bisection.), the "empty set"
case would segfault ;-).
^ permalink raw reply
* Re: [PATCH 1/5] strbuf API additions and enhancements.
From: Kalle Olavi Niemitalo @ 2007-09-20 7:20 UTC (permalink / raw)
To: git
In-Reply-To: <46F21097.5030901@eudaptics.com>
Johannes Sixt <j.sixt@eudaptics.com> writes:
> Edgar Toernig schrieb:
>> The second vsnprintf won't work as the first one consumed all args
>> from va_list ap. You need to va_copy the ap.
>
> Your analysis is not correct. The second vsnprintf receives the same
> argument pointer as the first, and, hence, consumes the same set of
> arguments.
C99 7.9.16.2p2 has a footnote: "As the functions vfprintf,
vfscanf, vprintf, vscanf, vsnprintf, vsprintf, and vsscanf invoke
the va_arg macro, the value of arg after the return is
indeterminate."
Normative text in 7.15p3 confirms this: "The object ap may be
passed as an argument to another function; if that function
invokes the va_arg macro with parameter ap, the value of ap in
the calling function is indeterminate and shall be passed to the
va_end macro prior to any further reference to ap."
Therefore va_copy is needed here, at least in principle.
^ permalink raw reply
* Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.
From: Pierre Habouzit @ 2007-09-20 8:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwsumkll8.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]
On Thu, Sep 20, 2007 at 04:27:31AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > * drop nfasprintf.
> > * move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,
> > and die() in case of overflow. It should be enough for imap commands, if
> > someone cares about imap-send, he's welcomed to fix it properly.
> > * replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf
> > logic, it's one place, we'll live with it.
> > To ease the change, output_buffer string list is replaced with a strbuf ;)
>
> While I'd agree with all of the above,
>
> > * rework trace.c API's so that only one of the trace functions takes a
> > vararg. It's used to format strerror()s and git command names, it should
> > never be more than a few octets long, let it work on a 8k static buffer
> > with vsnprintf or die loudly.
>
> and I'd agree with this in principle, there is a minor nit with
> the implementation and use in trace.c. E.g.
>
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 9b74ed2..c0f954e 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)
> > tmp = argv[0];
> > argv[0] = git_command;
> >
> > - trace_argv_printf(argv, -1, "trace: exec:");
> > + trace_printf("trace: exec:");
> > + trace_argv(argv, -1);
>
> This used to be a single call into trace.c which would format a
> single string to write(2) out. Now these two messages go
> through separate write(2) and can be broken up. I think the
> atomicity of the log/trace message was the primary reason the
> original had such a strange calling convention.
Okay, given that the formats (as you can see) are always very short,
and that it will always fit in a big enough static buffer, I'll
reinstate this and use a vsnprintf twice then.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [SUPERSEDES PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.
From: Pierre Habouzit @ 2007-09-20 8:43 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20070920082701.GA2053@artemis.corp>
* drop nfasprintf.
* move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,
and die() in case of overflow. It should be enough for imap commands, if
someone cares about imap-send, he's welcomed to fix it properly.
* replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf
logic, it's one place, we'll live with it.
To ease the change, output_buffer string list is replaced with a strbuf ;)
* rework trace.c to call vsnprintf itself. It's used to format strerror()s
and git command names, it should never be more than a few octets long, let
it work on a 8k static buffer with vsnprintf or die loudly.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
This reinstates the trace_argv_printf API. The implementation is
stupid, but is rewritten in a latter commit. I didn't wanted to bother
optimizing it.
cache.h | 2 -
imap-send.c | 13 ++++++++
merge-recursive.c | 74 ++++++++++++++++++++-----------------------
trace.c | 90 ++++++++++++++++-------------------------------------
4 files changed, 74 insertions(+), 105 deletions(-)
diff --git a/cache.h b/cache.h
index c57ccd6..b127c43 100644
--- a/cache.h
+++ b/cache.h
@@ -587,8 +587,6 @@ extern void *alloc_object_node(void);
extern void alloc_report(void);
/* trace.c */
-extern int nfasprintf(char **str, const char *fmt, ...);
-extern int nfvasprintf(char **str, const char *fmt, va_list va);
extern void trace_printf(const char *format, ...);
extern void trace_argv_printf(const char **argv, int count, const char *format, ...);
diff --git a/imap-send.c b/imap-send.c
index 905d097..e95cdde 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -105,6 +105,19 @@ static void free_generic_messages( message_t * );
static int nfsnprintf( char *buf, int blen, const char *fmt, ... );
+static int nfvasprintf(char **strp, const char *fmt, va_list ap)
+{
+ int len;
+ char tmp[8192];
+
+ len = vsnprintf(tmp, sizeof(tmp), fmt, ap);
+ if (len < 0)
+ die("Fatal: Out of memory\n");
+ if (len >= sizeof(tmp))
+ die("imap command overflow !\n");
+ *strp = xmemdupz(tmp, len);
+ return len;
+}
static void arc4_init( void );
static unsigned char arc4_getbyte( void );
diff --git a/merge-recursive.c b/merge-recursive.c
index 14b56c2..4e27549 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -85,63 +85,57 @@ struct stage_data
unsigned processed:1;
};
-struct output_buffer
-{
- struct output_buffer *next;
- char *str;
-};
-
static struct path_list current_file_set = {NULL, 0, 0, 1};
static struct path_list current_directory_set = {NULL, 0, 0, 1};
static int call_depth = 0;
static int verbosity = 2;
static int buffer_output = 1;
-static struct output_buffer *output_list, *output_end;
+static struct strbuf obuf = STRBUF_INIT;
-static int show (int v)
+static int show(int v)
{
return (!call_depth && verbosity >= v) || verbosity >= 5;
}
-static void output(int v, const char *fmt, ...)
+static void flush_output(void)
{
- va_list args;
- va_start(args, fmt);
- if (buffer_output && show(v)) {
- struct output_buffer *b = xmalloc(sizeof(*b));
- nfvasprintf(&b->str, fmt, args);
- b->next = NULL;
- if (output_end)
- output_end->next = b;
- else
- output_list = b;
- output_end = b;
- } else if (show(v)) {
- int i;
- for (i = call_depth; i--;)
- fputs(" ", stdout);
- vfprintf(stdout, fmt, args);
- fputc('\n', stdout);
+ if (obuf.len) {
+ fputs(obuf.buf, stdout);
+ strbuf_reset(&obuf);
}
- va_end(args);
}
-static void flush_output(void)
+static void output(int v, const char *fmt, ...)
{
- struct output_buffer *b, *n;
- for (b = output_list; b; b = n) {
- int i;
- for (i = call_depth; i--;)
- fputs(" ", stdout);
- fputs(b->str, stdout);
- fputc('\n', stdout);
- n = b->next;
- free(b->str);
- free(b);
+ if (show(v)) {
+ int len;
+ va_list ap;
+
+ strbuf_grow(&obuf, call_depth);
+ memset(obuf.buf + obuf.len, ' ', call_depth);
+ strbuf_setlen(&obuf, obuf.len + call_depth);
+
+ va_start(ap, fmt);
+ len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);
+ va_end(ap);
+
+ if (len < 0)
+ len = 0;
+ if (len > strbuf_avail(&obuf)) {
+ strbuf_grow(&obuf, len);
+ va_start(ap, fmt);
+ len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);
+ va_end(ap);
+ if (len > strbuf_avail(&obuf)) {
+ die("this should not happen, your snprintf is broken");
+ }
+ }
+
+ strbuf_setlen(&obuf, obuf.len + len);
+ if (!buffer_output)
+ flush_output();
}
- output_list = NULL;
- output_end = NULL;
}
static void output_commit_title(struct commit *commit)
diff --git a/trace.c b/trace.c
index 7961a27..91548a5 100644
--- a/trace.c
+++ b/trace.c
@@ -25,33 +25,6 @@
#include "cache.h"
#include "quote.h"
-/* Stolen from "imap-send.c". */
-int nfvasprintf(char **strp, const char *fmt, va_list ap)
-{
- int len;
- char tmp[1024];
-
- if ((len = vsnprintf(tmp, sizeof(tmp), fmt, ap)) < 0 ||
- !(*strp = xmalloc(len + 1)))
- die("Fatal: Out of memory\n");
- if (len >= (int)sizeof(tmp))
- vsprintf(*strp, fmt, ap);
- else
- memcpy(*strp, tmp, len + 1);
- return len;
-}
-
-int nfasprintf(char **str, const char *fmt, ...)
-{
- int rc;
- va_list args;
-
- va_start(args, fmt);
- rc = nfvasprintf(str, fmt, args);
- va_end(args);
- return rc;
-}
-
/* Get a trace file descriptor from GIT_TRACE env variable. */
static int get_trace_fd(int *need_close)
{
@@ -89,63 +62,54 @@ static int get_trace_fd(int *need_close)
static const char err_msg[] = "Could not trace into fd given by "
"GIT_TRACE environment variable";
-void trace_printf(const char *format, ...)
+void trace_printf(const char *fmt, ...)
{
- char *trace_str;
- va_list rest;
- int need_close = 0;
- int fd = get_trace_fd(&need_close);
+ char buf[8192];
+ va_list ap;
+ int fd, len, need_close = 0;
+ fd = get_trace_fd(&need_close);
if (!fd)
return;
- va_start(rest, format);
- nfvasprintf(&trace_str, format, rest);
- va_end(rest);
-
- write_or_whine_pipe(fd, trace_str, strlen(trace_str), err_msg);
-
- free(trace_str);
+ va_start(ap, fmt);
+ len = vsnprintf(buf, sizeof(buf), fmt, ap);
+ va_end(ap);
+ if (len >= sizeof(buf))
+ die("unreasonnable trace length");
+ write_or_whine_pipe(fd, buf, len, err_msg);
if (need_close)
close(fd);
}
-void trace_argv_printf(const char **argv, int count, const char *format, ...)
+void trace_argv_printf(const char **argv, int count, const char *fmt, ...)
{
- char *argv_str, *format_str, *trace_str;
- size_t argv_len, format_len, trace_len;
- va_list rest;
- int need_close = 0;
- int fd = get_trace_fd(&need_close);
+ char buf[8192];
+ va_list ap;
+ char *argv_str;
+ size_t argv_len;
+ int fd, len, need_close = 0;
+ fd = get_trace_fd(&need_close);
if (!fd)
return;
+ va_start(ap, fmt);
+ len = vsnprintf(buf, sizeof(buf), fmt, ap);
+ va_end(ap);
+ if (len >= sizeof(buf))
+ die("unreasonnable trace length");
+
/* Get the argv string. */
argv_str = sq_quote_argv(argv, count);
argv_len = strlen(argv_str);
- /* Get the formated string. */
- va_start(rest, format);
- nfvasprintf(&format_str, format, rest);
- va_end(rest);
-
- /* Allocate buffer for trace string. */
- format_len = strlen(format_str);
- trace_len = argv_len + format_len + 1; /* + 1 for \n */
- trace_str = xmalloc(trace_len + 1);
-
- /* Copy everything into the trace string. */
- strncpy(trace_str, format_str, format_len);
- strncpy(trace_str + format_len, argv_str, argv_len);
- strcpy(trace_str + trace_len - 1, "\n");
-
- write_or_whine_pipe(fd, trace_str, trace_len, err_msg);
+ write_or_whine_pipe(fd, buf, len, err_msg);
+ write_or_whine_pipe(fd, argv_str, argv_len, err_msg);
+ write_or_whine_pipe(fd, "\n", 1, err_msg);
free(argv_str);
- free(format_str);
- free(trace_str);
if (need_close)
close(fd);
--
1.5.3.2.1036.gca8d2
^ permalink raw reply related
* [SUPERSEDES PATCH 4/7] sq_quote_argv and add_to_string rework with strbuf's.
From: Pierre Habouzit @ 2007-09-20 8:44 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20070920082701.GA2053@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 7902 bytes --]
* sq_quote_buf is made public, and works on a strbuf.
* sq_quote_argv also works on a strbuf.
* make sq_quote_argv take a "maxlen" argument to check the buffer won't grow
too big.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
connect.c | 21 ++++++--------
git.c | 16 +++-------
quote.c | 94 +++++++++++++++---------------------------------------------
quote.h | 9 ++----
trace.c | 25 +++++++---------
5 files changed, 52 insertions(+), 113 deletions(-)
diff --git a/connect.c b/connect.c
index 1653a0e..06d279e 100644
--- a/connect.c
+++ b/connect.c
@@ -577,16 +577,13 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
if (pid < 0)
die("unable to fork");
if (!pid) {
- char command[MAX_CMD_LEN];
- char *posn = command;
- int size = MAX_CMD_LEN;
- int of = 0;
+ struct strbuf cmd;
- of |= add_to_string(&posn, &size, prog, 0);
- of |= add_to_string(&posn, &size, " ", 0);
- of |= add_to_string(&posn, &size, path, 1);
-
- if (of)
+ 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);
@@ -606,10 +603,10 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
ssh_basename++;
if (!port)
- execlp(ssh, ssh_basename, host, command, NULL);
+ execlp(ssh, ssh_basename, host, cmd.buf, NULL);
else
execlp(ssh, ssh_basename, "-p", port, host,
- command, NULL);
+ cmd.buf, NULL);
}
else {
unsetenv(ALTERNATE_DB_ENVIRONMENT);
@@ -618,7 +615,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
unsetenv(GIT_WORK_TREE_ENVIRONMENT);
unsetenv(GRAFT_ENVIRONMENT);
unsetenv(INDEX_ENVIRONMENT);
- execlp("sh", "sh", "-c", command, NULL);
+ execlp("sh", "sh", "-c", cmd.buf, NULL);
}
die("exec failed");
}
diff --git a/git.c b/git.c
index 6773c12..d7c6bca 100644
--- a/git.c
+++ b/git.c
@@ -187,19 +187,13 @@ static int handle_alias(int *argcp, const char ***argv)
if (alias_string) {
if (alias_string[0] == '!') {
if (*argcp > 1) {
- int i, sz = PATH_MAX;
- char *s = xmalloc(sz), *new_alias = s;
+ struct strbuf buf;
- add_to_string(&s, &sz, alias_string, 0);
+ strbuf_init(&buf, PATH_MAX);
+ strbuf_addstr(&buf, alias_string);
+ sq_quote_argv(&buf, (*argv) + 1, *argcp - 1, PATH_MAX);
free(alias_string);
- alias_string = new_alias;
- for (i = 1; i < *argcp &&
- !add_to_string(&s, &sz, " ", 0) &&
- !add_to_string(&s, &sz, (*argv)[i], 1)
- ; i++)
- ; /* do nothing */
- if (!sz)
- die("Too many or long arguments");
+ alias_string = buf.buf;
}
trace_printf("trace: alias to shell cmd: %s => %s\n",
alias_command, alias_string + 1);
diff --git a/quote.c b/quote.c
index d88bf75..edd1a09 100644
--- a/quote.c
+++ b/quote.c
@@ -12,37 +12,31 @@
* a'b ==> a'\''b ==> 'a'\''b'
* a!b ==> a'\!'b ==> 'a'\!'b'
*/
-#undef EMIT
-#define EMIT(x) do { if (++len < n) *bp++ = (x); } while(0)
-
static inline int need_bs_quote(char c)
{
return (c == '\'' || c == '!');
}
-static size_t sq_quote_buf(char *dst, size_t n, const char *src)
+void sq_quote_buf(struct strbuf *dst, const char *src)
{
- char c;
- char *bp = dst;
- size_t len = 0;
-
- EMIT('\'');
- while ((c = *src++)) {
- if (need_bs_quote(c)) {
- EMIT('\'');
- EMIT('\\');
- EMIT(c);
- EMIT('\'');
- } else {
- EMIT(c);
+ char *to_free = NULL;
+
+ if (dst->buf == src)
+ to_free = strbuf_detach(dst);
+
+ strbuf_addch(dst, '\'');
+ while (*src) {
+ size_t len = strcspn(src, "'\\");
+ strbuf_add(dst, src, len);
+ src += len;
+ while (need_bs_quote(*src)) {
+ strbuf_addstr(dst, "'\\");
+ strbuf_addch(dst, *src++);
+ strbuf_addch(dst, '\'');
}
}
- EMIT('\'');
-
- if ( n )
- *bp = 0;
-
- return len;
+ strbuf_addch(dst, '\'');
+ free(to_free);
}
void sq_quote_print(FILE *stream, const char *src)
@@ -62,11 +56,10 @@ void sq_quote_print(FILE *stream, const char *src)
fputc('\'', stream);
}
-char *sq_quote_argv(const char** argv, int count)
+void sq_quote_argv(struct strbuf *dst, const char** argv, int count,
+ size_t maxlen)
{
- char *buf, *to;
int i;
- size_t len = 0;
/* Count argv if needed. */
if (count < 0) {
@@ -74,53 +67,14 @@ char *sq_quote_argv(const char** argv, int count)
; /* just counting */
}
- /* Special case: no argv. */
- if (!count)
- return xcalloc(1,1);
-
- /* Get destination buffer length. */
- for (i = 0; i < count; i++)
- len += sq_quote_buf(NULL, 0, argv[i]) + 1;
-
- /* Alloc destination buffer. */
- to = buf = xmalloc(len + 1);
-
/* Copy into destination buffer. */
+ strbuf_grow(dst, 32 * count);
for (i = 0; i < count; ++i) {
- *to++ = ' ';
- to += sq_quote_buf(to, len, argv[i]);
+ strbuf_addch(dst, ' ');
+ sq_quote_buf(dst, argv[i]);
+ if (maxlen && dst->len > maxlen)
+ die("Too many or long arguments");
}
-
- return buf;
-}
-
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-int add_to_string(char **ptrp, int *sizep, const char *str, int quote)
-{
- char *p = *ptrp;
- int size = *sizep;
- int oc;
- int err = 0;
-
- if (quote)
- oc = sq_quote_buf(p, size, str);
- else {
- oc = strlen(str);
- memcpy(p, str, (size <= oc) ? size - 1 : oc);
- }
-
- if (size <= oc) {
- err = 1;
- oc = size - 1;
- }
-
- *ptrp += oc;
- **ptrp = '\0';
- *sizep -= oc;
- return err;
}
char *sq_dequote(char *arg)
diff --git a/quote.h b/quote.h
index 8a59cc5..78e8d3e 100644
--- a/quote.h
+++ b/quote.h
@@ -29,13 +29,10 @@
*/
extern void sq_quote_print(FILE *stream, const char *src);
-extern char *sq_quote_argv(const char** argv, int count);
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-extern int add_to_string(char **ptrp, int *sizep, const char *str, int quote);
+extern void sq_quote_buf(struct strbuf *, const char *src);
+extern void sq_quote_argv(struct strbuf *, const char **argv, int count,
+ size_t maxlen);
/* This unwraps what sq_quote() produces in place, but returns
* NULL if the input does not look like what sq_quote would have
diff --git a/trace.c b/trace.c
index 91548a5..efc656b 100644
--- a/trace.c
+++ b/trace.c
@@ -85,32 +85,29 @@ void trace_printf(const char *fmt, ...)
void trace_argv_printf(const char **argv, int count, const char *fmt, ...)
{
- char buf[8192];
va_list ap;
- char *argv_str;
- size_t argv_len;
+ struct strbuf trace;
int fd, len, need_close = 0;
fd = get_trace_fd(&need_close);
if (!fd)
return;
+ strbuf_init(&trace, 0);
+ strbuf_grow(&trace, 8192);
+
va_start(ap, fmt);
- len = vsnprintf(buf, sizeof(buf), fmt, ap);
+ len = vsnprintf(trace.buf, strbuf_avail(&trace) + 1, fmt, ap);
va_end(ap);
- if (len >= sizeof(buf))
+ if (len >= strbuf_avail(&trace))
die("unreasonnable trace length");
+ strbuf_setlen(&trace, len);
- /* Get the argv string. */
- argv_str = sq_quote_argv(argv, count);
- argv_len = strlen(argv_str);
-
- write_or_whine_pipe(fd, buf, len, err_msg);
- write_or_whine_pipe(fd, argv_str, argv_len, err_msg);
- write_or_whine_pipe(fd, "\n", 1, err_msg);
-
- free(argv_str);
+ sq_quote_argv(&trace, argv, count, 0);
+ strbuf_addch(&trace, '\n');
+ write_or_whine_pipe(fd, trace.buf, trace.len, err_msg);
+ strbuf_release(&trace);
if (need_close)
close(fd);
}
--
1.5.3.2.1036.gca8d2
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply related
* Re: [PATCH] Drop UTF-8 characters in manual pages
From: Lukas Sandström @ 2007-09-20 9:08 UTC (permalink / raw)
To: Git Mailing List, Junio C Hamano; +Cc: mh
In-Reply-To: <7vd4wgwczs.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> writes:
>> so
>> UTF-8 characters are just displayed as their sequence of bytes, which is
>> not very appealing.
>
> Perhaps not. I cannot decide what to do with
> Documentation/git-pack-redundant.txt, though.
>
These days when we have delta reusing and repacking is quite cheap,
I'd say just drop pack-redundant and its man page completely.
Of course, some pople might still be using it, but I haven't
felt the need for it in over a year.
/Lukas Sandström
^ permalink raw reply
* Re: [PATCH] Add git-rev-list --invert-match
From: Johannes Schindelin @ 2007-09-20 10:32 UTC (permalink / raw)
To: Bart Trojanowski; +Cc: git
In-Reply-To: <20070919202615.GK3076@jukie.net>
Hi,
On Wed, 19 Sep 2007, Bart Trojanowski wrote:
> git log --invert-match --grep="uninteresting"
IMHO this is only half a solution. Some of us want to say "git log
--grep=this --but-not-(in-the-whole-message) --grep=that".
Your code would be obsoleted by such a method (since it cannot be
extended), and further would also benefit git-grep, not only git-log.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] User Manual: add a chapter for submodules
From: Johannes Schindelin @ 2007-09-20 10:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Miklos Vajna, git, J. Bruce Fields
In-Reply-To: <7v1wcum0ox.fsf@gitster.siamese.dyndns.org>
Hi,
On Wed, 19 Sep 2007, Junio C Hamano wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
>
> > Signed-off-by: Michael Smith <msmith@cbnco.com>
> > Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> > ---
> >
> > Sorry, I sent the original patch again. So here is the updated second version.
>
> Looks Ok to me, although I didn't verify the examples by
> actually running them myself this time (last round I did).
So maybe we should do the same as with the tutorial: stick the examples
into a test script?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/5] Refactor struct transport_ops inlined into struct transport
From: Johannes Schindelin @ 2007-09-20 10:59 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Shawn O. Pearce, Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0709192226150.21941@iabervon.org>
Hi,
On Wed, 19 Sep 2007, Daniel Barkalow wrote:
> On Wed, 19 Sep 2007, Johannes Schindelin wrote:
>
> > On Wed, 19 Sep 2007, Shawn O. Pearce wrote:
> >
> > > diff --git a/transport.c b/transport.c
> > > index cc76e3f..d8458dc 100644
> > > --- a/transport.c
> > > +++ b/transport.c
> > > @@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)
> > > return 0;
> > > }
> > >
> > > -static const struct transport_ops rsync_transport;
> > > -
> > > static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {
> > > const char **argv;
> > > int argc;
> > > @@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)
> > > ret->url = url;
> > >
> > > if (!prefixcmp(url, "rsync://")) {
> > > - ret->ops = &rsync_transport;
> > > + /* not supported; don't populate any ops */
> > > +
> >
> > That is sneaky. What are the reasons to remove rsync support? I know it
> > is deprecated, but I'd still like to have it, especially for initial
> > clones on small-RAMed machines.
>
> It never got implemented in a way called from C. This is just removing the
> pointer to the empty struct where support would go. If anybody knows
> enough about interfacing with rsync to write the necessary functions, it
> can be restored.
Okay, will do once I find the time.
Ciao,
Dscho
^ 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