* Re: [PATCH] rebase -i: clean error message for --continue after failed exec
From: Junio C Hamano @ 2011-08-24 18:42 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <1314194508-12067-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> If after a failed "exec" instruction there are staged changes,...
I have to wonder why whatever "exec" runs is mucking with the index in the
first place. Shouldn't we forbid it?
^ permalink raw reply
* Re: [PATCH] rebase -i: clean error message for --continue after failed exec
From: Junio C Hamano @ 2011-08-24 18:54 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <7v62lmps6k.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> If after a failed "exec" instruction there are staged changes,...
>
> I have to wonder why whatever "exec" runs is mucking with the index in the
> first place. Shouldn't we forbid it?
I suspect your patch amounts to the same thing of forbidding, but
detecting the lack of $author_script feels like it is covering up the
symptom and not directly going for the cause of the symptom.
I wonder if doing something like this would be more direct approach to
achieve the same thing.
git-rebase--interactive.sh | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6ba7c1..31026dc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -472,24 +472,31 @@ do_next () {
git rev-parse --verify HEAD > "$state_dir"/stopped-sha
${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
status=$?
+ # Run in subshell because require_clean_work_tree can die.
+ dirty=f
+ (require_clean_work_tree "rebase") || dirty=t
if test "$status" -ne 0
then
warn "Execution failed: $rest"
+ test "$dirty" = f ||
+ warn "and made changes to the index and/or the working tree"
+
warn "You can fix the problem, and then run"
warn
warn " git rebase --continue"
warn
exit "$status"
fi
- # Run in subshell because require_clean_work_tree can die.
- if ! (require_clean_work_tree "rebase")
- then
+ if test "$dirty" = t
+ warn "Execution succeeded: $rest"
+ warn "but left changes to the index and/or the working tree"
warn "Commit or stash your changes, and then run"
warn
warn " git rebase --continue"
warn
exit 1
fi
+
;;
*)
warn "Unknown command: $command $sha1 $rest"
^ permalink raw reply related
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Heiko Voigt @ 2011-08-24 19:14 UTC (permalink / raw)
To: Brad King; +Cc: git, gitster
In-Reply-To: <680d2679c3275c01152500760311b5f96a93ea62.1314193375.git.brad.king@kitware.com>
Hi,
thanks for finding this subtle bug!
On Wed, Aug 24, 2011 at 09:59:50AM -0400, Brad King wrote:
> Since commit 68d03e4a (Implement automatic fast-forward merge for
> submodules, 2010-07-07) we try to suggest submodule commits that resolve
> a conflict. Consider a true recursive merge case
>
> b---bc
> / \ /
> o X
> \ / \
> c---cb
And here is a patch[1] that you can apply on top of yours which should fix
this. An extra pair of merge machinery knowing eyes appreciated. Its a
little bit workaroundish so if anymore has an idea how to fix this in
nicer way, please tell me.
[1]--8<----
From: Heiko Voigt <hvoigt@hvoigt.net>
Subject: [PATCH] protect submodule merge search against multiple calls for
the same path
When multiple merge-bases are found for two commits to be merged the
merge machinery will ask twice for a merge resolution. Currently its not
possible to use the revision-walking api for walking the same commits
multiple times. Since the result will not change we can simply fail
here if we are asked for a resolution of the same path again.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 9 +++++++++
t/t7405-submodule-merge.sh | 2 +-
2 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/submodule.c b/submodule.c
index 1ba9646..a4af08e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -646,6 +646,7 @@ int merge_submodule(unsigned char result[20], const char *path,
const unsigned char base[20], const unsigned char a[20],
const unsigned char b[20])
{
+ static char last_path[PATH_MAX] = {'\0'};
struct commit *commit_base, *commit_a, *commit_b;
int parent_count;
struct object_array merges;
@@ -699,6 +700,13 @@ int merge_submodule(unsigned char result[20], const char *path,
* user needs to confirm the resolution.
*/
+ /* in case of multiple merge-bases the merge algorithm will ask
+ * again for a resolution. We should not search twice for the
+ * same path.
+ */
+ if (!strcmp(path, last_path))
+ return 0;
+
/* find commit which merges them */
parent_count = find_first_merges(&merges, path, commit_a, commit_b);
switch (parent_count) {
@@ -726,6 +734,7 @@ int merge_submodule(unsigned char result[20], const char *path,
print_commit((struct commit *) merges.objects[i].item);
}
+ memcpy(last_path, path, strlen(path) + 1);
free(merges.objects);
return 0;
}
diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
index 8f6f2d6..603fb72 100755
--- a/t/t7405-submodule-merge.sh
+++ b/t/t7405-submodule-merge.sh
@@ -269,7 +269,7 @@ test_expect_success 'setup for recursive merge with submodule' '
'
# merge should leave submodule unmerged in index
-test_expect_failure 'recursive merge with submodule' '
+test_expect_success 'recursive merge with submodule' '
(cd merge-recursive &&
test_must_fail git merge top-bc &&
echo "160000 $(git rev-parse top-cb:sub) 2 sub" > expect2 &&
--
1.7.6.551.g4266ca
^ permalink raw reply related
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Junio C Hamano @ 2011-08-24 19:24 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Brad King, git
In-Reply-To: <20110824191438.GA45292@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> ... Its a
> little bit workaroundish so if anymore has an idea how to fix this in
> nicer way, please tell me.
>
> [1]--8<----
> From: Heiko Voigt <hvoigt@hvoigt.net>
> Subject: [PATCH] protect submodule merge search against multiple calls for
> the same path
>
> When multiple merge-bases are found for two commits to be merged the
> merge machinery will ask twice for a merge resolution. Currently its not
> possible to use the revision-walking api for walking the same commits
> multiple times.
I have been suspecting that most of this should be done in a separate
helper program that is run via run_command() interface, without
contaminating the object pool the main merge process has with data from
the submodule object store to begin with (i.e. add_submodule_odb() and
everything below should go). Wouldn't it be a lot cleaner solution?
^ permalink raw reply
* Re: [PATCH 0/2] Add an update=none option for 'loose' submodules
From: Heiko Voigt @ 2011-08-24 19:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jens Lehmann, git
In-Reply-To: <7vpqjvrebk.fsf@alter.siamese.dyndns.org>
Hi,
On Tue, Aug 23, 2011 at 02:46:39PM -0700, Junio C Hamano wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
> > It might surprise people. E.g. when their old scripts don't work anymore as
> > they did before because a submodule won't be populated or updated in the work
> > tree even though it is present in .git/config. So I agree that this should be
> > documented in the release notes so people can check if their expectations are
> > still met.
>
> Worse yet, their custom old scripts that they use to update submodules in
> their repository, if properly written, assume that anything registered in
> the .git/config file as [submodule "foo"] _must_ be populated, but they
> can no longer assume that and now has to look at submodule.foo.update and
> if it notices the variable is set to "none" leave the submodule repository
> alone. Having "submodule.foo" registered in the .git/config file alone
> used to mean the user is interested in "foo" submodule and wants to have a
> checkout for it, now it does not necessarily mean that.
>
> That is definitely a huge semantics change.
Ok seeing it that way. You are right. How about this?
-8<---
From: Heiko Voigt <hvoigt@hvoigt.net>
Subject: [PATCH] mention the new submodule.$name.update=none flag in the
ReleaseNotes
---
Documentation/RelNotes/1.7.7.txt | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/RelNotes/1.7.7.txt b/Documentation/RelNotes/1.7.7.txt
index 8de880f..b8edcf1 100644
--- a/Documentation/RelNotes/1.7.7.txt
+++ b/Documentation/RelNotes/1.7.7.txt
@@ -71,6 +71,10 @@ Updates since v1.7.6
submodule; it now goes on to update other submodules that can be
updated, and reports the ones with errors at the end.
+ * "git submodule update" does not clone/update a submodule when
+ submodule.$name.update is set to 'none'. This option is copied from
+ .gitmodules when a submodule is initialized.
+
* "git upload-pack" and "git receive-pack" learned to pretend only a
subset of the refs exist in a repository. This may help a site to
put many tiny repositories into one repository (this would not be
--
1.7.6.551.g4266ca
^ permalink raw reply related
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Heiko Voigt @ 2011-08-24 19:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brad King, git
In-Reply-To: <7vty96obo9.fsf@alter.siamese.dyndns.org>
On Wed, Aug 24, 2011 at 12:24:22PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > ... Its a
> > little bit workaroundish so if anymore has an idea how to fix this in
> > nicer way, please tell me.
> >
> > [1]--8<----
> > From: Heiko Voigt <hvoigt@hvoigt.net>
> > Subject: [PATCH] protect submodule merge search against multiple calls for
> > the same path
> >
> > When multiple merge-bases are found for two commits to be merged the
> > merge machinery will ask twice for a merge resolution. Currently its not
> > possible to use the revision-walking api for walking the same commits
> > multiple times.
>
> I have been suspecting that most of this should be done in a separate
> helper program that is run via run_command() interface, without
> contaminating the object pool the main merge process has with data from
> the submodule object store to begin with (i.e. add_submodule_odb() and
> everything below should go). Wouldn't it be a lot cleaner solution?
Hmm, I would like to keep it in process. Since there are platforms where
spawning new processes is very slow. If just the revision-walking is an
issue: I am currently working on extending it to be able to walk again,
because we also need that for the recursive push series.
But even if we are able to search twice or more (in or out process) we
would give the advice that there are multiple possible resolutions
for each merge base. For the merge search we do not take the bases into
account so the outcome will not change. So what I think would be
cleaner (also UI wise) is to remember whether we already gave advice and
not do it a second time.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Brad King @ 2011-08-24 20:02 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git
In-Reply-To: <20110824194618.GD45292@book.hvoigt.net>
On 8/24/2011 3:14 PM, Heiko Voigt wrote:
> thanks for finding this subtle bug!
Thanks for looking at it!
On 8/24/2011 3:46 PM, Heiko Voigt wrote:
> For the merge search we do not take the bases into
> account so the outcome will not change.
The test case creates history like this:
> b---bc
> / \ /
> o X
> \ / \
> c---cb
where b, c, bc, and cb all reference different submodule commits.
Isn't the merge search asked to search for a descendant of "b:sub" and "c:sub"
during the recursive part of the merge and then "bc:sub" and "cb:sub" during
the primary merge? Might those results be different?
As for the UI part, I think the user would be interested only in the search
results for the primary merge between HEAD and MERGE_HEAD. Results from the
intermediate merges might not make sense.
Thanks,
-Brad
^ permalink raw reply
* Re: [PATCH 6/6] Retain caches of submodule refs
From: Heiko Voigt @ 2011-08-24 20:05 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Junio C Hamano, Jeff King, Drew Northup, Jakub Narebski,
Josh Triplett
In-Reply-To: <4E54B394.2070006@alum.mit.edu>
On Wed, Aug 24, 2011 at 10:17:24AM +0200, Michael Haggerty wrote:
> On 08/13/2011 02:54 PM, Heiko Voigt wrote:
> > On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:
> >> diff --git a/refs.c b/refs.c
> >> index 8d1055d..f02cf94 100644
> >> --- a/refs.c
> >> +++ b/refs.c
> >
> > ...
> >
> >> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)
> >> */
> >> static struct cached_refs *get_cached_refs(const char *submodule)
> >> {
> >> - if (! submodule) {
> >> - if (!cached_refs)
> >> - cached_refs = create_cached_refs(submodule);
> >> - return cached_refs;
> >> - } else {
> >> - if (!submodule_refs)
> >> - submodule_refs = create_cached_refs(submodule);
> >> - else
> >> - /* For now, don't reuse the refs cache for submodules. */
> >> - clear_cached_refs(submodule_refs);
> >> - return submodule_refs;
> >> + struct cached_refs *refs = cached_refs;
> >> + if (! submodule)
> >> + submodule = "";
> >
> > Maybe instead of searching for the main refs store a pointer to them
> > locally so you can immediately return here. That will keep the
> > performance when requesting the main refs the same.
>
> I am assuming that the number of submodules will be small compared to
> the number of refs in a typical submodule. Therefore, given that the
> refs are stored in a big linked list and that the only thing that can
> realistically be done with the list is to iterate through it, the cost
> of finding the reference cache for a specific (sub-)module should be
> negligible compared to the cost of the iteration over refs in the cache.
> Treating the main module like the other submodules makes the code
> simpler, so I would prefer to leave it this way.
I just thought I mention it since it is a change and you are already
special casing the main module with this if(!submodule) but you are
probably right and in most cases (many refs and few submodules) this
change is negligible.
> If iteration over refs ever becomes a bottleneck, then optimization of
> the storage of refs within a (sub-)module would be a bigger win than
> special-casing the main module. And that is what I would like to work
> towards.
>
> > If I see it correctly you are always prepending to the linked list
>
> This is true.
>
> > and
> > in case many submodules get cached this could slow down the iteration
> > over the refs of the main repository.
>
> Is this a realistic concern? Remember that the extra search over
> submodules only occurs once for each scan through the list of references.
>
> I wrote an additional patch that moves the least-recently accessed
> module to the front of the list. But I doubt that the savings justify
> the 10-odd extra lines of code, so I kept it to myself. Please note
> that this approach gives pessimal performance (2x slower) if the
> submodules are iterated over repeatedly. If you would like me to add
> this patch to the patch series, please let me know.
No I don't think this is necessary.
> Long-term, it would be better to implement a "struct submodule" and use
> it to hold submodule-specific data like the ref cache. Then users could
> hold on to the "struct submodule *" and pass it, rather than the
> submodule name, to the functions that need it. But this goes beyond the
> scope of what I want to change now, especially since I have no
> experience even working with submodules.
>
> Summary: I hope I have convinced you that the extra overhead is
> negligible and does not justify additional code. But if you insist on
> more emphasis on performance (or obviously if you have numbers to back
> up your concerns) then I would be willing to put more work into it.
Yes I am also fine with the current state. I do not have a strong
opinion about this. This already adds enough improvements of the
submodule ref caching which looks a lot nicer than before.
Cheers Heiko
^ permalink raw reply
* git clean --exclude broken?
From: Todd Rinaldo @ 2011-08-24 19:15 UTC (permalink / raw)
To: git
I think I have found a new bug in 1.7.5:
# Setup:
mkdir tmp && cd tmp
git init
mkdir foo && touch foo/bar
mkdir bar && touch bar/baz
echo "/foo" > .gitignore
echo "/bar" >> .gitignore
git add .gitignore
git commit -m ignore
# The problem (Why is foo/ removed?)
$>git clean -dXf --exclude=/foo
Removing bar/
Removing foo/
I apologize if this isn't the correct channel to report a bug. Does anyone know if this is a known issue?
Thanks,
Todd Rinaldo
^ permalink raw reply
* Re: [PATCH] rebase -i: clean error message for --continue after failed exec
From: Jeff King @ 2011-08-24 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git
In-Reply-To: <7v62lmps6k.fsf@alter.siamese.dyndns.org>
On Wed, Aug 24, 2011 at 11:42:27AM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
> > If after a failed "exec" instruction there are staged changes,...
>
> I have to wonder why whatever "exec" runs is mucking with the index in the
> first place. Shouldn't we forbid it?
Certainly my only user has ever been "exec make test". But I wonder if
somebody is crazy enough to auto-generate some content and commit it.
OTOH, shouldn't it then be their responsibility to make the commit?
I.e., I can see at least the potential for mucking with the index, but I
really don't see a reason for _leaving_ the index in a mucked state.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase -i: clean error message for --continue after failed exec
From: Jeff King @ 2011-08-24 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git
In-Reply-To: <20110824202027.GA28900@sigill.intra.peff.net>
On Wed, Aug 24, 2011 at 04:20:27PM -0400, Jeff King wrote:
> Certainly my only user has ever been "exec make test". But I wonder if
Er, s/user/use.
> somebody is crazy enough to auto-generate some content and commit it.
> OTOH, shouldn't it then be their responsibility to make the commit?
> I.e., I can see at least the potential for mucking with the index, but I
> really don't see a reason for _leaving_ the index in a mucked state.
Having just read your followup patch, it looks sane. Exec commands are
free to do whatever they like with the index as long as it is left in a
clean state. That keeps the door open for semi-sane use cases, but will
catch unintended index manipulation.
-Peff
^ permalink raw reply
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Heiko Voigt @ 2011-08-24 20:27 UTC (permalink / raw)
To: Brad King; +Cc: Junio C Hamano, git
In-Reply-To: <4E5558BB.4040307@kitware.com>
On Wed, Aug 24, 2011 at 04:02:03PM -0400, Brad King wrote:
> On 8/24/2011 3:14 PM, Heiko Voigt wrote:
> > thanks for finding this subtle bug!
>
> Thanks for looking at it!
>
> On 8/24/2011 3:46 PM, Heiko Voigt wrote:
>> For the merge search we do not take the bases into
>> account so the outcome will not change.
>
> The test case creates history like this:
>
> > b---bc
> > / \ /
> > o X
> > \ / \
> > c---cb
>
> where b, c, bc, and cb all reference different submodule commits.
>
> Isn't the merge search asked to search for a descendant of "b:sub" and "c:sub"
> during the recursive part of the merge and then "bc:sub" and "cb:sub" during
> the primary merge? Might those results be different?
The merge is quite simple. All it does is check whether both changes
base->a or base->b point forward in the submodule. Then it checks
whether a is contained in b or the other way around. This is the only
case in which it will succeed automatically.
Supposing you merge bc into cb:
If I understand the situation correctly, the above is done first with
a := cb:sub, b := bc:sub, base := b:sub and then another time with
base := c:sub.
For the suggestion part only bc and cb are taken into account. That is
we search for the first commit in the submodule refs which contains both
bc:sub and cb:sub.
>
> As for the UI part, I think the user would be interested only in the search
> results for the primary merge between HEAD and MERGE_HEAD. Results from the
> intermediate merges might not make sense.
As stated above since bc:sub and cb:sub will not change in between two
searches the result for the suggestion will be the same. What I meant
was that the same result would be output twice (or more).
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 0/2] Add an update=none option for 'loose' submodules
From: Heiko Voigt @ 2011-08-24 20:38 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Junio C Hamano, git
In-Reply-To: <4E540B03.2030909@web.de>
On Tue, Aug 23, 2011 at 10:18:11PM +0200, Jens Lehmann wrote:
> Am 23.08.2011 21:43, schrieb Heiko Voigt:
> > Another change I am thinking of (which would definitely need an entry in
> > the release notes) is to change submodule foreach to iterate over all
> > gitmodule entries in the index/HEAD/worktree (not sure yet) instead of
> > "just entries that are in .git/config".
>
> When changing the default I think we'll surprise a lot of users (imagine
> someone running a "git submodule foreach pwd" when some submodules aren't
> populated). But adding an option to "git submodule foreach" (and maybe others)
> to get the list of submodules from the index or HEAD might make sense (while
> I'm not sure parsing the work tree does, as you'll basically have to pick up
> any .git you find. AFAICS a submodule is defined either by an entry in the
> .gitmodules file, in .git/config or through a gitlink entry in a commit or the
> index. So maybe the third alternative to index and HEAD is to use those found
> in .gitmodules?).
>
> Could you describe a use case for that?
Yes, a repository using the submodule.$name.checkout=none config.
Currently its hard to iterate over all submodules to set this config to
'checkout' locally. You can not use submodule foreach for that since it
will skip submodule directories that do not have .git in them.
But you are right this should obviously be done using an option like
git submodule foreach --head ...
or similar.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH] get_indexed_object can return NULL if nothing is in that slot; check for it
From: Brian Harring @ 2011-08-24 20:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmxeypudw.fsf@alter.siamese.dyndns.org>
On Wed, Aug 24, 2011 at 10:54:51AM -0700, Junio C Hamano wrote:
> Thanks for a fix.
>
> It is both interesting and disturbing to see that these small mistakes are
> discovered a week after the topics hit 'master', even though it has been
> cooking in 'next' for a week before that happened (in the case of this
> topic, it also hit 'maint' yesterday).
I'll admit I'm slightly surprised it slipped past for initial
development- that said, you have to explicitly trigger the race to
trigger the segfault. And that's not easy w/out building out a
custom setup- even w/ that setup, you need to go digging in server
logs to realize the previous serverside failure just converted to a
segfault. Clientside, it hung just the same.
Bit nonobvious. Plus, shit happens. ;)
Either way, I was poking at the source trying to figure out how to get
some unittests for an end to end testing of the http/smartserv; that
said I was having a helluva time finding a way to do it without
bundling a stub of a webserver. Suggestions would be welcome on that
one.
~brian
^ permalink raw reply
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Brad King @ 2011-08-24 20:40 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git
In-Reply-To: <20110824202721.GF45292@book.hvoigt.net>
On 8/24/2011 4:27 PM, Heiko Voigt wrote:
>>> b---bc
>>> / \ /
>>> o X
>>> \ / \
>>> c---cb
[snip]
> Supposing you merge bc into cb:
> If I understand the situation correctly, the above is done first with
> a := cb:sub, b := bc:sub, base := b:sub and then another time with
> base := c:sub.
When merging bc and cb there are two merge bases: b and c. The recursive
merge strategy first performs a "virtual" merge between b and c and uses
the result as a fictional merge base between bc and cb. Currently the
submodule merge search runs during the "virtual" merge and gives advice.
Then it later dies while trying to search during the "real" merge.
After applying my patch, try this:
$ cd t && ./t7405-submodule-merge.sh --verbose
...
Merging:
8cbd0fb cb
virtual top-bc
found 2 common ancestor(s):
f6b4d5a b
4d9cfab c
Merging:
f6b4d5a b
4d9cfab c
found 1 common ancestor(s):
a2ff72f a
warning: Failed to merge submodule sub (multiple merges found)
806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc
db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb
Auto-merging sub
CONFLICT (submodule): Merge conflict in sub
fatal: --ancestry-path given but there are no bottom commits
One can see that the advice given talks about merging "b:sub" and "c:sub"
and the suggested commits are actually "bc:sub" and "cb:sub". This advice
is not useful to someone mergeing bc and cb.
-Brad
^ permalink raw reply
* Re: [PATCH] get_indexed_object can return NULL if nothing is in that slot; check for it
From: Junio C Hamano @ 2011-08-24 21:10 UTC (permalink / raw)
To: Brian Harring; +Cc: git
In-Reply-To: <20110824204021.GA28157@beast>
Brian Harring <ferringb@gmail.com> writes:
> On Wed, Aug 24, 2011 at 10:54:51AM -0700, Junio C Hamano wrote:
>> Thanks for a fix.
>>
>> It is both interesting and disturbing to see that these small mistakes are
>> discovered a week after the topics hit 'master', even though it has been
>> cooking in 'next' for a week before that happened (in the case of this
>> topic, it also hit 'maint' yesterday).
>
> I'll admit I'm slightly surprised it slipped past for initial
> development- that said, you have to explicitly trigger the race to
> trigger the segfault. And that's not easy w/out building out a
> custom setup- even w/ that setup, you need to go digging in server
> logs to realize the previous serverside failure just converted to a
> segfault. Clientside, it hung just the same.
>
> Bit nonobvious. Plus, shit happens. ;)
>
> Either way, I was poking at the source trying to figure out how to get
> some unittests for an end to end testing of the http/smartserv; that
> said I was having a helluva time finding a way to do it without
> bundling a stub of a webserver. Suggestions would be welcome on that
> one.
I probably should have made it clear that I was not complaining, and if it
sounded as if I was complaining at _you_, my deepest apologies.
In any case, thanks, and congratulations for your first commit in git.git
;-)
^ permalink raw reply
* [WIP PATCH] revision-walking: allow iterating revisions multiple times
From: Heiko Voigt @ 2011-08-24 21:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Fredrik Gustafsson, git, jens.lehmann
In-Reply-To: <7vd3fxulw8.fsf@alter.siamese.dyndns.org>
---
Hi,
On Mon, Aug 22, 2011 at 03:22:31PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > Junio since you are one person listed in the api docs could you maybe
> > quickly explain to me what this flag is used for?
>
> It is used in order to avoid walking the object we have walked already.
>
> Which in turn means that once you walk chain of objects, unless you
> remember the ones you walked and clear the marks after you are done, you
> cannot walk the object chain for unrelated purposes. See how functions
> like get_merge_bases_many() walk portions of graph for their own purpose
> and then avoid disrupting others by calling clear_commit_marks(). The use
> of TMP_MARK (and its clearing after the function is done with the marked
> objects) in remove_duplicate_parents() serve the same purpose.
What do you think about this approach ? Its not yet correctly collecting
revisions for all situations but it fixes the demonstrated test failure.
revision.c | 24 ++++++++++++++++++++++++
revision.h | 3 +++
submodule.c | 3 +++
3 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/revision.c b/revision.c
index c46cfaa..e374c4a 100644
--- a/revision.c
+++ b/revision.c
@@ -500,6 +500,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
continue;
p->object.flags |= SEEN;
commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
+ if (revs->fill_reset_list)
+ add_object_array(&p->object, NULL, &revs->walked);
}
return 0;
}
@@ -527,6 +529,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
if (!(p->object.flags & SEEN)) {
p->object.flags |= SEEN;
commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
+ if (revs->fill_reset_list)
+ add_object_array(&p->object, NULL, &revs->walked);
}
if (revs->first_parent_only)
break;
@@ -1950,6 +1954,23 @@ static void set_children(struct rev_info *revs)
}
}
+void reset_revision_walk(struct rev_info *revs)
+{
+ int nr = revs->walked.nr;
+ struct object_array_entry *e = revs->walked.objects;
+
+ /* reset the seen flags set by prepare_revision_walk */
+ while (--nr >= 0) {
+ struct object *o = e->item;
+ o->flags &= ~(ALL_REV_FLAGS);
+ e++;
+ }
+ free(revs->walked.objects);
+ revs->walked.nr = 0;
+ revs->walked.alloc = 0;
+ revs->walked.objects = NULL;
+}
+
int prepare_revision_walk(struct rev_info *revs)
{
int nr = revs->pending.nr;
@@ -1964,6 +1985,9 @@ int prepare_revision_walk(struct rev_info *revs)
if (commit) {
if (!(commit->object.flags & SEEN)) {
commit->object.flags |= SEEN;
+ if (revs->fill_reset_list)
+ add_object_array(&commit->object, NULL,
+ &revs->walked);
commit_list_insert_by_date(commit, &revs->commits);
}
}
diff --git a/revision.h b/revision.h
index 3d64ada..6a0fa99 100644
--- a/revision.h
+++ b/revision.h
@@ -28,6 +28,7 @@ struct rev_info {
/* Starting list */
struct commit_list *commits;
struct object_array pending;
+ struct object_array walked;
/* Parents of shown commits */
struct object_array boundary_commits;
@@ -72,6 +73,7 @@ struct rev_info {
bisect:1,
ancestry_path:1,
first_parent_only:1;
+ unsigned int fill_reset_list:1;
/* Diff flags */
unsigned int diff:1,
@@ -169,6 +171,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
const char * const usagestr[]);
extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
+extern void reset_revision_walk(struct rev_info *revs);
extern int prepare_revision_walk(struct rev_info *revs);
extern struct commit *get_revision(struct rev_info *revs);
extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
diff --git a/submodule.c b/submodule.c
index dc95498..410d8e4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -441,6 +441,7 @@ static int inspect_superproject_commits(unsigned char new_sha1[20], const char *
strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
init_revisions(&rev, NULL);
+ rev.fill_reset_list = 1;
sha1_copy = xstrdup(sha1_to_hex(new_sha1));
argv[1] = sha1_copy;
argv[3] = remotes_arg.buf;
@@ -451,6 +452,8 @@ static int inspect_superproject_commits(unsigned char new_sha1[20], const char *
while ((commit = get_revision(&rev)) && do_continue)
do_continue = commit_need_pushing(commit, commit->parents, func, data);
+
+ reset_revision_walk(&rev);
free(sha1_copy);
strbuf_release(&remotes_arg);
--
1.7.6.553.g84dc
^ permalink raw reply related
* Re: git clean --exclude broken?
From: Junio C Hamano @ 2011-08-24 21:23 UTC (permalink / raw)
To: Todd Rinaldo; +Cc: git
In-Reply-To: <A04A4D84-16CC-438C-8828-0D11BE9DE2DA@cpanel.net>
Todd Rinaldo <toddr@cpanel.net> writes:
> I think I have found a new bug in 1.7.5:
My quick check indicates 1.7.3 behaves the same way, and 1.7.2.5 didn't
have --exclude option, so this does not seem to be anything particularly
new in the 1.7.5 release.
> # The problem (Why is foo/ removed?)
> $>git clean -dXf --exclude=/foo
> Removing bar/
> Removing foo/
Why is this command line giving -X that tells us not to use the ignore
rules, and --exclude option at the same time?
^ permalink raw reply
* Re: [PATCH/RFC 2/2] git-p4: Add complex test case for branch import
From: Vitor Antunes @ 2011-08-24 21:23 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Tor Arvid Lund
In-Reply-To: <CAOpHH-V9nm2NRD47gdUj6sLJSTypjABjE-JGCkADE=fxh2nSWg@mail.gmail.com>
On Wed, Aug 24, 2011 at 11:46 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> 1. When doing a "checkpoint" it would make sense to have some kind of
> feedback loop to know when fast-import completes it. In fact, I've
> just looked at fast-import man page and the "progress" command seems
> to do exactly that! Myabe we should crease a specific function for
> "checkpoint" that would call also "progress" instead of calling
> os.sleep().
# Force a checkpoint in fast-import and wait for it to finish
def checkpoint(self):
self.gitStream.write("checkpoint\n\n")
self.gitStream.write("progress checkpoint\n\n")
out = self.gitOutput.readline()
if self.verbose:
print "checkpoint finished: " + out
This seems to work! :)
> 2. In order to avoid needing to use "--force" it would be nice to have
> some form of "drop" command in fast-import that would allows us to
> actively drop an older commit after calling "reset" (this could even
> be an option of "reset"). This way fast-import would not find
> dangling commits in the end of the import. Maybe there's already
> some sort of command to achieve this in fast-import...? We could
> probably ask to one of its maintainers.
Just missing point 2 now.
--
Vitor Antunes
^ permalink raw reply
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Heiko Voigt @ 2011-08-24 21:32 UTC (permalink / raw)
To: Brad King; +Cc: Junio C Hamano, git
In-Reply-To: <4E5561D2.2080206@kitware.com>
Hi,
On Wed, Aug 24, 2011 at 04:40:50PM -0400, Brad King wrote:
> On 8/24/2011 4:27 PM, Heiko Voigt wrote:
>>>> b---bc
>>>> / \ /
>>>> o X
>>>> \ / \
>>>> c---cb
> [snip]
>> Supposing you merge bc into cb:
>> If I understand the situation correctly, the above is done first with
>> a := cb:sub, b := bc:sub, base := b:sub and then another time with
>> base := c:sub.
>
> When merging bc and cb there are two merge bases: b and c. The recursive
> merge strategy first performs a "virtual" merge between b and c and uses
> the result as a fictional merge base between bc and cb. Currently the
> submodule merge search runs during the "virtual" merge and gives advice.
> Then it later dies while trying to search during the "real" merge.
But what happens if this "virtual" merge of b and c does not succeed
like in our case?
> After applying my patch, try this:
>
> $ cd t && ./t7405-submodule-merge.sh --verbose
> ...
> Merging:
> 8cbd0fb cb
> virtual top-bc
> found 2 common ancestor(s):
> f6b4d5a b
> 4d9cfab c
> Merging:
> f6b4d5a b
> 4d9cfab c
> found 1 common ancestor(s):
> a2ff72f a
> warning: Failed to merge submodule sub (multiple merges found)
> 806049692f8921101f2e7223852e3bd74f7187c8: > Merge branch 'sub-c' into sub-bc
> db70dfacda48ce55365256a58eaf89b7da87cbe7: > Merge branch 'sub-b' into sub-cb
> Auto-merging sub
> CONFLICT (submodule): Merge conflict in sub
> fatal: --ancestry-path given but there are no bottom commits
Yeah I have seen this and it looked to me as if those two commits are
bc:sub and cb:sub since the commit message talks about c:sub merged into
bc:sub and vice versa.
> One can see that the advice given talks about merging "b:sub" and "c:sub"
> and the suggested commits are actually "bc:sub" and "cb:sub". This advice
> is not useful to someone mergeing bc and cb.
In this case we should only output the advice of the last merge of
course. It will have the same result in this case though since the merge
of bc:sub and cb:sub still has just these two candidates.
Cheers Heiko
^ permalink raw reply
* Re: Buggy handling of non-canonical ref names
From: Carlos Martín Nieto @ 2011-08-24 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git discussion list
In-Reply-To: <7vaaayps9z.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]
On Wed, 2011-08-24 at 11:40 -0700, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
> > What is the policy about reference names and their canonicalization?
>
> The overall policy has been that we care about well-formed input, and
> everything else is "undefined", even though as you found out some of them
> try to work sensibly.
>
> > $ git check-ref-format /foo/bar ; echo $?
> > 0
> >
> > $ git check-ref-format --print /foo/bar
> > /foo/bar
>
> I think these are bogus. Patches welcome.
>
The rules in the manpage don't forbit it, as we assume that $GIT_DIR/ is
going to be put in front. This makes /foo/bar mean the same as foo/bar
(both become would cause git to look at $GIT_DIR/foo/bar), but I agree
that it can be quite confusing.
> > However, creating a reference with such a name is equivalent to creating
> > a reference without the leading slash:
> >
> > $ git update-ref /foo/bar HEAD
> > $ cat .git/foo/bar
> > ef6cf90ba11dd6205f8b974692d795ea0b1c0bdd
> > $ git branch /bar/baz
> > $ git for-each-ref | grep baz
> > ef6cf90ba11dd6205f8b974692d795ea0b1c0bdd commit refs/heads/bar/baz
>
> These are just examples of "undefined being nice to the user as a bonus".
Or not, the user might have expected `git branch /bar/baz` to create a
reference in $GIT_DIR/bar/baz with the SHA of the current branch (yes,
it's probably unlikely, but this is what one might think when giving
references as absolute paths).
Be as it may, allowing /foo/bar is not a good thing, how about this
patch? It doesn't modify the manpage, as I wasn't sure whether this
should become an explicit rule.
-- >8 --
Subject: [PATCH] Don't allow reference names to start with '/'
Being able to name references using absolute filenames is confusing
at best and might give people wrong ideas.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
refs.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/refs.c b/refs.c
index 6f313a9..ab549e4 100644
--- a/refs.c
+++ b/refs.c
@@ -879,6 +879,10 @@ int check_ref_format(const char *ref)
int ret = CHECK_REF_FORMAT_OK;
const char *cp = ref;
+ /* we don't want refs expressed as absolute paths */
+ if (*cp == '/')
+ return CHECK_REF_FORMAT_ERROR;
+
level = 0;
while (1) {
while ((ch = *cp++) == '/')
--
1.7.5.2.354.g349bf
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: [WIP PATCH] revision-walking: allow iterating revisions multiple times
From: Junio C Hamano @ 2011-08-24 21:44 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Fredrik Gustafsson, git, jens.lehmann
In-Reply-To: <20110824211431.GH45292@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> +void reset_revision_walk(struct rev_info *revs)
> +{
> + int nr = revs->walked.nr;
> + struct object_array_entry *e = revs->walked.objects;
> +
> + /* reset the seen flags set by prepare_revision_walk */
> + while (--nr >= 0) {
> + struct object *o = e->item;
> + o->flags &= ~(ALL_REV_FLAGS);
> + e++;
> + }
> + free(revs->walked.objects);
> + revs->walked.nr = 0;
> + revs->walked.alloc = 0;
> + revs->walked.objects = NULL;
> +}
I am afraid that this is not good enough for general purpose. The object
you walk in the middle of doing something may have been marked for reasons
other than your extra walking before you started your walk. Imagine
* The command takes arguments like rev-list does;
* It calls setup_revisions(), which marks commits given from the command
line with marks like UNINTERESTING, and then prepare_revision_walk();
* It walks the commit graph and does interesting things on commits that
it discovers, by repeatedly calling get_revision(), e.g.:
while ((commit = get_revision()) != NULL) {
do_something_interesting(commit);
}
Now, you add a new caller that walks the commit graph for a different
reason from the primary revision walking done by the command somewhere
down in the callchain of do_something_interesting()---obviously you cannot
use the above reset_revision_walk() to clean things up, as it will break
the outer revision walk.
If on the other hand you will _never_ have more than one revision walk
going on, it may amount to the same thing to iterate over the object array
and clear all the flags.
Traditionally the way to do nested revision walk that can potentially be
done more than once (but never having such a sub-walk in parallel) was to
remember the start points of the subwalk, use private marks that are not
used in the outer walk during the subwalk, and call clear_commit_marks()
on these start points when a subwalk is done to clear only the marks the
subwalk used.
^ permalink raw reply
* Re: Buggy handling of non-canonical ref names
From: Junio C Hamano @ 2011-08-24 22:27 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <7vaaayps9z.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> What is the policy about reference names and their canonicalization?
>
> The overall policy has been that we care about well-formed input, and
> everything else is "undefined", even though as you found out some of them
> try to work sensibly.
>
>> $ git check-ref-format /foo/bar ; echo $?
>> 0
>>
>> $ git check-ref-format --print /foo/bar
>> /foo/bar
>
> I think these are bogus. Patches welcome.
I actually think the former is correct and the latter should strip the
leading slash. Essentially what "check-ref-format" (and the underlying
check_ref_format() function) validates is if the given user string can be
used under $GIT_DIR/refs/ to name a ref, and $GIT_DIR/refs//foo/bar is
(because we "tolerate duplicated slashes" cf. comment in the function) the
same as $GIT_DIR/refs/foo/bar and is allowed.
I do not know what the original motivation behind --print was, but it
seems to want to show it canonicalized, so it should strip the leading
slash as well.
I think what is missing is a unified way to canonicalize the refnames
(which led to the inconsistencies you observed), and I strongly suspect
that check_ref_format() should learn to return the canonicalized format
(if asked by the caller) and the caller should use the canonicalized
version after feeding end-user input to it.
Then the plumbing "check-ref-format --print" can use it just like any
other caller that should be (or already are) using check_ref_format()
to validate the end-user input.
Yes, such a change will update the overall policy I stated earlier and
narrow the scope of "undefined" down a bit, by uniformly codifying that
leading and duplicate slashes are removed to be nice to the user.
^ permalink raw reply
* Re: [PATCH] submodule: Demonstrate known breakage during recursive merge
From: Junio C Hamano @ 2011-08-24 22:43 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Brad King, git
In-Reply-To: <20110824194618.GD45292@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
>> I have been suspecting that most of this should be done in a separate
>> helper program that is run via run_command() interface, without
>> contaminating the object pool the main merge process has with data from
>> the submodule object store to begin with (i.e. add_submodule_odb() and
>> everything below should go). Wouldn't it be a lot cleaner solution?
>
> Hmm, I would like to keep it in process. Since there are platforms where
> spawning new processes is very slow.
Adding submodule's odb into the main process _will_ also have performance
penalties because it will make it more expensive to look up objects that
belong to the superproject when the superproject wants its own look up.
In case you haven't realized yet, walking revision graph multiple times
while making sure that you do not affect other revision traversals in
effect is hard to arrange right. But more importantly, correctness counts
more than performing quickly and giving a bogus result with premature
optimization that makes it harder to implement things correctly (and
harder to verify the change is correct).
^ permalink raw reply
* Re: git clean --exclude broken?
From: Todd Rinaldo @ 2011-08-24 23:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vliuio65w.fsf@alter.siamese.dyndns.org>
On Aug 24, 2011, at 4:23 PM, Junio C Hamano wrote:
> Todd Rinaldo <toddr@cpanel.net> writes:
>
>> I think I have found a new bug in 1.7.5:
>
> My quick check indicates 1.7.3 behaves the same way, and 1.7.2.5 didn't
> have --exclude option, so this does not seem to be anything particularly
> new in the 1.7.5 release.
No. I was just clarifying what my binary was for research purposes.
>
>> # The problem (Why is foo/ removed?)
>> $>git clean -dXf --exclude=/foo
>> Removing bar/
>> Removing foo/
>
> Why is this command line giving -X that tells us not to use the ignore
> rules, and --exclude option at the same time?
My more complicated use of the command wanted to use the .gitignore rules to cleanup ignored files with the exception of 1 directory. I believe -dxf --exclude is also broken in the same way.
^ 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