* Re: [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag
From: Junio C Hamano @ 2008-07-23 1:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: pasky, git, gitster
In-Reply-To: <alpine.DEB.1.00.0807230150480.8986@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> This is the same behavior as before, since indegree could not be
> non-zero when TOPOSORT was unset.
>
> Incidentally, this fixes the bug in show-branch where the 8th column
> was not shown: show-branch sorts the commits in topological order,
> assuming that all the commit flags are available for show-branch's
> private matters.
Do people still actively use show-branch as a G/CUI, especially after that
"log --graph" thing was introduced?
If that is the case, it might also make sense to stop using the object
flags but allocate necessary number of bits (not restricted to 25 or so)
pointed at by commit->util field to remove its limitation.
Hint, hint...
^ permalink raw reply
* Re: regression in 92392b4
From: Johannes Schindelin @ 2008-07-23 0:58 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Pierre Habouzit, Git ML, Junio C Hamano
In-Reply-To: <20080723004108.GB14668@spearce.org>
Hi,
On Tue, 22 Jul 2008, Shawn O. Pearce wrote:
> Reading above shows we got a "fatal: Out of memory, malloc failed" right
> before the segfault. What's odd is we segfaulted after we ran out of
> memory and should have die'd.
>
> There's at least two bugs in the above output:
>
> a) index-pack ran out of memory on a small pull (95 KiB).
> b) fetch segfaulted when index-pack failed.
>
> And this patch will unfortunately address neither of them. :-|
Yeah, I thought I had something, but no matter what I tried, I could not
find a breakage. Not even valgrind complains.
But as I said: it was just a guess.
Maybe it will be easier to reproduce by patching the code to malloc()
everything that is available first, and then continuing.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 2/2] sort_in_topological_order(): avoid setting a commit flag
From: Johannes Schindelin @ 2008-07-23 0:51 UTC (permalink / raw)
To: pasky, git, gitster
In-Reply-To: <alpine.DEB.1.00.0807230148130.8986@racer>
We used to set the TOPOSORT flag of commits during the topological
sorting, but we can just as well use the member "indegree" for it:
indegree is now incremented by 1 in the cases where the commit used
to have the TOPOSORT flag.
This is the same behavior as before, since indegree could not be
non-zero when TOPOSORT was unset.
Incidentally, this fixes the bug in show-branch where the 8th column
was not shown: show-branch sorts the commits in topological order,
assuming that all the commit flags are available for show-branch's
private matters.
But this was not true: TOPOSORT was identical to the flag corresponding
to the 8th ref. So the flags for the 8th column were unset by the
topological sorting.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This is another late-night patch done by yours-truly. However,
I tried extra hard to make sure that every occurrence of
indegree was properly changed, and I am pretty certain that
the reasoning with the unset TOPOSORT is correct.
But please check (I know, not necessary to ask for extra review
for my patches, but nevertheless).
commit.c | 13 ++++++-------
revision.h | 3 +--
t/t3202-show-branch-octopus.sh | 2 +-
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/commit.c b/commit.c
index 5148ec5..9dacfb8 100644
--- a/commit.c
+++ b/commit.c
@@ -436,8 +436,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
/* Mark them and clear the indegree */
for (next = orig; next; next = next->next) {
struct commit *commit = next->item;
- commit->object.flags |= TOPOSORT;
- commit->indegree = 0;
+ commit->indegree = 1;
}
/* update the indegree */
@@ -446,7 +445,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
while (parents) {
struct commit *parent = parents->item;
- if (parent->object.flags & TOPOSORT)
+ if (parent->indegree)
parent->indegree++;
parents = parents->next;
}
@@ -464,7 +463,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
for (next = orig; next; next = next->next) {
struct commit *commit = next->item;
- if (!commit->indegree)
+ if (commit->indegree == 1)
insert = &commit_list_insert(commit, insert)->next;
}
@@ -486,7 +485,7 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
for (parents = commit->parents; parents ; parents = parents->next) {
struct commit *parent=parents->item;
- if (!(parent->object.flags & TOPOSORT))
+ if (!parent->indegree)
continue;
/*
@@ -494,7 +493,8 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
* when all their children have been emitted thereby
* guaranteeing topological order.
*/
- if (!--parent->indegree) {
+ if (--parent->indegree == 1) {
+ parent->indegree = 0;
if (!lifo)
insert_by_date(parent, &work);
else
@@ -505,7 +505,6 @@ void sort_in_topological_order(struct commit_list ** list, int lifo)
* work_item is a commit all of whose children
* have already been emitted. we can emit it now.
*/
- commit->object.flags &= ~TOPOSORT;
*pptr = work_item;
pptr = &work_item->next;
}
diff --git a/revision.h b/revision.h
index fa68c65..f64e8ce 100644
--- a/revision.h
+++ b/revision.h
@@ -12,8 +12,7 @@
#define CHILD_SHOWN (1u<<6)
#define ADDED (1u<<7) /* Parents already parsed and added? */
#define SYMMETRIC_LEFT (1u<<8)
-#define TOPOSORT (1u<<9) /* In the active toposort list.. */
-#define ALL_REV_FLAGS ((1u<<10)-1)
+#define ALL_REV_FLAGS ((1u<<9)-1)
struct rev_info;
struct log_info;
diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh
index 8d50c23..7fe4a6e 100755
--- a/t/t3202-show-branch-octopus.sh
+++ b/t/t3202-show-branch-octopus.sh
@@ -49,7 +49,7 @@ cat > expect << EOF
+++++++++* [branch10^] initial
EOF
-test_expect_failure 'show-branch with more than 8 branches' '
+test_expect_success 'show-branch with more than 8 branches' '
git show-branch $(for i in $numbers; do echo branch$i; done) > out &&
test_cmp expect out
--
1.6.0.rc0.22.gf2096d.dirty
^ permalink raw reply related
* [PATCH 1/2] Add test to show that show-branch misses out the 8th column
From: Johannes Schindelin @ 2008-07-23 0:50 UTC (permalink / raw)
To: pasky, git, gitster
Noticed by Pasky.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t3202-show-branch-octopus.sh | 59 ++++++++++++++++++++++++++++++++++++++++
1 files changed, 59 insertions(+), 0 deletions(-)
create mode 100755 t/t3202-show-branch-octopus.sh
diff --git a/t/t3202-show-branch-octopus.sh b/t/t3202-show-branch-octopus.sh
new file mode 100755
index 0000000..8d50c23
--- /dev/null
+++ b/t/t3202-show-branch-octopus.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='test show-branch with more than 8 heads'
+
+. ./test-lib.sh
+
+numbers="1 2 3 4 5 6 7 8 9 10"
+
+test_expect_success 'setup' '
+
+ > file &&
+ git add file &&
+ test_tick &&
+ git commit -m initial &&
+
+ for i in $numbers
+ do
+ git checkout -b branch$i master &&
+ > file$i &&
+ git add file$i &&
+ test_tick &&
+ git commit -m branch$i || break
+ done
+
+'
+
+cat > expect << EOF
+! [branch1] branch1
+ ! [branch2] branch2
+ ! [branch3] branch3
+ ! [branch4] branch4
+ ! [branch5] branch5
+ ! [branch6] branch6
+ ! [branch7] branch7
+ ! [branch8] branch8
+ ! [branch9] branch9
+ * [branch10] branch10
+----------
+ * [branch10] branch10
+ + [branch9] branch9
+ + [branch8] branch8
+ + [branch7] branch7
+ + [branch6] branch6
+ + [branch5] branch5
+ + [branch4] branch4
+ + [branch3] branch3
+ + [branch2] branch2
++ [branch1] branch1
++++++++++* [branch10^] initial
+EOF
+
+test_expect_failure 'show-branch with more than 8 branches' '
+
+ git show-branch $(for i in $numbers; do echo branch$i; done) > out &&
+ test_cmp expect out
+
+'
+
+test_done
--
1.6.0.rc0.22.gf2096d.dirty
^ permalink raw reply related
* Re: [PATCH] Rename ".dotest/" to ".git/rebase" and ".dotest-merge" to "rebase-merge"
From: Junio C Hamano @ 2008-07-23 0:48 UTC (permalink / raw)
To: Stephan Beyer
Cc: Olivier Marin, Theodore Tso, Nanako Shiraishi,
Johannes Schindelin, René Scharfe, Joe Fiorini, git,
Jari Aalto
In-Reply-To: <20080722234703.GD5904@leksak.fem-net>
Stephan Beyer <s-beyer@gmx.net> writes:
> Hi,
>
> Junio C Hamano wrote:
>> Olivier Marin <dkr+ml.git@free.fr> writes:
>> > @@ -203,9 +204,10 @@ then
>> >
>> > case "$abort" in
>> > t)
>> > - rm -fr "$dotest" &&
>> > + git rerere clear &&
>> > git read-tree -m -u ORIG_HEAD &&
> [...]
>> diff --git a/git-am.sh b/git-am.sh
>> index a44bd7a..5cbf8f4 100755
>> --- a/git-am.sh
>> +++ b/git-am.sh
>> @@ -203,9 +203,9 @@ then
>>
>> case "$abort" in
>> t)
>> - rm -fr "$dotest" &&
>> - git read-tree -m -u ORIG_HEAD &&
>> - git reset ORIG_HEAD && :
>> + git rerere clear
>> + git read-tree --reset -u HEAD ORIG_HEAD
>
> Perhaps I am confused, but ...
> Why is there "HEAD" and "ORIG_HEAD" and not only "ORIG_HEAD"?
Just being a bit defensive -- in this case I think it might be Ok to say
"read-tree --reset -u ORIG_HEAD", but I haven't checked in a conflicted
case.
If some path was added between ORIG_HEAD (that is where we started from)
and HEAD (that is where we are and we decide we do not want it), and that
path is conflicted in the index, a single tree form "read-tree --reset -u
HEAD" would leave it behind in the working tree, wouldn't it?
^ permalink raw reply
* Re: regression in 92392b4
From: Shawn O. Pearce @ 2008-07-23 0:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pierre Habouzit, Git ML, Junio C Hamano
In-Reply-To: <alpine.DEB.1.00.0807230033000.8986@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 23 Jul 2008, Pierre Habouzit wrote:
>
> > Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
> > did, since git in next cannot fetch on a regular basis for me. The
> > culprit seems to be commit 92392b4:
> >
> > ┌─(1:11)──<~/dev/scm/git 92392b4...>──
> > └[artemis] git fetch
> > remote: Counting objects: 461, done.
> > remote: Compressing objects: 100% (141/141), done.
> > remote: Total 263 (delta 227), reused 155 (delta 121)
> > Receiving objects: 100% (263/263), 95.55 KiB, done.
> > fatal: Out of memory, malloc failed
> > fatal: index-pack failed
> > [2] 16674 abort (core dumped) git fetch
...
>
> Just a guess:
...
> diff --git a/index-pack.c b/index-pack.c
> index ac20a46..19c39e5 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -257,6 +257,7 @@ static void unlink_base_data(struct base_data *c)
> base_cache = NULL;
> if (c->data) {
> free(c->data);
> + c->data = NULL;
> base_cache_used -= c->size;
> }
> }
Oh. This is a pointless assignment. If you look at any call sites
for unlink_base_data() you will find that the struct passed in as
"c" here is going out of scope after unlink_base_data() returns. In
no such case does the value of c->data get tested once this free is
complete.
We need the if (c->data) guard because we only want to decrement
base_cache_used if the memory is still allocated. It may have been
released earlier, in which case base_cache_used has already been
decreased and we don't want to double-decrement it.
This patch makes the code more obvious, so Ack I guess, but it is
not a solution to Pierre's woes. Something else is wrong.
Reading above shows we got a "fatal: Out of memory, malloc failed"
right before the segfault. What's odd is we segfaulted after we
ran out of memory and should have die'd.
There's at least two bugs in the above output:
a) index-pack ran out of memory on a small pull (95 KiB).
b) fetch segfaulted when index-pack failed.
And this patch will unfortunately address neither of them. :-|
I've had a long past couple of days, and another one tomorrow.
I'm not going to be able to debug this myself until perhaps Thursday
or Friday. Sorry. If nobody beats me to it, I will put this on
the top of the pile and try to fix it once I get back online at my
new home.
--
Shawn.
^ permalink raw reply
* Re: [PATCH] bring description of git diff --cc up to date
From: Junio C Hamano @ 2008-07-23 0:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, David Greaves
In-Reply-To: <Pine.GSO.4.62.0807221812470.25746@harper.uchicago.edu>
Jonathan Nieder <jrnieder@uchicago.edu> writes:
> + This flag implies the '-c' option and makes the patch output
> + even more compact by omitting uninteresting hunks. A hunk is
> + considered uninteresting if the person merging had two versions
> + to choose between among all of the parents and the result shows
Hmm, I am not a native speaker, but the above makes me confused into
thinking that even if there are 47 parent versions, it is Ok if I looked
at only two versions and picked from one of them -- the description does
not seem to make it clear that it is required that the other 45 agree with
one of the two I looked at and picked from.
... if the contents in the parents had only two variants and the merge
result picked one of them without modification.
would be succinct enough, perhaps?
If we want to further elaborate, we could say something like:
In a two-parent merge, by definition, there can only be two variants
to choose from. Even in a merge with more than two parents, if some
parents share the same content, and all the other parents share
another, same content, there are only two variants to choose from.
but I think that is too verbose...
^ permalink raw reply
* Re: [PATCH] Rename ".dotest/" to ".git/rebase" and ".dotest-merge" to "rebase-merge"
From: Junio C Hamano @ 2008-07-23 0:16 UTC (permalink / raw)
To: Olivier Marin
Cc: Theodore Tso, Nanako Shiraishi, Johannes Schindelin,
René Scharfe, Stephan Beyer, Joe Fiorini, git, Jari Aalto
In-Reply-To: <4884917A.1060005@free.fr>
Olivier Marin <dkr+ml.git@free.fr> writes:
> The last thing that still annoy me is the --skip that refuse to skip in 3-way
> merge. Perhaps we can use the "git read-tree --reset -u" thing for skip too.
Hmm...
We traditionally left that as something the user deliberately should do to
signal --skip that the user knows he is dropping that change (by the way,
so did "git-rebase"). But with fb6e4e1 (Do git reset --hard HEAD when
using git rebase --skip, 2007-11-08), we run the reset upon rebase --skip,
so it probably is a good idea to match it here as well.
> diff --git a/git-am.sh b/git-am.sh
> index 60aaa4a..864c77e 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -202,8 +202,15 @@ then
> die "previous rebase directory $dotest still exists but mbox given."
> resume=yes
>
> - case "$abort" in
> - t)
> + case "$skip,$abort" in
> + t,)
> + git rerere clear
> + git read-tree --reset -u HEAD HEAD
> + orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
> + git reset HEAD
> + git update-ref ORIG_HEAD $orig_head
> + ;;
Sorry, I do not quite understand what this reset after the read-tree dance
is trying to do; you have already reset the index to the tree in HEAD when
you cleared the change involved in the patch application with that
two-tree form of read-tree.
^ permalink raw reply
* Re: [PATCH] Respect crlf attribute even if core.autocrlf has not been set
From: Dmitry Potapov @ 2008-07-23 0:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0807230019480.8986@racer>
On Wed, Jul 23, 2008 at 12:23:27AM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 23 Jul 2008, Dmitry Potapov wrote:
>
> > On Tue, Jul 22, 2008 at 10:56:04PM +0100, Johannes Schindelin wrote:
> > >
> > > When a file's crlf attribute is explicitely set, it does not make
> > > sense to ignore it, just because the config variable core.autocrlf has
> > > not been set.
> >
> > Hmm... About a week ago, I was about to propose the same change, but
> > after reading documentation and some thinking I was not able to convince
> > myself that this change would be the right thing to do.
>
> Well, I have a shared repository, where I set the attribute. Now, every
> once in a while, people check in text _with_ CR/LF. Yes, that is right, I
> marked it explicitely as crlf, yet I am on the whim of the people choosing
> to set the config variable or not.
>
> And I could not care less what the documentation says: if it does not make
> sense, it does not make sense.
If you think that the current documentation does not make sense, why
don't you write something that will make sense? Really, the current
behavior may not be the best one, but at least it is consistent with
documentation, while your change may confuse users, because they may
expect one behavior, but git will act differently.
If I understand your change correctly, you take into account the crlf
attribute unconditionally only in worktree-to-git conversion, while you
still ignore it if core.autocrlf=false on checkout. Is it correct? If
so, I think your patch does make sense, and it should not break anything
too badly, because you still respect core.autocrlf on checkout, but you
should have said that in your commit message.
Dmitry
^ permalink raw reply
* Re: [PATCH] Build configuration to skip ctime for modification test
From: Junio C Hamano @ 2008-07-23 0:12 UTC (permalink / raw)
To: Alex Riesen; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20080722203128.GB5113@blimp.local>
Alex Riesen <raa.lkml@gmail.com> writes:
> Johannes Schindelin, Tue, Jul 22, 2008 22:17:21 +0200:
>> Hi,
>>
>> On Tue, 22 Jul 2008, Alex Riesen wrote:
>>
>> > +#ifndef NO_TRUSTABLE_FILEMODE
>> > if (ce->ce_ctime != (unsigned int) st->st_ctime)
>> > changed |= CTIME_CHANGED;
>> > +#endif
>>
>> Surely you meant trust_executable_bit instead, right?
>
> No. Just what I said: we don't have filemode (like "at all") - so no
> ctime as well. But maybe you're right, and trust_executable_bit is
> more flexible. Or maybe both (the #ifdef _and_ trust_executable_bit)
> and must be used...
>
>> Otherwise, if you really want to tell at compile time,I think for clarity
>> you have to introduce another #define, since NO_TRUSTABLE_FILEMODE
>> definitely says something different than CTIME_IS_USELESS.
>
> I had that at first (NO_DEPENDABLE_CTIME, than IGNORE_CTIME), than
> deemed it excessive.
Why is it excessive? My initial reaction was "what does trustable
filemode nor trust_executable_bit has anything to do with ctime". Please
explain.
^ permalink raw reply
* Re: [PATCH] Respect crlf attribute even if core.autocrlf has not been set
From: Junio C Hamano @ 2008-07-23 0:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0807222255450.8986@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> When a file's crlf attribute is explicitely set, it does not make sense
> to ignore it, just because the config variable core.autocrlf has not
> been set.
I am not sure if I agree with that reasoning.
Attribute defines what each path is. Is it a text file, is it a binary?
The nature of the contents does not change between people on POSIX and
Windows, and that is why it is described in .gitattributes and cloned
across repositories.
On the other hand, the configuration defines what to do with contents with
various attributes in this particular repository. Do I want to see a text
file checked out with CRLF endings, or LF?
So it is perfectly valid and normal for a cross-platform minded project to
use the crlf atttribute to say "These files are text" and expect them to
be checked out with LF endings on POSIX while making sure they are checked
out with CRLF on Windows. Adding CR at the end of line for such files on
POSIX systems is positively a wrong thing to do in such a case.
Projects like the kernel that originate from LF side of the world may not
bother marking things as such, though.
^ permalink raw reply
* [PATCH] am --abort: Add to bash-completion and mention in git-rerere documentation
From: Stephan Beyer @ 2008-07-23 0:10 UTC (permalink / raw)
To: Olivier Marin
Cc: Junio C Hamano, Theodore Tso, Nanako Shiraishi, git,
Stephan Beyer
In-Reply-To: <4882350B.6020003@free.fr>
The git-rerere documentation talks about commands that invoke
"git rerere clear" automatically. git am --abort is added and
a typo is fixed additionally.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Documentation/git-rerere.txt | 2 +-
contrib/completion/git-completion.bash | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt
index beebd53..89f321b 100644
--- a/Documentation/git-rerere.txt
+++ b/Documentation/git-rerere.txt
@@ -37,7 +37,7 @@ its working state.
'clear'::
This resets the metadata used by rerere if a merge resolution is to be
-is aborted. Calling 'git-am --skip' or 'git-rebase [--skip|--abort]'
+aborted. Calling 'git-am [--skip|--abort]' or 'git-rebase [--skip|--abort]'
will automatically invoke this command.
'diff'::
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2edb341..8fc9145 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -489,7 +489,7 @@ _git_am ()
{
local cur="${COMP_WORDS[COMP_CWORD]}" dir="$(__gitdir)"
if [ -d "$dir"/rebase-apply ]; then
- __gitcomp "--skip --resolved"
+ __gitcomp "--skip --resolved --abort"
return
fi
case "$cur" in
--
1.5.6.4.459.gfa44d
^ permalink raw reply related
* Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories
From: Junio C Hamano @ 2008-07-23 0:09 UTC (permalink / raw)
To: Petr Baudis; +Cc: Johannes Schindelin, gitster, git
In-Reply-To: <20080722233652.GT10151@machine.or.cz>
Petr Baudis <pasky@suse.cz> writes:
> um, oops. I actually never got to know these by heart since I learnt
> to expliciply group the expressions early on. I guess my only excuse is
> that I've stumbled at 0bdf93cbf earlier and understood it the _wrong_
> way around since I'm getting really sleepy. ;-)
>
> I still think my change improves the code readibility so it could be
> kept, but I'm fairly neutral on this.
I cannot be neutral when a patch introduces unnecessary fork.
The quality of shell scripts in git.git seems to have deteriorated over
time but I do not think we would want to spend too much maintainer time to
go back and fix all of them. Please don't make things worse, at least.
^ permalink raw reply
* Re: [PATCH] Documentation/git-filter-branch: Remove Useless Use of Plumbing
From: Junio C Hamano @ 2008-07-23 0:05 UTC (permalink / raw)
To: Petr Baudis; +Cc: gitster, git
In-Reply-To: <20080722222418.15372.62190.stgit@localhost>
Petr Baudis <pasky@suse.cz> writes:
> The example to remove file using index-filter uses git update-index
> --remove where git rm --cached works as well.
I am not sure "works as well" is a good enough justification to choose a
Porcelain command over a plumbing command in this particular context.
After all, filter-branch is a scripting enviornment, isn't it?
There also is another subtle difference. "git rm" takes pathspecs while
"update-index" takes paths.
So after running one of these commands:
$ git rm --cached 'Makefil?'
$ git update-index --force-remove 'Makefil?'
output from:
$ git diff --cached --stat
would be different.
^ permalink raw reply
* Re: [PATCH] rebase -i: only automatically amend commit if HEAD did not change
From: Junio C Hamano @ 2008-07-22 23:55 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Johannes Schindelin, git, gitster
In-Reply-To: <32541b130807221522r2a43c49cl6400f00dbe7451a0@mail.gmail.com>
"Avery Pennarun" <apenwarr@gmail.com> writes:
> On 7/22/08, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> If the user called "rebase -i", marked a commit as "edit", "rebase
>> --continue" would automatically amend the commit when there were
>> staged changes.
>>
>> However, this is actively wrong when the current commit is not the
>> one marked with "edit". So guard against this.
>
> This patch is perhaps a symptom of something I've been meaning to ask
> about for a while.
>
> Why doesn't "edit" just stage the commit and not auto-commit it at
> all? That way an amend would *never* be necessary, and rebase
> --continue would always do a commit -a (if there was anything left to
> commit). The special case fixed by this patch would thus not be
> needed.
That would actually be in-line with the way how "rebase --skip" does the
resetting without asking the user to do so, wouldn't it?
^ permalink raw reply
* Re: [PATCH] Rename ".dotest/" to ".git/rebase" and ".dotest-merge" to "rebase-merge"
From: Stephan Beyer @ 2008-07-22 23:47 UTC (permalink / raw)
To: Junio C Hamano
Cc: Olivier Marin, Theodore Tso, Nanako Shiraishi,
Johannes Schindelin, René Scharfe, Joe Fiorini, git,
Jari Aalto
In-Reply-To: <7v3am5zfea.fsf@gitster.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> Olivier Marin <dkr+ml.git@free.fr> writes:
> > @@ -203,9 +204,10 @@ then
> >
> > case "$abort" in
> > t)
> > - rm -fr "$dotest" &&
> > + git rerere clear &&
> > git read-tree -m -u ORIG_HEAD &&
[...]
> diff --git a/git-am.sh b/git-am.sh
> index a44bd7a..5cbf8f4 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -203,9 +203,9 @@ then
>
> case "$abort" in
> t)
> - rm -fr "$dotest" &&
> - git read-tree -m -u ORIG_HEAD &&
> - git reset ORIG_HEAD && :
> + git rerere clear
> + git read-tree --reset -u HEAD ORIG_HEAD
Perhaps I am confused, but ...
Why is there "HEAD" and "ORIG_HEAD" and not only "ORIG_HEAD"?
Regards.
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* Re: regression in 92392b4
From: Pierre Habouzit @ 2008-07-22 23:37 UTC (permalink / raw)
To: spearce, Git ML, Junio C Hamano
In-Reply-To: <20080722231745.GD11831@artemis.madism.org>
[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]
On Tue, Jul 22, 2008 at 11:17:45PM +0000, Pierre Habouzit wrote:
> Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
> did, since git in next cannot fetch on a regular basis for me. The
> culprit seems to be commit 92392b4:
>
> ┌─(1:11)──<~/dev/scm/git 92392b4...>──
> └[artemis] git fetch
> remote: Counting objects: 461, done.
> remote: Compressing objects: 100% (141/141), done.
> remote: Total 263 (delta 227), reused 155 (delta 121)
> Receiving objects: 100% (263/263), 95.55 KiB, done.
> fatal: Out of memory, malloc failed
> fatal: index-pack failed
> [2] 16674 abort (core dumped) git fetch
>
> ┌─(1:12)──<~/dev/scm/git 92392b4...>──
> └[artemis] git checkout -m HEAD~1; make git-index-pack
> Previous HEAD position was 92392b4... index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
> HEAD is now at 03993e1... index-pack: Track the object_entry that creates each base_data
> GIT_VERSION = 1.5.6.3.3.g03993
> CC index-pack.o
> LINK git-index-pack
>
> ┌─(1:12)──<~/dev/scm/git 03993e1...>──
> └[artemis] git fetch
> remote: Counting objects: 461, done.
> remote: Compressing objects: 100% (141/141), done.
> remote: Total 263 (delta 227), reused 155 (delta 121)
> Receiving objects: 100% (263/263), 95.55 KiB, done.
> Resolving deltas: 100% (227/227), completed with 153 local objects.
> From git://git.kernel.org/pub/scm/git/git
> 5ba2c22..0868a30 html -> origin/html
> 2857e17..abeeabe man -> origin/man
> 93310a4..95f8ebb master -> origin/master
> 559998f..e8bf351 next -> origin/next
>
> You can see the commit sha's in the prompt. 03993e1 is fine, 92392b4 is
> broken, I've absolutely no clue about what happens.
>
> All I can say is that at some point in get_data_from_pack, obj[1].idx
> points to something that is *not* a sha so it's probably corrupted.
> (from index-pack.c).
Though reading the code, we trust c->data NULL-iness to mean we have
no data, and there is one code path that fails to reset it. The problem
is I'm not able to reproduce the bug, because I foolishly didn't
backuped the git repository that exhibited the failure, so I cannot know
if that can be the problem:
--- snip ---
From c3749f7bc50c642c5d437b2746d4ba589b7d9739 Mon Sep 17 00:00:00 2001
From: Pierre Habouzit <madcoder@debian.org>
Date: Wed, 23 Jul 2008 01:35:11 +0200
Subject: [PATCH] index-pack: missing pointer reset to NULL.
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
index-pack.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/index-pack.c b/index-pack.c
index ac20a46..19c39e5 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -257,6 +257,7 @@ static void unlink_base_data(struct base_data *c)
base_cache = NULL;
if (c->data) {
free(c->data);
+ c->data = NULL;
base_cache_used -= c->size;
}
}
--
1.6.0.rc0.153.ge8bf3.dirty
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories
From: Petr Baudis @ 2008-07-22 23:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git
In-Reply-To: <alpine.DEB.1.00.0807230025050.8986@racer>
Hi,
On Wed, Jul 23, 2008 at 12:27:07AM +0100, Johannes Schindelin wrote:
> On Wed, 23 Jul 2008, Petr Baudis wrote:
>
> > This also fixes suspicious shell boolean expression during a check
> > for dirty working tree.
>
> If you are talking about X && Y || Z, it is well established (and should
> not be suspicious to a shell hacker like the creator of Cogito) that Z is
> executed if either X fails, or X succeeds and Y fails.
um, oops. I actually never got to know these by heart since I learnt
to expliciply group the expressions early on. I guess my only excuse is
that I've stumbled at 0bdf93cbf earlier and understood it the _wrong_
way around since I'm getting really sleepy. ;-)
I still think my change improves the code readibility so it could be
kept, but I'm fairly neutral on this.
> > +test_expect_success 'rewrite bare repository identically' '
> > + (git config core.bare true && cd .git && git-filter-branch branch)
> > +'
> > +git config core.bare false
>
> Any reason why this is done outside the test?
If the test fails in the middle, not resetting this might negatively
affect the rest of the testsuite.
--
Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name. -- Ken Thompson and Dennis M. Ritchie
^ permalink raw reply
* Re: regression in 92392b4
From: Johannes Schindelin @ 2008-07-22 23:34 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: spearce, Git ML, Junio C Hamano
In-Reply-To: <20080722231745.GD11831@artemis.madism.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2507 bytes --]
Hi,
On Wed, 23 Jul 2008, Pierre Habouzit wrote:
> Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
> did, since git in next cannot fetch on a regular basis for me. The
> culprit seems to be commit 92392b4:
>
> ┌─(1:11)──<~/dev/scm/git 92392b4...>──
> └[artemis] git fetch
> remote: Counting objects: 461, done.
> remote: Compressing objects: 100% (141/141), done.
> remote: Total 263 (delta 227), reused 155 (delta 121)
> Receiving objects: 100% (263/263), 95.55 KiB, done.
> fatal: Out of memory, malloc failed
> fatal: index-pack failed
> [2] 16674 abort (core dumped) git fetch
>
> ┌─(1:12)──<~/dev/scm/git 92392b4...>──
> └[artemis] git checkout -m HEAD~1; make git-index-pack
> Previous HEAD position was 92392b4... index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
> HEAD is now at 03993e1... index-pack: Track the object_entry that creates each base_data
> GIT_VERSION = 1.5.6.3.3.g03993
> CC index-pack.o
> LINK git-index-pack
>
> ┌─(1:12)──<~/dev/scm/git 03993e1...>──
> └[artemis] git fetch
> remote: Counting objects: 461, done.
> remote: Compressing objects: 100% (141/141), done.
> remote: Total 263 (delta 227), reused 155 (delta 121)
> Receiving objects: 100% (263/263), 95.55 KiB, done.
> Resolving deltas: 100% (227/227), completed with 153 local objects.
> From git://git.kernel.org/pub/scm/git/git
> 5ba2c22..0868a30 html -> origin/html
> 2857e17..abeeabe man -> origin/man
> 93310a4..95f8ebb master -> origin/master
> 559998f..e8bf351 next -> origin/next
>
> You can see the commit sha's in the prompt. 03993e1 is fine, 92392b4 is
> broken, I've absolutely no clue about what happens.
>
> All I can say is that at some point in get_data_from_pack, obj[1].idx
> points to something that is *not* a sha so it's probably corrupted.
> (from index-pack.c).
Just a guess:
-- snipsnap --
[PATCH] index-pack: set base_data's data member to NULL after freeing it
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
index-pack.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/index-pack.c b/index-pack.c
index ac20a46..19c39e5 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -257,6 +257,7 @@ static void unlink_base_data(struct base_data *c)
base_cache = NULL;
if (c->data) {
free(c->data);
+ c->data = NULL;
base_cache_used -= c->size;
}
}
^ permalink raw reply related
* Re: [PATCH] bring description of git diff --cc up to date
From: Jonathan Nieder @ 2008-07-22 23:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Greaves
In-Reply-To: <7vd4l5lio1.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Actually Linus talks about "when you have two versions to
> choose from, and if the result matches one of them, then it is not
> interesting".
That is much better - thanks. (The description by Linus I was referring
to was from
<http://thread.gmane.org/gmane.comp.version-control.git/89415>:
"So "--cc" only shows output if: the merge itself actually changed
something from _all_ parents" - which is not too misleading since the
two-parent case is the usual one.)
How about this, then?
--- %< ---
Subject: document diff --cc's long-ago-changed semantics
In February 2006 [1], the definition of "interesting hunk" for
git's "compact combined diff" format changed, without any
corresponding change in documentation. This patch brings the
documentation up to date.
[1] commit bf1c32bdec8223785c779779d0a660a099f69a63
combine-diff: update --cc "uninteresting hunks" logic
---
Documentation/git-diff-tree.txt | 12 +++++++-----
Documentation/rev-list-options.txt | 9 +++++----
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 0e45b58..14dc70d 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -92,12 +92,14 @@ include::pretty-options.txt[]
--cc::
This flag changes the way a merge commit patch is displayed,
in a similar way to the '-c' option. It implies the '-c'
- and '-p' options and further compresses the patch output
- by omitting hunks that show differences from only one
- parent, or show the same change from all but one parent
- for an Octopus merge. When this optimization makes all
+ and '-p' options and makes the patch output
+ even more compact by omitting uninteresting hunks. A hunk is
+ considered uninteresting if the person merging had two versions
+ to choose between among all of the parents and the result shows
+ no changes from one of those versions.
+ When this optimization makes all
hunks disappear, the commit itself and the commit log
- message is not shown, just like in any other "empty diff" case.
+ message are not shown, just like in any other "empty diff" case.
--always::
Show the commit itself and the commit log message even
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b6f5d87..c61d05d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -111,10 +111,11 @@ options may be given. See linkgit:git-diff-files[1] for more options.
--cc::
- This flag implies the '-c' options and further compresses the
- patch output by omitting hunks that show differences from only
- one parent, or show the same change from all but one parent for
- an Octopus merge.
+ This flag implies the '-c' option and makes the patch output
+ even more compact by omitting uninteresting hunks. A hunk is
+ considered uninteresting if the person merging had two versions
+ to choose between among all of the parents and the result shows
+ no changes from one of those versions.
-r::
--
1.5.6.3.549.g8ca11
^ permalink raw reply related
* Re: [PATCH] git-filter-branch.sh: Allow running in bare repositories
From: Johannes Schindelin @ 2008-07-22 23:27 UTC (permalink / raw)
To: Petr Baudis; +Cc: gitster, git
In-Reply-To: <20080722223710.29084.61373.stgit@localhost>
Hi,
On Wed, 23 Jul 2008, Petr Baudis wrote:
> Commit 46eb449c restricted git-filter-branch to non-bare repositories
> unnecessarily; git-filter-branch can work on bare repositories just
> fine.
I think this is fine.
> This also fixes suspicious shell boolean expression during a check
> for dirty working tree.
If you are talking about X && Y || Z, it is well established (and should
not be suspicious to a shell hacker like the creator of Cogito) that Z is
executed if either X fails, or X succeeds and Y fails.
> +test_expect_success 'rewrite bare repository identically' '
> + (git config core.bare true && cd .git && git-filter-branch branch)
> +'
> +git config core.bare false
Any reason why this is done outside the test?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Respect crlf attribute even if core.autocrlf has not been set
From: Johannes Schindelin @ 2008-07-22 23:23 UTC (permalink / raw)
To: Dmitry Potapov; +Cc: git, gitster
In-Reply-To: <20080722231153.GN2925@dpotapov.dyndns.org>
Hi,
On Wed, 23 Jul 2008, Dmitry Potapov wrote:
> On Tue, Jul 22, 2008 at 10:56:04PM +0100, Johannes Schindelin wrote:
> >
> > When a file's crlf attribute is explicitely set, it does not make
> > sense to ignore it, just because the config variable core.autocrlf has
> > not been set.
>
> Hmm... About a week ago, I was about to propose the same change, but
> after reading documentation and some thinking I was not able to convince
> myself that this change would be the right thing to do.
Well, I have a shared repository, where I set the attribute. Now, every
once in a while, people check in text _with_ CR/LF. Yes, that is right, I
marked it explicitely as crlf, yet I am on the whim of the people choosing
to set the config variable or not.
And I could not care less what the documentation says: if it does not make
sense, it does not make sense.
> > +test_expect_success 'attribute crlf is heeded even without core.autocrlf' '
>
> s/heeded/needed/
Nope. "heeded" is what I meant. I am not a native speaker, so this could
be wrong. But "needed" is not what I meant (the sentence would not make
sense with "needed").
Ciao,
Dscho
^ permalink raw reply
* Re: Q: how to remove or move submodules?
From: Petr Baudis @ 2008-07-22 23:22 UTC (permalink / raw)
To: Rob Sanheim; +Cc: git
In-Reply-To: <fc113d400807122338o637cc159n5e19fea5a15dc866@mail.gmail.com>
Hi,
On Sun, Jul 13, 2008 at 02:38:51AM -0400, Rob Sanheim wrote:
> I don't see any info on what the 'right' way is to delete or move
> submodules around in any of the docs. Normally I end up just hacking
> my way through it and hand modifying the .gitmodules file, but this
> seems wrong. Is there a recommended way?
currently, you have to hand-modify the .gitmodules file, then remove
the directory from the working tree and then from the index. In part
inspired by your question, I have submitted patches to add git mv and
git rm support for submodules some time ago, but they probably won't
make it to 1.6.0.
Petr "Pasky" Baudis
^ permalink raw reply
* regression in 92392b4
From: Pierre Habouzit @ 2008-07-22 23:17 UTC (permalink / raw)
To: spearce; +Cc: Git ML, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]
Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
did, since git in next cannot fetch on a regular basis for me. The
culprit seems to be commit 92392b4:
┌─(1:11)──<~/dev/scm/git 92392b4...>──
└[artemis] git fetch
remote: Counting objects: 461, done.
remote: Compressing objects: 100% (141/141), done.
remote: Total 263 (delta 227), reused 155 (delta 121)
Receiving objects: 100% (263/263), 95.55 KiB, done.
fatal: Out of memory, malloc failed
fatal: index-pack failed
[2] 16674 abort (core dumped) git fetch
┌─(1:12)──<~/dev/scm/git 92392b4...>──
└[artemis] git checkout -m HEAD~1; make git-index-pack
Previous HEAD position was 92392b4... index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
HEAD is now at 03993e1... index-pack: Track the object_entry that creates each base_data
GIT_VERSION = 1.5.6.3.3.g03993
CC index-pack.o
LINK git-index-pack
┌─(1:12)──<~/dev/scm/git 03993e1...>──
└[artemis] git fetch
remote: Counting objects: 461, done.
remote: Compressing objects: 100% (141/141), done.
remote: Total 263 (delta 227), reused 155 (delta 121)
Receiving objects: 100% (263/263), 95.55 KiB, done.
Resolving deltas: 100% (227/227), completed with 153 local objects.
From git://git.kernel.org/pub/scm/git/git
5ba2c22..0868a30 html -> origin/html
2857e17..abeeabe man -> origin/man
93310a4..95f8ebb master -> origin/master
559998f..e8bf351 next -> origin/next
You can see the commit sha's in the prompt. 03993e1 is fine, 92392b4 is
broken, I've absolutely no clue about what happens.
All I can say is that at some point in get_data_from_pack, obj[1].idx
points to something that is *not* a sha so it's probably corrupted.
(from index-pack.c).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] Respect crlf attribute even if core.autocrlf has not been set
From: Dmitry Potapov @ 2008-07-22 23:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0807222255450.8986@racer>
On Tue, Jul 22, 2008 at 10:56:04PM +0100, Johannes Schindelin wrote:
>
> When a file's crlf attribute is explicitely set, it does not make sense
> to ignore it, just because the config variable core.autocrlf has not
> been set.
Hmm... About a week ago, I was about to propose the same change, but
after reading documentation and some thinking I was not able to convince
myself that this change would be the right thing to do.
First, let's look at what Git's documentation says:
===
`crlf`
^^^^^^
This attribute controls the line-ending convention.
Set::
Setting the `crlf` attribute on a path is meant to mark
the path as a "text" file. 'core.autocrlf' conversion
takes place without guessing the content type by
inspection.
<snip>
The `core.autocrlf` conversion
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If the configuration variable `core.autocrlf` is false, no
conversion is done.
===
So, my reading is that if I set the `crlf` attribute on some path, but
I have core.autocrlf=false, there will be no conversion.
And this can be used to mark text files in .gitattribute, which is
stored in the repository and thus it is shared among users with
different end-of-line ending, i.e. you can have something like this
in .gitattribute:
*.[ch] crlf
*.txt crlf
but on Unix, you have core.autocrlf=false, so no conversion is done,
while, on Windows, you set core.autocrlf=true, so you will have crlf
conversion without any guessing.
Now, I can agree with you that using the 'crlf' attribute to mark text
files may appear not very intuitive (you may expect that crlf means that
those files always need crlf conversion), but right now we do not have
any better way to mark text files and the using crlf in this role is
explicitly suggested by documentation. See above.
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> index 1be7446..0bb3e6f 100755
> --- a/t/t0020-crlf.sh
> +++ b/t/t0020-crlf.sh
> @@ -436,4 +436,14 @@ test_expect_success 'invalid .gitattributes (must not crash)' '
>
> '
>
> +test_expect_success 'attribute crlf is heeded even without core.autocrlf' '
s/heeded/needed/
Dmitry
^ 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