* Re: [RFC/PATCH 3/7] replace_object: add mechanism to replace objects found in "refs/replace/"
From: Christian Couder @ 2009-01-15 9:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4esf0tv.fsf@gitster.siamese.dyndns.org>
Le mardi 13 janvier 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > diff --git a/replace_object.c b/replace_object.c
> > new file mode 100644
> > index 0000000..25b3ef3
> > --- /dev/null
> > +++ b/replace_object.c
> > @@ -0,0 +1,116 @@
> > +#include "cache.h"
> > +#include "refs.h"
> > +
> > +static struct replace_object {
> > + unsigned char sha1[2][20];
> > +} **replace_object;
> > +
> > +static int replace_object_alloc, replace_object_nr;
> > +
> > +static int replace_object_pos(const unsigned char *sha1)
> > +{
> > + int lo, hi;
> > + lo = 0;
> > + hi = replace_object_nr;
> > + while (lo < hi) {
> > + int mi = (lo + hi) / 2;
> > + struct replace_object *rep = replace_object[mi];
> > + int cmp = hashcmp(sha1, rep->sha1[0]);
> > + if (!cmp)
> > + return mi;
> > + if (cmp < 0)
> > + hi = mi;
> > + else
> > + lo = mi + 1;
> > + }
> > + return -lo - 1;
> > +}
>
> Hmm, this is a tangent of this topic, but I wonder if we can do something
> more generic to factor out many binary search like this we have
> throughout the code. Also I wonder if they can be made more efficient by
> taking advantage of the fact that our hash is expected to produce a good
> uniform distribution, similar to the way patch-ids.c::patch_pos() does
> this.
>
> I guess the performance should not matter much for this table, as we
> expect there won't be massive object replacements.
>
> Also I recall Dscho muttered something about hashmap I didn't quite
> understand.
Yeah, maybe it's possible to get faster code and to refactor the
many binary search we have, but I will leave that for latter or for other
people interested in these topics, if you let me.
> > +static int register_replace_ref(const char *refname,
> > + const unsigned char *sha1,
> > + int flag, void *cb_data)
> > +{
> > + /* Get sha1 from refname */
> > + const char *slash = strrchr(refname, '/');
> > + const char *hash = slash ? slash + 1 : refname;
> > + struct replace_object * repl_obj = xmalloc(sizeof(*repl_obj));
>
> struct replace_object *repl_obj = ...
Ok.
> > + if (strlen(hash) != 40 || get_sha1_hex(hash, repl_obj->sha1[0])) {
> > + free(repl_obj);
> > + warning("bad replace ref name: %s", refname);
> > + }
> > +
> > + /* Copy sha1 from the read ref */
> > + hashcpy(repl_obj->sha1[1], sha1);
>
> Upon an error, you free and warn and then still copy into it?
Ooops, I forgot a "return 0;" statement after the warning.
> > + /* Register new object */
> > + if (register_replace_object(repl_obj, 1))
> > + warning("duplicate replace ref: %s", refname);
>
> I'd say this is a grave error and should be reported as a repository
> corruption.
If we let people have a set of replace refs in "refs/replace/" and another
one in "refs/replace/bisect/", and the latter one is used only when
bisecting, then it may happen that the same commit has one ref
in "refs/replace/" and another one in "refs/replace/bisect/". In this case
it should probably be considered as something we should not even warn on,
and the replace ref in "refs/replace/bisect/" should be used when
bisecting.
But, as we don't have a mechanism to do that yet, you are right, we should
probably "die" for now here.
> > +static void prepare_replace_object(void)
> > +{
> > + static int replace_object_prepared;
> > +
> > + if (replace_object_prepared)
> > + return;
> > +
> > + for_each_replace_ref(register_replace_ref, NULL);
> > + replace_object_prepared = 1;
> > +}
> > +
> > +/* We allow "recursive" replacement. Only within reason, though */
> > +#define MAXREPLACEDEPTH 5
> > +
> > +const unsigned char *lookup_replace_object(const unsigned char *sha1)
> > +{
> > + int pos, depth = MAXREPLACEDEPTH;
> > + const unsigned char *cur = sha1;
> > +
> > + prepare_replace_object();
> > +
> > + /* Try to recursively replace the object */
> > + do {
> > + if (--depth < 0)
> > + die("replace depth too high for object %s",
> > + sha1_to_hex(sha1));
> > +
> > + pos = replace_object_pos(cur);
> > + if (0 <= pos)
> > + cur = replace_object[pos]->sha1[1];
> > + } while (0 <= pos);
> > +
> > + return cur;
> > +}
>
> Since your paradigm is prepare replacement once at the beginning, never
> allowing to update it, I think you can update the table while you look it
> up. E.g. if A->B and B->C exists, and A is asked for, you find A->B (to
> tentatively make cur to point at B) and then you find B->C, and before
> returning you can rewrite the first mapping to A->C. Later look-up won't
> need to dereference the table twice that way.
>
> This assumes that there will be small number of replacements, but the
> same object can be asked for more than once during the process.
If we allow different sets of replace refs, for example A->B
in "refs/replace/" and B->C in "refs/replace/bisect/", then we cannot
rewrite as you suggest. We could add A->C in "refs/replace/bisect/", so
that it overcomes A->B and B->C when we bisect, but we would not gain much.
So I prefer not to do that right now. Maybe later if we decide we really
don't want to allow different sets of replace refs, we can do what you
suggest.
> > diff --git a/sha1_file.c b/sha1_file.c
> > index f08493f..4f2fd10 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -2163,10 +2163,18 @@ static void *read_object(const unsigned char
> > *sha1, enum object_type *type, void *read_sha1_file(const unsigned char
> > *sha1, enum object_type *type, unsigned long *size)
> > {
> > - void *data = read_object(sha1, type, size);
> > + const unsigned char *repl = lookup_replace_object(sha1);
> > + void *data = read_object(repl, type, size);
> > +
> > + /* die if we replaced an object with one that does not exist */
> > + if (!data && repl != sha1)
> > + die("replacement %s not found for %s",
> > + sha1_to_hex(repl), sha1_to_hex(sha1));
> > +
> > /* legacy behavior is to die on corrupted objects */
> > - if (!data && (has_loose_object(sha1) || has_packed_and_bad(sha1)))
> > - die("object %s is corrupted", sha1_to_hex(sha1));
> > + if (!data && (has_loose_object(repl) || has_packed_and_bad(repl)))
> > + die("object %s is corrupted", sha1_to_hex(repl));
> > +
> > return data;
> > }
>
> Later we'd need a global switch to forbid the replacement for
> connectivity walkers,
Yeah, I am slowly working on it. My next series (hopefully in a few days)
where the above errors are fixed will include it.
> but other than that I think this is sane.
>
> I also looked at 1/ and 2/ which looked Ok.
Thanks,
Christian.
^ permalink raw reply
* Re: [StGit] import --mail/--mbox question
From: Karl Hasselström @ 2009-01-15 9:49 UTC (permalink / raw)
To: Shinya Kuribayashi, Catalin Marinas; +Cc: git
In-Reply-To: <496E0656.2090705@ruby.dti.ne.jp>
On 2009-01-15 00:35:50 +0900, Shinya Kuribayashi wrote:
> Question: when importing Mozilla Thunderbird mails (eml or mbox),
> the imported patch always have committer's date in git log, not
> submitter's date. However, if importing the same mails with git am,
> we could see submitter's date in git log.
>
> Is this intentionally-designed log management of StGit? I would
> expect the submitter's date & locale is kept when importing patches
> from e-mails.
Hmm. I agree with you that the expected behavior when importing
patches from e-mails is that the author date is set to the date in the
e-mail, the way git-am does it.
Looking at the code, it looks like the intention is that it should
work that way, too. So I guess it's simply a bug. Catalin, do you have
a clue?
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Johan Herland @ 2009-01-15 10:01 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Anders Melchiorsen, gitster
In-Reply-To: <496EE74F.6000205@viscovery.net>
On Thursday 15 January 2009, Johannes Sixt wrote:
> Johannes Schindelin schrieb:
> > On Thu, 15 Jan 2009, Anders Melchiorsen wrote:
> >> Previously, the interactive rebase edit mode placed the user after the
> >> commit in question. That was awkward because a commit is supposedly
> >> immutable. Thus, she was forced to use "git commit --amend" for her
> >> changes.
> >
> > Maybe, maybe not. I frequently rebase with "edit" when I actually mean
> > "stop" (but "s" was taken from "squash" already). Then I test things,
> > possibly fixing them.
> >
> > So in that case, I do not want a git reset --soft HEAD^.
>
> Absolutely! I use "edit" for this purpose as well quite frequently.
What about providing both options?
"modify" does the "git reset --soft HEAD^" (Anders' suggestion)
"amend" requires a "git commit --amend" (current behaviour)
"edit" == "amend", but is deprecated and goes away in the future
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* rebase -p confusion in 1.6.1
From: Sitaram Chamarty @ 2009-01-15 10:39 UTC (permalink / raw)
To: git
Hello all,
While trying to understand "rebase -p", I came across some
very unexpected behaviour that made me throw in the towel
and ask for help!
The outputs I got really confused me. Before the "rebase
-p", the tree looked like
* 78ffda9... (refs/heads/work) b4
* be1e3a4... b3
* cd8d893... Merge branch 'master' into work
|\
| * 0153c27... (refs/heads/master) a4
| * 74f4387... a3
* | f1b0c1c... b2
* | 2e202d0... b1
|/
* b37ae36... a2
* ed1e1bc... a1
But afterward, this is what it looks like -- all the "b"
commits are gone!
* 0153c27... (refs/heads/work, refs/heads/master) a4
* 74f4387... a3
* b37ae36... a2
* ed1e1bc... a1
What did I do wrong/misunderstand?
Here's how to recreate. Note that "testci" is a shell
function and "lg" is a git alias. They are, respectively,
(1) testci() { for i; do echo $i > $i; git add $i; git commit -m $i; done; }
(2) git config alias.lg log --graph --pretty=oneline --abbrev-commit --decorate
git init
testci a1 a2
git checkout -b work
testci b1 b2
git checkout master
testci a3 a4
git checkout work
git merge master
testci b3 b4
git --no-pager lg # graph before rebase -p
git rebase -p master
git --no-pager lg # graph after rebase -p
^ permalink raw reply
* Re: [PATCH replacement for take 3 4/4] color-words: take an optional regular expression describing words
From: Thomas Rast @ 2009-01-15 10:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Santi Béjar, Junio C Hamano, Teemu Likonen
In-Reply-To: <200901150930.38100.trast@student.ethz.ch>
[-- Attachment #1: Type: text/plain, Size: 3984 bytes --]
Thomas Rast wrote:
> Johannes Schindelin wrote:
> > If a hunk header '@@ -2 +1,0 @@' is found that logically should be
> > '@@ -2 +2,0 @@', diff_words got confused.
> [...]
> > This might be a libxdiff issue, though.
>
> Looks like it's just bug-for-bug compatible with diff. At least my
> GNU diffutils 2.8.7 show the same behaviour.
I think the culprit is in
commit ca557afff9f7dad7a8739cd193ac0730d872e282
Author: Davide Libenzi <davidel@xmailserver.org>
Date: Mon Apr 3 18:47:55 2006 -0700
Clean-up trivially redundant diff.
Also corrects the line numbers in unified output when using
zero lines context.
[...]
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
[...]
@@ -244,7 +257,7 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,
memcpy(buf, "@@ -", 4);
nb += 4;
- nb += xdl_num_out(buf + nb, c1 ? s1: 0);
+ nb += xdl_num_out(buf + nb, c1 ? s1: s1 - 1);
if (c1 != 1) {
memcpy(buf + nb, ",", 1);
@@ -256,7 +269,7 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,
memcpy(buf + nb, " +", 2);
nb += 2;
- nb += xdl_num_out(buf + nb, c2 ? s2: 0);
+ nb += xdl_num_out(buf + nb, c2 ? s2: s2 - 1);
if (c2 != 1) {
memcpy(buf + nb, ",", 1);
Note how (for some reason I don't quite understand yet) "correcting"
the offsets involves subtracting 1 if there were no changes on that
side.
But skipping ahead to the end doesn't work if there are several such
instances where nothing was added. So I think it must be fixed as
follows.
---- 8< ----
diff --git a/diff.c b/diff.c
index 4174d88..d7bbf74 100644
--- a/diff.c
+++ b/diff.c
@@ -361,8 +361,9 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
diff_words->plus.orig[plus_first + plus_len - 1].end;
/*
- * since this is a --unified=0 diff, it can result in a single hunk
- * with a header like this: @@ -2 +1,0 @@
+ * libxdiff subtracts one from the offset if the corresponding
+ * length is 0. (This can only happen because we use
+ * --unified=0.)
*
* This breaks the assumption that minus_first == plus_first.
*
@@ -373,10 +374,9 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
* It is only necessary for the plus part, as we show the common
* words from that buffer.
*/
- if (plus_len == 0 && minus_first + minus_len
- == diff_words->minus.orig_nr)
+ if (plus_len == 0)
plus_begin = plus_end =
- diff_words->plus.orig[diff_words->plus.orig_nr - 1].end;
+ diff_words->plus.orig[plus_first + plus_len].end;
if (diff_words->current_plus != plus_begin)
fwrite(diff_words->current_plus,
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 744221b..875b464 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -156,4 +156,40 @@ test_expect_success 'test when words are only removed at the end' '
'
+echo 'abcd(Xefghijklmn(YZopqrst' > pre
+echo 'abcd(efghijklmn(opqrst' > post
+
+cat > expect <<\EOF
+<WHITE>diff --git a/pre b/post<RESET>
+<WHITE>index 434ff54..c4bb9f1 100644<RESET>
+<WHITE>--- a/pre<RESET>
+<WHITE>+++ b/post<RESET>
+<BROWN>@@ -1 +1 @@<RESET>
+abcd(<RED>X<RESET>efghijklmn(<RED>YZ<RESET>opqrst
+EOF
+
+test_expect_success 'no added words' '
+
+ word_diff --color-words=.
+
+'
+
+echo 'abcd(efghijklmn(opqrst' > pre
+echo 'abcd(Xefghijklmn(YZopqrst' > post
+
+cat > expect <<\EOF
+<WHITE>diff --git a/pre b/post<RESET>
+<WHITE>index c4bb9f1..434ff54 100644<RESET>
+<WHITE>--- a/pre<RESET>
+<WHITE>+++ b/post<RESET>
+<BROWN>@@ -1 +1 @@<RESET>
+abcd(<GREEN>X<RESET>efghijklmn(<GREEN>YZ<RESET>opqrst
+EOF
+
+test_expect_success 'no removed words' '
+
+ word_diff --color-words=.
+
+'
+
test_done
--
1.6.1.283.g653b2
--
Thomas Rast
trast@{inf,student}.ethz.ch
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: [BUG] assertion failure in builtin-mv.c with "git mv -k"
From: Michael J Gruber @ 2009-01-15 10:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Matthieu Moy, git
In-Reply-To: <7vbpu91zjf.fsf@gitster.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 14.01.2009 20:02:
...
> If "git mv" did not have adequate test coverage, then please add a test
> script with both expect_success (for cases that should have been there
> when somebody worked on "git mv" originally), and expect_failure to expose
> the bug you found in your first patch. Again, the second patch would
> update the code and flip expect_failure to expect_success.
>
> I see there is t7001-mv and even though it claims to concentrate on its
> operation from subdirectory, it has tests for more basic modes of the
> operation.
>
> So my recommendation would be to have a single patch that:
>
> (1) retitles t7001;
> (2) adds your new -k tests to it; and
> (3) adds the 1-liner fix.
>
Sorry to bother you again, even after your detailed reply, but I'm a bit
confused in multiple ways (I guess that makes for multiple bits...).
First, you replied to my post, not my patch v2, but (time-wise) after my
patch v2, so I'm not sure whether your reply applies to v2 as well or
that one is OK.
Second, I took the title of t7001 to mean "mv into subdir" or "mv in and
out subdir" in order to distiguish it from "mv oldname newname", albeit
in English that leaves room from improvement.
Third, various parts of that test are in vastly different styles, and I
haven't found any test writing directions other than "follow what is
there", which leaves several alternatives. (Some use the test-lib repo,
some create their own underneath, some make assumptions about the
contents of "$TEST_DIRECTORY"/../.)
Fourth, t7001-mv is missing any test for "mv -k", and 2 of my 3
additional tests cover cases which work (pass) without the fix, which is
why I kept it separate, being unrelated. Following all your arguments
lead to the conclusion I should squash 2+3 (fix + mark expect_pass) -
until I read your conclusion, which says squash all.
I'm happy to follow any variant ("1+2+3", "1 2+3", "1 2 3", in
increasing order of preference) so there's no need to discuss or explain
this further, just tell me "do x" ;)
Cheers,
Michael
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Sverre Rabbelier @ 2009-01-15 11:52 UTC (permalink / raw)
To: Johan Herland
Cc: git, Johannes Sixt, Johannes Schindelin, Anders Melchiorsen,
gitster
In-Reply-To: <200901151101.53441.johan@herland.net>
On Thu, Jan 15, 2009 at 11:01, Johan Herland <johan@herland.net> wrote:
> "modify" does the "git reset --soft HEAD^" (Anders' suggestion)
> "amend" requires a "git commit --amend" (current behaviour)
Why have amend do the same as edit? If you add an 'amend' one instead
make it drop you into an editor to change the commit message. That's a
workflow I often use. Often times I do not have a proper commit
message when I commit (sometimes it is the result of "git commit -a -m
"tmp"). To me having an 'amend' command that allows one to edit the
commit message would make sense a lot :).
> "edit" == "amend", but is deprecated and goes away in the future
And as such, have edit do what it currently does.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Johannes Schindelin @ 2009-01-15 12:24 UTC (permalink / raw)
To: Miles Bader
Cc: Boyd Stephen Smith Jr., Junio C Hamano, Anders Melchiorsen, git
In-Reply-To: <buo8wpdfbv9.fsf@dhapc248.dev.necel.com>
Hi,
On Thu, 15 Jan 2009, Miles Bader wrote:
> [I do wonder how on earth the current awkward behavior was accepted in
> the first place...]
Thanks for the praise! It is always nice to hear that one's work is
appreciated.
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Johannes Schindelin @ 2009-01-15 12:36 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Johan Herland, git, Johannes Sixt, Anders Melchiorsen, gitster
In-Reply-To: <bd6139dc0901150352t2d2fa388x3eb842bbc8c4baa6@mail.gmail.com>
Hi,
On Thu, 15 Jan 2009, Sverre Rabbelier wrote:
> On Thu, Jan 15, 2009 at 11:01, Johan Herland <johan@herland.net> wrote:
> > "modify" does the "git reset --soft HEAD^" (Anders' suggestion)
I could live with "modify".
> > "amend" requires a "git commit --amend" (current behaviour)
>
> Why have amend do the same as edit? If you add an 'amend' one instead
> make it drop you into an editor to change the commit message. That's a
> workflow I often use. Often times I do not have a proper commit
> message when I commit (sometimes it is the result of "git commit -a -m
> "tmp"). To me having an 'amend' command that allows one to edit the
> commit message would make sense a lot :).
>
> > "edit" == "amend", but is deprecated and goes away in the future
>
> And as such, have edit do what it currently does.
FWIW I fully agree.
If at all, I'd introduce 'examine' as a synonym to 'edit' (might be more
intuitive).
However, for the same reason (is it intuitive?) I am not fully convinced
of 'amend' either. Because --amend _can_ mean that you change the
diff of the commit. Maybe 'correct', 'redact' or 'rephrase'?
BTW I was not fully happy with 'edit' back then, either, which is the
reason why I showed the usage in the comment _above_ the commit list. But
nobody could suggest a name that I found convincingly better.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH take 3 0/4] color-words improvements
From: Johannes Schindelin @ 2009-01-15 12:41 UTC (permalink / raw)
To: Teemu Likonen; +Cc: Thomas Rast, Junio C Hamano, git, Santi Béjar
In-Reply-To: <8763khtbfc.fsf@iki.fi>
Hi,
On Thu, 15 Jan 2009, Teemu Likonen wrote:
> Johannes Schindelin (2009-01-14 23:06 +0100) wrote:
>
> > On Wed, 14 Jan 2009, Thomas Rast wrote:
> >> -aaa [aaa]
> >> +aaa (aaa) aaa
> >>
> >> would still give you
> >>
> >> aaa (aaa)<GREEN> aaa<RESET>
> >>
> >> which may be unexpected.
> >
> > But why should it be unexpected? If people say that every length of "a"
> > makes a word, and consequently everything else is clutter, then that's
> > that, no?
>
> It works logically but I'd very much like to see a some kind of advice
> in the man page. I already faced this (unexpected) situation and wasn't
> able to fix the regexp myself.
Exactly because it works logically, I do not want to change it. This is
what the user said, and for a change, it could be what the user meant.
You'll have to come up with a method to describe exactly what you want.
So what is it exactly? What would you want in such a situation? You
asked for words that consist solely of the letter 'a'. Now, the
surrounding stuff differs. What should Git do?
BTW this gets even worse when you compare the following:
bbb aaa
ccc aaa
--color-words=a+ will show
ccc aaa
(!!!)
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Adeodato Simó @ 2009-01-15 12:44 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Johan Herland, git, Johannes Sixt,
Anders Melchiorsen, gitster
In-Reply-To: <alpine.DEB.1.00.0901151325310.3586@pacific.mpi-cbg.de>
* Johannes Schindelin [Thu, 15 Jan 2009 13:36:10 +0100]:
> However, for the same reason (is it intuitive?) I am not fully convinced
> of 'amend' either. Because --amend _can_ mean that you change the
> diff of the commit.
Right.
> Maybe 'correct', 'redact' or 'rephrase'?
editmsg?
--
Adeodato Simó dato at net.com.org.es
Debian Developer adeodato at debian.org
He who has not a good memory should never take upon himself the trade of lying.
-- Michel de Montaigne
^ permalink raw reply
* Re: [PATCH replacement for take 3 4/4] color-words: take an optional regular expression describing words
From: Johannes Schindelin @ 2009-01-15 12:54 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Santi Béjar, Junio C Hamano, Teemu Likonen
In-Reply-To: <200901151140.20215.trast@student.ethz.ch>
Hi,
On Thu, 15 Jan 2009, Thomas Rast wrote:
> Thomas Rast wrote:
> > Johannes Schindelin wrote:
> > > If a hunk header '@@ -2 +1,0 @@' is found that logically should be
> > > '@@ -2 +2,0 @@', diff_words got confused.
> > [...]
> > > This might be a libxdiff issue, though.
> >
> > Looks like it's just bug-for-bug compatible with diff. At least my
> > GNU diffutils 2.8.7 show the same behaviour.
>
> I think the culprit is in
>
> commit ca557afff9f7dad7a8739cd193ac0730d872e282
> Author: Davide Libenzi <davidel@xmailserver.org>
> Date: Mon Apr 3 18:47:55 2006 -0700
>
> Clean-up trivially redundant diff.
>
> Also corrects the line numbers in unified output when using
> zero lines context.
> [...]
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> [...]
> @@ -244,7 +257,7 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2,
> memcpy(buf, "@@ -", 4);
> nb += 4;
>
> - nb += xdl_num_out(buf + nb, c1 ? s1: 0);
> + nb += xdl_num_out(buf + nb, c1 ? s1: s1 - 1);
Junio mentioned some POSIX document in which this behavior is actually
required. So I'll fix my code thusly:
-- snipsnap --
diff --git a/diff.c b/diff.c
index 3709651..219a242 100644
--- a/diff.c
+++ b/diff.c
@@ -353,30 +353,20 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
&minus_first, &minus_len, &plus_first, &plus_len))
return;
- minus_begin = diff_words->minus.orig[minus_first].begin;
- minus_end = minus_len == 0 ? minus_begin :
- diff_words->minus.orig[minus_first + minus_len - 1].end;
- plus_begin = diff_words->plus.orig[plus_first].begin;
- plus_end = plus_len == 0 ? plus_begin :
- diff_words->plus.orig[plus_first + plus_len - 1].end;
+ /* POSIX requires that first be decremented by one if len == 0... */
+ if (minus_len) {
+ minus_begin = diff_words->minus.orig[minus_first].begin;
+ minus_end =
+ diff_words->minus.orig[minus_first + minus_len - 1].end;
+ } else
+ minus_begin = minus_end =
+ diff_words->minus.orig[minus_first].end;
- /*
- * since this is a --unified=0 diff, it can result in a single hunk
- * with a header like this: @@ -2 +1,0 @@
- *
- * This breaks the assumption that minus_first == plus_first.
- *
- * So we have to fix it: whenever we reach the end of pre and post
- * texts, but nothing was added, we need to shift the plus part
- * to the end of the buffer.
- *
- * It is only necessary for the plus part, as we show the common
- * words from that buffer.
- */
- if (plus_len == 0 && minus_first + minus_len
- == diff_words->minus.orig_nr)
- plus_begin = plus_end =
- diff_words->plus.orig[diff_words->plus.orig_nr - 1].end;
+ if (plus_len) {
+ plus_begin = diff_words->plus.orig[plus_first].begin;
+ plus_end = diff_words->plus.orig[plus_first + plus_len - 1].end;
+ } else
+ plus_begin = plus_end = diff_words->plus.orig[plus_first].end;
if (diff_words->current_plus != plus_begin)
fwrite(diff_words->current_plus,
--
1.6.1.300.gbc493
^ permalink raw reply related
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Sverre Rabbelier @ 2009-01-15 12:45 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johan Herland, git, Johannes Sixt, Anders Melchiorsen, gitster
In-Reply-To: <alpine.DEB.1.00.0901151325310.3586@pacific.mpi-cbg.de>
On Thu, Jan 15, 2009 at 13:36, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> If at all, I'd introduce 'examine' as a synonym to 'edit' (might be more
> intuitive).
Examine suggests that you cannot change the commit (you can look, but
don't touch it!), no?
> However, for the same reason (is it intuitive?) I am not fully convinced
> of 'amend' either. Because --amend _can_ mean that you change the
> diff of the commit. Maybe 'correct', 'redact' or 'rephrase'?
OTOH, when you have no changes staged "git commit --ammend" will do
exactly that, it will let you edit the commit message of the last
commit.
> BTW I was not fully happy with 'edit' back then, either, which is the
> reason why I showed the usage in the comment _above_ the commit list. But
> nobody could suggest a name that I found convincingly better.
The coder's law #349: "The hardest part of writing new functionality
is coming up with a proper name that everyone agrees on".
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Sverre Rabbelier @ 2009-01-15 12:55 UTC (permalink / raw)
To: Pieter de Bie
Cc: Johannes Schindelin, Johan Herland, git, Johannes Sixt,
Anders Melchiorsen, gitster
In-Reply-To: <19A8FAC6-A27A-4D6B-A276-02EE17F0E5F5@frim.nl>
On Thu, Jan 15, 2009 at 13:52, Pieter de Bie <pieter@frim.nl> wrote:
> I think this demonstrates that you can do a lot more with 'edit' than just
> edit.
> 'redact' etc also don't cover it. Perhaps just a general 'pause' or
> something?
>
> You can then put something like 'pause -- pause, for example to amend
> commit'
> in the description part.
That makes sense, perhaps we could name the feature described by the
OP something like "reset", as that is what it actually does?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* [PATCH 2/2] t3404: Add test case for auto-amending only edited commits after "edit"
From: Stephan Beyer @ 2009-01-15 12:56 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Dmitry Potapov, gitster, Stephan Beyer
In-Reply-To: <1232024176-16600-1-git-send-email-s-beyer@gmx.net>
Add a test case for the bugfix introduced by commit c14c3c82d
"git-rebase--interactive: auto amend only edited commit".
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
t/t3404-rebase-interactive.sh | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 1182b46..2cc8e7a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -388,6 +388,23 @@ test_expect_success 'aborted --continue does not squash commits after "edit"' '
git rebase --abort
'
+test_expect_success 'auto-amend only edited commits after "edit"' '
+ test_tick &&
+ FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+ echo "edited again" > file7 &&
+ git add file7 &&
+ FAKE_COMMIT_MESSAGE="edited file7 again" git commit &&
+ echo "and again" > file7 &&
+ git add file7 &&
+ test_tick &&
+ (
+ FAKE_COMMIT_MESSAGE="and again" &&
+ export FAKE_COMMIT_MESSAGE &&
+ test_must_fail git rebase --continue
+ ) &&
+ git rebase --abort
+'
+
test_expect_success 'rebase a detached HEAD' '
grandparent=$(git rev-parse HEAD~2) &&
git checkout $(git rev-parse HEAD) &&
--
1.6.1.160.gecdb
^ permalink raw reply related
* [PATCH 1/2] t3404: Add test case for aborted --continue after "edit"
From: Stephan Beyer @ 2009-01-15 12:56 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Dmitry Potapov, gitster, Stephan Beyer
Add a test case for the bugfix introduced by commit 8beb1f33d
"git-rebase-interactive: do not squash commits on abort".
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
t/t3404-rebase-interactive.sh | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7d10a27..1182b46 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -373,6 +373,21 @@ test_expect_success '--continue tries to commit, even for "edit"' '
test $parent = $(git rev-parse HEAD^)
'
+test_expect_success 'aborted --continue does not squash commits after "edit"' '
+ old=$(git rev-parse HEAD) &&
+ test_tick &&
+ FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+ echo "edited again" > file7 &&
+ git add file7 &&
+ (
+ FAKE_COMMIT_MESSAGE=" " &&
+ export FAKE_COMMIT_MESSAGE &&
+ test_must_fail git rebase --continue
+ ) &&
+ test $old = $(git rev-parse HEAD) &&
+ git rebase --abort
+'
+
test_expect_success 'rebase a detached HEAD' '
grandparent=$(git rev-parse HEAD~2) &&
git checkout $(git rev-parse HEAD) &&
--
1.6.1.160.gecdb
^ permalink raw reply related
* [PATCH] t3501: check that commits are actually done
From: Stephan Beyer @ 2009-01-15 13:03 UTC (permalink / raw)
To: git; +Cc: gitster, Stephan Beyer
The basic idea of t3501 is to check whether revert
and cherry-pick works on renamed files.
But as there is no pure cherry-pick/revert test, it is
good to also check if commits are actually done in that
scenario.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
This patch was originally motivated because of a bug that
I introduced in my repo while hacking on cherry-pick/revert.
None of the cherry-pick test cases failed, so I thought
this tiny addition is useful.
t/t3501-revert-cherry-pick.sh | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 6da2128..bb4cf00 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -45,6 +45,7 @@ test_expect_success 'cherry-pick after renaming branch' '
git checkout rename2 &&
git cherry-pick added &&
+ test $(git rev-parse HEAD^) = $(git rev-parse rename2) &&
test -f opos &&
grep "Add extra line at the end" opos
@@ -54,6 +55,7 @@ test_expect_success 'revert after renaming branch' '
git checkout rename1 &&
git revert added &&
+ test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
test -f spoo &&
! grep "Add extra line at the end" spoo
--
1.6.1.160.gecdb
^ permalink raw reply related
* Re: [PATCH take 3 0/4] color-words improvements
From: Teemu Likonen @ 2009-01-15 13:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Thomas Rast, Junio C Hamano, git, Santi Béjar
In-Reply-To: <alpine.DEB.1.00.0901151337080.3586@pacific.mpi-cbg.de>
Johannes Schindelin (2009-01-15 13:41 +0100) wrote:
> Exactly because it works logically, I do not want to change it. This is
> what the user said, and for a change, it could be what the user meant.
I'm just saying that it would be helpful (to me at least) if the man
page included this advice. Thomas Rast already suggested this in his
version of the man page change:
You may want to append `|\S` to your regular expression to make sure
that it matches all non-whitespace characters.
^ permalink raw reply
* Re: [ANNOUNCE] tig-0.13
From: Jonas Fonseca @ 2009-01-15 13:06 UTC (permalink / raw)
To: git
In-Reply-To: <20090115014617.GC6937@b2j>
bill lam <cbill.lam@gmail.com> wrote Thu, Jan 15, 2009:
> On Thu, 15 Jan 2009, Jonas Fonseca wrote:
> > Yes, it works. You can either create a file called config.make with a
> > line saying:
> >
> > LDLIBS = -lncursesw
> >
> > or use the configure file. If you are not using the tarball generate it
> > with:
> >
> > make configure
>
> I use the git source. Even after make configure and ./configure, it
> still links to the non-unicode ncurses.
I haven't tested the configure script on a lot of system, so it might be
a bit debian/ubuntu/gentoo centered in that use of ncursesw requires the
presence of a {/usr/incude/}ncursesw/ncurses.h header. Where are the
unicode ncurses.h files found on your system?
> Should it make ncursesw as
> default if detected available albeit this can be changed manually?
I would prefer that the "default" (running make without configure) has
as few dependencies as possible. Since the unicode version of ncurses is
probably more rare than the non-unicode version it is probably a bad
idea to use ncursesw by default. However, if you use the configure
script ncursesw is the default, when it is available.
--
Jonas Fonseca
^ permalink raw reply
* Re: [PATCH v2] checkout: implement "-" shortcut name for last branch
From: Johannes Schindelin @ 2009-01-15 13:15 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Thomas Rast, git, Junio C Hamano
In-Reply-To: <496EE559.3060901@viscovery.net>
Hi,
On Thu, 15 Jan 2009, Johannes Sixt wrote:
> Thomas Rast schrieb:
> > Let git-checkout save the old branch as a symref in LAST_HEAD, and
> > make 'git checkout -' switch back to LAST_HEAD, like 'cd -' does in
> > the shell.
>
> /me likes this feature.
>
> git rebase (-i or not) calls checkout behind the scenes if the
> two-argument form is used:
>
> git rebase [-i] master topic
>
> and 'topic' is not the current branch. You may want to add a test that
> ensures that rebase sets LAST_HEAD in this case.
>
> You must make sure that commits referenced by LAST_HEAD are not
> garbage-collected. (I don't know if this happens anyway for symrefs in .git.)
Note: if you used reflogs for that feature, the garbage collection could
not have killed the commit. However, it is quite possible that the
branch was deleted.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Pieter de Bie @ 2009-01-15 12:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Johan Herland, git, Johannes Sixt,
Anders Melchiorsen, gitster
In-Reply-To: <alpine.DEB.1.00.0901151325310.3586@pacific.mpi-cbg.de>
On Jan 15, 2009, at 12:36 PM, Johannes Schindelin wrote:
> BTW I was not fully happy with 'edit' back then, either, which is the
> reason why I showed the usage in the comment _above_ the commit
> list. But
> nobody could suggest a name that I found convincingly better.
(BTW, I reply to this thread because I'm also often confused with the
rebase. The thing that hits me most is that with resolving conflicts,
you have to do a 'git commit' and with 'edit', you have to do a 'git
commit --amend'. This can get confusing if you set up an interactive
rebase where you have some new picks or squashes, and also an edit.
If the rebase stops, you first have to carefully read whether you're
supposed to do a 'git commit' or a 'git commit --amend', and remember
that until you're finished with the changes).
- Pieter
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Pieter de Bie @ 2009-01-15 12:52 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Johan Herland, git, Johannes Sixt,
Anders Melchiorsen, gitster
In-Reply-To: <alpine.DEB.1.00.0901151325310.3586@pacific.mpi-cbg.de>
On Jan 15, 2009, at 12:36 PM, Johannes Schindelin wrote:
> If at all, I'd introduce 'examine' as a synonym to 'edit' (might be
> more
> intuitive).
>
> However, for the same reason (is it intuitive?) I am not fully
> convinced
> of 'amend' either. Because --amend _can_ mean that you change the
> diff of the commit. Maybe 'correct', 'redact' or 'rephrase'?
I think this demonstrates that you can do a lot more with 'edit' than
just edit.
'redact' etc also don't cover it. Perhaps just a general 'pause' or
something?
You can then put something like 'pause -- pause, for example to
amend commit'
in the description part.
^ permalink raw reply
* autoconf: C99 format check
From: Julius Naperkowski @ 2009-01-15 13:22 UTC (permalink / raw)
To: git
I am trying to cross-compile git for mips on a x86 host. But it seems that it is
impossible to pass the C99 Format check in the configure script when
cross_compile mode is activated because the script quits even before it starts
the testprogramm. Is this behavior intentional?
configure: CHECKS for programs
checking for mips-linux-cc... ccache mips-linux-uclibc-gcc
checking for C compiler default output file name... a.out
checking whether the C compiler works... yes
checking whether we are cross compiling... yes
checking for suffix of executables...
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether ccache mips-linux-uclibc-gcc accepts -g... yes
checking for ccache mips-linux-uclibc-gcc option to accept ISO C89... none needed
checking if linker supports -R... no
checking if linker supports -Wl,-rpath,... yes
checking for mips-linux-gar... mips-linux-uclibc-ar
checking for gtar... /bin/tar
checking for asciidoc... no
configure: CHECKS for libraries
checking for SHA1_Init in -lcrypto... no
checking for SHA1_Init in -lssl... no
checking for curl_global_init in -lcurl... no
checking for XML_ParserCreate in -lexpat... no
checking for iconv in -lc... no
checking for iconv in -liconv... no
checking for deflateBound in -lz... no
checking for socket in -lc... yes
configure: CHECKS for header files
checking how to run the C preprocessor... ccache mips-linux-uclibc-gcc -E
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking sys/select.h usability... yes
checking sys/select.h presence... yes
checking for sys/select.h... yes
checking for old iconv()... yes
configure: CHECKS for typedefs, structures, and compiler characteristics
checking for struct dirent.d_ino... yes
checking for struct dirent.d_type... yes
checking for struct sockaddr_storage... yes
checking for struct addrinfo... yes
checking for getaddrinfo... (cached) yes
checking for library containing getaddrinfo... none required
checking whether formatted IO functions support C99 size specifiers...
configure: error: cannot run test program while cross compiling
See `config.log' for more details.
A snippet of the configure script:
...
4928: # Define NO_C99_FORMAT if your formatted IO functions (printf/scanf et.al.)
4929: # do not support the 'size specifiers' introduced by C99, namely ll, hh,
4930: # j, z, t. (representing long long int, char, intmax_t, size_t, ptrdiff_t).
4931: # some C compilers supported these specifiers prior to C99 as an extension.
4932: { echo "$as_me:$LINENO: checking whether formatted IO functions support
C99 size specifiers" >&5
4933: echo $ECHO_N "checking whether formatted IO functions support C99 size
specifiers... $ECHO_C" >&6; }
4934: if test "${ac_cv_c_c99_format+set}" = set; then
4935: echo $ECHO_N "(cached) $ECHO_C" >&6
4936: else
4937: # Actually git uses only %z (%zu) in alloc.c, and %t (%td) in mktag.c
4938: if test "$cross_compiling" = yes; then
4939: { { echo "$as_me:$LINENO: error: cannot run test program while cross
compiling
4940: See \`config.log' for more details." >&5
4941: echo "$as_me: error: cannot run test program while cross compiling
4942: See \`config.log' for more details." >&2;}
4943: { (exit 1); exit 1; }; }
4944: else
4945: cat >conftest.$ac_ext <<_ACEOF
4946: /* confdefs.h. */
4947: _ACEOF
4948: cat confdefs.h >>conftest.$ac_ext
4949: cat >>conftest.$ac_ext <<_ACEOF
...
--
Julius
^ permalink raw reply
* Re: [PATCH take 3 0/4] color-words improvements
From: Thomas Rast @ 2009-01-15 13:27 UTC (permalink / raw)
To: Teemu Likonen; +Cc: Johannes Schindelin, Junio C Hamano, git, Santi Béjar
In-Reply-To: <871vv4soul.fsf@iki.fi>
[-- Attachment #1: Type: text/plain, Size: 795 bytes --]
Teemu Likonen wrote:
> Johannes Schindelin (2009-01-15 13:41 +0100) wrote:
>
> > Exactly because it works logically, I do not want to change it. This is
> > what the user said, and for a change, it could be what the user meant.
>
> I'm just saying that it would be helpful (to me at least) if the man
> page included this advice. Thomas Rast already suggested this in his
> version of the man page change:
>
> You may want to append `|\S` to your regular expression to make sure
> that it matches all non-whitespace characters.
Dscho requested that I put the extended docs in one of my patches, so
it's currently in
http://article.gmane.org/gmane.comp.version-control.git/105716
Comments welcome of course.
--
Thomas Rast
trast@{inf,student}.ethz.ch
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: rebase -p confusion in 1.6.1
From: Johannes Schindelin @ 2009-01-15 13:34 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: git
In-Reply-To: <slrngmu4j5.e1u.sitaramc@sitaramc.homelinux.net>
Hi,
On Thu, 15 Jan 2009, Sitaram Chamarty wrote:
> While trying to understand "rebase -p", I came across some
> very unexpected behaviour that made me throw in the towel
> and ask for help!
>
> [... some script with some aliases ...]
I turned this into a proper test case (to show what would be most helpful
if you report bugs like this in the future).
If nobody beats me to it, I'll work on it tonight.
-- snipsnap --
t/t3409-rebase-preserve-merges.sh | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index e6c8327..5e2128c 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -92,4 +92,32 @@ test_expect_success '--continue works after a conflict' '
)
'
+test_commit () {
+ : > "$1" &&
+ git add "$1" &&
+ test_tick &&
+ git commit -m "$1" "$1"
+}
+
+test_expect_success 'test case from Sitaram' '
+
+ git checkout master &&
+ test_commit a1 &&
+ git checkout -b work &&
+ test_commit b1 &&
+ git checkout master &&
+ test_commit a3 &&
+ git checkout work &&
+ git merge master &&
+ test_commit b3 &&
+ echo before: &&
+ git log --graph --pretty=oneline --decorate --abbrev-commit &&
+ test -f b3 &&
+ git rebase -p master &&
+ echo after: &&
+ git log --graph --pretty=oneline --decorate --abbrev-commit &&
+ test -f b3
+
+'
+
test_done
^ 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