* Re: [RFC PATCH] git-gui: Allow staging multiple lines at once
From: Dirk Süsserott @ 2009-10-20 17:09 UTC (permalink / raw)
To: Jeff Epler; +Cc: git
In-Reply-To: <20091019195456.GA11121@unpythonic.net>
Am 19.10.2009 21:54 schrieb Jeff Epler:
> When applying less than a full hunk, it's still often desirable to apply
> a number of consecutive lines.
>
> This change makes it possible to sweep out a range of lines in the diff view
> with the left mouse button, then right click and "Stage Lines For Commit".
>
> The selected lines may span multiple hunks.
>
Awesome! I've been waiting for that very feature. Thanks in advance.
-- Dirk
^ permalink raw reply
* Re: [PATCH] Makefile: set PERL_PATH and SHELL_PATH unconditionally
From: Matt Kraai @ 2009-10-20 16:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vtyxuox7u.fsf@alter.siamese.dyndns.org>
On Tue, Oct 20, 2009 at 09:18:29AM -0700, Junio C Hamano wrote:
> Matt Kraai <kraai@ftbfs.org> writes:
>
> > Do not check whether PERL_PATH and SHELL_PATH are undefined before
> > setting their default values. This prevents them from being set via
> > environment variables.
>
> Is there an upside of "preventing them from getting set", by the way?
Not that I know of.
I originally thought that the checks were superfluous, but now I just
believe they're inconsistent and confusing to people like me who think
they understand Makefiles but don't. :)
--
Matt Kraai http://ftbfs.org/
^ permalink raw reply
* Re: [PATCH] Use faster byte swapping when compiling with MSVC
From: Junio C Hamano @ 2009-10-20 16:55 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sebastian Schuberth, Junio C Hamano, git, Marius Storm-Olsen,
Johannes Sixt
In-Reply-To: <alpine.DEB.1.00.0910201149020.4985@pacific.mpi-cbg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Well, in fact I am one of the msysgit poeple, although I mostly worked
>> on the installer until now. In general, I like my patches to be
>> reviewed, but this one is rather uncritical, I guess. So it's up to you,
>> Junio, I'm perfectly OK with waiting for an ACK.
>
> Apart from the fact that I do not have MSVC (and I don't want it, either),
> there is another strong reason why I think Sebastian does not need ACKs or
> SOBs on MSVC patches: he has plenty of experience as a maintainer of a
> rather big (commercial) software that has to compile on Windows, MacOSX
> and several Unix-type OSes (and it is known that Sebastian is a Windows
> guy).
>
> So I would trust Sebastian's patches (at least when it comes to MSVC)
> without even reviewing them.
I very appreciate a strong Ack in a specific area like this. I do skim
msysgit list from time to time, and in retrospect I realize I _could_ have
recognized Sebastian's name but somehow it didn't click.
I guess I should apply both patches to 'master', then. Thanks.
^ permalink raw reply
* Re: [PATCH] blame: make sure that the last line ends in an LF
From: Junio C Hamano @ 2009-10-20 16:55 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Git List, Johannes Schindelin
In-Reply-To: <fabb9a1e0910200615x5d487cdk6f50e11c96f2cb6c@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
>> Does the code assign blame
>> correctly around the last line of the original blob?
>
> Yes, it does,...
That is kind of surprising ;-) as I do remember that I never thought about
this issue of dealing with the incomplete lines while writing the blame
algorithm. I actually didn't even think about "well this will not work
because I am ignoring the incomplete lines".
>> Or am I worrying too much?
>
> No, I think your concerns are valid, we should go with (2) and DTRT.
> Does the updated patch address your concerns? If so I can send a new
> version.
Assuming the internal blame algorithm correctly works with such an input,
I'd be happier with an approach to allow users to tell the difference.
The --porcelain output was designed to be extensible, and it might make
sense to show the "this line is incomplete" as a separate bit, though.
^ permalink raw reply
* Re: git hang with corrupted .pack
From: Junio C Hamano @ 2009-10-20 16:52 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Andy Isaacson, git, Nicolas Pitre, Alex Riesen
In-Reply-To: <20091014142351.GI9261@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Z_BUF_ERROR is returned from inflate() if either the input buffer
> needs more input bytes, or the output buffer has run out of space.
> Previously we only considered the former case, as it meant we needed
> to move the stream's input buffer to the next window in the pack.
>
> We now abort the loop if inflate() returns Z_BUF_ERROR without
> consuming the entire input buffer it was given, or has filled
> the entire output buffer but has not yet returned Z_STREAM_END.
> Either state is a clear indicator that this loop is not working
> as expected, and should not continue.
When the inflated contents is of size 0, avail_out would be 0 and avail_in
would still have something because the input stream needs to have the end
of stream marker that is more than zero byte long.
If that is more than one-byte long, and your avail_in originally fed only
the first byte from that sequence, what happens? Wouldn't inflate eat all
what was given (now avail_in is zero), updated its internal state but it
still hasn't produced anything (avail_out is zero)?
I am not saying the end-of-stream is more than one-byte long (I didn't
check), but we had a similar bug arising from confusing "no more output
data" and "fully consumed input stream" (e.g. 456cdf6 (Fix loose object
uncompression check., 2007-03-19).
Something like that may be what is happening to cause problem Alex is
seeing.
I think the corrupt packdata detection needs an output buffer at least
one-byte larger than what is needed to store the correct result. Then
when we get BUF_ERROR:
- We never look at avail to see if it is zero; !avail_out is not the same
as "it stopped because it ran out of output space". It might mean
"there is nothing more to come but the input stream ended before
signalling that fact to the inflate engine fully".
- We do look at avail_out to find how much data we ended up getting. If
inflate has consumed more buffer than we planned to give it, the stream
is corrupt (at least it is not what we expected to see);
> st = git_inflate(&stream, Z_FINISH);
> + if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
> + break;
> curpos += stream.next_in - in;
> } while ((st == Z_OK || st == Z_BUF_ERROR) &&
> stream.total_out < sizeof(delta_head));
> @@ -1594,6 +1596,8 @@ static void *unpack_compressed_entry(struct packed_git *p,
> in = use_pack(p, w_curs, curpos, &stream.avail_in);
> stream.next_in = in;
> st = git_inflate(&stream, Z_FINISH);
> + if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
> + break;
> curpos += stream.next_in - in;
> } while (st == Z_OK || st == Z_BUF_ERROR);
> git_inflate_end(&stream);
> --
> 1.6.5.52.g0ff2e
>
> --
> Shawn.
^ permalink raw reply
* Re: Update working copy on push without touching several files
From: Thomas Rast @ 2009-10-20 16:45 UTC (permalink / raw)
To: Alex Amiryan; +Cc: git
In-Reply-To: <4ADDCC7A.8080607@amiryan.org>
Alex Amiryan wrote:
> Online versions of websites are maintained with git
> too. I need to have working copy of my remote git repository (online
> version of the site) updated by git push (which I do locally). The
> problem is that I have some files there (like database config) that have
> to be different from local ones and they must not be updated on git
> push.
My best results so far were with special config branches that are
auto-merged in post-receive. As a simple example, post-receive might
simply be
#!/bin/sh
git checkout -f master^0
git merge config
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* RE: Moving git
From: Richard @ 2009-10-20 16:37 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <alpine.LNX.2.00.0910201222080.14365@iabervon.org>
Thanks to anyone for the reply, but this issue has been resolved. I
might have submitted this several times, but Nabble keeps on nagging me
about multiple posts. So I'm just going to delete it.
-----Original Message-----
From: Daniel Barkalow [mailto:barkalow@iabervon.org]
Sent: 20 October 2009 17:34
To: Richard
Cc: git@vger.kernel.org
Subject: Re: Moving git
On Tue, 20 Oct 2009, Richard Lee wrote:
>
> Hi Git forum,
>
> I've just started using git yesterday, so I'm very new. So please
excuse if
> I've done something the wrong way.
>
> I cloned a git directory/repository? and then moved it. I'm trying to
prune
> branches and it gives
>
> ]fatal: '/var/www/vhosts/mydomain.co.uk/b2.git': unable to chdir or
not a
> git archive
> fatal: The remote end hung up unexpectedly
> ls-remote --heads /var/www/vhosts/mydomain.co.uk/b2.git: command
returned
> error: 1
>
> b2.git was the cloned bare thing I create following the instruction
here:
>
> http://book.git-scm.com/4_setting_up_a_private_repository.html
>
> Is there someway I can get git to update the git base directory?
The exact problem, I think, is that your clone has saved the original
location of the bare repository as the default upstream repository
location, and now it's not there. (It's a little hard to tell without
the
command that you were running when you got the error.)
If you edit the clone's .git/config, you should see the old location in
a
'[remote "origin"]' section. If you change this to the new location,
everything should work. You can also do it with "git remote" somehow,
but
I personally just edit the config file, so I don't know the details.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: git-pack-objects gitattributes?
From: Nasser Grainawi @ 2009-10-20 16:34 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Git Mailing List
In-Reply-To: <4ADCBAC7.9010601@drmicha.warpmail.net>
Michael J Gruber wrote:
> Nasser Grainawi venit, vidit, dixit 19.10.2009 20:47:
>> Nasser Grainawi wrote:
>>> Hello,
>>>
>>> I'm trying to avoid doing delta compression on a number of large binary
>>> files. I got a suggestion to use $GIT_DIR/info/attributes and a line
>>> like this:
>>> *.bin -delta
>>>
>>> This doesn't seem to show a big improvement (and honestly I can't find
>>> where in the git-pack-objects source the value of this attribute is used).
>>>
>>> Could someone shed some light on this attribute and any other
>>> improvements I could make for efficiently serving up a repo over
>>> git-daemon with near-weekly revisions of 100MB+ files?
>>>
>>> Thanks,
>>> Nasser
>> ping? any help? anyone?
>
> Well, describing a reproducable test case would help... as well as
> telling us your git version.
1.6.5
>
> builtin-pack-objects.c certainly refers to the delta attribute, see
> no_try_delta() and its callers.
Oops, somehow missed that while looking at the code right above it. Thanks.
>
> Have you checked your attrs with git-check-attr? How do you measure the
> improvements you expect?
I did check, it returns 'unset' like one would expect.
I guess the big problem is that I don't have a good test case. I would have
expected a 'git repack -adf' to spend less time saying "Compressing objects",
but that doesn't seem to be happening...
Oh, wait, never mind.
I was missing some of the binaries I was trying to skip. Adding some more
exceptions to the attributes file dropped the "Compressing objects" time from
20 minutes (or more) to maybe 10 seconds.
Thanks Michael!
Nasser
^ permalink raw reply
* Re: Moving git
From: Daniel Barkalow @ 2009-10-20 16:33 UTC (permalink / raw)
To: Richard Lee; +Cc: git
In-Reply-To: <25926819.post@talk.nabble.com>
On Tue, 20 Oct 2009, Richard Lee wrote:
>
> Hi Git forum,
>
> I've just started using git yesterday, so I'm very new. So please excuse if
> I've done something the wrong way.
>
> I cloned a git directory/repository? and then moved it. I'm trying to prune
> branches and it gives
>
> ]fatal: '/var/www/vhosts/mydomain.co.uk/b2.git': unable to chdir or not a
> git archive
> fatal: The remote end hung up unexpectedly
> ls-remote --heads /var/www/vhosts/mydomain.co.uk/b2.git: command returned
> error: 1
>
> b2.git was the cloned bare thing I create following the instruction here:
>
> http://book.git-scm.com/4_setting_up_a_private_repository.html
>
> Is there someway I can get git to update the git base directory?
The exact problem, I think, is that your clone has saved the original
location of the bare repository as the default upstream repository
location, and now it's not there. (It's a little hard to tell without the
command that you were running when you got the error.)
If you edit the clone's .git/config, you should see the old location in a
'[remote "origin"]' section. If you change this to the new location,
everything should work. You can also do it with "git remote" somehow, but
I personally just edit the config file, so I don't know the details.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH v2] new --dirty option for git describe
From: Junio C Hamano @ 2009-10-20 16:30 UTC (permalink / raw)
To: Jean Privat; +Cc: git, Shawn O. Pearce
In-Reply-To: <dffdbd190910200727r30e161ffka0b3cf764be26cd8@mail.gmail.com>
Jean Privat <jean@pryen.org> writes:
> +test_expect_success 'describe --dirty HEAD' '
> + git describe --dirty HEAD
> + test $? != 0
> +'
We tend to write this as
test_expect_success '...' '
test_must_fail git describe --dirty HEAD
'
The difference is when the tested command segfaults or dies in an
uncontrolled fashion; test_must_fail diagnoses it as a failure,
while "test $? != 0" at the end will say "ok, the command correctly
failed".
^ permalink raw reply
* Re: git fsck not identifying corrupted packs
From: Wesley J. Landaker @ 2009-10-20 16:20 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git
In-Reply-To: <200910201741.50764.robin.rosenberg@dewire.com>
On Tuesday 20 October 2009 09:41:50 Robin Rosenberg wrote:
> måndag 19 oktober 2009 21:27:48 skrev Wesley J. Landaker:
> > (Not CCing everyone, since this is mostly curiosa in the "using git as
> > it was never intended" section):
[...]
> > Filesystems are mostly reliable, but only until your crazy users do
> > strange and terrible things. I have a real, non-toy environment where I
> > use this stack as a [horrible] workaround for some issues beyond my
> > control:
> >
> > git -> ext4 -> lvm -> dmcrypt -> loop -> sshfs -> cygwin sshd -> SMB
> > share
My main point was to illustrate that having "git fsck" do a REALLY GOOD
CHECK is still desirable, as we still haven't reached the days of file-
system utopia where nothing ever gets corrupted (even with a smaller,
simpler stack).
The actual application where I use this stack is because of odd requirements
and circumstances like data must be physically stored on a particular
Windows server on the network that uses a weird authentication method that
samba doesn't support, and it has to go over the network encrypted anyway,
there are lots of holes in the data, so I want ext4 for the extent support,
file-size limitations on the target, etc.
It's a really an exotic love-hate mix between an off-by-one-please-no-never-
again kind of situation coupled with a bit of "because I can".
> The obvious follow up question here is: Why?
If you are both nerdy and morbidly curious enough to care, send me a "but,
no ... really, WHY?!" with the git list CC dropped and we can talk about
details and/or other crazy stuff. (I don't want to get wildly off-topic on
this list.)
^ permalink raw reply
* Re: [PATCH] Makefile: set PERL_PATH and SHELL_PATH unconditionally
From: Junio C Hamano @ 2009-10-20 16:18 UTC (permalink / raw)
To: Matt Kraai; +Cc: git
In-Reply-To: <1256029588-24128-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> Do not check whether PERL_PATH and SHELL_PATH are undefined before
> setting their default values. This prevents them from being set via
> environment variables.
Is there an upside of "preventing them from getting set", by the way?
^ permalink raw reply
* [PATCH v2.5] new --dirty option for git describe
From: Jean Privat @ 2009-10-20 15:56 UTC (permalink / raw)
To: git, git
With the --dirty option, git describe works on HEAD but append "-dirty"
if the working tree is dirty. If the working tree is clean, nothing
is appended.
$ git describe --dirty
v1.6.5-15-gc274db7
$ echo >> Makefile
$ git describe --dirty
v1.6.5-15-gc274db7-dirty
--dirty can also be used to specify what is append if the working
tree is dirty.
$ git describe --dirty=.mod
v1.6.5-15-gc274db7.mod
Many build scripts use `git describe` to produce a version number based
on the description of HEAD (on which is based the working tree) + saying
that if the working tree is dirty or no.
This patch helps the writing of such scripts since `git describe --dirty`
do directly the intended thing.
Alternatives specifications:
1 Describe the working tree by default and describe HEAD only if "HEAD"
is explicitly specified
Pro: does the right thing by default (both for users and for scripts)
Pro: is coherent with other git commands that works on the working tree
by default
Con: breaks existing scripts (since the world is not ideal)
2 Use --worktree instead of --dirty
Pro: does what it says: "git describe --worktree" will describe the
working three
Con: is incoherent with other commands that do not require a --worktree
option to work on the working tree
Con: unusable with an optional value: "git describe --worktree=.mod"
is quite unintuitive.
3 Use --dirty as in this patch
Pro: make sense to specify an optional value (what the dirty mark is)
Pro: do not have any of the big cons of previous alternatives
* does not break scripts
* is not inconsistent with other git commands
Pro: has an easy fallback to alternative 1 if the world become suddenly
ideal, or at least allows some kind of future transition plan if
people *really* bother:
1- ask that scripts use either "git describe HEAD" or
"git describe --dirty" (no more "git describe")
2- change default
Once the transition is done, the role of the --dirty option will
just be the way to specify an alternative mark (and a noop if alone)
Signed-off-by: Jean Privat <jean@pryen.org>
---
I tried to integrate the points of view of both Junio and Shawn.
However, I am not sure that what I propose is the right solution.
Changes since v2:
No more line breaks (I hope)
Changes since v1:
use --dirty (alternative 3) instead of defaulting on workdir (alternative 1)
-J-
---
Documentation/git-describe.txt | 6 ++++++
builtin-describe.c | 25 ++++++++++++++++++++++++-
t/t6120-describe.sh | 15 +++++++++++++++
3 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index b231dbb..5253d86 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -9,6 +9,7 @@ git-describe - Show the most recent tag that is reachable from a commit
SYNOPSIS
--------
'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] <committish>...
+'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]
DESCRIPTION
-----------
@@ -27,6 +28,11 @@ OPTIONS
<committish>...::
Committish object names to describe.
+--dirty[=<mark>]::
+ Describe the working tree.
+ It means describe HEAD and appends <mark> (`-dirty` by
+ default) if the working tree is dirty.
+
--all::
Instead of using only the annotated tags, use any ref
found in `.git/refs/`. This option enables matching
diff --git a/builtin-describe.c b/builtin-describe.c
index df67a73..84af981 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -5,12 +5,14 @@
#include "builtin.h"
#include "exec_cmd.h"
#include "parse-options.h"
+#include "diff.h"
#define SEEN (1u<<0)
#define MAX_TAGS (FLAG_BITS - 1)
static const char * const describe_usage[] = {
"git describe [options] <committish>*",
+ "git describe [options] --dirty",
NULL
};
@@ -23,6 +25,13 @@ static int max_candidates = 10;
static int found_names;
static const char *pattern;
static int always;
+static const char *dirty;
+
+/* diff-index command arguments to check if working tree is dirty. */
+static const char *diff_index_args[] = {
+ "diff-index", "--quiet", "HEAD", "--", NULL
+};
+
struct commit_name {
struct tag *tag;
@@ -208,6 +217,8 @@ static void describe(const char *arg, int last_one)
display_name(n);
if (longformat)
show_suffix(0, n->tag ? n->tag->tagged->sha1 : sha1);
+ if (dirty)
+ printf("%s", dirty);
printf("\n");
return;
}
@@ -265,7 +276,10 @@ static void describe(const char *arg, int last_one)
if (!match_cnt) {
const unsigned char *sha1 = cmit->object.sha1;
if (always) {
- printf("%s\n", find_unique_abbrev(sha1, abbrev));
+ printf("%s", find_unique_abbrev(sha1, abbrev));
+ if (dirty)
+ printf("%s", dirty);
+ printf("\n");
return;
}
die("cannot describe '%s'", sha1_to_hex(sha1));
@@ -300,6 +314,8 @@ static void describe(const char *arg, int last_one)
display_name(all_matches[0].name);
if (abbrev)
show_suffix(all_matches[0].depth, cmit->object.sha1);
+ if (dirty)
+ printf("%s", dirty);
printf("\n");
if (!last_one)
@@ -324,6 +340,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
"only consider tags matching <pattern>"),
OPT_BOOLEAN(0, "always", &always,
"show abbreviated commit object as fallback"),
+ {OPTION_STRING, 0, "dirty", &dirty, "mark",
+ "append <mark> on dirty working tree (default: \"-dirty\")",
+ PARSE_OPT_OPTARG, NULL, "-dirty"},
OPT_END(),
};
@@ -360,7 +379,11 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
}
if (argc == 0) {
+ if (dirty && !cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1, diff_index_args, prefix))
+ dirty = NULL;
describe("HEAD", 1);
+ } else if (dirty) {
+ die("--dirty is incompatible with committishes");
} else {
while (argc-- > 0) {
describe(*argv++, argc == 0);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 8c7e081..ee38f34 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -123,6 +123,21 @@ test_expect_success 'rename tag Q back to A' '
test_expect_success 'pack tag refs' 'git pack-refs'
check_describe A-* HEAD
+check_describe "A-*[0-9a-f]" --dirty
+
+test_expect_success 'set-up dirty working tree' '
+ echo >>file
+'
+
+check_describe "A-*[0-9a-f]-dirty" --dirty
+
+check_describe "A-*[0-9a-f].mod" --dirty=.mod
+
+test_expect_success 'describe --dirty HEAD' '
+ git describe --dirty HEAD
+ test $? != 0
+'
+
test_expect_success 'set-up matching pattern tests' '
git tag -a -m test-annotated test-annotated &&
echo >>file &&
--
1.6.5
^ permalink raw reply related
* Re: git fsck not identifying corrupted packs
From: Robin Rosenberg @ 2009-10-20 15:41 UTC (permalink / raw)
To: Wesley J. Landaker; +Cc: git
In-Reply-To: <200910191327.49092.wjl@icecavern.net>
måndag 19 oktober 2009 21:27:48 skrev Wesley J. Landaker:
> (Not CCing everyone, since this is mostly curiosa in the "using git as it
> was never intended" section):
>
> On Monday 19 October 2009 13:03:42 Junio C Hamano wrote:
> > Once a packfile is created and we always use it read-only, there didn't
> > seem to be much point in suspecting that the underlying filesystems or
> > disks may corrupt them in such a way that is not caught by the SHA-1
> > checksum over the entire packfile and per object checksum. That trust in
> > the filesystems might have been a good tradeoff between fsck performance
> > and reliability on platforms git was initially developed on and for, but
> > it might not be true anymore as we run on more platforms these days.
>
> Filesystems are mostly reliable, but only until your crazy users do strange
> and terrible things. I have a real, non-toy environment where I use this
> stack as a [horrible] workaround for some issues beyond my control:
>
> git -> ext4 -> lvm -> dmcrypt -> loop -> sshfs -> cygwin sshd -> SMB share
The obvious follow up question here is: Why?
-- robin
^ permalink raw reply
* Re: git hang with corrupted .pack
From: Alex Riesen @ 2009-10-20 15:36 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Nicolas Pitre, Shawn O. Pearce, Andy Isaacson,
git
In-Reply-To: <fabb9a1e0910200823h280a875p98c61eb5e5e6018a@mail.gmail.com>
On Tue, Oct 20, 2009 at 17:23, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Tue, Oct 20, 2009 at 10:14, Alex Riesen <raa.lkml@gmail.com> wrote:
>> - buffer = xmalloc(size + 1);
>> + buffer = xmalloc(size + 8);
>
>> - stream.avail_out = size;
>> + stream.avail_out = size + 8;
>
> That seems wrong at first glance, you go from '+1' to '+8' on the
> first part, and then from '+0' to '+8' in the second part, am I
> missing something obvious?
Yes. The "size" (the variable) is never changed. It is just the output
buffer size, not how many data expected or something like that.
IOW, I just wanted the buffer to be obviously (to zlib) more than enough.
^ permalink raw reply
* Re: git hang with corrupted .pack
From: Sverre Rabbelier @ 2009-10-20 15:23 UTC (permalink / raw)
To: Alex Riesen
Cc: Junio C Hamano, Nicolas Pitre, Shawn O. Pearce, Andy Isaacson,
git
In-Reply-To: <81b0412b0910200814v269e91fbkd7841308685e1c54@mail.gmail.com>
Heya,
On Tue, Oct 20, 2009 at 10:14, Alex Riesen <raa.lkml@gmail.com> wrote:
> - buffer = xmalloc(size + 1);
> + buffer = xmalloc(size + 8);
> - stream.avail_out = size;
> + stream.avail_out = size + 8;
That seems wrong at first glance, you go from '+1' to '+8' on the
first part, and then from '+0' to '+8' in the second part, am I
missing something obvious?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: git hang with corrupted .pack
From: Alex Riesen @ 2009-10-20 15:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nicolas Pitre, Shawn O. Pearce, Andy Isaacson, git
In-Reply-To: <7vbpk985t1.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]
On Thu, Oct 15, 2009 at 09:39, Junio C Hamano <gitster@pobox.com> wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
>
>> I confirm this test without the fix reproduces the infinite loop (and
>> does stall the test suite).
>
> Thanks, both of you.
I seem to have problems with this change (on Cygwin). Sometimes
accessing an object in a pack fails in unpack_compressed_entry.
When it happens, both avail_in and avail_out of the stream are 0,
and the reported status is Z_BUF_ERROR.
Output with the second attached patch:
error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697),
avail_out=0 (was 1256)
error: *** unpack_compressed_entry failed
error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613
at offset 2620741 from
.git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack
fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted
I cannot reproduce the problem on a normal system (a 64bit, reasonably modern
Linux in my case). An attempt to use an upgraded zlib on this cygwin system was
not successful, there was an updated library, but a clean recompile didn't
change anything.
I worked the case around by allocating a bit more than uncompressed data need.
In case someone else also sees the problem, below is how. The size of the
overallocation is arbitrary.
diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..66c2519 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct
packed_git *p,
z_stream stream;
unsigned char *buffer, *in;
- buffer = xmalloc(size + 1);
- buffer[size] = 0;
+ buffer = xmalloc(size + 8);
+ memset(buffer + size, 0, 8);
memset(&stream, 0, sizeof(stream));
stream.next_out = buffer;
- stream.avail_out = size;
+ stream.avail_out = size + 8;
git_inflate_init(&stream);
do {
--
1.6.5.59.g000dd
The problematic repo is a little big to post (it's my cygwin-git repo),
I'll have to find hosting for it first.
[-- Attachment #2: 0001-Workaround-inflate-sometimes-failing-to-unpack-data.diff --]
[-- Type: application/octet-stream, Size: 1341 bytes --]
From 643fdf50e4343c3ce3a4b99183dba8d35a10c877 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 20 Oct 2009 16:42:26 +0200
Subject: [PATCH 1/2] Workaround inflate sometimes fails to unpack data
When it happens, zlib stream's avail_in and avail_out are 0, the returned
status is Z_BUF_ERROR. The values weren't 0 before calling inflate.
error: *** inflate error: 0x862380 size=1256, avail_in=0 (was 697), avail_out=0 (was 1256)
error: *** unpack_compressed_entry failed
error: failed to read object 3296766eb5531ef051ae392114de5d75556f5613 at offset 2620741 from .git/objects/pack/pack-996206790aaefbf4d34c86b3ff546bb924546b7c.pack
fatal: object 3296766eb5531ef051ae392114de5d75556f5613 is corrupted
---
sha1_file.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 4cc8939..66c2519 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1585,11 +1585,11 @@ static void *unpack_compressed_entry(struct packed_git *p,
z_stream stream;
unsigned char *buffer, *in;
- buffer = xmalloc(size + 1);
- buffer[size] = 0;
+ buffer = xmalloc(size + 8);
+ memset(buffer + size, 0, 8);
memset(&stream, 0, sizeof(stream));
stream.next_out = buffer;
- stream.avail_out = size;
+ stream.avail_out = size + 8;
git_inflate_init(&stream);
do {
--
1.6.5.59.g000dd
[-- Attachment #3: 0002-Debugging-strange-failure-in-zlibs-inflate.diff --]
[-- Type: application/octet-stream, Size: 1910 bytes --]
From fc5b47d953f171d7591349acad9a23d3ec683ce9 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 20 Oct 2009 15:53:33 +0200
Subject: [PATCH 2/2] Debugging strange failure in zlibs inflate
---
sha1_file.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 66c2519..1bf240e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1594,10 +1594,24 @@ static void *unpack_compressed_entry(struct packed_git *p,
git_inflate_init(&stream);
do {
in = use_pack(p, w_curs, curpos, &stream.avail_in);
+ unsigned prev_ain = stream.avail_in;
+ unsigned prev_aout = stream.avail_out;
+ if (!stream.avail_in)
+ error("*** avail_in=0, inflate will fail!");
+ if (!stream.avail_out)
+ error("*** avail_out=0, inflate will fail!");
stream.next_in = in;
st = git_inflate(&stream, Z_FINISH);
if (st == Z_BUF_ERROR && (stream.avail_in || !stream.avail_out))
+ {
+ error("*** inflate error: %p size=%lu, "
+ "avail_in=%d (was %u), "
+ "avail_out=%d (was %u)",
+ buffer, size,
+ stream.avail_in, prev_ain,
+ stream.avail_out, prev_aout);
break;
+ }
curpos += stream.next_in - in;
} while (st == Z_OK || st == Z_BUF_ERROR);
git_inflate_end(&stream);
@@ -1654,6 +1668,8 @@ static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
delta_base_cached -= ent->size;
} else {
ret = xmemdupz(ent->data, ent->size);
+ if (!ret)
+ fprintf(stderr, "*** no memory\n");
}
*type = ent->type;
*base_size = ent->size;
@@ -1815,6 +1831,8 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
case OBJ_BLOB:
case OBJ_TAG:
data = unpack_compressed_entry(p, &w_curs, curpos, *sizep);
+ if (!data)
+ error("*** unpack_compressed_entry failed");
break;
default:
data = NULL;
--
1.6.5.59.g000dd
^ permalink raw reply related
* Re: Documentation video for svn-git
From: Wesley J. Landaker @ 2009-10-20 15:04 UTC (permalink / raw)
To: jamesmikedupont@googlemail.com; +Cc: git
In-Reply-To: <ee9cc730910200734t66cfe15emd7314ae443e6ac1c@mail.gmail.com>
On Tuesday 20 October 2009 08:34:21 jamesmikedupont@googlemail.com wrote:
> The purpose is that it will reach more people on youtube,
> and people who are lazy can just listen to the video
> and not everyone has screen reader software available to them at all
> times. I learn alot listening to espeak...
Fair enough. I just wasn't sure if I was missing a huge use case here. =)
^ permalink raw reply
* Update working copy on push without touching several files
From: Alex Amiryan @ 2009-10-20 14:43 UTC (permalink / raw)
To: git
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hello. I am a web site developer. My team is developing web sites
locally using git. Online versions of websites are maintained with git
too. I need to have working copy of my remote git repository (online
version of the site) updated by git push (which I do locally). The
problem is that I have some files there (like database config) that have
to be different from local ones and they must not be updated on git
push. I have made "git update-index --assume-unchanged
incs/config.db.php" but they are not being updated only when I do "git
pull" remotely. I have used some post-update hooks which I found on the
net, but they all do "git reset --hard HEAD" which restores my config
files. Can you please help me to write post-update hook which updates
remote working copy but doesn't touch my assume-unchanged marked files.
- --
Alex Amiryan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAkrdzHMACgkQ1KOfm1RDUTH3eQCgsEy+349Q/BnqLyl+6uQcZ871
lZgAn38ZlB4r5Utdt9PbxH/oCCIU2cjM
=2FQ/
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: Documentation video for svn-git
From: jamesmikedupont @ 2009-10-20 14:34 UTC (permalink / raw)
To: Wesley J. Landaker; +Cc: git
In-Reply-To: <200910200820.18986.wjl@icecavern.net>
The purpose is that it will reach more people on youtube,
and people who are lazy can just listen to the video
and not everyone has screen reader software available to them at all times.
I learn alot listening to espeak...
mike
On Tue, Oct 20, 2009 at 4:20 PM, Wesley J. Landaker <wjl@icecavern.net> wrote:
> On Tuesday 20 October 2009 00:30:49 jamesmikedupont@googlemail.com wrote:
>> I have created a computer reading of Sam's svn-git text :
>>
>> http://www.archive.org/details/SvnGitVideo
>>
>> It runs 1.5 hours.
>>
>> I can also do other texts, also the source is checked in to create
>> them yourselves.
>> https://code.launchpad.net/~jamesmikedupont/introspectorreader/wikipedia-
>>strategy
>
> I'm definite fan free software text-to-speech software. (I know espeak when
> I hear it!). But I have to ask: besides being a cool technology demo, what
> is the use case for this? How is this better/different than, for instance,
> just using a screen-reader on-the-fly?
>
^ permalink raw reply
* [PATCH v2] new --dirty option for git describe
From: Jean Privat @ 2009-10-20 14:27 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn O. Pearce
In-Reply-To: <1255800990-7806-1-git-send-email-jean@pryen.org>
With the --dirty option, git describe works on HEAD but append "-dirty"
if the working tree is dirty. If the working tree is clean, nothing
is appended.
$ git describe --dirty
v1.6.5-15-gc274db7
$ echo >> Makefile
$ git describe --dirty
v1.6.5-15-gc274db7-dirty
--dirty can also be used to specify what is append if the working
tree is dirty.
$ git describe --dirty=.mod
v1.6.5-15-gc274db7.mod
Many build scripts use `git describe` to produce a version number based
on the description of HEAD (on which is based the working tree) + saying
that if the working tree is dirty or no.
This patch helps the writing of such scripts since `git describe --dirty`
do directly the intended thing.
Alternatives specifications:
1 Describe the working tree by default and describe HEAD only if "HEAD"
is explicitly specified
Pro: does the right thing by default (both for users and for scripts)
Pro: is coherent with other git commands that works on the working tree
by default
Con: breaks existing scripts (since the world is not ideal)
2 Use --worktree instead of --dirty
Pro: does what it says: "git describe --worktree" will describe the
working three
Con: is incoherent with other commands that do not require a --worktree
option to work on the working tree
Con: unusable with an optional value: "git describe --worktree=.mod"
is quite unintuitive.
3 Use --dirty as in this patch
Pro: make sense to specify an optional value (what the dirty mark is)
Pro: do not have any of the big cons of previous alternatives
* does not break scripts
* is not inconsistent with other git commands
Pro: has an easy fallback to alternative 1 if the world become suddenly
ideal, or at least allows some kind of future transition plan if
people *really* bother:
1- ask that scripts use either "git describe HEAD" or
"git describe --dirty" (no more "git describe")
2- change default
Once the transition is done, the role of the --dirty option will
just be the way to specify an alternative mark (and a noop if alone)
Signed-off-by: Jean Privat <jean@pryen.org>
---
I tried to integrate the points of view of both Junio and Shawn.
However, I am not sure that what I propose is the right solution.
-J-
---
Documentation/git-describe.txt | 6 ++++++
builtin-describe.c | 25 ++++++++++++++++++++++++-
t/t6120-describe.sh | 15 +++++++++++++++
3 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index b231dbb..5253d86 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -9,6 +9,7 @@ git-describe - Show the most recent tag that is
reachable from a commit
SYNOPSIS
--------
'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] <committish>...
+'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]
DESCRIPTION
-----------
@@ -27,6 +28,11 @@ OPTIONS
<committish>...::
Committish object names to describe.
+--dirty[=<mark>]::
+ Describe the working tree.
+ It means describe HEAD and appends <mark> (`-dirty` by
+ default) if the working tree is dirty.
+
--all::
Instead of using only the annotated tags, use any ref
found in `.git/refs/`. This option enables matching
diff --git a/builtin-describe.c b/builtin-describe.c
index df67a73..84af981 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -5,12 +5,14 @@
#include "builtin.h"
#include "exec_cmd.h"
#include "parse-options.h"
+#include "diff.h"
#define SEEN (1u<<0)
#define MAX_TAGS (FLAG_BITS - 1)
static const char * const describe_usage[] = {
"git describe [options] <committish>*",
+ "git describe [options] --dirty",
NULL
};
@@ -23,6 +25,13 @@ static int max_candidates = 10;
static int found_names;
static const char *pattern;
static int always;
+static const char *dirty;
+
+/* diff-index command arguments to check if working tree is dirty. */
+static const char *diff_index_args[] = {
+ "diff-index", "--quiet", "HEAD", "--", NULL
+};
+
struct commit_name {
struct tag *tag;
@@ -208,6 +217,8 @@ static void describe(const char *arg, int last_one)
display_name(n);
if (longformat)
show_suffix(0, n->tag ? n->tag->tagged->sha1 : sha1);
+ if (dirty)
+ printf("%s", dirty);
printf("\n");
return;
}
@@ -265,7 +276,10 @@ static void describe(const char *arg, int last_one)
if (!match_cnt) {
const unsigned char *sha1 = cmit->object.sha1;
if (always) {
- printf("%s\n", find_unique_abbrev(sha1, abbrev));
+ printf("%s", find_unique_abbrev(sha1, abbrev));
+ if (dirty)
+ printf("%s", dirty);
+ printf("\n");
return;
}
die("cannot describe '%s'", sha1_to_hex(sha1));
@@ -300,6 +314,8 @@ static void describe(const char *arg, int last_one)
display_name(all_matches[0].name);
if (abbrev)
show_suffix(all_matches[0].depth, cmit->object.sha1);
+ if (dirty)
+ printf("%s", dirty);
printf("\n");
if (!last_one)
@@ -324,6 +340,9 @@ int cmd_describe(int argc, const char **argv,
const char *prefix)
"only consider tags matching <pattern>"),
OPT_BOOLEAN(0, "always", &always,
"show abbreviated commit object as fallback"),
+ {OPTION_STRING, 0, "dirty", &dirty, "mark",
+ "append <mark> on dirty working tree (default: \"-dirty\")",
+ PARSE_OPT_OPTARG, NULL, "-dirty"},
OPT_END(),
};
@@ -360,7 +379,11 @@ int cmd_describe(int argc, const char **argv,
const char *prefix)
}
if (argc == 0) {
+ if (dirty && !cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
diff_index_args, prefix))
+ dirty = NULL;
describe("HEAD", 1);
+ } else if (dirty) {
+ die("--dirty is incompatible with committishes");
} else {
while (argc-- > 0) {
describe(*argv++, argc == 0);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 8c7e081..ee38f34 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -123,6 +123,21 @@ test_expect_success 'rename tag Q back to A' '
test_expect_success 'pack tag refs' 'git pack-refs'
check_describe A-* HEAD
+check_describe "A-*[0-9a-f]" --dirty
+
+test_expect_success 'set-up dirty working tree' '
+ echo >>file
+'
+
+check_describe "A-*[0-9a-f]-dirty" --dirty
+
+check_describe "A-*[0-9a-f].mod" --dirty=.mod
+
+test_expect_success 'describe --dirty HEAD' '
+ git describe --dirty HEAD
+ test $? != 0
+'
+
test_expect_success 'set-up matching pattern tests' '
git tag -a -m test-annotated test-annotated &&
echo >>file &&
--
1.6.5
^ permalink raw reply related
* Re: [PATCH v2] new --dirty option for git describe
From: Jean Privat @ 2009-10-20 14:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn O. Pearce
In-Reply-To: <dffdbd190910200727r30e161ffka0b3cf764be26cd8@mail.gmail.com>
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -9,6 +9,7 @@ git-describe - Show the most recent tag that is
> reachable from a commit
%#$@!&&@ !!!
I followed instructions from Documentation/SubmittingPatches about
sending patches trought gmail imap, it did not seem to work.
I will resend with git-send-email (and try on something else that
git@vger.kernel.org)
Sorry for that.
-J
^ permalink raw reply
* Re: Documentation video for svn-git
From: Wesley J. Landaker @ 2009-10-20 14:20 UTC (permalink / raw)
To: jamesmikedupont@googlemail.com; +Cc: git
In-Reply-To: <ee9cc730910192330i6c593143w5f35a7a1f66810a1@mail.gmail.com>
On Tuesday 20 October 2009 00:30:49 jamesmikedupont@googlemail.com wrote:
> I have created a computer reading of Sam's svn-git text :
>
> http://www.archive.org/details/SvnGitVideo
>
> It runs 1.5 hours.
>
> I can also do other texts, also the source is checked in to create
> them yourselves.
> https://code.launchpad.net/~jamesmikedupont/introspectorreader/wikipedia-
>strategy
I'm definite fan free software text-to-speech software. (I know espeak when
I hear it!). But I have to ask: besides being a cool technology demo, what
is the use case for this? How is this better/different than, for instance,
just using a screen-reader on-the-fly?
^ permalink raw reply
* Re: Git rebase using "wrong" commit
From: Thomas Rast @ 2009-10-20 13:21 UTC (permalink / raw)
To: Philip Allison; +Cc: git, Thomas Adam
In-Reply-To: <1256037982.7122.31.camel@gridbug.soton.smoothwall.net>
Philip Allison wrote:
> Please find attached a working copy of the repository, just before any
> attempted rebase.
[...]
> bug/2 has, effectively, been merged into master; bug/1 has not; hence,
> there is not (on master) any equivalent change to that which was made to
> resolve the conflict.
Ok, so I gather the (simplified) history looks like
*------A (master)
\\__ \
\ \ \
B--M1--M2 (topic)
where A and B conflict, so M2 is nontrivial. B is the *second* parent
to M1, which I originally thought would affect the 'rebase -p', but it
doesn't; see below. (You're talking about bug/2, but the repository
shows bug/2 == master, so there are only two branches involved.)
> Occasionally, we want to release some bug fixes, but not others. IOW,
> when integration/bug-fixes comes to be rebased onto master, we wish to
> preserve some of the topic branch merges, but not others. This usually
> works fine, but an issue has cropped up where there is a conflict
> between two fixes, and only one of the two bug fixes has been released;
> now, when we come to perform the rebase, it is not working as desired.
The inherent difficulty with doing a rebase here is, what about the
merges? By definition, git-rebase needs to somehow "rebuild" the
commits, as defined by the changes they do, on the new base.
There are several ways how merges can be rebased, and you tried some:
* Not at all; discard the merges. The result would be
*---A (master)
\
\
B' (topic)
B' will differ substantially from B because there will be a (rebase)
conflict. This is the normal mode of operation, as you noticed:
> "git rebase -i master" output (whilst on HEAD of integration/bug-fixes):
>
> -----
> pick b17a93c Fix file 1
>
> # Rebase cd8273f..9f79ca3 onto cd8273f
> -----
* Attempt to rebuild merges; this is the -p flag. Assume for a second
that you have a different history
*---A (master)
\
\
C---M2 (topic2)
/
*
where M2 is a merge with some other branch. 'git rebase -i -p
master topic2' will attempt to build
*---A (master)
\
\
C---M2 (topic2)
/
*
Going back to your scenario, this means building
*------A--. (master)
\ \ \
\ \ \
B-----M1--M2 (topic)
Now there appears to be some bug with rebase -p, because it insists
that there is no work at all to do (the buffer is just 'noop').
However, rebase -p is known to be somewhat ill-defined and broken.
I think that in this case, what actually happens is that the merge
from master to topic confuses the initialisation of $REWRITTEN in
# line 668
for c in $(git merge-base --all $HEAD $UPSTREAM)
do
echo $ONTO > "$REWRITTEN"/$c || die "blah"
done
because suddenly the merge-base is A, meaning that the later test
# line 712
for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
do
if test -f "$REWRITTEN"/$p -a \( $p != $ONTO -o $sha1 = $first_after_upstream \)
fails for all commits except M2. So what's *actually* rewritten is
M2; but doing so is pointless in rebase's eyes, because it assumes
the topic will be based on master eventually, so merging master into
it is a no-op. Hence there is nothing to do.
> The "-m" and "-p" options to rebase don't seem to be helping. I have
> tried getting rebase to "pick 2bc19f9" via -i, but that isn't working
> either:
-m means something entirely different, namely that git-rebase should
use an (internal) merge instead of format-patch|am, which has some
advantages but a big speed disadvantage. This doesn't matter for
rebase -i as it never uses format-patch|am.
As for manually picking merge commits, that fails because you're just
trying to fool git-cherry-pick into doing something it can't.
> We have a specific branch (integration/bug-fixes) which is under
> continual rebase, shared amongst several developers.
[...]
> At the moment I don't know where to go from here, short of manually
> recreating the branch as I want it, which I am loathe to do.
Note that there is no real reason to rebase continually unless you
want to kick out flawed topics or old versions of branches. For
example, git.git only rebases 'next' immediately after a release.
Furthermore, even if you go for the 'pu' model which is rebuilt all
the time, you can automate this with a script. There is one in the
'todo' branch of git.git, called PU.
(I myself just use a list of topics I want merged, and a simple 'git
merge $topic' loop.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] blame: make sure that the last line ends in an LF
From: Sverre Rabbelier @ 2009-10-20 13:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Johannes Schindelin
In-Reply-To: <7vbpk2sg6m.fsf@alter.siamese.dyndns.org>
Heya,
On Tue, Oct 20, 2009 at 02:00, Junio C Hamano <gitster@pobox.com> wrote:
> (2) Do the right thing, by coming up with a notation to show that the
> final line is incomplete, perhaps similar to "\No newline ..."
> notation used by "diff".
What about 'git blame -p', can we just go about changing the format like that?
For purpose of the discussion below let's assume we squash in the following:
-- <8 --
diff --git a/builtin-blame.c b/builtin-blame.c
index dd16b22..cf492a0 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1606,7 +1606,7 @@ static void emit_porcelain(struct scoreboard
*sb, struct blame_entry *ent)
}
if (sb->final_buf_size && cp[-1] != '\n')
- putchar('\n');
+ printf("\n\\ No newline at end of file\n");
}
static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
@@ -1672,7 +1672,7 @@ static void emit_other(struct scoreboard *sb,
struct blame_entry *ent, int opt)
}
if (sb->final_buf_size && cp[-1] != '\n')
- putchar('\n');
+ printf("\n\\ No newline at end of file\n");
}
static void output(struct scoreboard *sb, int option)
--
-- <8 --
> Does the code assign blame
> correctly around the last line of the original blob?
Yes, it does, when there is no trailing newline an extra "\ No newline
at end of file" is printed, but the last line is still attributed
correctly.
> What if an older
> version ended with an incomplete line and a later version changed the line
> (without adding the terminating LF)?
Nothing changes, the blame on that last line is attributed correctly
and the "\ No newline at end of file" is printed.
> What if a later version changed the
> line and added the terminating LF?
The trailing "\ No newline at end of file" is no longer printed and
the last line is correctly attributed to the commit that added the
trailing LF.
> What if a later version only added the
> terminating LF and did nothing else? Are these three cases handled
> correctly?
Same as above.
> After thinking issues like the above, I read the patch and I see it does
> not take neither approach. That makes me feel nervous.
Reading your reply I see that if you care about the presence (or
absence) of a trailing newline the current patch would be problematic,
as it makes it impossible to see in the blame output whether there was
a trailing newline or not.
> By tweaking only the output routine you _might_ be getting correct output,
> but even then it looks to me like the end result is working correctly not
> by design but by accident. IOW, the patch may be better than nothing, but
> somehow it just feels like it is papering over the real issue than being a
> proper fix.
>
> Or am I worrying too much?
No, I think your concerns are valid, we should go with (2) and DTRT.
Does the updated patch address your concerns? If so I can send a new
version.
--
Cheers,
Sverre Rabbelier
^ 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