* Re: [PATCH] Don't dereference NULL upon lookup_tree failure.
From: Jim Meyering @ 2007-12-22 0:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list, Matthew Farrellee
In-Reply-To: <7vve6r39bp.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> Sorry, I asked for a wrong thing. parse_object() reads and
>>> finds out the type, so type there is presumably the right type
>>> of the object (which is OBJ_TREE). Then parse_object_buffer()
>>> checks if it has already seen the object of the same SHA-1 and
>>> finds that somebody had earlier told that SHA-1 name is of a
>>> commit object. Either you found a SHA-1 collision (highly
>>> unlikely) or the earlier caller had lied. And I think what
>>> really needs to be fixed is that lying caller. That is not in
>>> the above call chain.
>>
>> I presume that parsecvs is the culprit, in constructing
>> an invalid repository...
>
> Independent of who creates a "invalid repository", our tools
> should be prepared to be fed bad data and not get upset.
> Somebody in our code read a non-commit (or it did not read
> anything) and told the object layer it is a commit. That
> codepath needs to be tightened up, no?
Yes, of course.
Unfortunately, I won't have time to investigate for a while.
^ permalink raw reply
* [PATCH v3] Make git send-email accept $EDITOR with arguments
From: Gustaf Hendeby @ 2007-12-22 0:40 UTC (permalink / raw)
To: gitster; +Cc: peff, luciano, git, Gustaf Hendeby
In-Reply-To: <7vd4sz4uii.fsf@gitster.siamese.dyndns.org>
Currently git send-email does not accept $EDITOR with arguments, eg,
emacs -nw, when starting an editor to produce a cover letter. This
patch changes this by letting the shell handle the option parsing.
Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>
---
This is based on Junio's suggestion on most readable and compatible
solution. I'm not sure if it is identical to the C solution for git
tag, but it seems to be a reasonable solution.
git-send-email.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 248d035..e47994a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -400,7 +400,7 @@ EOT
close(C);
my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
- system($editor, $compose_filename);
+ system('sh', '-c', '$0 $@', $editor, $compose_filename);
open(C2,">",$compose_filename . ".final")
or die "Failed to open $compose_filename.final : " . $!;
--
1.5.4.rc1.16.gc817f
^ permalink raw reply related
* [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: Gustaf Hendeby @ 2007-12-22 0:43 UTC (permalink / raw)
To: gitster; +Cc: luciano, git, Gustaf Hendeby
In-Reply-To: <7vhcic9e17.fsf@gitster.siamese.dyndns.org>
git send-email with the --compose option now puts its temporary file
in $GIT_DIR instead of the current working directory. The file should
be removed on exit, still it is nice not to mess around in the working
directory if not necessary.
Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>
---
git-send-email.perl | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index e47994a..c4eae9d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -150,9 +150,6 @@ my $auth;
sub unique_email_list(@);
sub cleanup_compose_files();
-# Constants (essentially)
-my $compose_filename = ".msg.$$";
-
# Variables we fill in automatically, or via prompting:
my (@to,@cc,@initial_cc,@bcclist,@xh,
$initial_reply_to,$initial_subject,@files,$author,$sender,$compose,$time);
@@ -170,6 +167,11 @@ if ($@) {
$term = new FakeTerm "$@: going non-interactive";
}
+# Constants (essentially)
+my $compose_filename = $repo->command(qw/rev-parse --git-dir/);
+chomp($compose_filename);
+$compose_filename = $compose_filename . "/.msg.$$";
+
# Behavior modification variables
my ($quiet, $dry_run) = (0, 0);
--
1.5.4.rc1.16.gc817f
^ permalink raw reply related
* Re: [PATCH] git-send-email: Add --suppress-all-from option.
From: Theodore Tso @ 2007-12-22 0:55 UTC (permalink / raw)
To: Joel Becker; +Cc: Junio C Hamano, David Brown, git
In-Reply-To: <20071221192120.GA13171@mail.oracle.com>
On Fri, Dec 21, 2007 at 11:21:20AM -0800, Joel Becker wrote:
> Yay, even better that we're going to evaluate the sucker (I was
> just complaining about this yesterday to someone, so how apropos that it
> comes up on-list).
> First and foremost, I think git-send-email should not default to
> anything. It was quite a surprise, the first time I tried to use it, to
> discover I had to add two options to ~/.gitconfig just for sane
> behavior. Never mind that I couldn't suppress the author-cc. I think
> that a naive "git send-email --to bob@bob.com foo.patch" should only go
> to bob, period.
> We can then add ways to auto-cc. I don't mind typing the extra
> bits. Heck, we could even define a --review that does what is currently
> the default - cc-everyone-who-might-care-as-we-go-upstream.
Where e-mail addresses are used by default by git-send-email should
probably be a git configuration option (and then we will need to
document the heck out of this when we change it, so this is probably a
git-1.6.0 thing), so projects can tell all of their contributors if
they need to make changes in their config file for the proper
defaults. Right now the defaults are more or less perfect for the
Linux Kernel development processes, but other folks might not like
them. On the flip side, just removing all of the current auto-cc's
would be awfully inconvenient for the Linux kernel development
community.
- Ted
^ permalink raw reply
* Re: [PATCH v3] Make git send-email accept $EDITOR with arguments
From: Junio C Hamano @ 2007-12-22 1:05 UTC (permalink / raw)
To: Gustaf Hendeby; +Cc: peff, luciano, git
In-Reply-To: <1198284052-20590-1-git-send-email-hendeby@isy.liu.se>
Gustaf Hendeby <hendeby@isy.liu.se> writes:
> Currently git send-email does not accept $EDITOR with arguments, eg,
> emacs -nw, when starting an editor to produce a cover letter. This
> patch changes this by letting the shell handle the option parsing.
>
> Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>
> ---
>
> This is based on Junio's suggestion on most readable and compatible
> solution. I'm not sure if it is identical to the C solution for git
> tag, but it seems to be a reasonable solution.
>
> git-send-email.perl | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 248d035..e47994a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -400,7 +400,7 @@ EOT
> close(C);
>
> my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> - system($editor, $compose_filename);
> + system('sh', '-c', '$0 $@', $editor, $compose_filename);
>
> open(C2,">",$compose_filename . ".final")
> or die "Failed to open $compose_filename.final : " . $!;
> --
> 1.5.4.rc1.16.gc817f
Thanks. Has this been tested? IOW, did you compose this
message with this patch?
^ permalink raw reply
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: Junio C Hamano @ 2007-12-22 1:09 UTC (permalink / raw)
To: Gustaf Hendeby; +Cc: luciano, git
In-Reply-To: <1198284202-20666-1-git-send-email-hendeby@isy.liu.se>
Don't you have $repo (an instance of Git) at that point? You
should be able to ask repo_path() about it, shouldn't you?
^ permalink raw reply
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: David Symonds @ 2007-12-22 1:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gustaf Hendeby, luciano, git
In-Reply-To: <7vmys3358v.fsf@gitster.siamese.dyndns.org>
On Dec 22, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Don't you have $repo (an instance of Git) at that point? You
> should be able to ask repo_path() about it, shouldn't you?
Isn't git-send-email still useful outside a Git repo?
Dave.
^ permalink raw reply
* Re: Problem with git-svn
From: Eric Wong @ 2007-12-22 4:29 UTC (permalink / raw)
To: Pascal Obry; +Cc: git list
In-Reply-To: <476AD1AB.8040406@gmail.com>
Pascal Obry <pascal.obry@gmail.com> wrote:
> Eric,
>
> > Ah, oops, I was off-by-one with the revision number. But git-svn does
> > look to be doing the right thing here, because it followed history into
> > /importfromcvs/trunk/ and file.el was part of it.
>
> Yes part of it but before the creation of the /importfromcvs/trunk/ that
> was moved later as /trunk/PROJ.
>
> < I meant moved as /trunk/PROJ1 then /trunk/PROJ2... and so on.
>
> In /importfromcvs/trunk/ there was many projects imported. One per one,
> each time moving it into /trunk/PROJ.
>
> If I look at history of /trunk/PROJ:
>
> $ svn log svn+ssh://myserver/trunk/PROJ
>
> The last revision is 45775, so I think git-svn should not look past this
> revision. So I'm very surprised by the current behavior and think it is
> a bug to import file.el at revision 9458. Note that the workaround for
> me is to use:
Since r48468 was where /importfromcvs/trunk got renamed into /trunk/PROJ
(from your previous message http://mid.gmane.org/4764FE2C.1010103@obry.net)
/importfromcvs/trunk exists at r45775, but /trunk/PROJ does not; and
git-svn at least follows that (which is what I suppose everybody wants).
However...
Did /importfromcvs/trunk exist all the way between r9458 and r48468? Or
was that directory replaced entirely by something else along the way?
git-svn may be following copy history too aggressively, in this case.
On the other hand, this was somewhat intended because it could also
be a way to track merges as "moving" tags[1].
> $ git svn clone svn+ssh://myserver/trunk/PROJ --revision=45775:HEAD
>
> But it would be lot cleaner to have git-svn handling this properly I think.
Does --no-follow-parent work in your case? Or does it go too far
in stopping at r48468 (probably).
[1] - I follow a project that has a RELEASE branch that is occasionally
deleted and replaced with the contents of trunk. git-svn will actually
import this as a merge commit with two parents:
1. trunk (obviously)
2. the previous RELEASE, which was deleted
This allows old (but deleted) history to still be visible, which is what
*I* always want. However, it could cause undesired behavior in your
case...
Note that running 'svn log <URL of #2 case>' will NOT show what git-svn
log will show for two; and I think it's nice that git-svn will track
history that is harder to find with SVN
(which would require svn log -v <URL of #2 case>@<rev>).
Maybe another switch should be added (--merge-foster-parent?)
--
Eric Wong
^ permalink raw reply
* Re: git-svn: pulling from local mirror but committing via svn+ssh
From: Eric Wong @ 2007-12-22 4:35 UTC (permalink / raw)
To: Sam Vilain; +Cc: Mike Frysinger, git
In-Reply-To: <8bd0f97a0712200821j778c12a9g9d42bf02482d953c@mail.gmail.com>
Mike Frysinger <vapier.adi@gmail.com> wrote:
> i have local (fast) mirrors of the svn trees i use, so i normally keep
> everything pointing at those. when i need to commit, i use `svn
> switch --relocate ...` to flip to the svn+ssh master, and then flop
> back to the local mirror. it actually works out nicely.
>
> my reading of git-svn(1) seems to indicate that git-svn cant really
> handle switching of repository URLs on the fly ?
> http://www.kernel.org/pub/software/scm/git/docs/git-svn.html
>
> i tried tweaking my .git/config and changing the url to my svn+ssh,
> but it seems any modification in the URL just causes errors ...
> vapier@G5[ppc] git $ git-svn dcommit
> Use of uninitialized value in concatenation (.) or string at
> /usr/bin/git-svn line 384.
> Committing to ...
> Unable to determine upstream SVN information from HEAD history
>
> guess i'll have to bite the bullet and just clone the svn+ssh repo :/
>
> (i'm using 1.5.3.7 over here)
Sam:
Can useSvmProps handle this? I honestly forgot how that stuff
was supposed to work with SVK/svn-mirror.
Mike:
On the other hand, there's little point in keeping local mirrors around
when using git-svn since git already does that for you :)
--
Eric Wong
^ permalink raw reply
* Re: Commit a series of patches to SVN without rebase
From: Eric Wong @ 2007-12-22 4:53 UTC (permalink / raw)
To: Jörg Sommer; +Cc: git, Karl Hasselström
In-Reply-To: <20071220164044.GA22683@alea.gnuu.de>
Jörg Sommer <joerg@alea.gnuu.de> wrote:
> Hi,
>
> I've a number of patches in git I want to send to a SVN repository. git
> svn dcommit does a rebase after each commit which makes the whole commit
> takes very long. Is it possible to skip the rebase? All patches are in
> one branch without merges, a simple chain. Is it save to use --no-rebase
> in this case?
Right now, only if the changes don't depend on each other (they all
modify different files).
The following patch should allow dcommit of multiple changes that depend
on each other with --no-rebase. --no-rebase was Karl's idea, and I've
never used it myself so I don't know how it works.
I don't have time to write a test case for --no-rebase at the
moment, but any reports/feedback would be helpful. Thanks.
diff --git a/git-svn.perl b/git-svn.perl
index c51f1e7..9b1113a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -418,7 +418,7 @@ sub cmd_dcommit {
warn "Attempting to commit more than one change while ",
"--no-rebase is enabled.\n",
"If these changes depend on each other, re-running ",
- "without --no-rebase will be required."
+ "without --no-rebase may be required."
}
while (1) {
my $d = shift @$linear_refs or last;
@@ -453,6 +453,7 @@ sub cmd_dcommit {
$parents->{$d};
}
$_fetch_all ? $gs->fetch_all : $gs->fetch;
+ $last_rev = $cmt_rev;
next if $_no_rebase;
# we always want to rebase against the current HEAD,
@@ -512,7 +513,6 @@ sub cmd_dcommit {
$parents = \%p;
$linear_refs = \@l;
}
- $last_rev = $cmt_rev;
}
}
unlink $gs->{index};
--
Eric Wong
^ permalink raw reply related
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: Junio C Hamano @ 2007-12-22 6:49 UTC (permalink / raw)
To: David Symonds; +Cc: Junio C Hamano, Gustaf Hendeby, luciano, git
In-Reply-To: <ee77f5c20712211718g230802b6jb70e5db1f6a43973@mail.gmail.com>
"David Symonds" <dsymonds@gmail.com> writes:
> On Dec 22, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Don't you have $repo (an instance of Git) at that point? You
>> should be able to ask repo_path() about it, shouldn't you?
>
> Isn't git-send-email still useful outside a Git repo?
Then why does it run "rev-parse --git-dir"?
^ permalink raw reply
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: David Symonds @ 2007-12-22 7:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gustaf Hendeby, luciano, git
In-Reply-To: <7vhcib2phi.fsf@gitster.siamese.dyndns.org>
On Dec 22, 2007 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > On Dec 22, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Don't you have $repo (an instance of Git) at that point? You
> >> should be able to ask repo_path() about it, shouldn't you?
> >
> > Isn't git-send-email still useful outside a Git repo?
>
> Then why does it run "rev-parse --git-dir"?
I'm suggesting that it should still function just fine without being
inside a repo, so it should adequately handle "rev-parse --git-dir"
returning 128.
Dave.
^ permalink raw reply
* Re: [PATCH] Allow selection of different cleanup modes for commit messages
From: Junio C Hamano @ 2007-12-22 7:59 UTC (permalink / raw)
To: Alex Riesen; +Cc: Linus Torvalds, git
In-Reply-To: <20071221230851.GA3260@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
> Junio C Hamano, Fri, Dec 21, 2007 22:43:49 +0100:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>> > The patch is on top of my previos patches. Junio, if you wish, I can
>> > squash and resend.
>>
>> That would be a sane thing to do.
>
> Done
Thanks. I am afraid I have to ask you to either refute my
misunderstanding or a second round.
> +--cleanup=<mode>::
> + This option sets how the commit message is cleaned up.
> + The '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
> + and 'default'. The 'default' mode will strip leading and
> + trailing empty lines and #commentary from the commit message
> + only if the message is to be edited. Otherwise only whitespace
> + removed. The 'verbatim' mode wont change message at all,
> + 'whitespace' removes just leading/trailing whitespace lines
> + and 'strip' removes both whitespace and commentary.
(minor) s/wont/does not/
> @@ -394,20 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
> return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
> }
>
> + if (!no_edit) {
> + if (in_merge)
> + fprintf(fp,
> + "#\n"
> + "# It looks like you may be committing a MERGE.\n"
> + "# If this is not correct, please remove the file\n"
> + "# %s\n"
> + "# and try again.\n"
> + "#\n",
> + git_path("MERGE_HEAD"));
> + if (cleanup_mode != CLEANUP_NONE)
> + fprintf(fp,
> + "\n"
> + "# Please enter the commit message for your changes.\n");
> + if (cleanup_mode == CLEANUP_DEFAULT)
> + fprintf(fp,
> + "# (Comment lines starting with '#' will not be included)\n");
Can't cleanup_mode be CLEANUP_ALL at this point, and if so
shouldn't this insn be given as well?
In addition, if:
- we are talking with editor, and
- the mode is verbatim, and
- we added any "#" line ourselves (e.g. in_merge adds the insn
shown above, or perhaps we added "git status" output),
then perhaps we would want to say that "#" lines need to be
stripped by the user; otherwise it will be left in the commit.
> @@ -431,10 +451,13 @@ static int message_is_empty(struct strbuf *sb, int start)
> const char *nl;
> int eol, i;
>
> + if (cleanup_mode == CLEANUP_NONE && sb->len)
> + return 0;
> +
> /* See if the template is just a prefix of the message. */
> strbuf_init(&tmpl, 0);
> if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> - stripspace(&tmpl, 1);
> + stripspace(&tmpl, !no_edit);
We at this point know that some stripping would happen. The
template needs to be cleaned the same way as the commit message
was stripped. Is checking no_edit the right thing to do? What
if cleanup_mode was CLEANUP_ALL and there is no editing going
on?
> @@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> if (p != NULL)
> strbuf_setlen(&sb, p - sb.buf + 1);
>
> - stripspace(&sb, 1);
> + if (cleanup_mode != CLEANUP_NONE)
> + stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
> + !no_edit: cleanup_mode == CLEANUP_ALL);
When writing such a long ? : expression, laying it out like:
condition
? if-true
: if-false
would be easier to read. You tilt your head sideways 90
degrees, just like you read a smiley ;-), and you will see the
parse tree.
But I suspect, assuming that the two issues around CLEANUP_DEFAULT vs
CLEANUP_ALL I mentioned above are indeed bugs, it may be cleaner
to:
- parse the options; you get one of NONE/DEFAULT/SPACE/ALL
- convert DEFAULT to SPACE or ALL if editor is in effect.
- do not worry about no_editor or DEFAULT in the rest of the
code when calling stripspace(). The only values you will
see are NONE, SPACE or ALL.
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 21ac785..6219378 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -89,4 +89,44 @@ test_expect_success 'verbose' '
>
> '
>
> +test_expect_success 'verbatim commit messages' '
> +
These tests check too many things at once, and it would be very
hard to diagnose when breakage does happen. Please split them
perhaps into three separate tests like this:
> + echo >>negative &&
> + { echo;echo "# text";echo; } >expect &&
> + git commit --cleanup=verbatim -t expect -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
> + diff -u expect actual &&
This is one test --- making sure the commit body match the
unedited template verbatim, except the status part to be shown
in the editor. But we are not using editor in this test and I
thought the point of this --cleanup=verbatim exercise was that
the "status" part is not even have to be added to the commit log
message when editor is not in effect... Isn't the "head -n 3"
just sweeping a bug under the rug?
> + echo >>negative &&
> + git commit --cleanup=verbatim -F expect -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> + diff -u expect actual &&
And this is another test.
> + echo >>negative &&
> + git commit --cleanup=verbatim -m "$(cat expect)" -a &&
> + git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> + diff -u expect actual
And this is yet another.
> +'
Also, the tests do not check "-F file -e", "-m msg -e" etc. We
would want to make sure the right insn are shown to the user in
the editor, wouldn't we?
^ permalink raw reply
* Re: [PATCH] git-tag: fix -l switch handling regression.
From: Junio C Hamano @ 2007-12-22 8:01 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <20071221211833.GA10318@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> On Fri, Dec 21, 2007 at 04:32:43PM +0000, Junio C Hamano wrote:
> ...
>> I thought this depended on some other changes. As is, doesn't
>> it break the t7004 test?
>
> Well there are tests that test:
>
> git tag -n xxx -l ...
>
> or
>
> git tag -n "" -l ...
>
> but I think we agreed those test nothing legitimate, and that the
> tests have to be removed. SO yes it hides another patch to cleanse t7004
> from the broken tests.
Thanks for a clarification; I'll resurect parts of my test
fix-ups and squash that in.
^ permalink raw reply
* Re: unexpected git-cherry-pick conflict
From: Junio C Hamano @ 2007-12-22 8:20 UTC (permalink / raw)
To: Gerrit Pape; +Cc: Johannes Schindelin, git
In-Reply-To: <20071221103743.2210.qmail@80f7ec9d6d380d.315fe32.mid.smarden.org>
Gerrit Pape <pape@smarden.org> writes:
> On Sat, Jul 07, 2007 at 09:58:08PM +0100, Johannes Schindelin wrote:
>> On Mon, 25 Jun 2007, Gerrit Pape wrote:
>> > Hi, did you get to this yet?, not to stress you, just to make sure we
>> > don't forget about it.
>>
>> Okay. Since now both you and Junio asked for it, and I made today a Git
>> day for me, I looked into this.
>
> Hi, the discussion on this topic unfortunately didn't result in a patch.
I know. Unfortunately the code structure in merge-recursive
around directory-file conflict handling needs major surgery to
fix this, and it won't be until somebody really sits down and
rethink the issue. I've been quietly working on some ideas and a
small nascent part of it is in 'pu' (or 'offcuts', perhaps, I do
not remember offhand), but it will take time.
^ permalink raw reply
* Re: Serious bug with pretty format strings & empty bodies?
From: Junio C Hamano @ 2007-12-22 8:39 UTC (permalink / raw)
To: Jonathan del Strother; +Cc: git
In-Reply-To: <57518fd10712210247s2fcbbf61ke67bbdc0f5ffe70b@mail.gmail.com>
"Jonathan del Strother" <maillist@steelskies.com> writes:
> Has anyone actually managed to reproduce my problem? I've got
> multiple repos here that show the problem in several commits, made by
> different people. However, I can't actually come up with a way to
> reproduce it at will...
Marking an issue as a "serious bug" without giving enough
material for reproduction nor diagnosis tends to discourage
people from looking into it seriously, as the issue cannot be
even judged if it is really serious. For example:
On Dec 19, 2007 6:44 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> Could you try
>
> git cat-file 18d2480ab689b483ef1ebbdb3f7420904049ba0b
>
> (or any other problematic commit) and post its output here?
You mean git cat-file commit ... ?
I get the normal output, but the problematic commits don't show a
newline character at the end of the cat-file output.
"I get the normal output" is not what Alex asked you to supply,
nor would be sufficient information. There may be some
abnormality in the commit object that you probably did not spot,
but Alex or other people may have been able to if you were
actually posted its output here.
I have been suspecting that there is a NUL in the middle of the
message somehow (I know the lowest-level plumbing commit-tree
allows any byte sequence in the log message if you worked hard
enough), which the parser is not prepared to cope with, but we
haven't seen enough evidence to support nor refute that theory.
We do not have much to work from.
One thing I noticed funny in your original message was "-1-".
Is it essential that the number is spelled incorrectly to
reproduce this problem?
$ git rev-list -1- --pretty=format:"%s%n%b"
18d2480ab689b483ef1ebbdb3f7420904049ba0b
commit 18d2480ab689b483ef1ebbdb3f7420904049ba0b
Try to flush log files before terminating the app
tree 57bc7cf30a10aee96251852125cf30fd2c81d7aa
parent 04c833865828538315fcdf6e187da077869ce444
author Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
committer Jonathan del Strother <jon.delStrother@bestbefore.tv> 1197901755 +0000
Check that ThreadWorker's work method actually returns a value with
method signatures
^ permalink raw reply
* Re: [PATCH v3] Make git send-email accept $EDITOR with arguments
From: Gustaf Hendeby @ 2007-12-22 8:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: peff, luciano, git
In-Reply-To: <7vr6hf35fh.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Gustaf Hendeby <hendeby@isy.liu.se> writes:
>
>> Currently git send-email does not accept $EDITOR with arguments, eg,
>> emacs -nw, when starting an editor to produce a cover letter. This
>> patch changes this by letting the shell handle the option parsing.
>>
>> Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>
>> ---
>>
>> This is based on Junio's suggestion on most readable and compatible
>> solution. I'm not sure if it is identical to the C solution for git
>> tag, but it seems to be a reasonable solution.
>>
>> git-send-email.perl | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 248d035..e47994a 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -400,7 +400,7 @@ EOT
>> close(C);
>>
>> my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
>> - system($editor, $compose_filename);
>> + system('sh', '-c', '$0 $@', $editor, $compose_filename);
>>
>> open(C2,">",$compose_filename . ".final")
>> or die "Failed to open $compose_filename.final : " . $!;
>> --
>> 1.5.4.rc1.16.gc817f
>
> Thanks. Has this been tested? IOW, did you compose this
> message with this patch?
Yes, I sent out the patch with git send-email, with git including the
patch itself and EDITOR=emacsclient -a emacs. I have also played around
with it a bit trying to make sure it is valid, but no more rigorous
testing that that. I'm not really sure how I would do that.
Thanks for all comments from everyone, I have learned a lot!
/Gustaf
^ permalink raw reply
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: Junio C Hamano @ 2007-12-22 8:52 UTC (permalink / raw)
To: David Symonds; +Cc: Junio C Hamano, Gustaf Hendeby, luciano, git
In-Reply-To: <ee77f5c20712212304s598d344dg41d03f58084d794e@mail.gmail.com>
"David Symonds" <dsymonds@gmail.com> writes:
> On Dec 22, 2007 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "David Symonds" <dsymonds@gmail.com> writes:
>>
>> > On Dec 22, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> >> Don't you have $repo (an instance of Git) at that point? You
>> >> should be able to ask repo_path() about it, shouldn't you?
>> >
>> > Isn't git-send-email still useful outside a Git repo?
>>
>> Then why does it run "rev-parse --git-dir"?
>
> I'm suggesting that it should still function just fine without being
> inside a repo, so it should adequately handle "rev-parse --git-dir"
> returning 128.
Ah, true. Then the current behaviour to use the $(pwd) for
temporary file area would be Ok for now.
^ permalink raw reply
* Re: [PATCH] Move git send-email cover letter temporary file to $GIT_DIR
From: Gustaf Hendeby @ 2007-12-22 9:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Symonds, luciano, git
In-Reply-To: <7vd4szyuva.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> "David Symonds" <dsymonds@gmail.com> writes:
>
>> On Dec 22, 2007 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> "David Symonds" <dsymonds@gmail.com> writes:
>>>
>>>> On Dec 22, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Don't you have $repo (an instance of Git) at that point? You
>>>>> should be able to ask repo_path() about it, shouldn't you?
>>>> Isn't git-send-email still useful outside a Git repo?
>>> Then why does it run "rev-parse --git-dir"?
>> I'm suggesting that it should still function just fine without being
>> inside a repo, so it should adequately handle "rev-parse --git-dir"
>> returning 128.
>
> Ah, true. Then the current behaviour to use the $(pwd) for
> temporary file area would be Ok for now.
Ok, just drop the patch I don't feel strongly about it.
However, the code today demands that git send-email is run from within a
git repository - it seems that $repo = Git->repository() assumes that.
I'd suggest changing this behavior so that git send-email becomes
runnable from outside a git repository. Unfortunately, I'm starting to
get in above my head here, for one I really don't know the helper
functions in Git.pm. Is there any good place to read up on what is in
Git.pm, except for the code itself?
/Gustaf
^ permalink raw reply
* Re: git-svn: pulling from local mirror but committing via svn+ssh
From: Sam Vilain @ 2007-12-22 9:16 UTC (permalink / raw)
To: Eric Wong; +Cc: Mike Frysinger, git
In-Reply-To: <20071222043528.GB18812@soma>
Eric Wong wrote:
>> i have local (fast) mirrors of the svn trees i use, so i normally keep
>> everything pointing at those. when i need to commit, i use `svn
>> switch --relocate ...` to flip to the svn+ssh master, and then flop
>> back to the local mirror. it actually works out nicely.
> Can useSvmProps handle this? I honestly forgot how that stuff
> was supposed to work with SVK/svn-mirror.
It's precisely this use case. If you are syncing with SVN::Mirror, part
of SVK, or via svnsync, these tools leave breadcrumbs as svn properties
that point to where the repository was copied from. So, in your commit
log the message will be the URL of the immediately[*] upstream
repository. SVN didn't ever really support clone per se, what did you
use to copy the repository?
One thing that isn't documented to my knowledge is that you can use 'git
commit --amend' to alter the URL in the 'git-svn-id:' in the commit
message of the top commit as a crude form of grafting. If the master
repository changes URL or UUID that's probably what you'd have to do.
But in your case, you probably want to figure out which switches you're
missing.
Sam.
* - Usually these tools don't allow for more than one level of indirection
>
>
> Mike:
>
> On the other hand, there's little point in keeping local mirrors around
> when using git-svn since git already does that for you :)
>
^ permalink raw reply
* Re: [PATCH] Don't dereference NULL upon lookup_tree failure.
From: Junio C Hamano @ 2007-12-22 9:25 UTC (permalink / raw)
To: Jim Meyering; +Cc: git list, Matthew Farrellee
In-Reply-To: <8763yrvb3g.fsf@rho.meyering.net>
Jim Meyering <jim@meyering.net> writes:
>>> Junio C Hamano <gitster@pobox.com> wrote:
>>> ...
>> Independent of who creates a "invalid repository", our tools
>> should be prepared to be fed bad data and not get upset.
>> Somebody in our code read a non-commit (or it did not read
>> anything) and told the object layer it is a commit. That
>> codepath needs to be tightened up, no?
>
> Yes, of course.
> Unfortunately, I won't have time to investigate for a while.
Although I haven't seen the problematic repository, I think what
is going on is that the repository has objects with pointers to
other objects labelled with wrong types.
For example, a tag can tag any type of object.
$ git cat-file tag v1.5.4-rc1
object 74f6b03c5c8fceef416de9f9a18e5d64712b6d96
type commit
tag v1.5.4-rc1
tagger Junio C Hamano <gitster@pobox.com> 1198114975 -0800
GIT 1.5.4-rc1
...
When parsing such a tag object, it instantiates an in-core tag
object that has a pointer to an in-core commit object, because
the tag says that it points at commit 74f6b0, without reading
the tagged object itself. This is to allow the user of that
in-core tag object to ask "what do you point at" and get an
in-core object that represents the pointee. If (and only if)
the user choose to follow that pointer and look at the contents
of the pointee, it can then call parse_object() on that in-core
object. In other words, the object layer is designed to work
lazily.
So if you earlier parsed such a tag but if the object 74f6b0
were an object of different type, we would hit the codepath you
identified that leads to a NULL dereference. And "lying caller"
cannot really do much better (except perhaps actually validating
the presense and the type of the object, which would incur
additional overhead). The situation is the same if:
- a tree object records an entry with:
- mode bits 040000 pointing at an object that is not a tree; or
- mode bits 100(644|755) pointing at an object that is not a blob; or
- mode bits 160000 pointing at an object that is not a commit; or
- a commit object has a "parent" line that is not a commit; or
- a commit object has a "tree" line that is not a tree.
Also we cannot just say "then we would take safety over
overhead" and make the lying callers validate the pointee;
subproject commits pointed by gitlinks (i.e. entry with 160000
mode bits in a tree object) are not even required to be
available.
Your fix is all we can sensibly do. I however think you would
need similar fix to the same function for other object types, as
they dereference a potentially NULL pointer the same way.
^ permalink raw reply
* Re: git-svn: pulling from local mirror but committing via svn+ssh
From: Mike Frysinger @ 2007-12-22 9:32 UTC (permalink / raw)
To: Sam Vilain; +Cc: Eric Wong, git
In-Reply-To: <476CD5E1.9020204@vilain.net>
On Dec 22, 2007 4:16 AM, Sam Vilain <sam@vilain.net> wrote:
> Eric Wong wrote:
> > > i have local (fast) mirrors of the svn trees i use, so i normally keep
> > > everything pointing at those. when i need to commit, i use `svn
> > > switch --relocate ...` to flip to the svn+ssh master, and then flop
> > > back to the local mirror. it actually works out nicely.
> >
> > Can useSvmProps handle this? I honestly forgot how that stuff
> > was supposed to work with SVK/svn-mirror.
>
> It's precisely this use case. If you are syncing with SVN::Mirror, part
> of SVK, or via svnsync, these tools leave breadcrumbs as svn properties
> that point to where the repository was copied from. So, in your commit
> log the message will be the URL of the immediately[*] upstream
> repository. SVN didn't ever really support clone per se, what did you
> use to copy the repository?
we publish the raw svn tree via rsync so people can create their own svn mirrors
sounds like it's just easier to mirror the original and since this
isnt a terribly large repo (~50 meg w/out any scm stuff), that's ok
-mike
^ permalink raw reply
* Re: [PATCH] Don't dereference NULL upon lookup_tree failure.
From: Jim Meyering @ 2007-12-22 13:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list, Matthew Farrellee
In-Reply-To: <7vzlw3xeqy.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
...
> Your fix is all we can sensibly do. I however think you would
> need similar fix to the same function for other object types, as
> they dereference a potentially NULL pointer the same way.
Good point. Patch below. I wondered if these lookup functions have
always been able to return NULL, since there are many remaining uses
where the possibility of a NULL return value is not handled.
Here are a few that stood out in the output of a quick search:
git-grep -E -A3 'lookup_(commit|tag|blob|tree) *\('
Note: I did not look at cases where the return value is an argument
to some other function.
------------------
builtin-fmt-merge-msg.c:
head = lookup_commit(head_sha1);
...
for (i = 0; i < origins.nr; i++)
shortlog(origins.list[i], origins.payload[i],
head, &rev, limit);
------------------
builtin-read-tree.c: struct tree *subtree = lookup_tree(entry.
sha1);
builtin-read-tree.c- if (!subtree->object.parsed)
------------------
combine-diff.c: struct commit *commit = lookup_commit(sha1);
combine-diff.c- struct commit_list *parents;
------------------
http-push.c: struct commit *head = lookup_commit(head_sha1);
http-push.c: struct commit *branch = lookup_commit(branch_sha1);
http-push.c- struct commit_list *merge_bases = get_merge_bases(head, branch, 1);
------------------
reachable.c: struct tree *tree = lookup_tree(sha1);
reachable.c- add_pending_object(revs, &tree->object, "");
------------------
tag.c: item->tagged = &lookup_blob(sha1)->object;
tag.c- } else if (!strcmp(type, tree_type)) {
tag.c: item->tagged = &lookup_tree(sha1)->object;
tag.c- } else if (!strcmp(type, commit_type)) {
tag.c: item->tagged = &lookup_commit(sha1)->object;
tag.c- } else if (!strcmp(type, tag_type)) {
tag.c: item->tagged = &lookup_tag(sha1)->object;
tag.c- } else {
--------------------------
unpack-trees.c: struct tree *tree = lookup_tree(posns[i]->sha1);
...
unpack-trees.c- parse_tree(tree);
Here's the revised patch:
==================================================================
From 94152217e8e57d3932b4ba6f7ee014da1f4346d3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Fri, 21 Dec 2007 11:56:32 +0100
Subject: [PATCH] Don't dereference NULL upon lookup failure.
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
object.c | 42 +++++++++++++++++++++++++++++-------------
1 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/object.c b/object.c
index 16793d9..9945b25 100644
--- a/object.c
+++ b/object.c
@@ -138,27 +138,43 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
if (type == OBJ_BLOB) {
struct blob *blob = lookup_blob(sha1);
- parse_blob_buffer(blob, buffer, size);
- obj = &blob->object;
+ if (!blob)
+ obj = NULL;
+ else {
+ parse_blob_buffer(blob, buffer, size);
+ obj = &blob->object;
+ }
} else if (type == OBJ_TREE) {
struct tree *tree = lookup_tree(sha1);
- obj = &tree->object;
- if (!tree->object.parsed) {
- parse_tree_buffer(tree, buffer, size);
- eaten = 1;
+ if (!tree)
+ obj = NULL;
+ else {
+ obj = &tree->object;
+ if (!tree->object.parsed) {
+ parse_tree_buffer(tree, buffer, size);
+ eaten = 1;
+ }
}
} else if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(sha1);
- parse_commit_buffer(commit, buffer, size);
- if (!commit->buffer) {
- commit->buffer = buffer;
- eaten = 1;
+ if (!commit)
+ obj = NULL;
+ else {
+ parse_commit_buffer(commit, buffer, size);
+ if (!commit->buffer) {
+ commit->buffer = buffer;
+ eaten = 1;
+ }
+ obj = &commit->object;
}
- obj = &commit->object;
} else if (type == OBJ_TAG) {
struct tag *tag = lookup_tag(sha1);
- parse_tag_buffer(tag, buffer, size);
- obj = &tag->object;
+ if (!tag)
+ obj = NULL;
+ else {
+ parse_tag_buffer(tag, buffer, size);
+ obj = &tag->object;
+ }
} else {
warning("object %s has unknown type id %d\n", sha1_to_hex(sha1), type);
obj = NULL;
--
1.5.4.rc1.16.g60f3b
^ permalink raw reply related
* [PATCH] Emit helpful status for accidental "git stash" save
From: Wincent Colaiuta @ 2007-12-22 14:22 UTC (permalink / raw)
To: git; +Cc: gitster, Wincent Colaiuta
In-Reply-To: <7vmys36d7n.fsf@gitster.siamese.dyndns.org>
El 21/12/2007, a las 20:48, Junio C Hamano escribió:
> (4) A tool should support safety for clueless people when it is
> reasonable.
>
> Even though "I did not know what command foo does, so I
> tried running it and it did something unexpected" is a
> silly excuse to rob quickie "git stash" from people who
> know better, we cannot avoid the fact that there are
> clueless people. I think checking stash.iknowwhatitdoes to
> detect first-time users, and explaining what it does to
> them, may make sense. And we can take hints from the patch
> that started this thread how to do this.
>
> The decision here is that I am open to a change that
> implements the one-time safety instruction.
Here's a less invasive change that I think addresses this point (4)
that you make.
-- 8< --
Emit helpful status for accidental "git stash" save
If the user types "git stash" mistakenly thinking that this will list
their stashes he/she may be surprised to see that it actually saved
a new stash and reset their working tree and index.
In the worst case they might not know how to recover the state. So
help them by telling them exactly what was saved and also how to
restore it immediately.
Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
git-stash.sh | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index f16fd9c..a2f3723 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -99,7 +99,8 @@ save_stash () {
git update-ref -m "$stash_msg" $ref_stash $w_commit ||
die "Cannot save the current status"
- printf >&2 'Saved "%s"\n' "$stash_msg"
+ printf >&2 'Saved working directory and index state "%s"\n' "$stash_msg"
+ echo >&2 '(To restore them type "git stash apply")'
}
have_stash () {
--
1.5.4.rc0.68.g15eb8-dirty
^ permalink raw reply related
* Re: [PATCH v0] sha1_name: grok <revision>:./<relative-path>
From: Johannes Schindelin @ 2007-12-22 14:33 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <fcaeb9bf0712210617x2bafa33cp15815a59fc631f45@mail.gmail.com>
Hi,
On Fri, 21 Dec 2007, Nguyen Thai Ngoc Duy wrote:
> [...] from my user perspective, the right approach is to make
> <treeish>:path always be relative to current directory.
As said by Junio, this would be a bad decision.
BTW please do not quote parts of the email that you do not comment on; it
takes half a minute of _everybody_ who tries to read your mail, only to
realise that it was time wasted.
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox