* [RFH] straightening out "read-tree -m"
@ 2007-03-18 7:25 10% Junio C Hamano
0 siblings, 0 replies; 3+ results
From: Junio C Hamano @ 2007-03-18 7:25 UTC (permalink / raw)
To: git
While everybody else was having fun with low-hanging fruit
performance tweaking ;-), I was looking at rather depressing
picture.
I do not have much energy left to look into this further
tonight, but I noticed this while drawing a case table to deal
with the recent "why can't I switch between these two branches"
problem.
First part is sort of "accomplishment" for tonight. Two case
tables that describe desired semantics (meaning, the current
"read-tree -m -u" does not do this, and neither does
merge-recursive because it shares the same unpack-trees
backend).
* 2-way "read-tree -m -u A B".
While switching from branch A to branch B, if a path "foo" is
directory/file in A while the path is directory/file in B, then
these things should happen...
A B Outcome
----------------------------------------------------------------
D D No problem.
D F This should result in refusal when there are
local modification to paths in "foo" directory,
or there are untracked (but unignored) paths in
"foo/".
F D This should result in refusal if "foo" has local
modification.
F F This should result in refusal if "foo" has local
modification.
The patch to "verify_absent()" I sent out earlier tries to deal
with the second case above, although it still has issues
(namely, the *indpos needs to be recomputed after replacing
potentially multiple entries under "foo/" with "foo" from B,
which the patch does not do).
Then I started to wonder if we have similar problem on the 3-way
side.
* 3-way "read-tree -m -u O A B"
While on branch A, if you try to merge branch B using O as the
merge base, if a path "foo" is D/F in A/B/C (there are 8
combinations), these things should happen...
O A B Outcome
----------------------------------------------------------------
D D D No problem. Paths in the "foo/" directory
are merged subject to the usual rule.
D D F File "foo", which did not exist in ancestor
and we did not touch, is created. Which means
that contents of "foo/" is about to be
lost. We need to refuse this unless
everything in "foo/" matches between O
and A, there is no local modifications
in "foo/", and there is no untracked but
unignored paths in "foo/".
D F D File "foo", which did not exist in
ancestor and they did not touch, is
created, so we would want to keep it.
Which means that their changes to paths
in "foo/" will be lost. We should leave
conflict for paths in "foo/" that are
different between O and B.
D F F No problem. This is usual "added on
both branches" conflict.
F D D No problem. Paths in the "foo/"
directory have usual "added on both
branches" conflict.
F D F No problem. If file "foo" was modified
by them (i.e. O!=B) then usual "remove-modify"
conflict arises.
F F D No problem. If file "foo" was modified
by us (i.e. O!=A) then usual "remove-modify"
conflict arises. Also "foo" must not
have local modifications.
F F F No problem, usual 3-way rule applies.
Also "foo" must not have local modifications.
The sad story is how confused the current unpack-trees is, given
the test case for the above 3-way.
The attached script demonstrates one aspect of the sorry state
of three-way merge done with unpack-trees. You would notice
that there are multiple paths that have both stage '0' entry and
higher stage entries at the same time --- I do not think this
should not happen.
Git v1.0.0 to v1.2.0 refuse to merge but leaves two stage '0'
entries for path '4' and '8' (which is already incorrect but in
a different way). v1.3.0 and later leaves stage 0/1/2/3 entry
for path '8'.
Depressing...
#!/bin/sh
# If your 'test' is precious, do not run this script!
/bin/rm -fr test
create () {
rm -f .git/index
mark="$1"
shift
num=0
for t
do
num=$(($num+1))
rm -fr "$num"
case "$t" in
D)
mkdir "$num"; t="$num/$num" ;;
F)
t="$num" ;;
esac
echo "$mark" >"$t"
git add "$t"
done
git commit -m "$mark: $*"
}
mkdir test || exit
cd test
git init-db
create O D D D D F F F F
git tag O
git branch a
git branch b
rm -rf ?
git checkout a
create A D D F F D D F F
git tag A
rm -rf ?
git checkout b
create B D F D F D F D F
git tag B
rm -rf ?
git checkout a
git read-tree -m O A B
git ls-files -s
^ permalink raw reply [relevance 10%]
* Re: bug in read-tree -m on A -> A/A
2007-03-16 5:01 14% ` Junio C Hamano
@ 2007-03-16 6:25 13% ` Junio C Hamano
0 siblings, 0 replies; 3+ results
From: Junio C Hamano @ 2007-03-16 6:25 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
Junio C Hamano <junkio@cox.net> writes:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> .... The following test appears to
>> trigger the same failure,...
>
> You have file "A" on one branch, and file "A/A" on another
> branch. You are on the latter branch and switching to the
> former one.
>
> The following patch illustrates where you need to implement an
> alternate, loosened check, but should not be applied to your
> tree as-is. If you have local modification to path "A/A", this
> will lose it.
This does not do the bottom-up merge, but tries to catch the
lossy case within the limit of the current framework.
Only lightly tested, and I won't be applying it as-is yet as I
am not thinking very clearly tonight (no, I am not drunk, just
under the weather a bit).
Testing and improvements are very much appreciated.
---
unpack-trees.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 74 insertions(+), 1 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 2e2232c..2288762 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -468,6 +468,60 @@ static void invalidate_ce_path(struct cache_entry *ce)
cache_tree_invalidate_path(active_cache_tree, ce->name);
}
+static void verify_clean_subdirectory(const char *path, const char *action,
+ struct unpack_trees_options *o)
+{
+ /*
+ * we are about to extract "path"; we would not want to lose
+ * anything in the existing directory there.
+ */
+ int namelen;
+ int pos, i;
+ struct dir_struct d;
+ char *pathbuf;
+
+ /*
+ * First let's make sure we do not have a local modification
+ * in that directory.
+ */
+ namelen = strlen(path);
+ pos = cache_name_pos(path, namelen);
+ if (0 <= pos)
+ return; /* we have it as nondirectory */
+ pos = -pos - 1;
+ for (i = pos; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int len = ce_namelen(ce);
+ if (len < namelen ||
+ strncmp(path, ce->name, namelen) ||
+ ce->name[namelen] != '/')
+ break;
+ /*
+ * ce->name is an entry in the subdirectory.
+ */
+ verify_uptodate(ce, o);
+ }
+
+ /*
+ * Then we need to make sure that we do not lose a locally
+ * present file that is not ignored.
+ */
+ if (!o->dir)
+ return;
+
+ pathbuf = xmalloc(namelen + 2);
+ memcpy(pathbuf, path, namelen);
+ strcpy(pathbuf+namelen, "/");
+
+ memset(&d, 0, sizeof(d));
+ d.exclude_per_dir = o->dir->exclude_per_dir;
+ i = read_directory(&d, path, pathbuf, namelen+1);
+ if (i)
+ die("Updating '%s' would lose untracked files in it",
+ path);
+ free(pathbuf);
+}
+
/*
* We do not want to remove or overwrite a working tree file that
* is not tracked, unless it is ignored.
@@ -479,9 +533,28 @@ static void verify_absent(const char *path, const char *action,
if (o->index_only || o->reset || !o->update)
return;
- if (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))
+
+ if (!lstat(path, &st)) {
+ if (o->dir && excluded(o->dir, path))
+ /*
+ * path is explicitly excluded, so it is Ok to
+ * overwrite it.
+ */
+ return;
+ if (S_ISDIR(st.st_mode))
+ /*
+ * We are checking out path "foo" and
+ * found "foo/." in the working tree.
+ * This is tricky -- if we have modified
+ * files that are in "foo/" we would lose
+ * it.
+ */
+ verify_clean_subdirectory(path, action, o);
+ return;
+
die("Untracked working tree file '%s' "
"would be %s by merge.", path, action);
+ }
}
static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
^ permalink raw reply related [relevance 13%]
* Re: bug in read-tree -m on A -> A/A
@ 2007-03-16 5:01 14% ` Junio C Hamano
2007-03-16 6:25 13% ` Junio C Hamano
0 siblings, 1 reply; 3+ results
From: Junio C Hamano @ 2007-03-16 5:01 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
"Shawn O. Pearce" <spearce@spearce.org> writes:
> .... The following test appears to
> trigger the same failure,...
You have file "A" on one branch, and file "A/A" on another
branch. You are on the latter branch and switching to the
former one.
The following patch illustrates where you need to implement an
alternate, loosened check, but should not be applied to your
tree as-is. If you have local modification to path "A/A", this
will lose it.
-- >8 --
diff --git a/unpack-trees.c b/unpack-trees.c
index 2e2232c..345b2ee 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -479,9 +479,27 @@ static void verify_absent(const char *path, const char *action,
if (o->index_only || o->reset || !o->update)
return;
- if (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))
+
+ if (!lstat(path, &st)) {
+ if (o->dir && excluded(o->dir, path))
+ /*
+ * path is explicitly excluded, so it is Ok to
+ * overwrite it.
+ */
+ return;
+ if (S_ISDIR(st.st_mode))
+ /*
+ * We are checking out path "foo" and
+ * found "foo/." in the working tree.
+ * This is tricky -- if we have modified
+ * files that are in "foo/" we would lose
+ * it if we just uncoditinally return here.
+ */
+ return;
+
die("Untracked working tree file '%s' "
"would be %s by merge.", path, action);
+ }
}
static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
-- 8< --
To solve this sanely I think you would need to merge bottom-up,
which is quite a large change to read-tree. Currently the code
asks "is it Ok to extract A from the new tree?" and if we say
yes here it would remove all existing cache entries "A/*" and
replaces it with "A". After that happens, you would not even
have a chance to see if A/A has local modifications, or if you
have a local file that is not even known to git, say A/C, that
will also be lost by this tree switching.
^ permalink raw reply related [relevance 14%]
Results 1-3 of 3 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2007-03-16 4:19 bug in read-tree -m on A -> A/A Shawn O. Pearce
2007-03-16 5:01 14% ` Junio C Hamano
2007-03-16 6:25 13% ` Junio C Hamano
2007-03-18 7:25 10% [RFH] straightening out "read-tree -m" Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).