* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Junio C Hamano @ 2007-11-05 19:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Rene Scharfe, gitster
In-Reply-To: <Pine.LNX.4.64.0711041915290.4362@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Unfortunately, we cannot reuse the result of that function, which
> would be cleaner: there are more users than just git log. Most
> notably, git-archive with "$Format:...$" substitution.
That makes sense.
> diff --git a/pretty.c b/pretty.c
> index 490cede..241e91c 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -393,6 +393,7 @@ void format_commit_message(const struct commit *commit,
> int i;
> enum { HEADER, SUBJECT, BODY } state;
> const char *msg = commit->buffer;
> + char *active = interp_find_active(format, table, ARRAY_SIZE(table));
> ...
> + if (active[IHASH])
> + interp_set_entry(table, IHASH,
> + sha1_to_hex(commit->object.sha1));
> + if (active[IHASH_ABBREV])
> + interp_set_entry(table, IHASH_ABBREV,
> find_unique_abbrev(commit->object.sha1,
> DEFAULT_ABBREV));
Instead of allocating a separate array and freeing at the end,
wouldn't it make more sense to have a bitfield that records what
is used by the format string inside the array elements?
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Pierre Habouzit @ 2007-11-05 19:50 UTC (permalink / raw)
To: Steven Grimm; +Cc: Junio C Hamano, git
In-Reply-To: <CD2E6759-9E7E-41E6-8B58-AB6CA9604111@midwinter.com>
[-- Attachment #1: Type: text/plain, Size: 952 bytes --]
On Mon, Nov 05, 2007 at 07:28:03PM +0000, Steven Grimm wrote:
> On Nov 5, 2007, at 11:01 AM, Pierre Habouzit wrote:
> >When git-revert has a file argument then redirect the user to what he
> >probably meant.
>
> That's a big improvement. Basically everyone I show git to gets "revert"
> wrong at first.
>
> >+ die("Cannot find commit '%s', did you meant: "
> >+ "git checkout HEAD -- '%s'", arg, arg);
>
> But that suggested command is not going to convince anyone they were
> wrong about git being hard to learn. I wonder if instead of saying, "I
> know what you meant, but I'm going to make you type a different command,"
> we should make git revert just do what the user meant.
That's an option, but it wouldn't be maint material 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
* Re: [PATCH] Use parseopts in builtin-fetch
From: Pierre Habouzit @ 2007-11-05 19:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, git
In-Reply-To: <7vir4gqzdu.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]
On Mon, Nov 05, 2007 at 07:13:33PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> >> I mostly did this and the next one for practice with the API. I'm
> >> impressed that "git fetch -vv" is even handled correctly without anything
> >> special.
> >
> > About that: OPTION_BOOLEAN increments the associated variable, to
> > support this case specifically.
> >
> > The last thing that really miss in parse-options is a way to recurse
> > into a sub-array of struct option, to be able to port the generic diff
> > and revision arguments.
>
> Another micronit is I found lacking is that it is a bit too
> cumbersome to accept only a subset of integer as a value
> (e.g. "this option takes a positive integer, not zero nor
> negative"). The caller can set up a callback to handle that,
> though.
As a general rule we may want to have some kind of "ranged" integers.
but like you said the callback is always here for that, and if we begin
to have dozens of those we may consider creating a new type for those if
needed.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] git-commit.sh: Fix usage checks regarding paths given when they do not make sense
From: Björn Steinbrink @ 2007-11-05 19:36 UTC (permalink / raw)
To: gitster; +Cc: paolo.bonzini, krh, git, Björn Steinbrink
The checks that looked for paths given to git-commit in addition to
--all or --interactive expected only 3 values, while the case statement
actually provides 4, so the check was never triggered.
The bug was introduced in 6cbf07efc5702351897dee4742525c9b9f7828ac when
the case statement was extended to handle --interactive.
Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
git-commit.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-commit.sh b/git-commit.sh
index fcb8443..d4471ff 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -282,9 +282,9 @@ unset only
case "$all,$interactive,$also,$#" in
*t,*t,*)
die "Cannot use -a, --interactive or -i at the same time." ;;
-t,,[1-9]*)
+t,,,[1-9]*)
die "Paths with -a does not make sense." ;;
-,t,[1-9]*)
+,t,,[1-9]*)
die "Paths with --interactive does not make sense." ;;
,,t,0)
die "No paths with -i does not make sense." ;;
--
1.5.3.5.561.g140d-dirty
^ permalink raw reply related
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Steven Grimm @ 2007-11-05 19:28 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <1194289301-7800-1-git-send-email-madcoder@debian.org>
On Nov 5, 2007, at 11:01 AM, Pierre Habouzit wrote:
> When git-revert has a file argument then redirect the user to what he
> probably meant.
That's a big improvement. Basically everyone I show git to gets
"revert" wrong at first.
> + die("Cannot find commit '%s', did you meant: "
> + "git checkout HEAD -- '%s'", arg, arg);
But that suggested command is not going to convince anyone they were
wrong about git being hard to learn. I wonder if instead of saying, "I
know what you meant, but I'm going to make you type a different
command," we should make git revert just do what the user meant.
There is already precedent for that kind of mixed-mode UI:
git checkout my-branch
vs.
git checkout my/source/file.c
-Steve
^ permalink raw reply
* Re: [PATCH 4/4] Implement git commit and status as a builtin commands.
From: Björn Steinbrink @ 2007-11-05 19:23 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: Junio C Hamano, git
In-Reply-To: <1194289073.13968.16.camel@hinata.boston.redhat.com>
On 2007.11.05 13:57:53 -0500, Kristian Høgsberg wrote:
> On Sat, 2007-11-03 at 16:06 +0100, Björn Steinbrink wrote:
> > On 2007.11.02 11:33:09 -0400, Kristian Høgsberg wrote:
> > > + if (all && interactive)
> > > + die("Cannot use -a, --interactive or -i at the same time.");
> >
> > Shouldn't that be "if (all && (interactive || also))"?
Ah, damn. I (obviously) misread the error message as:
foo and (bar or bar2)
IOW "and" instead of the first comma.
Actually it should be:
if ((all && (interactive || also)) || (interactive && also))
(or whatever more simple version you come up with)
> The shell script just has
>
> case "$all,$interactive,$also,$#" in
> *t,*t,*)
> die "Cannot use -a, --interactive or -i at the same time." ;;
>
> which doesn't seem to care about the value of $also. As far as I
> understand git commit, it doesn't make sense to pass any of -a, -i, -o
> or --interactive at the same time so I guess I could join the checks
Note that there are only two commas. The asterisks catch everything and
$# won't be "t", so that catches anything with at least two t's.
Also note, that the two checks after that one in git-commit.sh as of the
current master (140dd77a5cb2e61dcb942e245a2474fae95e42a5) are broken
(I'll send a patch in a separate mail).
> if (also && only)
> die("Only one of --include/--only can be used.");
> if (all && interactive)
> die("Cannot use -a, --interactive or -i at the same
> time.");
>
> into something like
>
> if (also + only + all + interactive > 1)
> die("Only one of --include/--only/--all/--interactive can be used.");
>
> Does that sound right?
--include is not in the manpage, and I only glanced over git-commit.sh,
but yeah, at least to me, that sounds right.
Björn
^ permalink raw reply
* Re: [PATCH] Use parseopts in builtin-fetch
From: Junio C Hamano @ 2007-11-05 19:13 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Daniel Barkalow, git
In-Reply-To: <20071105085513.GB25574@artemis.corp>
Pierre Habouzit <madcoder@debian.org> writes:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
>> I mostly did this and the next one for practice with the API. I'm
>> impressed that "git fetch -vv" is even handled correctly without anything
>> special.
>
> About that: OPTION_BOOLEAN increments the associated variable, to
> support this case specifically.
>
> The last thing that really miss in parse-options is a way to recurse
> into a sub-array of struct option, to be able to port the generic diff
> and revision arguments.
Another micronit is I found lacking is that it is a bit too
cumbersome to accept only a subset of integer as a value
(e.g. "this option takes a positive integer, not zero nor
negative"). The caller can set up a callback to handle that,
though.
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Pierre Habouzit @ 2007-11-05 19:10 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: gitster, git
In-Reply-To: <20071105190556.GG22767@fieldses.org>
[-- Attachment #1: Type: text/plain, Size: 726 bytes --]
On Mon, Nov 05, 2007 at 07:05:56PM +0000, J. Bruce Fields wrote:
> On Mon, Nov 05, 2007 at 08:04:11PM +0100, Pierre Habouzit wrote:
> > On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:
> > > + if (!lstat(name, &st)) {
> > > + die("Cannot find commit '%s', did you meant: "
>
> s/meant/mean/
Yeah I wrote that out of the iritation of the 192812948th question about
that on #git, I should have read my patch it seems :)
Though, if there will be a new maint release, I do believe that this
patch is maint material.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: J. Bruce Fields @ 2007-11-05 19:05 UTC (permalink / raw)
To: Pierre Habouzit, gitster, git
In-Reply-To: <20071105190411.GG6205@artemis.corp>
On Mon, Nov 05, 2007 at 08:04:11PM +0100, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:
> > + if (!lstat(name, &st)) {
> > + die("Cannot find commit '%s', did you meant: "
s/meant/mean/
> > + "git checkout HEAD -- '%s'", arg, arg);
> > + } else {
> > + die("Cannot find commit '%s'", arg);
> > + }
> > + }
--b.
^ permalink raw reply
* Re: [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Pierre Habouzit @ 2007-11-05 19:04 UTC (permalink / raw)
To: gitster; +Cc: git
In-Reply-To: <1194289301-7800-1-git-send-email-madcoder@debian.org>
[-- Attachment #1: Type: text/plain, Size: 3461 bytes --]
On Mon, Nov 05, 2007 at 07:01:41PM +0000, Pierre Habouzit wrote:
> When git-revert has a file argument then redirect the user to what he
> probably meant.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> builtin-revert.c | 24 +++++++++++++++++-------
> gitk | 2 +-
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/builtin-revert.c b/builtin-revert.c
> index 62ab1fa..9660048 100644
> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -38,7 +38,7 @@ static const char *me;
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> -static void parse_args(int argc, const char **argv)
> +static void parse_args(int argc, const char **argv, const char *prefix)
> {
> const char * const * usage_str =
> action == REVERT ? revert_usage : cherry_pick_usage;
> @@ -58,8 +58,18 @@ static void parse_args(int argc, const char **argv)
> usage_with_options(usage_str, options);
> arg = argv[0];
>
> - if (get_sha1(arg, sha1))
> - die ("Cannot find '%s'", arg);
> + if (get_sha1(arg, sha1)) {
> + struct stat st;
> + const char *name;
> +
> + name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
> + if (!lstat(name, &st)) {
> + die("Cannot find commit '%s', did you meant: "
> + "git checkout HEAD -- '%s'", arg, arg);
> + } else {
> + die("Cannot find commit '%s'", arg);
> + }
> + }
> commit = (struct commit *)parse_object(sha1);
> if (!commit)
> die ("Could not find %s", sha1_to_hex(sha1));
> @@ -225,7 +235,7 @@ static int merge_recursive(const char *base_sha1,
> return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
> }
>
> -static int revert_or_cherry_pick(int argc, const char **argv)
> +static int revert_or_cherry_pick(int argc, const char **argv, const char *prefix)
> {
> unsigned char head[20];
> struct commit *base, *next, *parent;
> @@ -237,7 +247,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
> git_config(git_default_config);
> me = action == REVERT ? "revert" : "cherry-pick";
> setenv(GIT_REFLOG_ACTION, me, 0);
> - parse_args(argc, argv);
> + parse_args(argc, argv, prefix);
>
> /* this is copied from the shell script, but it's never triggered... */
> if (action == REVERT && !no_replay)
> @@ -405,12 +415,12 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
> edit = 1;
> no_replay = 1;
> action = REVERT;
> - return revert_or_cherry_pick(argc, argv);
> + return revert_or_cherry_pick(argc, argv, prefix);
> }
>
> int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
> {
> no_replay = 0;
> action = CHERRY_PICK;
> - return revert_or_cherry_pick(argc, argv);
> + return revert_or_cherry_pick(argc, argv, prefix);
> }
> diff --git a/gitk b/gitk
> index 1da0b0a..ab8bab2 100755
> --- a/gitk
> +++ b/gitk
> @@ -1,6 +1,6 @@
> #!/bin/sh
> # Tcl ignores the next line -*- tcl -*- \
> -exec wish "$0" -- "$@"
> +exec wish8.5 "$0" -- "$@"
>
> # Copyright (C) 2005-2006 Paul Mackerras. All rights reserved.
> # This program is free software; it may be used, copied, modified
> --
> 1.5.3.5.1541.gd2b5c-dirty
F*CK this chunk is obviously spurious.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] git-revert is one of the most misunderstood command in git, help users out.
From: Pierre Habouzit @ 2007-11-05 19:01 UTC (permalink / raw)
To: gitster, Junio C Hamano; +Cc: git, Pierre Habouzit
When git-revert has a file argument then redirect the user to what he
probably meant.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
builtin-revert.c | 24 +++++++++++++++++-------
gitk | 2 +-
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/builtin-revert.c b/builtin-revert.c
index 62ab1fa..9660048 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -38,7 +38,7 @@ static const char *me;
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, const char *prefix)
{
const char * const * usage_str =
action == REVERT ? revert_usage : cherry_pick_usage;
@@ -58,8 +58,18 @@ static void parse_args(int argc, const char **argv)
usage_with_options(usage_str, options);
arg = argv[0];
- if (get_sha1(arg, sha1))
- die ("Cannot find '%s'", arg);
+ if (get_sha1(arg, sha1)) {
+ struct stat st;
+ const char *name;
+
+ name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
+ if (!lstat(name, &st)) {
+ die("Cannot find commit '%s', did you meant: "
+ "git checkout HEAD -- '%s'", arg, arg);
+ } else {
+ die("Cannot find commit '%s'", arg);
+ }
+ }
commit = (struct commit *)parse_object(sha1);
if (!commit)
die ("Could not find %s", sha1_to_hex(sha1));
@@ -225,7 +235,7 @@ static int merge_recursive(const char *base_sha1,
return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
}
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv, const char *prefix)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -237,7 +247,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
git_config(git_default_config);
me = action == REVERT ? "revert" : "cherry-pick";
setenv(GIT_REFLOG_ACTION, me, 0);
- parse_args(argc, argv);
+ parse_args(argc, argv, prefix);
/* this is copied from the shell script, but it's never triggered... */
if (action == REVERT && !no_replay)
@@ -405,12 +415,12 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
edit = 1;
no_replay = 1;
action = REVERT;
- return revert_or_cherry_pick(argc, argv);
+ return revert_or_cherry_pick(argc, argv, prefix);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
no_replay = 0;
action = CHERRY_PICK;
- return revert_or_cherry_pick(argc, argv);
+ return revert_or_cherry_pick(argc, argv, prefix);
}
diff --git a/gitk b/gitk
index 1da0b0a..ab8bab2 100755
--- a/gitk
+++ b/gitk
@@ -1,6 +1,6 @@
#!/bin/sh
# Tcl ignores the next line -*- tcl -*- \
-exec wish "$0" -- "$@"
+exec wish8.5 "$0" -- "$@"
# Copyright (C) 2005-2006 Paul Mackerras. All rights reserved.
# This program is free software; it may be used, copied, modified
--
1.5.3.5.1541.gd2b5c-dirty
^ permalink raw reply related
* Re: [PATCH 4/4] Implement git commit and status as a builtin commands.
From: Kristian Høgsberg @ 2007-11-05 18:57 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: Junio C Hamano, git
In-Reply-To: <20071103150637.GA11172@atjola.homenet>
On Sat, 2007-11-03 at 16:06 +0100, Björn Steinbrink wrote:
> On 2007.11.02 11:33:09 -0400, Kristian Høgsberg wrote:
> > + if (all && interactive)
> > + die("Cannot use -a, --interactive or -i at the same time.");
>
> Shouldn't that be "if (all && (interactive || also))"?
The shell script just has
case "$all,$interactive,$also,$#" in
*t,*t,*)
die "Cannot use -a, --interactive or -i at the same time." ;;
which doesn't seem to care about the value of $also. As far as I
understand git commit, it doesn't make sense to pass any of -a, -i, -o
or --interactive at the same time so I guess I could join the checks
if (also && only)
die("Only one of --include/--only can be used.");
if (all && interactive)
die("Cannot use -a, --interactive or -i at the same
time.");
into something like
if (also + only + all + interactive > 1)
die("Only one of --include/--only/--all/--interactive can be used.");
Does that sound right?
Kristian
^ permalink raw reply
* Re: [bug in next ?] git-fetch/git-push issue
From: Pierre Habouzit @ 2007-11-05 18:19 UTC (permalink / raw)
To: Nicolas Pitre, Daniel Barkalow, Jeff King, Git ML
In-Reply-To: <20071105175654.GD6205@artemis.corp>
[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]
On Mon, Nov 05, 2007 at 05:56:54PM +0000, Pierre Habouzit wrote:
> With the current tip of next[0], I have this bizare issue:
>
> * I have two branches say master, and next, I'm on next.
>
> * my master lags behind origin/master, but next is a fast-forward wrt
> origin/next.
>
> Now I git push:
>
> ┌─(18:16)──<~/some/repo next>──
> └[artemis] git push
> error: remote 'refs/heads/master' is not an ancestor of
> local 'refs/heads/master'.
> Maybe you are not up-to-date and need to pull first?
> updating 'refs/heads/next'
> from xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> to yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> Counting objects: 24, done.
> Compressing objects: 100% (14/14), done.
> Writing objects: 100% (14/14), done.
> Total 14 (delta 12), reused 0 (delta 0)
> refs/heads/next: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -> yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> updating local tracking ref 'refs/remotes/origin/master'
> updating local tracking ref 'refs/remotes/origin/next'
[...]
> I believe there is something rotten in the kingdom of Denmark… though
> my heads seems to always be OK, I think it's just an output issue.
Okay I'm wrong. it happened again, in fact after the git-push I don't
have a origin/master anymore.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [bug in next ?] git-fetch/git-push issue
From: Daniel Barkalow @ 2007-11-05 18:17 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Nicolas Pitre, Jeff King, Git ML
In-Reply-To: <20071105175654.GD6205@artemis.corp>
On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> With the current tip of next[0], I have this bizare issue:
>
> * I have two branches say master, and next, I'm on next.
>
> * my master lags behind origin/master, but next is a fast-forward wrt
> origin/next.
>
> Now I git push:
>
> error: remote 'refs/heads/master' is not an ancestor of
> local 'refs/heads/master'.
> Maybe you are not up-to-date and need to pull first?
> updating 'refs/heads/next'
> from xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> to yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> Counting objects: 24, done.
> Compressing objects: 100% (14/14), done.
> Writing objects: 100% (14/14), done.
> Total 14 (delta 12), reused 0 (delta 0)
> refs/heads/next: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -> yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> updating local tracking ref 'refs/remotes/origin/master'
I think this is the bit that's wrong. I blame Jeff, in 334f4831. :)
The issue is that, in the previous version, we'd hit a continue on the
not-an-ancestor message and not reach the update_tracking_ref() section
for that ref. In 334f4831, all of the updating is after the loop, and it
doesn't filter out the refs that didn't actually get pushed.
Probably, all of the "continue;"s in do_send_pack() should also strip off
the peer_ref. Something like (totally untested):
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 947c42b..6141672 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -254,12 +254,16 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
if (will_delete_ref && !allow_deleting_refs) {
error("remote does not support deleting refs");
ret = -2;
+ free(ref->peer_ref);
+ ref->peer_ref = NULL;
continue;
}
if (!will_delete_ref &&
!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
if (args.verbose)
fprintf(stderr, "'%s': up-to-date\n", ref->name);
+ free(ref->peer_ref);
+ ref->peer_ref = NULL;
continue;
}
@@ -303,6 +307,8 @@ static int do_send_pack(int in, int out, struct remote *remote, int nr_refspec,
ref->name,
ref->peer_ref->name);
ret = -2;
+ free(ref->peer_ref);
+ ref->peer_ref = NULL;
continue;
}
}
^ permalink raw reply related
* [bug in next ?] git-fetch/git-push issue
From: Pierre Habouzit @ 2007-11-05 17:56 UTC (permalink / raw)
To: Nicolas Pitre, Daniel Barkalow, Jeff King; +Cc: Git ML
[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]
With the current tip of next[0], I have this bizare issue:
* I have two branches say master, and next, I'm on next.
* my master lags behind origin/master, but next is a fast-forward wrt
origin/next.
Now I git push:
┌─(18:16)──<~/some/repo next>──
└[artemis] git push
error: remote 'refs/heads/master' is not an ancestor of
local 'refs/heads/master'.
Maybe you are not up-to-date and need to pull first?
updating 'refs/heads/next'
from xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
to yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
Counting objects: 24, done.
Compressing objects: 100% (14/14), done.
Writing objects: 100% (14/14), done.
Total 14 (delta 12), reused 0 (delta 0)
refs/heads/next: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -> yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
updating local tracking ref 'refs/remotes/origin/master'
updating local tracking ref 'refs/remotes/origin/next'
And then I fetch, and here happens something really awkward:
┌─(18:17)──<~/some/repo next>──
└[artemis] git fetch
From ssh://some.host/some/repo.git
* [new branch] master -> origin/master
I believe there is something rotten in the kingdom of Denmark… though
my heads seems to always be OK, I think it's just an output issue.
The fun part is that if next has nothing to push, nothing happens:
┌─(18:55)──<~/dev/mmsx next>──
└[artemis] git push
error: remote 'refs/heads/master' is not an ancestor of
local 'refs/heads/master'.
Maybe you are not up-to-date and need to pull first?
error: failed to push to 'ssh://some.host/some/repo.git'
┌─(18:55)──<~/dev/mmsx next>──
└[artemis] git fetch && echo $?
0
[0] actually it's a bit farther than the current next, but for
parseopt thingies that are irrelevant here. My current origin/next
is 76374a65c41b80fa83f27b4dd924bd3967a07d69
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Use parseopts in builtin-fetch
From: Daniel Barkalow @ 2007-11-05 17:41 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <20071105085513.GB25574@artemis.corp>
On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> > I mostly did this and the next one for practice with the API. I'm
> > impressed that "git fetch -vv" is even handled correctly without anything
> > special.
>
> About that: OPTION_BOOLEAN increments the associated variable, to
> support this case specifically.
>
> The last thing that really miss in parse-options is a way to recurse
> into a sub-array of struct option, to be able to port the generic diff
> and revision arguments.
>
> Though, there is a difficulty here that I've not yet found how to
> circumvent tastefully: right now options take an absolute pointer to
> _the_ variable that will be filled with values. I need to be able to
> relocate such a structure for sub-arrays for quite obvious reasons, and
> that is quite hard to achieve without hazardous APIs. I currently lean
> in the direction of simply memdup-ing the array and do fix-ups on
> *values pointers. Though how to do that in a graceful way is not obvious
> to me yet :)
I'm not entirely clear as to why the "diff options" to an arbitrary
command can't configure variables in the diff engine. The tricky thing
would be supporting a single command that has two sets of the same
options (if git log could show the same stuff in two different ways at the
same time, for example).
But, in any case, it should be possible for an OPT_* macro to take a
struct type and a member name, do the offsetof and store that, and the
inclusion macro would take the absolute pointer to the struct.
Next time I get a chance, I'll see if I can come up with something
suitable.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* [PATCH] t7501-commit.sh: Add test case for fixing author in amend commit.
From: Kristian Høgsberg @ 2007-11-05 17:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git, Kristian Høgsberg
In-Reply-To: <Pine.LNX.4.64.0711051020330.4362@racer.site>
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
How about this?
t/t7501-commit.sh | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index b151b51..c5d122f 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -163,4 +163,19 @@ test_expect_success 'partial commit that involves removal (3)' '
'
+author="The Real Author <someguy@his.email.org>"
+test_expect_success 'amend commit to fix author' '
+
+ oldtick=$GIT_AUTHOR_DATE &&
+ test_tick &&
+ git reset --hard &&
+ git cat-file -p HEAD |
+ sed -e "s/author.*/author $author $oldtick/" \
+ -e "s/^\(committer.*> \).*$/\1$GIT_COMMITTER_DATE/" > \
+ expected &&
+ git commit --amend --author="$author" &&
+ git cat-file -p HEAD > current &&
+ diff expected current
+
+'
test_done
--
1.5.3.4
^ permalink raw reply related
* Re: [PATCH] Use parseopts in builtin-fetch
From: Daniel Barkalow @ 2007-11-05 17:14 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, git
In-Reply-To: <20071105084333.GA25574@artemis.corp>
On Mon, 5 Nov 2007, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > I mostly did this and the next one for practice with the API. I'm
> > impressed that "git fetch -vv" is even handled correctly without anything
> > special. Now that I've done it, assuming I did it right, it might as well
> > get added to the series.
>
> I believe the same patches (or very similar ones) are in pu but are
> not in next yet because they conflict with the builtin-fetch recent
> series.
>
> see http://git.madism.org/?p=git.git;a=blobdiff;f=builtin-fetch.c;h=12b1c4;hp=6b1750d;hb=7407915;hpb=61610e6
Ah, okay, forgot to look there. In any case, I was mostly looking for what
mistakes I shouldn't make in future conversions.
> > + OPT_BOOLEAN('q', "quiet", &quiet, "fetch silently"),
>
> there is an OPT__QUIET(&quiet) for this one.
>
> > + i = 1;
> > if (i < argc) {
> > int j = 0;
> > refs = xcalloc(argc - i + 1, sizeof(const char *));
>
> this is wrong, you meant i = 0, and frankly, it's better to just strip
> i altogether.
I didn't consume the remote name's slot, and started at the next one. But
you're right that it's probably nicest to do to *argv++, argc-- thing and
be zero-based for the list afterwards. I think I need the 'i' in this
case, because the names get somewhat converted from the exact list given.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH] Use parseopts in builtin-fetch
From: Kristian Høgsberg @ 2007-11-05 17:02 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Daniel Barkalow, Junio C Hamano, git
In-Reply-To: <20071105085513.GB25574@artemis.corp>
On Mon, 2007-11-05 at 09:55 +0100, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:
> > I mostly did this and the next one for practice with the API. I'm
> > impressed that "git fetch -vv" is even handled correctly without anything
> > special.
>
> About that: OPTION_BOOLEAN increments the associated variable, to
> support this case specifically.
>
> The last thing that really miss in parse-options is a way to recurse
> into a sub-array of struct option, to be able to port the generic diff
> and revision arguments.
>
> Though, there is a difficulty here that I've not yet found how to
> circumvent tastefully: right now options take an absolute pointer to
> _the_ variable that will be filled with values. I need to be able to
> relocate such a structure for sub-arrays for quite obvious reasons, and
> that is quite hard to achieve without hazardous APIs. I currently lean
> in the direction of simply memdup-ing the array and do fix-ups on
> *values pointers. Though how to do that in a graceful way is not obvious
> to me yet :)
What about requiring sub-arrays entries to take a pointer to a struct as
their 'value' and requiring that all value pointers in the sub-array be
relative to this struct, ie
(void *) offsetof(struct diff_options, detect_rename)
Then you can have something like
OPT__SUBARRAY(diff_options, &rev.diff_opt);
in your options array and it will fill out the specified my_diff_opts.
cheers,
Kristian
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Pierre Habouzit @ 2007-11-05 16:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Linus Torvalds, git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0711051623450.4362@racer.site>
[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]
On Mon, Nov 05, 2007 at 04:29:43PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 5 Nov 2007, Linus Torvalds wrote:
>
> > On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> > >
> > > After kicking this around a bit more on IRC, we had another idea.
> > > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(),
> > > only this time in diff.h: ....
> >
> > I think the preprocessor approach would tend to be simpler, which is an
> > advantage. But whichever approach is chosen, I think one important issue
> > is to make sure that options that *hide* other options are correctly
> > handled in the help printout..
>
> Yep. See my patch 3/3, which just used a char[256] for the short names,
> and a path-list for the long names.
>
> > But that's an implementation issue. The same certainly *can* be done
> > with a recursive setup, just passing a linked list of what the earlier
> > levels were (which is what we do in other places). And it's not like the
> > recursion is going to be very deep or complex.
>
> Exactly.
>
> The more pressing issue is that we have pointers in the option structure,
> which point back to the variables expected to hold the option values.
>
> The recurse approach would need fixing up those (or some ugly copying of
> a struct diff_options).
>
> But the preprocessor approach means wasting space (since we basically have
> the same options in different builtins),
The "lost" space is the number of options x sizeof(struct option), the
latter being (if I'm correct):
on i386: 9 * 4 = 36 octets
on amd64: 4 x 2 + 8 * 4 + 8 (padding) + 8 * 2 = 64 octets.
It's not even near being an issue :)
> and it means that the callback
> functions needed to parse e.g. the diff colour names need to be public.
> Which is not the worst thing, of course.
Well it's certainly less ugly than copying the diff_options or
reseting it or anything like that. I don't care if we need to make a
couple of opt-parsing function public more than what we could have
needed.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] git-svn: fix dcommit clobbering when committing a series of diffs
From: Eric Wong @ 2007-11-05 16:40 UTC (permalink / raw)
To: Peter Baumann; +Cc: Junio C Hamano, git
In-Reply-To: <20071105142254.GA20277@xp.machine.xx>
Peter Baumann <waste.manager@gmx.de> wrote:
> On Mon, Nov 05, 2007 at 03:21:47AM -0800, Eric Wong wrote:
> > +
> > +test_expect_success 'unrelated some unrelated changes to git' "
>
> The first unrelated seems odd here.
Oops, must have been a brain glitch. Good catch, thanks.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Johannes Schindelin @ 2007-11-05 16:29 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, Junio C Hamano, Pierre Habouzit
In-Reply-To: <alpine.LFD.0.999.0711050755340.15101@woody.linux-foundation.org>
Hi,
On Mon, 5 Nov 2007, Linus Torvalds wrote:
> On Mon, 5 Nov 2007, Johannes Schindelin wrote:
> >
> > After kicking this around a bit more on IRC, we had another idea.
> > Instead of introducing OPT_RECURSE(), do something like OPT__QUIET(),
> > only this time in diff.h: ....
>
> I think the preprocessor approach would tend to be simpler, which is an
> advantage. But whichever approach is chosen, I think one important issue
> is to make sure that options that *hide* other options are correctly
> handled in the help printout..
Yep. See my patch 3/3, which just used a char[256] for the short names,
and a path-list for the long names.
> But that's an implementation issue. The same certainly *can* be done
> with a recursive setup, just passing a linked list of what the earlier
> levels were (which is what we do in other places). And it's not like the
> recursion is going to be very deep or complex.
Exactly.
The more pressing issue is that we have pointers in the option structure,
which point back to the variables expected to hold the option values.
The recurse approach would need fixing up those (or some ugly copying of
a struct diff_options).
But the preprocessor approach means wasting space (since we basically have
the same options in different builtins), and it means that the callback
functions needed to parse e.g. the diff colour names need to be public.
Which is not the worst thing, of course.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] parseopt: introduce OPT_RECURSE to specify shared options
From: Linus Torvalds @ 2007-11-05 16:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Pierre Habouzit
In-Reply-To: <Pine.LNX.4.64.0711051340490.4362@racer.site>
On Mon, 5 Nov 2007, Johannes Schindelin wrote:
>
> After kicking this around a bit more on IRC, we had another idea. Instead
> of introducing OPT_RECURSE(), do something like OPT__QUIET(), only this
> time in diff.h: ....
I think the preprocessor approach would tend to be simpler, which is an
advantage. But whichever approach is chosen, I think one important issue
is to make sure that options that *hide* other options are correctly
handled in the help printout..
We have a few cases where a "recursive" option is hidden. For example, the
option "-n" can mean different things in different contexts: in
git-format-patch, for example, the "-n" is handled *before* calling
setup_revisions() and allt he normal revision flags, which means that the
format-patch -specific meaning of "-n" overrides the normal "revision"
meaning.
I suspect that in this kind of situation, it's actually easier to have a
single linear array of options, because the option parser can just walk
back and check "oh, I already saw this short option, so I should not
output it as documentation because this version of it is hidden by the
earlier one".
But that's an implementation issue. The same certainly *can* be done with
a recursive setup, just passing a linked list of what the earlier levels
were (which is what we do in other places). And it's not like the
recursion is going to be very deep or complex.
Linus
^ permalink raw reply
* Re: [PATCH] Implement selectable group ownership in git-init
From: Wincent Colaiuta @ 2007-11-05 15:26 UTC (permalink / raw)
To: Francesco Pretto; +Cc: Junio C Hamano, git
In-Reply-To: <472F3212.4090800@gmail.com>
El 5/11/2007, a las 16:09, Francesco Pretto escribió:
> Wincent Colaiuta ha scritto:
>>
>> using the administration tools designed for managing access, just
>> like
>> every other SCM (and every server, and every piece of software
>> which can
>> be accessed by many on a multi-user system).
>>
>
> I don't agree in general: in SCMs and other multi-user softwares,
> the access
> control configuration can be safely postponed just because it's in
> their
> standard usage pattern that the access should be conditioned by a
> daemon
> to be configured later. It's not the case of git, just because git
> is very
> tied to *nix permissions.
> But as it is now, it could seems that it's good to put committers in
> the (for
> example) git group, just because you have a git administrative account
> git:git . This is caused, imo, by the fact that the flow of creating
> a shared
> repository for a specific work/project group with git-init run by an
> administrative user (as it should be) is something like this:
>
> - Do it wrong;
> - Fix it immediately.
>
> I don't like the "Do it wrong" part. I'm trying to produce a sane and
> transparent patch to implement the selectable group just in case of
> repository
> first initialization. Why do I care so much of first time users?
> Dunno, but
> I think it's important.
What's stopping you from using "sudo -u"?
Wincent
^ permalink raw reply
* Re: [PATCH] Implement selectable group ownership in git-init
From: Francesco Pretto @ 2007-11-05 15:09 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Junio C Hamano, git
In-Reply-To: <8EF5148D-C1F0-4329-A221-82D0B7E9932C@wincent.com>
Wincent Colaiuta ha scritto:
>
> using the administration tools designed for managing access, just like
> every other SCM (and every server, and every piece of software which can
> be accessed by many on a multi-user system).
>
I don't agree in general: in SCMs and other multi-user softwares, the access
control configuration can be safely postponed just because it's in their
standard usage pattern that the access should be conditioned by a daemon
to be configured later. It's not the case of git, just because git is very
tied to *nix permissions.
But as it is now, it could seems that it's good to put committers in the (for
example) git group, just because you have a git administrative account
git:git . This is caused, imo, by the fact that the flow of creating a shared
repository for a specific work/project group with git-init run by an
administrative user (as it should be) is something like this:
- Do it wrong;
- Fix it immediately.
I don't like the "Do it wrong" part. I'm trying to produce a sane and
transparent patch to implement the selectable group just in case of repository
first initialization. Why do I care so much of first time users? Dunno, but
I think it's important.
^ 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