* Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Junio C Hamano @ 2013-02-22 18:35 UTC (permalink / raw)
To: Brandon Casey; +Cc: pclouds, jrnieder, john, git
In-Reply-To: <1361525158-3648-1-git-send-email-drafnel@gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line. This provided a nice
> hint to the user that something should be filled in. Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Ok. Here's a patch on top of 959a2623 bc/append-signed-off-by. It
> implements the "2 blank lines preceding sob" behavior.
>
> -Brandon
>
> sequencer.c | 5 +++--
> t/t7502-commit.sh | 12 ++++++++++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 53ee49a..2dac106 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> const char *append_newlines = NULL;
> size_t len = msgbuf->len - ignore_footer;
>
> - if (len && msgbuf->buf[len - 1] != '\n')
> + /* ensure a blank line precedes our signoff */
> + if (!len || msgbuf->buf[len - 1] != '\n')
> append_newlines = "\n\n";
> - else if (len > 1 && msgbuf->buf[len - 2] != '\n')
> + else if (len == 1 || msgbuf->buf[len - 2] != '\n')
> append_newlines = "\n";
Maybe I am getting slower with age, but it took me 5 minutes of
staring the above to convince me that it is doing the right thing.
The if/elseif cascade is dealing with three separate things and the
logic is a bit dense:
* Is the buffer completely empty? We need to add two LFs to give a
room for the title and body;
* Otherwise:
- Is the final line incomplete? We need to add one LF to make it a
complete line whatever we do.
- Is the final line an empty line? We need to add one more LF to
make sure we have a blank line before we add S-o-b.
I wondered if we can rewrite it to make the logic clearer (that is
where I spent most of the 5 minutes), but I did not think of a
better way; probably the above is the best we could do.
Thanks.
By the way, I think we would want to introduce a symbolic constants
for the possible return values from has_conforming_footer(). The
check that appears after this hunk
if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
sob.buf, sob.len);
is hard to grok without them.
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit
From: Jeff King @ 2013-02-22 18:37 UTC (permalink / raw)
To: Joshua Clayton; +Cc: git, Erik Faye-Lund
In-Reply-To: <CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com>
On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:
> Subject: Re: [PATCH] Fix in Git.pm cat_blob crashes on large files
> (resubmit with reviewed-by)
One thing I forgot to note in my other response: the subject is part of
the commit message, so information like "resubmit with..." should go
with other cover letter material after the "---".
It's also customary to say something like "PATCHv2" in the brackets
(which does get stripped off), and mention what is changed since v1
after the "---". That can help reviewers remember what happened, and it
can help the maintainer understand where in the process of review the
patch is.
-Peff
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)
From: Junio C Hamano @ 2013-02-22 18:49 UTC (permalink / raw)
To: Joshua Clayton, Adam Roben; +Cc: git, Jeff King, Erik Faye-Lund
In-Reply-To: <CAMB+bfLvpKNLaEUyUUYsO5n2y+9tyd_QcnPVzX0s2Z2t3Fr9=g@mail.gmail.com>
Joshua Clayton <stillcompiling@gmail.com> writes:
> Read and write each 1024 byte buffer, rather than trying to buffer
> the entire content of the file.
> Previous code would crash on all files > 2 Gib, when the offset variable
> became negative (perhaps below the level of perl), resulting in a crash.
> On a 32 bit system, or a system with low memory it might crash before
> reaching 2 GiB due to memory exhaustion.
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>
> ---
Thanks.
>> Subject: Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)
Please drop the () part. A rule of thumb is to make "git show"
output understandable by people who read it 6 months from now. They
do not care if the commit is a re-submission.
It seems that this issue was with us since the very beginning of
this sub since it was introduced at 7182530d8cad (Git.pm: Add
hash_and_insert_object and cat_blob, 2008-05-23).
> perl/Git.pm | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 931047c..cc91288 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -949,13 +949,16 @@ sub cat_blob {
> last unless $bytesLeft;
>
> my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
> - my $read = read($in, $blob, $bytesToRead, $bytesRead);
> + my $read = read($in, $blob, $bytesToRead);
> unless (defined($read)) {
> $self->_close_cat_blob();
> throw Error::Simple("in pipe went bad");
> }
> -
> $bytesRead += $read;
> + unless (print $fh $blob) {
> + $self->_close_cat_blob();
> + throw Error::Simple("couldn't write to passed
> in filehandle");
> + }
Corrupt patch, line-wrapped by your MUA.
I wonder if we still need $bytesRead variable. You have $size that
is the size of the whole blob, so
my $bytesLeft = $size;
while (1) {
my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
my $bytesRead = read($in, $blob, $bytesToRead);
... check errors and use the $blob ...
$bytesLeft -= $bytesRead;
}
may be simpler and easier to read, no?
^ permalink raw reply
* Re: [PATCH] archive: let remote clients get reachable commits
From: Junio C Hamano @ 2013-02-22 19:15 UTC (permalink / raw)
To: Jeff King; +Cc: Sergey Sergeev, git@vger.kernel.org
In-Reply-To: <20130222182654.GA18934@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 22, 2013 at 10:06:56AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > How are you proposing to verify master~12 in that example? Because
>> > during parsing, it starts with "master", and we remember that?
>>
>> By not cheating (i.e. using get_sha1()), but making sure you can
>> parse "master" and the adornment on it "~12" is something sane.
>
> So, like these patches:
>
> http://article.gmane.org/gmane.comp.version-control.git/188386
>
> http://article.gmane.org/gmane.comp.version-control.git/188387
>
> ? They do not allow arbitrary sha1s that happen to point to branch tips,
> but I am not sure whether that is something people care about or not.
>
>> That is why I said "this is harder than one would naively think, but
>> limiting will make it significantly easier". I didn't say that it
>> would become "trivial", did I?
>
> I'm not implying it would be trivial. It was an honest question, since
> you did not seem to want to do the pass-more-information-out-of-get-sha1
> approach last time this came up.
That does not match my recollection ($gmane/188427). In fact, I had
those two patches you quoted earlier in mind when I wrote the "limit
it and it will become easier" response.
> Even though those patches above are from me, I've come to the conclusion
> that the best thing to do is to harmonize with upload-pack. Then you
> never have the "well, but I could fetch it, so why won't upload-archive
> let me get it" argument.
That sounds like a sensible yardstick.
> 1. split name at first colon (like we already do)
>
> 2. make sure the left-hand side is reachable according to the same
> rules that upload-pack uses.
Well, "upload-pack" under the hood operates on a bare 40-hex. If
you mean to do "split name at first colon, run get_sha1() on the
LHS" in step 1., I would agree this is a good direction to go.
> Right we just say "is it a ref". It should be:
With s/should/could optionally/, I would agree.
> That leaves the only inaccessible thing as direct-sha1s of trees and
> blobs that are reachable from commits.
Yes (with s/are reachable/are only reachable/).
^ permalink raw reply
* Re: [PATCH] Fix in Git.pm cat_blob crashes on large files (resubmit with reviewed-by)
From: Joshua Clayton @ 2013-02-22 19:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, Erik Faye-Lund
In-Reply-To: <20130222183419.GB18934@sigill.intra.peff.net>
Thanks for all the input and patience.
On Fri, Feb 22, 2013 at 10:34 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 22, 2013 at 09:30:57AM -0800, Joshua Clayton wrote:
>
>> Read and write each 1024 byte buffer, rather than trying to buffer
>> the entire content of the file.
>
> OK. Did you ever repeat your timing with a larger symmetric buffer? That
> should probably be a separate patch on top, but it might be worth doing
> while we are thinking about it.
>
>> Previous code would crash on all files > 2 Gib, when the offset variable
>> became negative (perhaps below the level of perl), resulting in a crash.
>
> I'm still slightly dubious of this, just because it doesn't match my
> knowledge of perl (which is admittedly imperfect). I'm curious how you
> diagnosed it?
I first had the memory exhaustion problem running my git repo on a 32 vm.
After bumping the memory from 512 to 4 GiB, and that failing to fix it
I moved to my workstation with 16 GiB
...reproduced
After the initial crash, I added
print $size, " ", $bytesToRead, " ", $bytesRead, "\n";
right before the read command, and it does indeed crash right after
the $bytesRead variable crosses LONG_MAX
...
2567089913 1024 2147482624
2567089913 1024 2147483648
2567089913 1024 2147484672
Offset outside string at /usr/share/perl5/Git.pm line 901, <GEN36> line 2604.
Note that $bytesRead is still positive.
I know very little perl, but that symptom seems pretty clear
>
>> On a 32 bit system, or a system with low memory it might crash before
>> reaching 2 GiB due to memory exhaustion.
>>
>> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
>> Reviewed-by: Jeff King <peff@peff.net>
>
> The commit message is a good place to mention any side effects, and why
> they are not a problem. Something like:
>
> The previous code buffered the whole blob before writing, so any error
> reading from cat-file would result in zero bytes being written to the
> output stream. After this change, the output may be left in a
> partially written state (or even fully written, if we fail when
> parsing the final newline from cat-file). However, it's not reasonable
> for callers to expect anything about the state of the output when we
> return an error (after all, even with full buffering, we might fail
> during the writing process). So any caller which cares about this is
> broken already, and we do not have to worry about them.
>
>> ---
>> perl/Git.pm | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> The patch itself looks fine to me.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] submodule update: when using recursion, show full path
From: Jens Lehmann @ 2013-02-22 19:17 UTC (permalink / raw)
To: Will Entriken; +Cc: git, Phil Hord, Stefan Zager
In-Reply-To: <CAFwrLX7CroJ1Au-w0G7jo7F7DAu5=u2E6iVc9YUTLytVBuHVhw@mail.gmail.com>
Thanks. Your code changes are looking good and the commit message
explains what you did and why you did it. A few comments below.
Am 22.02.2013 05:25, schrieb Will Entriken:
>>From d3fe2c76e6fa53e4cfa6f81600685c21bdadd4e3 Mon Sep 17 00:00:00 2001
> From: William Entriken <github.com@phor.net>
> Date: Thu, 21 Feb 2013 23:10:07 -0500
>
> Subject: [PATCH] submodule update: when using recursion, show full path
The lines above aren't necessary as they are taken from the mail header.
> Previously when using update with recursion, only the path for the
> inner-most module was printed. Now the path is printed from GIT_DIR.
You should replace "from GIT_DIR" with something like "relative to the
directory the recursion started in" here.
> This now matches the behavior of submodule foreach
Please add a '.' at the end of the sentence.
> ---
>
> First patch. Several tests failed, but they were failing before I
> started. This is against maint, I would consider this a (low priority)
> bug.
Strange that you have failing tests, for me everything runs fine (With
or without your patch). But I agree that this is a low priority bug.
> How does it look? Please let me know next steps.
This patch does not apply due to removed leading whitespaces and a
few wrapped lines. Please see Documentation/git-format-patch.txt on
how to convince the mailer of your choice to send the patch out
unmangled.
> Signed-off-by: William Entriken <github.com@phor.net>
The Signed-off-by belongs just before the "---" line above, as
everything between "---" and the line below won't make it into the
commit message.
> git-submodule.sh | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2365149..f2c53c9 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -588,7 +588,7 @@ cmd_update()
> die_if_unmatched "$mode"
> if test "$stage" = U
> then
> - echo >&2 "Skipping unmerged submodule $sm_path"
> + echo >&2 "Skipping unmerged submodule $prefix$sm_path"
> continue
> fi
> name=$(module_name "$sm_path") || exit
> @@ -602,7 +602,7 @@ cmd_update()
>
> if test "$update_module" = "none"
> then
> - echo "Skipping submodule '$sm_path'"
> + echo "Skipping submodule '$prefix$sm_path'"
> continue
> fi
>
> @@ -611,7 +611,7 @@ cmd_update()
> # Only mention uninitialized submodules when its
> # path have been specified
> test "$#" != "0" &&
> - say "$(eval_gettext "Submodule path '\$sm_path' not initialized
> + say "$(eval_gettext "Submodule path '\$prefix\$sm_path' not initialized
> Maybe you want to use 'update --init'?")"
> continue
> fi
> @@ -624,7 +624,7 @@ Maybe you want to use 'update --init'?")"
> else
> subsha1=$(clear_local_git_env; cd "$sm_path" &&
> git rev-parse --verify HEAD) ||
> - die "$(eval_gettext "Unable to find current revision in submodule
> path '\$sm_path'")"
> + die "$(eval_gettext "Unable to find current revision in submodule
> path '\$prefix\$sm_path'")"
> fi
>
> if test "$subsha1" != "$sha1" -o -n "$force"
> @@ -643,7 +643,7 @@ Maybe you want to use 'update --init'?")"
> (clear_local_git_env; cd "$sm_path" &&
> ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
> test -z "$rev") || git-fetch)) ||
> - die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
> + die "$(eval_gettext "Unable to fetch in submodule path '\$prefix\$sm_path'")"
> fi
>
> # Is this something we just cloned?
> @@ -657,20 +657,20 @@ Maybe you want to use 'update --init'?")"
> case "$update_module" in
> rebase)
> command="git rebase"
> - die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path
> '\$sm_path'")"
> - say_msg="$(eval_gettext "Submodule path '\$sm_path': rebased into '\$sha1'")"
> + die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path
> '\$prefix\$sm_path'")"
> + say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': rebased
> into '\$sha1'")"
> must_die_on_failure=yes
> ;;
> merge)
> command="git merge"
> - die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path
> '\$sm_path'")"
> - say_msg="$(eval_gettext "Submodule path '\$sm_path': merged in '\$sha1'")"
> + die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path
> '\$prefix\$sm_path'")"
> + say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': merged
> in '\$sha1'")"
> must_die_on_failure=yes
> ;;
> *)
> command="git checkout $subforce -q"
> - die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule
> path '\$sm_path'")"
> - say_msg="$(eval_gettext "Submodule path '\$sm_path': checked out '\$sha1'")"
> + die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule
> path '\$prefix\$sm_path'")"
> + say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': checked
> out '\$sha1'")"
> ;;
> esac
>
> @@ -688,11 +688,16 @@ Maybe you want to use 'update --init'?")"
>
> if test -n "$recursive"
> then
> - (clear_local_git_env; cd "$sm_path" && eval cmd_update "$orig_flags")
> + (
> + prefix="$prefix$sm_path/"
> + clear_local_git_env
> + cd "$sm_path" &&
> + eval cmd_update "$orig_flags"
> + )
> res=$?
> if test $res -gt 0
> then
> - die_msg="$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
> + die_msg="$(eval_gettext "Failed to recurse into submodule path
> '\$prefix\$sm_path'")"
> if test $res -eq 1
> then
> err="${err};$die_msg"
> --
> 1.7.11.3
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: Is this a bug?
From: Phil Hord @ 2013-02-22 19:29 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Erik Faye-Lund, David Wade, git@vger.kernel.org
In-Reply-To: <CACsJy8DjatRbL=J-MZmQFnd+_7i-WwYHSnkY_ga++fx1R5Whmw@mail.gmail.com>
On Tue, Feb 19, 2013 at 6:02 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Feb 19, 2013 at 4:47 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> On Tue, Feb 19, 2013 at 10:32 AM, David Wade <DAWAD@statoil.com> wrote:
>>> Hi,
>>>
>>> I wrote a commit message beginning with a hash (#) character, like this: 'git commit -m "#ifdef ...." '
>>>
>>> Everything went okay when committing, but then I tried 'git commit -amend' and without editing the commit message I was told I had an empty commit message.
>>>
>>> Is this a problem with my text editor (vim 7.2) or git itself? (git version 1.7.2.2 under RedHat 5.8) Or something I'm not supposed to do ;-) ?
>>
>> The problem is that when doing interactive editing of messages (like
>> 'git commit --amend' does), git considers '#' as a comment-character.
>> You can disable this by using the --cleanup=verbatim switch (or some
>> other suiting cleanup-setting, see 'git help commit').
>
> Nobody is always conscious about the leading # in commit message to do
> that. I once edited a commit message and the auto-wrap feature put '#'
> at the beginning of the line. I saved and went on without noticing one
> line was lost until much later :( Perhaps we should change the comment
> signature a bit to reduce accidents, like only recognize '#' lines as
> comments after a special line like
>
> # this is not a comment
> ### START OF COMMENT ###
> # this is a comment
Or maybe --amend should imply --cleanup=whitespace.
--
Phil
^ permalink raw reply
* Re: [PATCH v2] branch: segfault fixes and validation
From: Junio C Hamano @ 2013-02-22 20:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Per Cederqvist
In-Reply-To: <1361533663-3172-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> branch_get() can return NULL (so far on detached HEAD only) but some
> code paths in builtin/branch.c cannot deal with that and cause
> segfaults. While at there, make sure we bail out when the user gives 2
> or more arguments, but we only process the first one and silently
> ignore the rest.
Explain "2 or more arguments" in what context, perhaps? Otherwise
it makes it sound as if "git branch foo bar baz" is covered with
this patch, no?
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> >
> >> branch_get() can return NULL (so far on detached HEAD only)...
> >
> > Do you anticipate any other cases where the API call should validly
> > return NULL?
>
> No. But I do not see any guarantee that it may never do that in
> future either. Which is why I was deliberately vague with "could not
> figure out...". But you also correctly observed my laziness there. So
> how about this? It makes a special case for HEAD but not insist on
> detached HEAD as the only cause.
Sure. It looks better.
What you can do is to have a single helper function that can explain
why branch_get() returned NULL (or extend branch_get() to serve that
purpose as well); then you do not have to duplicate the logic twice
on the caller's side (and there may be other callers that want to do
the same).
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6371bf9..82ed337 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> } else if (new_upstream) {
> struct branch *branch = branch_get(argv[0]);
>
> + if (argc > 1)
> + die(_("too many branches to set new upstream"));
> +
> + if (!branch) {
> + if (!argc || !strcmp(argv[0], "HEAD"))
> + die(_("HEAD does not point to any branch. Is it detached?"));
> + die(_("no such branch '%s'"), argv[0]);
> + }
> +
> if (!ref_exists(branch->refname))
> die(_("branch '%s' does not exist"), branch->name);
The latter part of the new code triggers when branch_get() returns
NULL while doing "git branch --set-upstream-to=X [Y]". When "Y" is
string "HEAD" or missing, the first die() is triggered and says a
funny thing. If HEAD does not point to any branch, by definition it
is detached. The user may say "Yes, I know it is detached." but the
message does not help the user to figure out what to do next.
Instead of asking "Is it detached?", perhaps we can say something
like "You told me to set the upstream of HEAD to branch X, but " in
front? At least, that will be a better explanation for the reason
why the operation is failing.
The existing test might be wrong, by the way. Your HEAD may point
at a branch Y but you may not have any commit on it yet, and you may
want to allow setting the upstream of that to-be-born branch to
another branch X with "branch --set-upstream-to=X [Y|HEAD]".
While I think it is insane to do anything before creating the first
commit on your current branch (or using "checkout --orphan" in
general) and it may not be worth our time to babysit users who do
so, but the following sequence may feel natural to them:
git checkout --orphan X
git branch --set-upstream-to=master
... perhaps create an initial commit, perhaps not ...
git merge @{upstream}
For that to work sanely, perhaps the pattern
branch = branch_get();
if (!branch)
die due to no branch;
if (!ref_exists(branch->refname))
die due to typo in branch name
may need to be fixed globally, replacing ref_exists(branch->refname)
with branch_exists(branch) that returns true if branch->refname is
an existing ref, or the branch in question was obtained by checking
with current_branch (in remote.c), or something like that.
^ permalink raw reply
* Re: [PATCH 1/2] index-format.txt: mention of v4 is missing in some places
From: Junio C Hamano @ 2013-02-22 20:46 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1361534964-4232-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/technical/index-format.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
> index 27c716b..0810251 100644
> --- a/Documentation/technical/index-format.txt
> +++ b/Documentation/technical/index-format.txt
> @@ -12,7 +12,7 @@ Git index format
> The signature is { 'D', 'I', 'R', 'C' } (stands for "dircache")
>
> 4-byte version number:
> - The current supported versions are 2 and 3.
> + The current supported versions are 2, 3 and 4.
>
> 32-bit number of index entries.
>
> @@ -93,8 +93,8 @@ Git index format
> 12-bit name length if the length is less than 0xFFF; otherwise 0xFFF
> is stored in this field.
>
> - (Version 3) A 16-bit field, only applicable if the "extended flag"
> - above is 1, split into (high to low bits).
> + (Version 3 or later) A 16-bit field, only applicable if the
> + "extended flag" above is 1, split into (high to low bits).
>
> 1-bit reserved for future
Depending on how the first later version decides to encode the
additional flag information, "or later" may need to be changed to
"and 4" when it happens. As we cannot predict the future, I think
"or later" is just as good as "add 4" for now.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] read-cache.c: use INDEX_FORMAT_{LB,UB} in verify_hdr()
From: Junio C Hamano @ 2013-02-22 20:49 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1361534964-4232-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> 9d22778 (read-cache.c: write prefix-compressed names in the index -
> 2012-04-04) defined these. Interestingly, they were not used by
> read-cache.c, or anywhere in that patch. They were used in
> builtin/update-index.c later for checking supported index
> versions. Use them here too.
Thanks.
> - if (hdr_version < 2 || 4 < hdr_version)
> + if (hdr_version < INDEX_FORMAT_LB ||
> + hdr_version > INDEX_FORMAT_UB)
> return error("bad index version %d", hdr_version);
^ permalink raw reply
* [PATCHv3] Fix in Git.pm cat_blob crashes on large files
From: Joshua Clayton @ 2013-02-22 21:01 UTC (permalink / raw)
To: git; +Cc: Joshua Clayton, Jeff King, Erik Faye-Lund, Junio C Hamano
Read and write each 1024 byte buffer, rather than trying to buffer
the entire content of the file.
Previous code would crash on all files > 2 Gib, when the offset variable
became negative (perhaps below the level of perl), resulting in a crash.
On a 32 bit system, or a system with low memory it might crash before
reaching 2 GiB due to memory exhaustion.
This code may leave a partial file behind in case of failure, where the
old code would leave a completely empty file.
Neither version verifies the correctness of the content.
Calling code must take care of verification and cleanup.
Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
---
perl/Git.pm | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..db6e0a8 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -942,20 +942,22 @@ sub cat_blob {
my $size = $1;
my $blob;
- my $bytesRead = 0;
+ my $bytesLeft = $size;
while (1) {
- my $bytesLeft = $size - $bytesRead;
last unless $bytesLeft;
my $bytesToRead = $bytesLeft < 1024 ? $bytesLeft : 1024;
- my $read = read($in, $blob, $bytesToRead, $bytesRead);
+ my $read = read($in, $blob, $bytesToRead);
unless (defined($read)) {
$self->_close_cat_blob();
throw Error::Simple("in pipe went bad");
}
-
- $bytesRead += $read;
+ unless (print $fh $blob) {
+ $self->_close_cat_blob();
+ throw Error::Simple("couldn't write to passed in filehandle");
+ }
+ $bytesLeft -= $read;
}
# Skip past the trailing newline.
@@ -970,11 +972,6 @@ sub cat_blob {
throw Error::Simple("didn't find newline after blob");
}
- unless (print $fh $blob) {
- $self->_close_cat_blob();
- throw Error::Simple("couldn't write to passed in filehandle");
- }
-
return $size;
}
--
1.7.10.4
I'm trying to send this with git-send-email this time around to beat
the linewrapping problem.
^ permalink raw reply related
* post-receive-email hook with custom_showrev
From: Adam Mercer @ 2013-02-22 20:57 UTC (permalink / raw)
To: git
Hi
I'm trying to setup the post-receive-email hook, from contrib, using a
custom_showrev, from the script I do this by setting hooks.showrev
# hooks.showrev
# The shell command used to format each revision in the email, with
# "%s" replaced with the commit id. Defaults to "git rev-list -1
# --pretty %s", displaying the commit id, author, date and log
# message. To list full patches separated by a blank line, you
# could set this to "git show -C %s; echo".
# To list a gitweb/cgit URL *and* a full patch for each change set, use this:
# "t=%s; printf 'http://.../?id=%%s' \$t; echo;echo; git show -C \$t; echo"
# Be careful if "..." contains things that will be expanded by shell "eval"
# or printf.
in my repositories config I have showrev set to:
[hooks]
showrev = t=%s; printf
'http://server/cgit/repository/commit/?id=%%s' \$t; echo;echo; git
show -C \$t; echo
But in the emails from the post-receive-email hook I get something like:
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "LALSuite".
The branch, master has been updated
via 10f97dd6db3861e35e517301f6c1dec30be90012 (commit)
from 8c7dfa89cec5ac0a5b9520967b92a927734611f0 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
-----------------------------------------------------------------------
Summary of changes:
lal/README | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
So it seems as if showrev is being ignored? Can anyone see what I'm doing wrong?
Cheers
Adam
^ permalink raw reply
* https_proxy and libcurl
From: Phil Hord @ 2013-02-22 21:19 UTC (permalink / raw)
To: git@vger.kernel.org
I have been unable to clone via http proxy because of a wrongly
configured proxy setup in my lab.
I had this env:
http_proxy=http://proxy.myco.com
https_proxy=https://proxy.myco.com
The problem is that libcurl ignores the protocol part of the proxy
url, and it defaults to port 1080. wget honors the protocol specifier,
but it defaults to port 80 if none is given.
So, I thought my env was equivalent to this:
http_proxy=proxy.myco.com:80
https_proxy=proxy.myco.com:443
So it is with wget. But with curl, it is equivalent to this:
http_proxy=proxy.myco.com:1080
https_proxy=proxy.myco.com:1080
Git clone gave a confusing (but correct) error message:
$ export https_proxy=https://proxy.myco.com
$ git clone https://github.com/git/git
Cloning into 'git'...
error: The requested URL returned error: 500 while accessing
https://github.com/git/git/info/refs?service=git-upload-pack
fatal: HTTP request failed
If I didn't have https_proxy set at all, I got a long timeout as it
tried to connect directly and ran into our firewall.
$ unset https_proxy
$ git clone https://github.com/git/git
Cloning into 'git'...
error: Failed connect to github.com:443; Connection timed out
while accessing
https://github.com/git/git/info/refs?service=git-upload-pack
fatal: HTTP request failed
This also did not help, of course:
'git config http.proxy https://proxy.myco.com'
But this did:
'git config http.proxy https://proxy.myco.com:443'
The fix was to specify the port explicitly, like this:
http_proxy=proxy.myco.com:80
https_proxy=proxy.myco.com:443
Phil
[1] An added wrinkle is that there is a proxy server listening on
1080, but it does not support encrypted connections. A listener on
443 does handle https requests correctly.
^ permalink raw reply
* Re: [PATCHv3] Fix in Git.pm cat_blob crashes on large files
From: Junio C Hamano @ 2013-02-22 21:20 UTC (permalink / raw)
To: Joshua Clayton; +Cc: git, Jeff King, Erik Faye-Lund
In-Reply-To: <1361566878-20117-1-git-send-email-stillcompiling@gmail.com>
Thanks.
^ permalink raw reply
* Re: Is this a bug?
From: Junio C Hamano @ 2013-02-22 21:48 UTC (permalink / raw)
To: Phil Hord; +Cc: Duy Nguyen, Erik Faye-Lund, David Wade, git@vger.kernel.org
In-Reply-To: <CABURp0rO5zJywFN16=Sn20b2DAVA7XBs4EC4GxGbxftXqUS6gA@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> Or maybe --amend should imply --cleanup=whitespace.
I am fairly negative on that.
Such a hidden linkage, even if it is documented, is just one more
thing people need to learn.
It _might_ be interesting (note: I did not say "worthwhile" here) to
think what happens if we scanned the message (either coming from the
commit being amended, -F file option, or -m message option), picked
a punctuation character that does not appear at the beginning of the
lines in it, and automatically adjusted core.commentchar if '#'
appears at the beginning of one of the lines, though.
^ permalink raw reply
* Re: https_proxy and libcurl
From: Junio C Hamano @ 2013-02-22 21:55 UTC (permalink / raw)
To: Phil Hord; +Cc: git@vger.kernel.org
In-Reply-To: <CABURp0qQ6tO0B4Ya6OStX59SJqG-Jx1F4g6MUL7tVwR_6VgDhw@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> I have been unable to clone via http proxy because of a wrongly
> configured proxy setup in my lab.
>
> I had this env:
>
> http_proxy=http://proxy.myco.com
> https_proxy=https://proxy.myco.com
>
> The problem is that libcurl ignores the protocol part of the proxy
> url, and it defaults to port 1080. wget honors the protocol specifier,
> but it defaults to port 80 if none is given.
IIRC, the historical norm is to set these to <host>:<port>.
So many people mistakenly write them with <method>:// that some
tools over time learned to strip and ignore that prefix, though.
> The fix was to specify the port explicitly, like this:
>
> http_proxy=proxy.myco.com:80
> https_proxy=proxy.myco.com:443
Yeah, that is the correct syntax to use. Is there anything you want
Git to do to be more helpful?
^ permalink raw reply
* Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Brandon Casey @ 2013-02-22 22:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: pclouds, jrnieder, john, git
In-Reply-To: <7vbobcdwo7.fsf@alter.siamese.dyndns.org>
On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>> diff --git a/sequencer.c b/sequencer.c
>> index 53ee49a..2dac106 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>> const char *append_newlines = NULL;
>> size_t len = msgbuf->len - ignore_footer;
>>
>> - if (len && msgbuf->buf[len - 1] != '\n')
>> + /* ensure a blank line precedes our signoff */
>> + if (!len || msgbuf->buf[len - 1] != '\n')
>> append_newlines = "\n\n";
>> - else if (len > 1 && msgbuf->buf[len - 2] != '\n')
>> + else if (len == 1 || msgbuf->buf[len - 2] != '\n')
>> append_newlines = "\n";
>
> Maybe I am getting slower with age, but it took me 5 minutes of
> staring the above to convince me that it is doing the right thing.
>
> The if/elseif cascade is dealing with three separate things and the
> logic is a bit dense:
>
> * Is the buffer completely empty? We need to add two LFs to give a
> room for the title and body;
>
> * Otherwise:
>
> - Is the final line incomplete? We need to add one LF to make it a
> complete line whatever we do.
>
> - Is the final line an empty line? We need to add one more LF to
> make sure we have a blank line before we add S-o-b.
>
> I wondered if we can rewrite it to make the logic clearer (that is
> where I spent most of the 5 minutes), but I did not think of a
> better way; probably the above is the best we could do.
We could unroll the conditionals into individual blocks and add your
comments from above like:
if (!len) {
/* The buffer is completely empty. Leave room for the title and body. */
append_newlines = "\n\n";
} else if (msgbuf->buf[len - 1] != '\n') {
/* Incomplete line. Complete the line and add a blank one */
append_newlines = "\n\n";
} else if (len == 1) {
/*
* Buffer contains a single newline. Add another so that we leave
* room for the title and body.
*/
append_newlines = "\n";
} ...
Not sure that it will reduce the amount of time needed to understand
what's going on, but at least it describes the expectations made by
each block.
> Thanks.
>
> By the way, I think we would want to introduce a symbolic constants
> for the possible return values from has_conforming_footer(). The
> check that appears after this hunk
>
> if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
> strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
> sob.buf, sob.len);
>
> is hard to grok without them.
Yeah, Jonathan said the same thing and I agree. I was hoping someone
else would beat me to it.
-Brandon
^ permalink raw reply
* [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Brandon Casey @ 2013-02-22 22:05 UTC (permalink / raw)
To: gitster; +Cc: git, pclouds, jrnieder, john, Brandon Casey
In-Reply-To: <CA+sFfMdok7wRDhgq7i=b3cu3LB+poExvxLBxYkg8L3pN92bEYg@mail.gmail.com>
From: Brandon Casey <drafnel@gmail.com>
Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
a blank line before the Signed-off-by line. This provided a nice
hint to the user that something should be filled in. Let's restore that
behavior, but now let's ensure that the Signed-off-by line is preceded
by two blank lines to hint that something should be filled in, and that
a blank line should separate it from the Signed-off-by line.
Plus, add a test for this behavior.
Reported-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
How about something like this?
-Brandon
sequencer.c | 27 +++++++++++++++++++++++++--
t/t7502-commit.sh | 12 ++++++++++++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 53ee49a..a07d2d0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,10 +1127,33 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
const char *append_newlines = NULL;
size_t len = msgbuf->len - ignore_footer;
- if (len && msgbuf->buf[len - 1] != '\n')
+ if (!len) {
+ /*
+ * The buffer is completely empty. Leave foom for
+ * the title and body to be filled in by the user.
+ */
append_newlines = "\n\n";
- else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+ } else if (msgbuf->buf[len - 1] != '\n') {
+ /*
+ * Incomplete line. Complete the line and add a
+ * blank one so that there is an empty line between
+ * the message body and the sob.
+ */
+ append_newlines = "\n\n";
+ } else if (len == 1) {
+ /*
+ * Buffer contains a single newline. Add another
+ * so that we leave room for the title and body.
+ */
+ append_newlines = "\n";
+ } else if (msgbuf->buf[len - 2] != '\n') {
+ /*
+ * Buffer ends with a single newline. Add another
+ * so that there is an empty line between the message
+ * body and the sob.
+ */
append_newlines = "\n";
+ } /* else, the buffer already ends with two newlines. */
if (append_newlines)
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..a53a1e0 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
'
+test_expect_success 'commit -s places sob on third line after two empty lines' '
+ git commit -s --allow-empty --allow-empty-message &&
+ cat <<-EOF >expect &&
+
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+
+ EOF
+ egrep -v '^#' .git/COMMIT_EDITMSG >actual &&
+ test_cmp expect actual
+'
+
write_script .git/FAKE_EDITOR <<\EOF
mv "$1" "$1.orig"
(
--
1.8.1.3.566.gaa39828
^ permalink raw reply related
* Re: https_proxy and libcurl
From: Daniel Stenberg @ 2013-02-22 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Hord, git@vger.kernel.org
In-Reply-To: <7v7gm0c8ub.fsf@alter.siamese.dyndns.org>
On Fri, 22 Feb 2013, Junio C Hamano wrote:
>> http_proxy=http://proxy.myco.com
>> https_proxy=https://proxy.myco.com
>>
>> The problem is that libcurl ignores the protocol part of the proxy
>> url, and it defaults to port 1080. wget honors the protocol specifier,
>> but it defaults to port 80 if none is given.
>
> IIRC, the historical norm is to set these to <host>:<port>.
>
> So many people mistakenly write them with <method>:// that some tools over
> time learned to strip and ignore that prefix, though.
You're right, but also what exactly is the https:// _to_ the proxy supposed to
mean? The standard procedure to connect to a proxy when communicating with
either HTTP or HTTPS is using plain HTTP.
If you would want port 443 for a HTTPS connection to a proxy, you'd use:
https_proxy=http://proxy.myco.com:443
Or without the ignore protocol prefix:
https_proxy=proxy.myco.com:443
--
/ daniel.haxx.se
^ permalink raw reply
* Crashes while trying to show tag objects with bad timestamps
From: Mantas Mikulėnas @ 2013-02-22 22:30 UTC (permalink / raw)
To: git
When messing around with various repositories, I noticed that git 1.8
(currently using 1.8.2.rc0.22.gb3600c3) has problems parsing tag objects
that have invalid timestamps.
Times in tag objects appear to be kept as Unix timestamps, but I didn't
realize this at first, and ran something roughly equivalent to:
git cat-file -p $tagname | git hash-object -w -t tag --stdin
creating a tag object the "tagger" line containing formatted time
instead of a Unix timestamp.
Git doesn't handle the resulting tag objects nicely at all. For example,
running `git cat-file -p` on the new object outputs a really odd
timestamp "Thu Jun Thu Jan 1 00:16:09 1970 +0016" (I'm guessing it
parses the year as Unix time), and `git show` outright crashes
(backtrace included below.)
I would have expected both commands to print a "tag object corrupt"
message, or maybe even a more specific "bad timestamp in tagger line"...
To reproduce:
printf '%s\n' \
'object 4b825dc642cb6eb9a060e54bf8d69288fbee4904' 'type tree' \
'tag test' 'tagger User <user@none> Thu Jun 9 16:44:04 2005 +0000' \
'' 'Test tag' | git hash-object -w -t tag --stdin | xargs git show
> #0 0x00007f42560bb5f3 in ____strtoull_l_internal () from /usr/lib/libc.so.6
> No symbol table info available.
> #1 0x00000000004b4c81 in pp_user_info (pp=pp@entry=0x7fff3c30f1a0,
> what=what@entry=0x50c13b "Tagger", sb=sb@entry=0x7fff3c30f140,
> line=0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", encoding=0x507e20 "UTF-8") at pretty.c:431
> name = {alloc = 24, len = 15, buf = 0xc24690 "Jilles Tjoelker"}
> mail = {alloc = 24, len = 15, buf = 0xc24750 "jilles@stack.nl"}
> ident = {
> name_begin = 0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n",
> name_end = 0xc267d6 " <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n",
> mail_begin = 0xc267d8 "jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", mail_end = 0xc267e7 "> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n",
> date_begin = 0x0, date_end = 0x0, tz_begin = 0x0, tz_end = 0x0}
> linelen = <optimized out>
> line_end = <optimized out>
> date = <optimized out>
> mailbuf = 0xc267d8 "jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
> namebuf = 0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
> namelen = 33
> maillen = 15
> max_length = 78
> time = <optimized out>
> tz = <optimized out>
> #2 0x0000000000439af5 in show_tagger (buf=<optimized out>, len=<optimized out>,
> rev=<optimized out>) at builtin/log.c:400
> pp = {fmt = CMIT_FMT_MEDIUM, abbrev = 0, subject = 0x0, after_subject = 0x0,
> preserve_subject = 0, date_mode = DATE_NORMAL, date_mode_explicit = 0,
> need_8bit_cte = 0, notes_message = 0x0, reflog_info = 0x0,
> output_encoding = 0x0, mailmap = 0x0, color = 0}
> out = {alloc = 0, len = 0, buf = 0x7a8188 <strbuf_slopbuf> ""}
> #3 show_tag_object (rev=0x7fff3c30f1f0,
> sha1=0xc2be44 "\230\211\275\331\365Q\306z\017\071d\331\035\062\247a\347~M8P", <incomplete sequence \303>) at builtin/log.c:427
> new_offset = 151
> type = OBJ_TAG
> buf = 0xc26770 "object ffa28d13e40e03bd367d0219c7eb516be0f180d2\ntype commit\ntag hyperion-1.0rc1\ntagger Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
> size = 165
> offset = <optimized out>
--
Mantas Mikulėnas <grawity@gmail.com>
^ permalink raw reply
* Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Jeff King @ 2013-02-22 22:35 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, git, pclouds, jrnieder, john, Brandon Casey
In-Reply-To: <1361570727-20255-1-git-send-email-bcasey@nvidia.com>
On Fri, Feb 22, 2013 at 02:05:27PM -0800, Brandon Casey wrote:
> From: Brandon Casey <drafnel@gmail.com>
>
> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line. This provided a nice
> hint to the user that something should be filled in. Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> How about something like this?
FWIW, as a casual reader of this series, I find this to be way easier
to follow than the previous round.
-Peff
^ permalink raw reply
* Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b
From: Junio C Hamano @ 2013-02-22 22:38 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Casey, git, pclouds, jrnieder, john, Brandon Casey
In-Reply-To: <20130222223513.GA21579@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> FWIW, as a casual reader of this series, I find this to be way easier
> to follow than the previous round.
It is assuring to know that I am not the only one getting slow with
age ;-)
Thanks.
^ permalink raw reply
* Re: Crashes while trying to show tag objects with bad timestamps
From: Jeff King @ 2013-02-22 22:46 UTC (permalink / raw)
To: Mantas Mikulėnas; +Cc: git
In-Reply-To: <kg8ri2$vjb$1@ger.gmane.org>
On Sat, Feb 23, 2013 at 12:30:28AM +0200, Mantas Mikulėnas wrote:
> When messing around with various repositories, I noticed that git 1.8
> (currently using 1.8.2.rc0.22.gb3600c3) has problems parsing tag objects
> that have invalid timestamps.
>
> Times in tag objects appear to be kept as Unix timestamps, but I didn't
> realize this at first, and ran something roughly equivalent to:
> git cat-file -p $tagname | git hash-object -w -t tag --stdin
> creating a tag object the "tagger" line containing formatted time
> instead of a Unix timestamp.
Thanks, that makes it easy to replicate. It looks like it is not just
tags, but rather the pp_user_info function does not realize that
split_ident may return NULL for the date field if it is unparseable.
Something like this stops the crash and just gives a bogus date:
diff --git a/pretty.c b/pretty.c
index eae57ad..9688857 100644
--- a/pretty.c
+++ b/pretty.c
@@ -428,8 +428,16 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(&name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
- time = strtoul(ident.date_begin, &date, 10);
- tz = strtol(date, NULL, 10);
+
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, &date, 10);
+ tz = strtol(date, NULL, 10);
+ }
+ else {
+ /* ident line had malformed date */
+ time = 0;
+ tz = 0;
+ }
if (pp->fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, "From: ");
I guess we should probably issue a warning, too. Also disappointingly,
git-fsck does not seem to detect this breakage at all.
> Git doesn't handle the resulting tag objects nicely at all. For example,
> running `git cat-file -p` on the new object outputs a really odd
> timestamp "Thu Jun Thu Jan 1 00:16:09 1970 +0016" (I'm guessing it
> parses the year as Unix time), and `git show` outright crashes
> (backtrace included below.)
If "cat-file -p" is not using the usual pretty-print routines, it
probably should. I'll take a look.
-Peff
^ permalink raw reply related
* Re: Crashes while trying to show tag objects with bad timestamps
From: Junio C Hamano @ 2013-02-22 22:53 UTC (permalink / raw)
To: Jeff King; +Cc: Mantas Mikulėnas, git
In-Reply-To: <20130222224655.GB21579@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I guess we should probably issue a warning, too. Also disappointingly,
> git-fsck does not seem to detect this breakage at all.
Yes for the warning, and no for disappointing. IIRC, in the very
early implementations allowed tag object without dates.
I _think_ we can start tightening fsck, though.
^ permalink raw reply
* [RFC/PATCH] hash-object doc: "git hash-object -w" can write invalid objects
From: Jonathan Nieder @ 2013-02-22 23:01 UTC (permalink / raw)
To: Mantas Mikulėnas; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <kg8ri2$vjb$1@ger.gmane.org>
When using "hash-object -w" to create non-blob objects, it is
generally a good policy to run "git fsck" afterward to make sure the
resulting object is valid. Add a warning to the manpage.
While it at, gently nudge the user of "hash-object -w" toward
higher-level interfaces for creating or modifying trees, commits, and
tags.
Reported-by: Mantas Mikulėnas <grawity@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi Mantas,
Mantas Mikulėnas wrote:
> When messing around with various repositories, I noticed that git 1.8
> (currently using 1.8.2.rc0.22.gb3600c3) has problems parsing tag objects
> that have invalid timestamps.
[...]
> Git doesn't handle the resulting tag objects nicely at all. For example,
> running `git cat-file -p` on the new object outputs a really odd
> timestamp "Thu Jun Thu Jan 1 00:16:09 1970 +0016" (I'm guessing it
> parses the year as Unix time),
The usual rule is that with invalid objects (e.g. as detected by "git
fsck"), any non-crash result is acceptable. Garbage in, garbage out.
> and `git show` outright crashes
> (backtrace included below.)
Probably worth fixing.
I notice that git-hash-object(1) doesn't contain any reference to
git-fsck(1). How about something like this, to start?
Perhaps by default hash-object should automatically fsck the objects
it is asked to create.
Thanks,
Jonathan
Documentation/git-hash-object.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt
index 02c1f12..8ed8c6e 100644
--- a/Documentation/git-hash-object.txt
+++ b/Documentation/git-hash-object.txt
@@ -30,6 +30,8 @@ OPTIONS
-w::
Actually write the object into the object database.
+ This does not check that the resulting object is valid;
+ for that, see linkgit:git-fsck[1].
--stdin::
Read the object from standard input instead of from a file.
@@ -53,6 +55,14 @@ OPTIONS
conversion. If the file is read from standard input then this
is always implied, unless the --path option is given.
+SEE ALSO
+--------
+linkgit:git-mktree[1],
+linkgit:git-commit-tree[1],
+linkgit:git-tag[1],
+linkgit:git-filter-branch[1],
+sha1sum(1)
+
GIT
---
Part of the linkgit:git[1] suite
--
1.8.1.4
^ permalink raw reply related
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