* Re: How to re-use setups in multiple tests?
From: Tom Clarke @ 2007-10-01 12:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0710011243230.28395@racer.site>
In this case the first test rebases the branch created in setup (it's
testing the rebase merge strategy), the second test should do the same
thing, except check there is a warning if a --message option is
passed.
I suppose I could find the old pre-rebase head and work with that, but
that doesn't seem that clean to me.
Here's the code (non-working):
#!/bin/sh
test_description='merge-rebase backend test'
. ./test-lib.sh
test_expect_success setup '
echo hello >a &&
git add a &&
test_tick && git commit -m initial &&
git checkout -b branch &&
echo hello >b &&
git add b &&
test_tick && git commit -m onbranch &&
git checkout master &&
echo update >a &&
git add a &&
test_tick && git commit -m update
'
test_expect_success 'merging using rebase does not create merge
commit' '
git checkout branch &&
git merge -s rebase master &&
( git log --pretty=oneline ) >actual &&
(
echo "4db7a5a013e67aa623d1fd294e8d46e89b3ace8f
onbranch"
echo "893371811dbd13e85c098b72d1ab42bcfd24c2db update"
echo "0e960b10429bf3f1e168ee2cc7d531ac7c622580
initial"
) >expected &&
git diff -w -u expected actual
'
test_expect_success 'merging using rebase with message gives warning'
'
#doesn't work because the branch has already been rebased and
is therefore up to date
git checkout branch &&
git merge -m "a message" -s rebase master 2>&1 expected &&
(
echo "warning: Message is not used for rebase merge
strategy"
) >expected &&
git diff -w -u expected actual
'
test_done
-Tom
On 10/1/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 1 Oct 2007, Tom Clarke wrote:
>
> > I'm wondering if there's a pattern for re-using setups across several
> > tests, similar to how a setUp function is used in xUnit. The problem is
> > I need the setup to actually be re-run, for each test to start from a
> > clean slate, so using the following doesn't work as the setup is just
> > run before the first test.
>
> We typically do the clean up phase explicitely. Or avoid it.
>
> Example: you want to do something to a branch, but the next step should
> use the original state of the branch.
>
> Solution: "git checkout -b new-branch HEAD~5"
>
> Sorry, unless you are a little less mysterious about the exact use case
> you have in mind, I cannot help more.
>
> Ciao,
> Dscho
>
>
^ permalink raw reply
* Re: [PATCH v2] git stash: document apply's --index switch
From: Johannes Schindelin @ 2007-10-01 11:49 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Junio C Hamano, Bruno Haible, git
In-Reply-To: <1191239139-26992-1-git-send-email-vmiklos@frugalware.org>
Hi,
On Mon, 1 Oct 2007, Miklos Vajna wrote:
> > - the lines are a bit too long, no?
>
> all lines are shorter than 80 chars, i think it is correct. or am i
> wrong?
No, you are not. But judging from the lines before your change:
> @@ -71,6 +71,11 @@ apply [<stash>]::
> +
> This operation can fail with conflicts; you need to resolve them
> by hand in the working tree.
> ++
> +If the `--index` option is used, this command tries to reinstate not only the
> [...]
it seemed to me that the lines were meant to be shorter. At least they
are easier to read when quoted as a diff in an email.
But it really does not matter much to me.
Ciao,
Dscho
^ permalink raw reply
* Re: How to re-use setups in multiple tests?
From: Johannes Schindelin @ 2007-10-01 11:45 UTC (permalink / raw)
To: Tom Clarke; +Cc: git
In-Reply-To: <550f9510710010327l3e729ff1tbbb9b6c674c1cb11@mail.gmail.com>
Hi,
On Mon, 1 Oct 2007, Tom Clarke wrote:
> I'm wondering if there's a pattern for re-using setups across several
> tests, similar to how a setUp function is used in xUnit. The problem is
> I need the setup to actually be re-run, for each test to start from a
> clean slate, so using the following doesn't work as the setup is just
> run before the first test.
We typically do the clean up phase explicitely. Or avoid it.
Example: you want to do something to a branch, but the next step should
use the original state of the branch.
Solution: "git checkout -b new-branch HEAD~5"
Sorry, unless you are a little less mysterious about the exact use case
you have in mind, I cannot help more.
Ciao,
Dscho
^ permalink raw reply
* Re: git-svn and hierarchal branches.
From: Sam Vilain @ 2007-10-01 11:45 UTC (permalink / raw)
To: David Brown; +Cc: git
In-Reply-To: <20070930070754.GA10264@old.davidb.org>
David Brown wrote:
> I'm trying to use git-svn to track the 'Tango' SVN repository
> <http://www.dsource.org/projects/tango/wiki/Download>
>
> Their layout is mostly standard, except that they have an extra hierarchy
> in the branch directory (branch/user1, branch/user2, branch/D1_0 for a
> release).
>
> Is this something I can easily track with git-svn, or am I best trying to
> do it manually?
Track each branch with a separate git-svn remote.
Sam.
^ permalink raw reply
* Re: [PATCH] shortlog: remove --help option parsing
From: Johannes Schindelin @ 2007-10-01 11:42 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: git
In-Reply-To: <20071001102020.GA8315@diku.dk>
Hi,
On Mon, 1 Oct 2007, Jonas Fonseca wrote:
> - else if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
> + else if (!strcmp(argv[1], "-h"))
I see that this is my fault.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
From: Johannes Schindelin @ 2007-10-01 11:39 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: Kristian Høgsberg, gitster, git
In-Reply-To: <20071001103148.GC8315@diku.dk>
Hi,
On Mon, 1 Oct 2007, Jonas Fonseca wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote Mon, Oct 01, 2007:
>
> > On Sun, 30 Sep 2007, Jonas Fonseca wrote:
> >
> > > Also, I think for this to be more usable for other built-in programs
> > > it shouldn't modify argv, but instead take both argc and argv (so we
> > > don't need to have code like "*++(*argv)" ;), parse _all_ options in
> > > one go, and return the index (of argv) for any remaining options.
> >
> > We _have_ to modify argv. For example, "git log master -p" is
> > perfectly valid.
>
> Ah, yes this could be nice to also finally have (more universally) in
> git. But for this to be possible I don't see any reason for it to modify
> the pointer to argv. Instead, it can just reshuffle entries in argv.
In that case, I misunderstood you. Indeed, I'd only reshuffle the
entries of argv.
Ciao,
Dscho
^ permalink raw reply
* git-browser and branch names
From: David Symonds @ 2007-10-01 11:24 UTC (permalink / raw)
To: Git
Hi,
I've been using git-browser hooked up to gitweb for several weeks now
(git 1.5.2.1), and it's going great. However, I've hit my first bug:
it seems that certain branch head names cause the whole diagram (and
log lines, etc.) to fail to render, only displaying the "Starting
from: all <repo>.git heads" stuff down the bottom. As to the various
names I've tested:
okay: master, 2.5j, 2.5-dev, a.b, 2-5, 2.b, b.2, 2.5.0
fails: 2.5, 2.6
I've experimented by just using 'git branch -m <old> <new>' to rename
a branch that I'd prefer to just call '2.5', so I'm pretty sure that
I'm not causing things to break myself.
It seems that it's when the branch name looks like a decimal number,
but there's nothing that appears in Apache's error_log. This leads me
to believe it's a Javascript bug.
Can anyone give me pointers or suggestions as to where to start
debugging this? Anyone else encountered this?
Thanks,
Dave.
^ permalink raw reply
* Re: [PATCH] Support tags in uncommit - use git_id instead of rev_parse
From: Karl Hasselström @ 2007-10-01 11:16 UTC (permalink / raw)
To: Pavel Roskin; +Cc: Catalin Marinas, git
In-Reply-To: <20070930172647.18972.49369.stgit@tt.roinet.com>
Sounds like a good idea. Test? :-)
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH] Don't checkout the full tree if avoidable
From: Eric Wong @ 2007-10-01 11:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Steven Walter
In-Reply-To: <7vejgftgef.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Steven Walter <stevenrwalter@gmail.com> writes:
>
> > In most cases of branching, the tree is copied unmodified from the trunk
> > to the branch. When that is done, we can simply start with the parent's
> > index and apply the changes on the branch as usual.
> >
> > Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
>
> Eric, do you like this one?
Junio, thanks for pinging me about it, I haven't been following the ML
very closely and forgot about this issue.
Steven Walter wrote:
> One criticism of the patch: the trees_match function probably needs to
> be re-written. My SVN::Perl-foo is weak.
Yep :)
Steven:
How does the following work for you? Which version of SVN do you have,
by the way? I just found a bug with the way SVN::Client::diff() is
exported for SVN 1.1.4, hence the SVN::Pool->new_default_sub usage.
From: Steven Walter <stevenrwalter@gmail.com>
Date: Fri, 28 Sep 2007 13:24:19 -0400
Subject: [PATCH] Don't checkout the full tree if avoidable
In most cases of branching, the tree is copied unmodified from the trunk
to the branch. When that is done, we can simply start with the parent's
index and apply the changes on the branch as usual.
[ew: rewritten from Steven's original to use SVN::Client instead
of the command-line svn client.
Since SVN::Client connects separately, we'll share our
authentication providers array between our usages of
SVN::Client and SVN::Ra, too. Bypassing the high-level
SVN::Client library can avoid this, but the code will be
much more complex. Regardless, any implementation of this
seems to require restarting a connection to the remote
server.
Also of note is that SVN 1.4 and later allows a more
efficient diff_summary to be done instead of a full diff,
but since this code is only to support SVN < 1.4.4, we'll
ignore it for now.]
Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
git-svn.perl | 64 +++++++++++++++++++++++++++++++++++++++++++--------------
1 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 484b057..777e436 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1847,6 +1847,16 @@ sub find_parent_branch {
$gs->ra->gs_do_switch($r0, $rev, $gs,
$self->full_url, $ed)
or die "SVN connection failed somewhere...\n";
+ } elsif ($self->ra->trees_match($new_url, $r0,
+ $self->full_url, $rev)) {
+ print STDERR "Trees match:\n",
+ " $new_url\@$r0\n",
+ " ${\$self->full_url}\@$rev\n",
+ "Following parent with no changes\n";
+ $self->tmp_index_do(sub {
+ command_noisy('read-tree', $parent);
+ });
+ $self->{last_commit} = $parent;
} else {
print STDERR "Following parent with do_update\n";
$ed = SVN::Git::Fetcher->new($self);
@@ -3027,28 +3037,32 @@ BEGIN {
}
}
+sub _auth_providers () {
+ [
+ SVN::Client::get_simple_provider(),
+ SVN::Client::get_ssl_server_trust_file_provider(),
+ SVN::Client::get_simple_prompt_provider(
+ \&Git::SVN::Prompt::simple, 2),
+ SVN::Client::get_ssl_client_cert_file_provider(),
+ SVN::Client::get_ssl_client_cert_prompt_provider(
+ \&Git::SVN::Prompt::ssl_client_cert, 2),
+ SVN::Client::get_ssl_client_cert_pw_prompt_provider(
+ \&Git::SVN::Prompt::ssl_client_cert_pw, 2),
+ SVN::Client::get_username_provider(),
+ SVN::Client::get_ssl_server_trust_prompt_provider(
+ \&Git::SVN::Prompt::ssl_server_trust),
+ SVN::Client::get_username_prompt_provider(
+ \&Git::SVN::Prompt::username, 2)
+ ]
+}
+
sub new {
my ($class, $url) = @_;
$url =~ s!/+$!!;
return $RA if ($RA && $RA->{url} eq $url);
SVN::_Core::svn_config_ensure($config_dir, undef);
- my ($baton, $callbacks) = SVN::Core::auth_open_helper([
- SVN::Client::get_simple_provider(),
- SVN::Client::get_ssl_server_trust_file_provider(),
- SVN::Client::get_simple_prompt_provider(
- \&Git::SVN::Prompt::simple, 2),
- SVN::Client::get_ssl_client_cert_file_provider(),
- SVN::Client::get_ssl_client_cert_prompt_provider(
- \&Git::SVN::Prompt::ssl_client_cert, 2),
- SVN::Client::get_ssl_client_cert_pw_prompt_provider(
- \&Git::SVN::Prompt::ssl_client_cert_pw, 2),
- SVN::Client::get_username_provider(),
- SVN::Client::get_ssl_server_trust_prompt_provider(
- \&Git::SVN::Prompt::ssl_server_trust),
- SVN::Client::get_username_prompt_provider(
- \&Git::SVN::Prompt::username, 2),
- ]);
+ my ($baton, $callbacks) = SVN::Core::auth_open_helper(_auth_providers);
my $config = SVN::Core::config_get_config($config_dir);
$RA = undef;
my $self = SVN::Ra->new(url => $url, auth => $baton,
@@ -3112,6 +3126,24 @@ sub get_log {
$ret;
}
+sub trees_match {
+ my ($self, $url1, $rev1, $url2, $rev2) = @_;
+ my $ctx = SVN::Client->new(auth => _auth_providers);
+ my $out = IO::File->new_tmpfile;
+
+ # older SVN (1.1.x) doesn't take $pool as the last parameter for
+ # $ctx->diff(), so we'll create a default one
+ my $pool = SVN::Pool->new_default_sub;
+
+ $ra_invalid = 1; # this will open a new SVN::Ra connection to $url1
+ $ctx->diff([], $url1, $rev1, $url2, $rev2, 1, 1, 0, $out, $out);
+ $out->flush;
+ my $ret = (($out->stat)[7] == 0);
+ close $out or croak $!;
+
+ $ret;
+}
+
sub get_commit_editor {
my ($self, $log, $cb, $pool) = @_;
my @lock = $SVN::Core::VERSION ge '1.2.0' ? (undef, 0) : ();
--
Eric Wong
^ permalink raw reply related
* Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
From: Jonas Fonseca @ 2007-10-01 10:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristian Høgsberg, gitster, git
In-Reply-To: <Pine.LNX.4.64.0710011114310.28395@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote Mon, Oct 01, 2007:
> Hi,
>
> On Sun, 30 Sep 2007, Jonas Fonseca wrote:
>
> > Also, I think for this to be more usable for other built-in programs it
> > shouldn't modify argv, but instead take both argc and argv (so we don't
> > need to have code like "*++(*argv)" ;), parse _all_ options in one go,
> > and return the index (of argv) for any remaining options.
>
> We _have_ to modify argv. For example, "git log master -p" is perfectly
> valid.
Ah, yes this could be nice to also finally have (more universally) in
git. But for this to be possible I don't see any reason for it to modify
the pointer to argv. Instead, it can just reshuffle entries in argv.
--
Jonas Fonseca
^ permalink raw reply
* [PATCH] mv: allow verbosity to be enabled using -v
From: Jonas Fonseca @ 2007-10-01 10:27 UTC (permalink / raw)
To: git
The infrastructure was already there but the option parsing for it was
missing. Also, move full command line documentation to synopsis section.
Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
Documentation/git-mv.txt | 10 +++++-----
builtin-mv.c | 6 +++++-
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 2c9cf74..9cd1177 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -8,15 +8,14 @@ git-mv - Move or rename a file, a directory, or a symlink
SYNOPSIS
--------
-'git-mv' <options>... <args>...
+[verse]
+'git-mv' [-f] [-n] [-v] <source> <destination>
+'git-mv' [-f] [-n] [-v] [-k] <source> ... <destination directory>
DESCRIPTION
-----------
This script is used to move or rename a file, directory or symlink.
- git-mv [-f] [-n] <source> <destination>
- git-mv [-f] [-n] [-k] <source> ... <destination directory>
-
In the first form, it renames <source>, which must exist and be either
a file, symlink or directory, to <destination>.
In the second form, the last argument has to be an existing
@@ -36,7 +35,8 @@ OPTIONS
file unless '-f' is given.
-n::
Do nothing; only show what would happen
-
+-v::
+ Be verbose.
Author
------
diff --git a/builtin-mv.c b/builtin-mv.c
index b944651..620998e 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -10,7 +10,7 @@
#include "path-list.h"
static const char builtin_mv_usage[] =
-"git-mv [-n] [-f] (<source> <destination> | [-k] <source>... <destination>)";
+"git-mv [-n] [-f] [-v] (<source> <destination> | [-k] <source>... <destination>)";
static const char **copy_pathspec(const char *prefix, const char **pathspec,
int count, int base_name)
@@ -99,6 +99,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
ignore_errors = 1;
continue;
}
+ if (!strcmp(arg, "-v")) {
+ verbose = 1;
+ continue;
+ }
usage(builtin_mv_usage);
}
count = argc - i - 1;
--
1.5.3.2.1029.gc1d178
--
Jonas Fonseca
^ permalink raw reply related
* How to re-use setups in multiple tests?
From: Tom Clarke @ 2007-10-01 10:27 UTC (permalink / raw)
To: git
I'm wondering if there's a pattern for re-using setups across several
tests, similar to how a setUp function is used in xUnit. The problem
is I need the setup to actually be re-run, for each test to start from
a clean slate, so using the following doesn't work as the setup is
just run before the first test.
test_expect_success setup '
# setup repostory to a particular state
'
test_expect_success test1 '
# some test that expects the state to be as defined in setup, and
changes state of repository
'
test_expect_success test2 '
# another test that expects the state to be as defined in setup
'
Is there a convention for doing this that's already used? Perhaps
pulling the setup code into a function or duplicating the code? Or is
it better to create a separate test file for tests that need to be
isolated?
Thanks,
-Tom
^ permalink raw reply
* Re: Referring a commit-id remote repo.
From: Johannes Schindelin @ 2007-10-01 10:25 UTC (permalink / raw)
To: David Brown; +Cc: Git
In-Reply-To: <20071001041635.GA22102@old.davidb.org>
Hi,
On Sun, 30 Sep 2007, David Brown wrote:
> The question I have: is there any way I can look at this particular
> commit ID on the remote repo? I couldn't come up with any way to get
> git fetch to retrieve it.
Unless you have push access, no. And this is very much by design. For
example, when somebody mistakenly pushed a secret (like what lines in the
kernel infringe on M$ patents, if any) it should be possible to rebase (in
a hurry), force a push, and have the safe feeling that nobody can fetch
the secret any longer.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] shortlog: remove --help option parsing
From: Jonas Fonseca @ 2007-10-01 10:20 UTC (permalink / raw)
To: git
This option is handled globally.
Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
builtin-shortlog.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 3fe7546..518f27f 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -264,7 +264,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
wrap_lines = 1;
parse_wrap_args(argv[1], &in1, &in2, &wrap);
}
- else if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
+ else if (!strcmp(argv[1], "-h"))
usage(shortlog_usage);
else
break;
--
1.5.3.2.1029.gc1d178
--
Jonas Fonseca
^ permalink raw reply related
* Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
From: Johannes Schindelin @ 2007-10-01 10:14 UTC (permalink / raw)
To: Jonas Fonseca; +Cc: Kristian Høgsberg, gitster, git
In-Reply-To: <20070930131133.GA11209@diku.dk>
Hi,
On Sun, 30 Sep 2007, Jonas Fonseca wrote:
> Also, I think for this to be more usable for other built-in programs it
> shouldn't modify argv, but instead take both argc and argv (so we don't
> need to have code like "*++(*argv)" ;), parse _all_ options in one go,
> and return the index (of argv) for any remaining options.
We _have_ to modify argv. For example, "git log master -p" is perfectly
valid.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 0/5] fork/exec removal series
From: David Kastrup @ 2007-10-01 9:49 UTC (permalink / raw)
To: git
In-Reply-To: <47009CC7.70300@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Johannes Schindelin schrieb:
>> On Sun, 30 Sep 2007, Johannes Sixt wrote:
>>> These cases I hope to be able to treat as "coroutine":
>>>
>>> - sideband demultiplexer in builtin-fetch-pack.c
>>> - internal rev-list in upload-pack
>>> - the two-way pipe handling in convert.c and builtin-upload-archive.c
>>>
>>> There are probably more in daemon.c and imap-send.c.
>>>
>>> BTW, the convert.c case (apply_filter) is most interesting for me,
>>> since I have a real-world use-case for a clean-filter.
>>
>> Calling it coroutine is interesting... But yes, I agree that these
>> three cases cannot be handled otherwise.
>
> Suggestions for a better name are appreciated!
I think coroutine is commonly used as the name for _synchronous_
context switches aka message passing. Basically the same as
subroutine calls, except that the called subroutine has its own
dynamic context (instruction pointer, call stack, control flow) that
gets activated and suspended.
If there is parallelism implied, "thread" is the more appropriate
name.
--
David Kastrup
^ permalink raw reply
* Re: [PATCH 1/4] Add a simple option parser for use by builtin-commit.c.
From: Jonas Fonseca @ 2007-09-30 13:11 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: gitster, git
In-Reply-To: <1190868632-29287-1-git-send-email-krh@redhat.com>
Hello Kristian,
I have some comments on your patch. Some of the "improvement" might have
to wait until after your builtin-commit changes hits git.git. However,
if we could agree on some of the general changes, I could start porting
other of the main porcelain commands to use the option parser without
depending on the state of the remaining builtin-commit series.
Kristian Høgsberg <krh@redhat.com> wrote Thu, Sep 27, 2007:
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
> Makefile | 2 +-
> parse-options.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> parse-options.h | 29 +++++++++++++++++++++
> 3 files changed, 104 insertions(+), 1 deletions(-)
> create mode 100644 parse-options.c
> create mode 100644 parse-options.h
>
> diff --git a/Makefile b/Makefile
> index 62bdac6..d90e959 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -310,7 +310,7 @@ LIB_OBJS = \
> alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
> color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
> convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
> - transport.o bundle.o
> + transport.o bundle.o parse-options.o
>
> BUILTIN_OBJS = \
> builtin-add.o \
> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..2fb30cd
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,74 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +
> +int parse_options(const char ***argv,
> + struct option *options, int count,
> + const char *usage_string)
> +{
> + const char *value, *eq;
> + int i;
> +
> + if (**argv == NULL)
> + return 0;
> + if ((**argv)[0] != '-')
> + return 0;
> + if (!strcmp(**argv, "--"))
> + return 0;
I don't know if this is a bug, but you do not remove "--" from argv,
which is later (in the patch that adds builtin-commit.c) passed to
add_files_to_cache and then get_pathspec where it is not removed or
detected either.
> +
> + value = NULL;
> + for (i = 0; i < count; i++) {
> + if ((**argv)[1] == '-') {
> + if (!prefixcmp(options[i].long_name, **argv + 2)) {
> + if (options[i].type != OPTION_BOOLEAN)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + eq = strchr(**argv + 2, '=');
> + if (eq && options[i].type != OPTION_BOOLEAN &&
> + !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_BOOLEAN)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + if (options[i].type != OPTION_BOOLEAN) {
> + value = **argv + 2;
> + goto match;
> + }
> + }
> + }
> +
> + usage(usage_string);
> +
> + match:
I think the goto can be avoided by simply breaking out of the above loop
when an option has been found and add ...
> + switch (options[i].type) {
case OPTION_LAST
usage(usage_string);
break;
> + case OPTION_BOOLEAN:
> + *(int *)options[i].value = 1;
> + break;
I've been looking at builtin-blame.c which IMO has some of the most
obscure option parsing and maybe this can be changed to increment in
order to support stuff like changing the meaning by passing the same arg
multiple times (e.g. "-C -C -C") better.
Blame option parsing also sports (enum) flags being masked together,
this can of course be rewritten to a boolean option followed by
masking when parse_options is done (to keep it sane).
> + case OPTION_STRING:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
Maybe change this ...
> + *(const char **)options[i].value = value;
> + break;
> + case OPTION_INTEGER:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
... and this to:
if (!value) {
error("option %s requires a value.", (*argv)[-1]);
usage(usage_string);
}
> + *(int *)options[i].value = atoi(value);
> + break;
> + default:
> + assert(0);
> + }
> +
> + (*argv)++;
> +
> + return 1;
> +}
> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..39399c3
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,29 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> + OPTION_BOOLEAN,
> + OPTION_STRING,
> + OPTION_INTEGER,
> + OPTION_LAST,
> +};
> +
> +struct option {
> + enum option_type type;
> + const char *long_name;
> + char short_name;
> + void *value;
> +};
Space vs tab indentation.
One of the last things I miss from Cogito is the nice abbreviated help
messages that was available via '-h'. I don't know if it would be
acceptable (at least for the main porcelain commands) to put this
functionality into the option parser by adding a "description" member to
struct option and have parse_options print a nice:
<error message if any>
<usage string>
<option summary>
on failure, or, if that is regarded as too verbose, simply when -h is
detected.
> +
> +/* Parse the given options against the list of known options. The
> + * order of the option structs matters, in that ambiguous
> + * abbreviations (eg, --in could be short for --include or
> + * --interactive) are matched by the first option that share the
> + * prefix.
> + */
This prefix aware option parsing has not been ported over to the other
builtins when they were lifted from shell code. It might be nice to have
of course. Is it really needed?
> +
> +extern int parse_options(const char ***argv,
> + struct option *options, int count,
> + const char *usage_string);
I think the interface could be improved a bit. For example, it doesn't
need to count argument since the last entry in the options array is
OPTION_LAST and thus the size can be detected that way.
Also, I think for this to be more usable for other built-in programs it
shouldn't modify argv, but instead take both argc and argv (so we don't
need to have code like "*++(*argv)" ;), parse _all_ options in one go,
and return the index (of argv) for any remaining options.
Then the following:
while (parse_options(argv, commit_options, ARRAY_SIZE(commit_options),
builtin_commit_usage))
;
becomes:
int i;
...
i = parse_options(argc, argv, commit_options, builtin_commit_usage);
This fits better with how option parsing is currently done. Take
builtin-add for example:
for (i = 1 ; i < argc ; i++) {
const char *arg = argv[i];
/* ... */
}
if (argc <= i)
usage(builtin_rm_usage);
[ BTW, blame option parsing actually wants to know if "--" has been seen,
but I think that can be worked around by simply checking argv[i - 1]
after calling the option parser. ]
> +
> +#endif
OK, I will stop these ramblings here. I hope the fact that I read your
patch both back and forth and added comments in the process didn't make
it too confusing.
--
Jonas Fonseca
^ 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-10-01 9:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <7vtzpbrzye.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> Letting git_connect() die on error unconditionally is poison for any
>> libification efforts. So here's a plan:
>>
>> 1. Let git_connect() return a struct child_process even for the
>> non-forking case. This way a return value of NULL can uniquely
>> identify a failure.
>> ...
>> Since my patch doesn't do (1), it can't do (2), i.e. keep the error
>> checks -
>> they must be removed because no unique failure value exists. So I
>> could complete (1) in a new version of this patch, in order to also do
>> (2). What is your preference?
>
> In any case, I'd rather first have one that hides fork/exec
> behind child_process first without changing the call to die() in
> git_connect() in this round. I am still in "post feature
> release clean-up" mood ;-)
Sure: The die()s are converted in a later step.
My problem is that if I don't wrap the non-fork connections somehow in this
first round, I *must* remove the error checks because there is no unique
failure return value anymore.
> As to error indication, it somehow does not feel right to return
> something called "child _process_" structure when we want to
> tell the caller that there is no process to wait for in the
> no-error case, although the fact that we can use .in/.out fd in
> the structure when we _do_ have child process is attractive.
Did you mean: "even if we don't have a child process"?
How about a typedef if you dislike the name?
> As an alternative, we could keep the "NULL return means there
> was no need to fork" semantics of git_connect(), and instead add
> "int *status_ret" parameter for the caller to check.
Seriously? Add an *out* parameter when we can get rid of one and have a
return value, too?
-- Hannes
^ 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-10-01 8:39 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Sixt, git
In-Reply-To: <4700A067.3010004@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Junio C Hamano schrieb:
>> Johannes Sixt <johannes.sixt@telecom.at> writes:
>>
>>> This prepares the API of git_connect() and finish_connect() to operate on
>>> a struct child_process. Currently, we just use that object as a placeholder
>>> for the pid that we used to return. A follow-up patch will change the
>>> implementation of git_connect() and finish_connect() to make full use
>>> of the object.
>>
>> Good description, except removal of checks for negative return
>> of the calling sites raised my eyebrow and had me spend a few
>> more minutes to review than necessary (see below).
>
> I've thought about this issue a bit more.
>
> Letting git_connect() die on error unconditionally is poison for any
> libification efforts. So here's a plan:
>
> 1. Let git_connect() return a struct child_process even for the
> non-forking case. This way a return value of NULL can uniquely
> identify a failure.
> ...
> Since my patch doesn't do (1), it can't do (2), i.e. keep the error
> checks -
> they must be removed because no unique failure value exists. So I
> could complete (1) in a new version of this patch, in order to also do
> (2). What is your preference?
In any case, I'd rather first have one that hides fork/exec
behind child_process first without changing the call to die() in
git_connect() in this round. I am still in "post feature
release clean-up" mood ;-)
As to error indication, it somehow does not feel right to return
something called "child _process_" structure when we want to
tell the caller that there is no process to wait for in the
no-error case, although the fact that we can use .in/.out fd in
the structure when we _do_ have child process is attractive.
As an alternative, we could keep the "NULL return means there
was no need to fork" semantics of git_connect(), and instead add
"int *status_ret" parameter for the caller to check.
^ permalink raw reply
* Re: [PATCH] rebase: add --signoff option
From: Junio C Hamano @ 2007-10-01 8:20 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: git
In-Reply-To: <11911689111797-git-send-email-prohaska@zib.de>
Steffen Prohaska <prohaska@zib.de> writes:
> When preparing a series of commits for upstream you may
> need to signoff commits if you forgot to do so earlier.
> This patch teaches git-rebase to signoff during rebase
> if you pass the option --signoff.
>
> Notes
> 1) --signoff cannot be used simultaneously with --interactive.
> 2) --signoff forces a rebase even if current path is a
> descendant of <upstream>.
>
> Signed-off-by: Steffen Prohaska <prohaska@zib.de>
I do not think it is fatal for --signoff to be incompatible with
the "interactive" mode is fatal, but lack of mention in the
documentation is. Also this would need test scripts to prevent
it from getting broken by future changes by others. I'd like to
see ones that test at least the cases where (1) nobody has
sign-off, (2) you do not have sign-off but others do, and (3)
you already have sign-off at the end.
But I do like the general concept.
^ permalink raw reply
* Re: [PATCH] Improve bash prompt to detect various states like an unfinished merge
From: Junio C Hamano @ 2007-10-01 8:15 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: gitster, git, johannes.sixt, spearce
In-Reply-To: <1191111645134-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> This patch makes the git prompt (when enabled) show if a merge or a
> rebase is unfinished. It also detects if a bisect is being done as
> well as detached checkouts.
Since you show the name of the branch anyway, I have to wonder
why you should say BISECT.
Also if you know you normally get branch name, lack of branch
name would indicate detached HEAD, I would presume.
But other state information may be helpful.
^ permalink raw reply
* Re: [PATCH] Don't checkout the full tree if avoidable
From: Junio C Hamano @ 2007-10-01 7:58 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Steven Walter
In-Reply-To: <1191000259190-git-send-email-stevenrwalter@gmail.com>
Steven Walter <stevenrwalter@gmail.com> writes:
> In most cases of branching, the tree is copied unmodified from the trunk
> to the branch. When that is done, we can simply start with the parent's
> index and apply the changes on the branch as usual.
>
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
Eric, do you like this one?
> ---
> git-svn.perl | 18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 484b057..2ca2042 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1847,6 +1847,13 @@ sub find_parent_branch {
> $gs->ra->gs_do_switch($r0, $rev, $gs,
> $self->full_url, $ed)
> or die "SVN connection failed somewhere...\n";
> + } elsif ($self->trees_match($new_url, $r0,
> + $self->full_url, $rev)) {
> + $self->tmp_index_do(sub {
> + command_noisy('read-tree', $parent);
> + });
> + $self->{last_commit} = $parent;
> + # Assume copy with no changes
> } else {
> print STDERR "Following parent with do_update\n";
> $ed = SVN::Git::Fetcher->new($self);
> @@ -1859,6 +1866,17 @@ sub find_parent_branch {
> return undef;
> }
>
> +sub trees_match {
> + my ($self, $url1, $rev1, $url2, $rev2) = @_;
> +
> + my $ret=1;
> + open(my $fh, "svn diff $url1\@$rev1 $url2\@$rev2 |");
> + $ret=0 if (<$fh>);
> + close($fh);
> +
> + return $ret;
> +}
> +
> sub do_fetch {
> my ($self, $paths, $rev) = @_;
> my $ed;
> --
> 1.5.3.1
^ permalink raw reply
* Re: git-svn merge helper
From: Benoit SIGOURE @ 2007-10-01 7:56 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: git
In-Reply-To: <20071001025059.GA29323@atjola.homenet>
[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]
On Oct 1, 2007, at 4:50 AM, Björn Steinbrink wrote:
> On 2007.09.30 16:15:49 +0200, Benoit SIGOURE wrote:
>> On Sep 30, 2007, at 1:05 PM, Björn Steinbrink wrote:
>>
>>> Hi,
>>>
>>> I recently discovered git-svn and absolutey love it. One thing
>>> that I'm
>>> missing though, is an equivalent of "svn merge" for merging
>>> between svn
>>> remotes, to support the SVN way of using "squashed" merges, where
>>> you
>>> just note the merge meta-data in the commit message. "git merge"
>>> didn't
>>> work for me (and probably isn't expected to work) to merge
>>> between two
>>> svn branches, so I've resorted to cherry-picking the required
>>> commits
>>> one by one into a temporary branch and then squashing them
>>> together by
>>> doing a --squash merge with a second temporary branch (as in [1]).
>>
>> I fail to see why you'd want to reproduce the broken behavior of
>> svn merge.
>> Anyways, git-svn is a great way to merge SVN branches,
>> unfortunately it
>> can't detect when merges happened on the SVN side. I think you
>> can use it
>> nevertheless by manually adding a graft at the last merge point,
>> which
>> would help you merging the right revisions without having to
>> specify what
>> needs to be merged (unless someone made another merge on the SVN
>> side, in
>> which case you need to update your graft).
>
> Then how does that work? The manpage explicitly says that I should not
> use git-{pull,merge} on branches I want to dcommit from. And a trivial
> test immediately got the expected effect of git-svn trying to
> commit to
> trunk instead of the branch.
Ah, yes, you're right. Well, this will work the day we can pass an
option to git-svn dcommit to tell it where the commit must be sent.
--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]
^ permalink raw reply
* [gitk] select only the part of a patch related to a path
From: picca @ 2007-10-01 7:02 UTC (permalink / raw)
To: git
Hello
When I want to look at the history of a particular file ex:
gitk path/to/my/file
I see only the commits corresponding to that file and this is ok.
But sometimes I just want to see the modifications done on this file.
So i would like to see only the relevant part of the commit.
Is it possible to do with gitk ?
Thanks
Frederic
^ 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-10-01 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git
In-Reply-To: <7vbqbjg9zz.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
>
>> This prepares the API of git_connect() and finish_connect() to operate on
>> a struct child_process. Currently, we just use that object as a placeholder
>> for the pid that we used to return. A follow-up patch will change the
>> implementation of git_connect() and finish_connect() to make full use
>> of the object.
>
> Good description, except removal of checks for negative return
> of the calling sites raised my eyebrow and had me spend a few
> more minutes to review than necessary (see below).
I've thought about this issue a bit more.
Letting git_connect() die on error unconditionally is poison for any
libification efforts. So here's a plan:
1. Let git_connect() return a struct child_process even for the
non-forking case. This way a return value of NULL can uniquely
identify a failure.
2. Keep the error checks in the callers (adjust to test for NULL).
3. Change the die() calls to return failure.
4. Note that the int fd[2] parameter to git_connect() is really an
output: Remove it and use .in and .out of the returned struct
child_process instead.
And maybe:
5. Reuse somehow the struct child_process that git_proxy_connect()
already fills in.
Since my patch doesn't do (1), it can't do (2), i.e. keep the error checks -
they must be removed because no unique failure value exists. So I could
complete (1) in a new version of this patch, in order to also do (2). What
is your preference?
-- Hannes
PS: I've postponed the completion of this plan - in favor of the MinGW port
integration - because it only helps libification.
^ permalink raw reply
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