* [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: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
* [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: [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
* 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: [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
* [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] 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
* 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: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] 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 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] 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
* [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] 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
* 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 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] upload-pack: Use finish_{command,async}() instead of waitpid().
From: Junio C Hamano @ 2007-11-05 20:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <200711042046.48257.johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
> - If pack-objects sees an error, it terminates with failure. Since this
> breaks the pipe to rev-list, rev-list is killed with SIGPIPE.
I was a bit uneasy about this part. We do not explicitly tell
rev-list not to ignore SIGPIPE, so it could spin fruitlessly
listing revs and calling write(2). But the error from that
write should already be handled correctly anyway, so this should
be Ok.
> The test case checks for failures in rev-list (a missing
> object). Any hints how to trigger a failure in pack-objects
> that does not also trigger in rev-list would be welcome.
How about removing a blob from the test repository to corrupt
it? rev-list --objects I think would happily list the blob
because it sees its name in its containing tree without checking
its existence.
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: René Scharfe @ 2007-11-05 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7v8x5cqxn0.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
> 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?
How about (ab)using the value field? Let interp_find_active() mark
unneeded entries with NULL, and the rest with some cookie. All table
entries with non-NULL values need to be initialized. interp_set_entry()
needs to be aware of this cookie, as it mustn't free() it. The cookie
could be the address of a static char* in interpolate.c.
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Junio C Hamano @ 2007-11-05 20:24 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <alpine.LFD.0.999.0711041124050.15101@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> It wasn't totally trivial, but it doesn't seem to be excessively subtle
> either. About half the patch is moving around some code to look at whether
> the commit is interesting or not and rewriting the parents, so that it can
> be shared with the revision walker.
Very nicely done.
> + while (list) {
> + struct commit *commit = list->item;
> + unsigned int flags = commit->object.flags;
> +
> + list = list->next;
> + if (flags & UNINTERESTING)
> + continue;
> + if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> + if (commit->parents && !commit->parents->next)
> + continue;
> + }
When looking at:
if (A && B && C) {
if (D && E)
continue;
}
an uninitiated might say "Huh? Why use nested 'if'?", but to
somebody who knows how revision traversal works, the above split
is a more logical way to test this condition. Maybe one liner
comment is in order?
> +static void show_early_header(struct rev_info *rev, const char *stage, int nr)
> +{
> + if (rev->shown_one) {
> + rev->shown_one = 0;
> + if (rev->commit_format != CMIT_FMT_ONELINE)
> + putchar(rev->diffopt.line_termination);
> + }
> + printf("Final output: %d %s\n", nr, stage);
> +}
As you noted, this is more like "Partial output" now.
How about painting the bikeshed pink by saying:
Partial output: 20
Partial output: 70
Final output: 70000
> + /* Did we already get enough commits for the early output? */
> + if (!i)
> + return;
> +
> + /*
> + * ..if no, then repeat it twice a second until we
> + * do.
> + *
> + * NOTE! We don't use "it_interval", because if the
> + * reader isn't listening, we want our output to be
> + * throttled by the writing, and not have the timer
> + * trigger every second even if we're blocked on a
> + * reader!
> + */
A comment like this is very much appreciated.
> + early_output_timer.it_value.tv_sec = 0;
> + early_output_timer.it_value.tv_usec = 500000;
> + setitimer(ITIMER_REAL, &early_output_timer, NULL);
> }
^ permalink raw reply
* Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed
From: Jon Loeliger @ 2007-11-05 20:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Johannes Schindelin, Git List
In-Reply-To: <472F7B2F.4050608@lsrfire.ath.cx>
On Mon, 2007-11-05 at 14:21, René Scharfe wrote:
> How about (ab)using the value field? Let interp_find_active() mark
> unneeded entries with NULL, and the rest with some cookie. All table
> entries with non-NULL values need to be initialized. interp_set_entry()
> needs to be aware of this cookie, as it mustn't free() it. The cookie
> could be the address of a static char* in interpolate.c.
Or adding a "flags/properties" field and having it
note things like static entries, active, etc...?
jdl
^ permalink raw reply
* Re: [PATCH 3/2] Enhance --early-output format
From: Linus Torvalds @ 2007-11-05 20:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Paul Mackerras, Marco Costalba, Git Mailing List
In-Reply-To: <7vsl3kphjp.fsf@gitster.siamese.dyndns.org>
On Mon, 5 Nov 2007, Junio C Hamano wrote:
>
> > + if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> > + if (commit->parents && !commit->parents->next)
> > + continue;
> > + }
>
> When looking at:
>
> if (A && B && C) {
> if (D && E)
> continue;
> }
>
> an uninitiated might say "Huh? Why use nested 'if'?", but to
> somebody who knows how revision traversal works, the above split
> is a more logical way to test this condition. Maybe one liner
> comment is in order?
I'd almost prefer not to.
If people feel the code is subtle enough that a comment is in order to
explain *what* something does, I would rather introduce helper functions
than add comments. I'm not a big believer in comments in the middle of
code to explain the code, but I *am* a big believer in trying to make the
code easy to read without them.
(I don't dislike comments per se, but I'd *much* rather have the comments
explain what is going on at a higher level. Comments that talk about the
details of the code itself is likely bogus).
So we could add a commit to say what is going on ("ignore commits that
have only one parent and didn't change the tree"), but I'd not want to
explain why that particular layout of code.
That said, I've often found the "TREECHANGE" bit annoying. The fact that
we always have to test it together with testing "rev->prune_fn" and often
also the "rev->dense" flag is just annoying. I'd almost like to just
always set the bit if "rev->prune_fn" isn't set. Alternatively, the code
could be rewritten to just have a few inline helper functions, and it
could perhaps be written as
if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
continue;
I dunno. I have no really *strong* opinions, although I suspect that
anybody who looks at that particular piece of code had better understand
these issues well enough anyway that it doesn't much matter..
> As you noted, this is more like "Partial output" now.
> How about painting the bikeshed pink by saying:
>
> Partial output: 20
> Partial output: 70
> Final output: 70000
I'm fine with it. The reason I did it the way I did was that this way I
didn't need to change "gitk" - I could just use Pauls original patch
as-is, since that code didn't care *what* came after the "Final output",
and didn't care how many times it showed up.
Linus
^ permalink raw reply
* Re: [bug in next ?] git-fetch/git-push issue
From: Jeff King @ 2007-11-05 21:07 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Pierre Habouzit, Nicolas Pitre, Git ML
In-Reply-To: <Pine.LNX.4.64.0711051259580.7357@iabervon.org>
On Mon, Nov 05, 2007 at 01:17:14PM -0500, Daniel Barkalow wrote:
> 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.
Nope, that's not the problem. We _only_ update any tracking refs at all
if ret == 0, and if we fail to push, then we are setting ret to -2.
Hrm. Oh wait, it looks like we then totally write over the current value
of 'ret' when we do pack_objects. Oops.
I'm unclear how to fix this, as I'm not really sure what ret is
_supposed_ to be communicating. What does the '-2' mean, as compared to
a '-4'? Should we be doing a 'ret += pack_objects(out, remote_refs)' or
some other bit-masking magic?
-Peff
^ permalink raw reply
* Re: [PATCH] Make git-clean a builtin
From: Junio C Hamano @ 2007-11-05 21:14 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: git
In-Reply-To: <11942029474058-git-send-email-shawn.bohrer@gmail.com>
Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to
> the examples.
>
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
> ---
> diff --git a/builtin-clean.c b/builtin-clean.c
> new file mode 100644
> index 0000000..4141eb4
> --- /dev/null
> +++ b/builtin-clean.c
> @@ -0,0 +1,157 @@
> +/*
> + * "git clean" builtin command
> + *
> + * Copyright (C) 2007 Shawn Bohrer
> + *
> + * Based on git-clean.sh by Pavel Roskin
> + */
> +
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +
> +static int disabled = 1;
This means we are committed to make clean.requireForce default
to true, which is fine by me. I need to warn the users about
this early.
> +static int show_only = 0;
> +static int remove_directories = 0;
> +static int quiet = 0;
> +static int ignored = 0;
> +static int ignored_only = 0;
Please do not explicitly initialize static variables to zero.
^ permalink raw reply
* Re: [PATCH 0/3] more terse push output
From: Junio C Hamano @ 2007-11-05 21:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Nicolas Pitre, Johannes Schindelin
In-Reply-To: <20071105050517.GA6244@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> - the 'ref is not up-to-date, maybe you need to push' message has gone
> away in favor of the terse '[rejected] ... (non-fast forward)'. I
> know there was some discussion recently of enhancing that message.
> Is this perhaps too terse?
I kind of like this part.
I also like the part that stops mentioning the "pretend fetching
back" action. This would mostly parrot the same set of refs for
people who do use the tracking branches anyway.
^ 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