* 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
* 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: [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: 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 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: 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: [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: 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: [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: [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
* 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: [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
* 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 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 PATCH] Check for local changes with "goto"
From: Karl Hasselström @ 2009-01-15 8:37 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
In-Reply-To: <20090114225945.11098.88671.stgit@localhost.localdomain>
I'm not opposed to the change as such (even if I personally think it's
very convenient to allow operations like goto if the local changes
don't touch the same files), but ...
On 2009-01-14 22:59:45 +0000, Catalin Marinas wrote:
> stgit/commands/common.py | 7 +++++++
> stgit/commands/goto.py | 8 +++++++-
... are you sure it wouldn't be better to build the check into
transaction.py, so that all new-infrastructure command would share it?
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ 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 8:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Santi Béjar, Junio C Hamano, Teemu Likonen
In-Reply-To: <alpine.DEB.1.00.0901150235122.3586@pacific.mpi-cbg.de>
[-- Attachment #1: Type: text/plain, Size: 363 bytes --]
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.
--
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: [StGit PATCH] Add --file option to pick
From: Karl Hasselström @ 2009-01-15 8:26 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
In-Reply-To: <20090114225930.11098.2144.stgit@localhost.localdomain>
On 2009-01-14 22:59:30 +0000, Catalin Marinas wrote:
> + opt('-f', '--files', action = 'append',
> + short = 'Only fold the given files'),
The long form should probably be "--file", since you only list one
file for every flag (and since you call it "--file" in the last hunk).
And the help text could be something like
Only fold the given file (for multiple files, use -f more than once)
> + raise CmdException, 'Patch folding failed'
This is not important, but I believe the recommended syntax for
raising exceptions nowadays is
CmdException('Patch folding failed')
since that's what'll continue to work in Python 3, or something like
that. (I see you already use it in the multi-line case in the other
patch, where this syntax is clearly the better choice.)
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH 3/3] implement pattern matching in ce_path_match
From: Clemens Buchacher @ 2009-01-15 8:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes
In-Reply-To: <7vvdshzfpk.fsf@gitster.siamese.dyndns.org>
On Wed, Jan 14, 2009 at 02:27:03PM -0800, Junio C Hamano wrote:
> In places we read paths from the index or from the work tree and add them
> as pathspec elements---you would want to mark them as non-globbing, too.
> Which probably means that "is it Ok to glob this" setting has to be per
> pathspec array elements.
Right. This certainly complicates things. Also note that this invalidates
1/3, because even if '?' matched exactly, it can still match '*', and vice
versa. Depending on ordering one of these two cases would pose a problem.
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Johannes Sixt @ 2009-01-15 7:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Anders Melchiorsen, git, gitster
In-Reply-To: <alpine.DEB.1.00.0901150149130.3586@pacific.mpi-cbg.de>
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.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] checkout: implement "-" shortcut name for last branch
From: Johannes Sixt @ 2009-01-15 7:27 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <1231978322-21228-1-git-send-email-trast@student.ethz.ch>
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.)
-- Hannes
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Junio C Hamano @ 2009-01-15 6:13 UTC (permalink / raw)
To: Miles Bader
Cc: Boyd Stephen Smith Jr., Anders Melchiorsen, git,
Johannes.Schindelin
In-Reply-To: <buo8wpdfbv9.fsf@dhapc248.dev.necel.com>
Miles Bader <miles@gnu.org> writes:
> [I do wonder how on earth the current awkward behavior was accepted in
> the first place...]
That's actually very easy to explain. Both the contributor and the
maintainer were rather familiar with the workflow using tools before
"rebase -i" appeared, and to them, editing an existing commit was
equivalent to first plant yourself on the commit to be amended, issue
"commit --amend", and continue on with other tasks (similarly, "picking" an
existing commit is to cherry-pick the commit to the state whatever the
previous sequence of commands left). In other words, they both thought in
terms of the underlying command sequence and it did not appear unnatural
at all to them.
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Boyd Stephen Smith Jr. @ 2009-01-15 5:00 UTC (permalink / raw)
To: Miles Bader; +Cc: Junio C Hamano, Anders Melchiorsen, git, Johannes.Schindelin
In-Reply-To: <buo8wpdfbv9.fsf@dhapc248.dev.necel.com>
[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]
On Wednesday 14 January 2009, Miles Bader <miles@gnu.org> wrote about 'Re:
[RFC PATCH] Make the rebase edit mode really end up in an edit state':
>"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:
>>>We may need a version bump to 1.7.0 to update the UI for this command,
>>> but please do test rigorously to build a stronger case for a saner UI.
>>
>> Instead of changing the meaning of edit, how about introducing a
>> "replace" command?
>
>That seems like at best an awkward workaround, not a real solution to
>the problem,
Actually, I think it's a better solution (or rather a solution to the real
problem) because others have already said they like and expect the current
edit behavior.
The "real problem" is you need different behavior for your interactive
rebasing.
>which is that the term "edit XXXX" suggests you're starting
>with XXXX and modifying it.
Exactly. "edit" is: While interactively rebuilding history (rebase -i), you
get to the first place that commit existed and then modify it
(commit --amend or other tools).
>The term "replace" by contrast, seems more
>to connote entirely removing XXXX and substituting something else.
Exactly. "replace" is: While rebasing you stop just before the commit
existed (changes are even staged) and decide to do something else (like
using add -i and a few commit commands to split the thing up or whatever).
>[I do wonder how on earth the current awkward behavior was accepted in
>the first place...]
(Actually, I do too, but it's accepted and expected behavior now--good
reason not to change it.)
--
Boyd Stephen Smith Jr. ,= ,-_-. =.
bss@iguanasuicide.net ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-'
http://iguanasuicide.net/ \_/
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH take 3 0/4] color-words improvements
From: Teemu Likonen @ 2009-01-15 4:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Thomas Rast, Junio C Hamano, git, Santi Béjar
In-Reply-To: <alpine.DEB.1.00.0901142258250.3586@pacific.mpi-cbg.de>
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.
^ permalink raw reply
* Re: [RFC PATCH] Make the rebase edit mode really end up in an edit state
From: Miles Bader @ 2009-01-15 4:10 UTC (permalink / raw)
To: Boyd Stephen Smith Jr.
Cc: Junio C Hamano, Anders Melchiorsen, git, Johannes.Schindelin
In-Reply-To: <200901142049.54775.bss@iguanasuicide.net>
"Boyd Stephen Smith Jr." <bss@iguanasuicide.net> writes:
>>We may need a version bump to 1.7.0 to update the UI for this command,
>> but please do test rigorously to build a stronger case for a saner UI.
>
> Instead of changing the meaning of edit, how about introducing a "replace"
> command?
That seems like at best an awkward workaround, not a real solution to
the problem, which is that the term "edit XXXX" suggests you're starting
with XXXX and modifying it. The term "replace" by contrast, seems more
to connote entirely removing XXXX and substituting something else.
[I do wonder how on earth the current awkward behavior was accepted in
the first place...]
-Miles
--
"Most attacks seem to take place at night, during a rainstorm, uphill,
where four map sheets join." -- Anon. British Officer in WW I
^ permalink raw reply
* Re: jgit merge question
From: Shawn O. Pearce @ 2009-01-15 4:01 UTC (permalink / raw)
To: David Birchfield; +Cc: git
In-Reply-To: <7F1F22DF-7E4F-4888-A404-2A68F663989A@asu.edu>
David Birchfield <dbirchfield@asu.edu> wrote:
> thanks again for your help, and really sorry for the newbie questions.
>
> how do I grab those 8 commits?
>
> I did originally use git clone on this uri: git://
> android.git.kernel.org/tools/egit.git - but I don't see the
> modifications there.
They are in a side branch:
git pull git://android.git.kernel.org/tools/egit.git for-gerrit2
--
Shawn.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox