* [PATCH 1/8] apply: reformat comment
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 6:02 ` [PATCH 2/8] apply: accept --threeway command line option Junio C Hamano
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
With the "--reject" option, apply_fragments() can return 0, while marking
the fragment that was rejected as such, and there was a comment for that,
but it was stated rather poorly.
Update the comment and enclose it in a block, as this part will be getting
new code in later patches.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 725712d..fc7f07b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3066,8 +3066,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
img = strbuf_detach(&buf, &len);
prepare_image(&image, img, len, !patch->is_binary);
- if (apply_fragments(&image, patch) < 0)
- return -1; /* note with --reject this succeeds. */
+ if (apply_fragments(&image, patch) < 0) {
+ /* Note: with --reject, the above call succeeds. */
+ return -1;
+ }
patch->result = image.buf;
patch->resultsize = image.len;
add_to_fn_table(patch);
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/8] apply: accept --threeway command line option
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
2012-05-10 6:02 ` [PATCH 1/8] apply: reformat comment Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 12:40 ` Nguyen Thai Ngoc Duy
2012-05-10 6:02 ` [PATCH 3/8] apply: split load_preimage() helper function out Junio C Hamano
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
This is the beginning of teaching the three-way merge fallback logic "git
am -3" uses to the underlying "git apply". It only implements the command
line parsing part, and does not do anything interesting yet.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index fc7f07b..cb6aad5 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -46,6 +46,7 @@ static int apply_with_reject;
static int apply_verbosely;
static int allow_overlap;
static int no_add;
+static int threeway;
static const char *fake_ancestor;
static int line_termination = '\n';
static unsigned int p_context = UINT_MAX;
@@ -3024,6 +3025,11 @@ static void prepare_fn_table(struct patch *patch)
}
}
+static int try_threeway_fallback(struct image *image, struct patch *patch)
+{
+ return -1; /* for now */
+}
+
static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
{
struct strbuf buf = STRBUF_INIT;
@@ -3068,7 +3074,8 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
if (apply_fragments(&image, patch) < 0) {
/* Note: with --reject, the above call succeeds. */
- return -1;
+ if (!threeway || try_threeway_fallback(&image, patch) < 0)
+ return -1;
}
patch->result = image.buf;
patch->resultsize = image.len;
@@ -3981,6 +3988,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
"apply a patch without touching the working tree"),
OPT_BOOLEAN(0, "apply", &force_apply,
"also apply the patch (use with --stat/--summary/--check)"),
+ OPT_BOOL(0, "threeway", &threeway,
+ "attempt three-way merge if a patch does not apply"),
OPT_FILENAME(0, "build-fake-ancestor", &fake_ancestor,
"build a temporary index based on embedded index information"),
{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/8] apply: accept --threeway command line option
2012-05-10 6:02 ` [PATCH 2/8] apply: accept --threeway command line option Junio C Hamano
@ 2012-05-10 12:40 ` Nguyen Thai Ngoc Duy
2012-05-10 15:31 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-05-10 12:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, May 10, 2012 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This is the beginning of teaching the three-way merge fallback logic "git
> am -3" uses to the underlying "git apply". It only implements the command
> line parsing part, and does not do anything interesting yet.
Can we reuse '-3/--3way' instead of --threeway? I already have hard
time remembering which command uses -m/--merge, which one -3.
--
Duy
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/8] apply: accept --threeway command line option
2012-05-10 12:40 ` Nguyen Thai Ngoc Duy
@ 2012-05-10 15:31 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 15:31 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Thu, May 10, 2012 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This is the beginning of teaching the three-way merge fallback logic "git
>> am -3" uses to the underlying "git apply". It only implements the command
>> line parsing part, and does not do anything interesting yet.
>
> Can we reuse '-3/--3way' instead of --threeway? I already have hard
> time remembering which command uses -m/--merge, which one -3.
Oh, not just "Can we reuse", but we definitely should. Thanks for
pointing it out.
It was just that I didn't check what "am" exactly calls it, as I always
just type "am -s3c" these days ;-).
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/8] apply: split load_preimage() helper function out
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
2012-05-10 6:02 ` [PATCH 1/8] apply: reformat comment Junio C Hamano
2012-05-10 6:02 ` [PATCH 2/8] apply: accept --threeway command line option Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 6:02 ` [PATCH 4/8] apply: clear_image() clears things a bit more Junio C Hamano
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
The function apply_data() gets a patch for a single path, reads the
preimage in core, and applies the change represented in the patch.
Separate out the first part that reads the preimage into a separate
helper function load_preimage().
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index cb6aad5..9f6f74f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3025,15 +3025,10 @@ static void prepare_fn_table(struct patch *patch)
}
}
-static int try_threeway_fallback(struct image *image, struct patch *patch)
-{
- return -1; /* for now */
-}
-
-static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
+static int load_preimage(struct image *image,
+ struct patch *patch, struct stat *st, struct cache_entry *ce)
{
struct strbuf buf = STRBUF_INIT;
- struct image image;
size_t len;
char *img;
struct patch *tpatch;
@@ -3070,7 +3065,21 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
}
img = strbuf_detach(&buf, &len);
- prepare_image(&image, img, len, !patch->is_binary);
+ prepare_image(image, img, len, !patch->is_binary);
+ return 0;
+}
+
+static int try_threeway_fallback(struct image *image, struct patch *patch)
+{
+ return -1; /* for now */
+}
+
+static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
+{
+ struct image image;
+
+ if (load_preimage(&image, patch, st, ce) < 0)
+ return -1;
if (apply_fragments(&image, patch) < 0) {
/* Note: with --reject, the above call succeeds. */
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/8] apply: clear_image() clears things a bit more
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (2 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 3/8] apply: split load_preimage() helper function out Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 6:02 ` [PATCH 5/8] apply: refactor read_file_or_gitlink() Junio C Hamano
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
The clear_image() function did not clear the line table in the image
structure; this does not matter for the current callers, as the function
is only called from the codepaths that deal with binary patches where the
line table is never populated, and the codepaths that do populate the line
table free it themselves.
But it will start to matter when we introduce a codepath to retry a failed
patch, so make sure it clears and frees everything.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 9f6f74f..628a89e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -372,8 +372,8 @@ static void prepare_image(struct image *image, char *buf, size_t len,
static void clear_image(struct image *image)
{
free(image->buf);
- image->buf = NULL;
- image->len = 0;
+ free(image->line_allocated);
+ memset(image, 0, sizeof(*image));
}
/* fmt must contain _one_ %s and no other substitution */
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/8] apply: refactor read_file_or_gitlink()
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (3 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 4/8] apply: clear_image() clears things a bit more Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 6:02 ` [PATCH 6/8] apply: fall back on three-way merge Junio C Hamano
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
Reading a blob out of the object store does not have to require that the
caller has a cache entry for it.
Create a read_blob_object() helper function that takes the object name and
mode, and use it to reimplement the original function as a thin wrapper to
it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 628a89e..b1dd23c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2931,20 +2931,17 @@ static int apply_fragments(struct image *img, struct patch *patch)
return 0;
}
-static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)
+static int read_blob_object(struct strbuf *buf, const unsigned char *sha1, unsigned mode)
{
- if (!ce)
- return 0;
-
- if (S_ISGITLINK(ce->ce_mode)) {
+ if (S_ISGITLINK(mode)) {
strbuf_grow(buf, 100);
- strbuf_addf(buf, "Subproject commit %s\n", sha1_to_hex(ce->sha1));
+ strbuf_addf(buf, "Subproject commit %s\n", sha1_to_hex(sha1));
} else {
enum object_type type;
unsigned long sz;
char *result;
- result = read_sha1_file(ce->sha1, &type, &sz);
+ result = read_sha1_file(sha1, &type, &sz);
if (!result)
return -1;
/* XXX read_sha1_file NUL-terminates */
@@ -2953,6 +2950,13 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)
return 0;
}
+static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)
+{
+ if (!ce)
+ return 0;
+ return read_blob_object(buf, ce->sha1, ce->ce_mode);
+}
+
static struct patch *in_fn_table(const char *name)
{
struct string_list_item *item;
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (4 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 5/8] apply: refactor read_file_or_gitlink() Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 7:26 ` Matthieu Moy
2012-05-10 20:31 ` Jeff King
2012-05-10 6:02 ` [PATCH 7/8] apply: plug the three-way merge logic in Junio C Hamano
` (2 subsequent siblings)
8 siblings, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
Grab the preimage blob the patch claims to be based on out of the object
store, apply the patch, and then call three-way-merge function.
This step does not plug the actual three-way merge logic yet, but we are
getting there.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 44 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index b1dd23c..798a634 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3073,11 +3073,53 @@ static int load_preimage(struct image *image,
return 0;
}
-static int try_threeway_fallback(struct image *image, struct patch *patch)
+static int three_way_merge(struct image *image,
+ char *path,
+ unsigned char *base,
+ unsigned char *ours,
+ unsigned char *theirs)
{
return -1; /* for now */
}
+static int try_threeway_fallback(struct image *image, struct patch *patch,
+ struct stat *st, struct cache_entry *ce)
+{
+ unsigned char pre_sha1[20], post_sha1[20], our_sha1[20];
+ struct strbuf buf = STRBUF_INIT;
+ size_t len;
+ char *img;
+ struct image tmp_image;
+
+ /* No point falling back to 3-way merge in these cases */
+ if (patch->is_binary || patch->is_new || patch->is_delete ||
+ S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
+ return -1;
+
+ /* Preimage the patch was prepared for */
+ if (get_sha1(patch->old_sha1_prefix, pre_sha1) ||
+ read_blob_object(&buf, pre_sha1, patch->old_mode))
+ return error("repository lacks necessary blobs to fall back on 3-way merge.");
+ img = strbuf_detach(&buf, &len);
+ prepare_image(&tmp_image, img, len, 1);
+ /* Apply the patch to get the post image */
+ if (apply_fragments(&tmp_image, patch) < 0) {
+ clear_image(&tmp_image);
+ return -1;
+ }
+ hash_sha1_file(tmp_image.buf, tmp_image.len, blob_type, post_sha1);
+ clear_image(&tmp_image);
+
+ /* pre_sha1[] is common, post_sha1[] is theirs */
+ load_preimage(&tmp_image, patch, st, ce);
+ hash_sha1_file(tmp_image.buf, tmp_image.len, blob_type, our_sha1);
+ clear_image(&tmp_image);
+
+ /* in-core three-way merge between post and our using pre as base */
+ return three_way_merge(image,
+ patch->new_name, pre_sha1, our_sha1, post_sha1);
+}
+
static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
{
struct image image;
@@ -3087,7 +3129,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
if (apply_fragments(&image, patch) < 0) {
/* Note: with --reject, the above call succeeds. */
- if (!threeway || try_threeway_fallback(&image, patch) < 0)
+ if (!threeway || try_threeway_fallback(&image, patch, st, ce) < 0)
return -1;
}
patch->result = image.buf;
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 6:02 ` [PATCH 6/8] apply: fall back on three-way merge Junio C Hamano
@ 2012-05-10 7:26 ` Matthieu Moy
2012-05-10 15:10 ` Junio C Hamano
2012-05-10 20:31 ` Jeff King
1 sibling, 1 reply; 19+ messages in thread
From: Matthieu Moy @ 2012-05-10 7:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> + /* Preimage the patch was prepared for */
> + if (get_sha1(patch->old_sha1_prefix, pre_sha1) ||
> + read_blob_object(&buf, pre_sha1, patch->old_mode))
> + return error("repository lacks necessary blobs to fall back on 3-way merge.");
What happens if there are multiple objects for the same pre_sha1? It's
just 7 characters, so conflicts are unlikely but possible. Ideally, we
could try applying the patch to all the blobs having the same prefix
sha1, and take the first one for which the patch applies cleanly.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 7:26 ` Matthieu Moy
@ 2012-05-10 15:10 ` Junio C Hamano
2012-05-10 15:41 ` Matthieu Moy
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 15:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> + /* Preimage the patch was prepared for */
>> + if (get_sha1(patch->old_sha1_prefix, pre_sha1) ||
>> + read_blob_object(&buf, pre_sha1, patch->old_mode))
>> + return error("repository lacks necessary blobs to fall back on 3-way merge.");
>
> What happens if there are multiple objects for the same pre_sha1?
We probably would not get anythning out of get_sha1() due to ambiguity and
we would punt.
We could inroduce get_all_possible_sha1_among_ambigous_ones() to grab an
array, try to apply and use the first one, but I do not know if it is
really worth the effort. The usefulness of -3 is limited to only two cases
in practice, and neither case gives you a big chance of seeing such a
collision. Either you are applying a patch from others that was based on
something you shared with them in the past, hence it is very likely that
you and they shared a very similar set of objects that determined the
length of the abbreviated object name shown on the "index" line, in which
case the chance of ambiguity is not that great, or you are using the diff
to apply pipeline in a workflow similar to the ones I recently described
in messages in another thread, in which case the patch is taken from your
own repository and the length of the abbreviated object name shown on the
"index" line is determined not to be ambiguous in the first place.
If we happened to grab totally unrelated pre_sha1, the patch won't apply,
and we would not cause any damage.
An interesting point that is worth noting is that if we happened to grab a
pre_sha1 that is different from what the original patch was based on, it
does not introduce a mis-apply risk, either. We only fall back to the
merge after making sure the patch applies to the "original" (which may not
be exactly what the patch was based on), which ensures that the preimage
shown in the hunks of the patch match. Because the application of the
patch to the "original" found will by definition only modify the part that
is shown in the hunks of the patch, any difference between the preimage
we find and the true preimage the patch was based on will cancel out in
the three-way merge step.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 15:10 ` Junio C Hamano
@ 2012-05-10 15:41 ` Matthieu Moy
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Moy @ 2012-05-10 15:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> What happens if there are multiple objects for the same pre_sha1?
>
> [...]The usefulness of -3 is limited to only two cases
> in practice, and neither case gives you a big chance of seeing such a
> collision.
Ah, OK, I missed the fact that the index line was using
find_unique_abbrev(), not a fix number of characters. Thanks for the
clarification.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 6:02 ` [PATCH 6/8] apply: fall back on three-way merge Junio C Hamano
2012-05-10 7:26 ` Matthieu Moy
@ 2012-05-10 20:31 ` Jeff King
2012-05-10 21:06 ` Junio C Hamano
1 sibling, 1 reply; 19+ messages in thread
From: Jeff King @ 2012-05-10 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, May 09, 2012 at 11:02:23PM -0700, Junio C Hamano wrote:
> +static int try_threeway_fallback(struct image *image, struct patch *patch,
> + struct stat *st, struct cache_entry *ce)
> +{
> + unsigned char pre_sha1[20], post_sha1[20], our_sha1[20];
> + struct strbuf buf = STRBUF_INIT;
> + size_t len;
> + char *img;
> + struct image tmp_image;
> +
> + /* No point falling back to 3-way merge in these cases */
> + if (patch->is_binary || patch->is_new || patch->is_delete ||
> + S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
> + return -1;
Is it true that there is no point in doing a 3-way fallback when
patch->is_binary? What if the user has a custom merge driver?
For that matter, a custom driver could handle additions or deletions,
too (e.g., for a sorted record-oriented file, merging two additions
might just mean collating the records).
It seems like we should just keep the logic here as stupid as possible,
try to setup the 3-way content, and then hand it off to the merge code
to try to make something happen. The only thing we have to lose is a
little bit of efficiency in setting up blobs that are unlikely to
actually get merged.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 20:31 ` Jeff King
@ 2012-05-10 21:06 ` Junio C Hamano
2012-05-10 22:24 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 21:06 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> On Wed, May 09, 2012 at 11:02:23PM -0700, Junio C Hamano wrote:
> ...
>> + /* No point falling back to 3-way merge in these cases */
>> + if (patch->is_binary || patch->is_new || patch->is_delete ||
>> + S_ISGITLINK(patch->old_mode) || S_ISGITLINK(patch->new_mode))
>> + return -1;
>
> Is it true that there is no point in doing a 3-way fallback when
> patch->is_binary? What if the user has a custom merge driver?
> ...
> It seems like we should just keep the logic here as stupid as possible,
Actually, the motivation behind that "No point" is to keep the logic as
stupid as possible.
We had a history of setting up logic that covers cases far wider than we
originally designed the behaviours for, leaving the corner case behaviour
of the code "undefined", without saying exactly what is and what is not
the defined use case, which lead users to try unexpected (possibly but not
necessarily stupid) things and getting a random "you get whatever the code
happens to do" results. The above is an attempt to limit the scope of the
initial implementation to a narrow, well defined, and commonly occurring
subset of the problem space.
I can buy that lifting "must not be binary" would be very cheap, but
adding support for new/delete case would mean the index-stuffing part
needs to gain more code, so removing the "not new, not delete"
conditionals actually goes against keeping the logic as stupid as
possible.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] apply: fall back on three-way merge
2012-05-10 21:06 ` Junio C Hamano
@ 2012-05-10 22:24 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2012-05-10 22:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, May 10, 2012 at 02:06:23PM -0700, Junio C Hamano wrote:
> > Is it true that there is no point in doing a 3-way fallback when
> > patch->is_binary? What if the user has a custom merge driver?
> > ...
> > It seems like we should just keep the logic here as stupid as possible,
>
> Actually, the motivation behind that "No point" is to keep the logic as
> stupid as possible.
> [...]
> I can buy that lifting "must not be binary" would be very cheap, but
> adding support for new/delete case would mean the index-stuffing part
> needs to gain more code, so removing the "not new, not delete"
> conditionals actually goes against keeping the logic as stupid as
> possible.
Fair enough. It's by far the minority case, so we can wait until
it is somebody's itch to scratch.
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/8] apply: plug the three-way merge logic in
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (5 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 6/8] apply: fall back on three-way merge Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 6:02 ` [PATCH 8/8] apply: register conflicted stages to the index Junio C Hamano
2012-05-10 7:31 ` [PATCH 0/8] "git apply --threeway" Matthieu Moy
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
When a patch does not apply to what we have, but we know the preimage the
patch was made against, we apply the patch to the preimage to compute what
the patch author wanted the result to look like, and attempt a three-way
merge between the result and our version, using the intended preimage as
the base version.
When we are applying the patch using the index, we would additionally need
to add the object names of these three blobs involved in the merge, which
is not yet done in this step, but we add a field to "struct patch" so that
later write-out step can use it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 47 insertions(+), 6 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 798a634..e090e18 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -16,6 +16,8 @@
#include "dir.h"
#include "diff.h"
#include "parse-options.h"
+#include "xdiff-interface.h"
+#include "ll-merge.h"
/*
* --check turns on checking that the working tree matches the
@@ -194,12 +196,17 @@ struct patch {
unsigned int is_copy:1;
unsigned int is_rename:1;
unsigned int recount:1;
+ unsigned int did_threeway:1;
+ unsigned int conflicted_threeway:1;
struct fragment *fragments;
char *result;
size_t resultsize;
char old_sha1_prefix[41];
char new_sha1_prefix[41];
struct patch *next;
+
+ /* three-way fallback result */
+ unsigned char threeway_stage[3][20];
};
static void free_fragment_list(struct fragment *list)
@@ -3075,11 +3082,33 @@ static int load_preimage(struct image *image,
static int three_way_merge(struct image *image,
char *path,
- unsigned char *base,
- unsigned char *ours,
- unsigned char *theirs)
+ const unsigned char *base,
+ const unsigned char *ours,
+ const unsigned char *theirs)
{
- return -1; /* for now */
+ mmfile_t base_file, our_file, their_file;
+ mmbuffer_t result = { 0 };
+ int status;
+
+ read_mmblob(&base_file, base);
+ read_mmblob(&our_file, ours);
+ read_mmblob(&their_file, theirs);
+ status = ll_merge(&result, path,
+ &base_file, "base",
+ &our_file, "ours",
+ &their_file, "theirs", NULL);
+ free(base_file.ptr);
+ free(our_file.ptr);
+ free(their_file.ptr);
+ if (status < 0 || !result.ptr) {
+ free(result.ptr);
+ return -1;
+ }
+ clear_image(image);
+ image->buf = result.ptr;
+ image->len = result.size;
+
+ return status;
}
static int try_threeway_fallback(struct image *image, struct patch *patch,
@@ -3088,6 +3117,7 @@ static int try_threeway_fallback(struct image *image, struct patch *patch,
unsigned char pre_sha1[20], post_sha1[20], our_sha1[20];
struct strbuf buf = STRBUF_INIT;
size_t len;
+ int status;
char *img;
struct image tmp_image;
@@ -3116,8 +3146,19 @@ static int try_threeway_fallback(struct image *image, struct patch *patch,
clear_image(&tmp_image);
/* in-core three-way merge between post and our using pre as base */
- return three_way_merge(image,
- patch->new_name, pre_sha1, our_sha1, post_sha1);
+ status = three_way_merge(image, patch->new_name,
+ pre_sha1, our_sha1, post_sha1);
+ if (status < 0)
+ return status;
+
+ patch->did_threeway = 1;
+ if (status) {
+ patch->conflicted_threeway = 1;
+ hashcpy(patch->threeway_stage[0], pre_sha1);
+ hashcpy(patch->threeway_stage[1], our_sha1);
+ hashcpy(patch->threeway_stage[2], post_sha1);
+ }
+ return 0;
}
static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 8/8] apply: register conflicted stages to the index
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (6 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 7/8] apply: plug the three-way merge logic in Junio C Hamano
@ 2012-05-10 6:02 ` Junio C Hamano
2012-05-10 7:31 ` [PATCH 0/8] "git apply --threeway" Matthieu Moy
8 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-05-10 6:02 UTC (permalink / raw)
To: git
Now we have all the necessary logic to fall back on three-way merge when
the patch does not cleanly apply, insert the conflicted entries to the
index as appropriate. This obviously triggers only when the "--index"
option is used.
When we fall back to three-way merge and some of the merges fail, just
like the case where the "--reject" option was specified and we had to
write some "*.rej" files out for unapplicable patches, exit the command
with non-zero status without showing the diffstat and summary. Otherwise
they would make the list of problematic paths scroll off the display.
This wraps up the series, at least for now. We might want to also arrange
to call into the rerere machinery to clean things up, but that is left for
later updates.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 65 +++++++++++++++++++++++++++++++++++----
t/t4108-apply-threeway.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 137 insertions(+), 6 deletions(-)
create mode 100755 t/t4108-apply-threeway.sh
diff --git a/builtin/apply.c b/builtin/apply.c
index e090e18..d8c201b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3130,6 +3130,9 @@ static int try_threeway_fallback(struct image *image, struct patch *patch,
if (get_sha1(patch->old_sha1_prefix, pre_sha1) ||
read_blob_object(&buf, pre_sha1, patch->old_mode))
return error("repository lacks necessary blobs to fall back on 3-way merge.");
+
+ fprintf(stderr, "Falling back to three-way merge...\n");
+
img = strbuf_detach(&buf, &len);
prepare_image(&tmp_image, img, len, 1);
/* Apply the patch to get the post image */
@@ -3148,8 +3151,10 @@ static int try_threeway_fallback(struct image *image, struct patch *patch,
/* in-core three-way merge between post and our using pre as base */
status = three_way_merge(image, patch->new_name,
pre_sha1, our_sha1, post_sha1);
- if (status < 0)
+ if (status < 0) {
+ fprintf(stderr, "Failed to fall back on three-way merge...\n");
return status;
+ }
patch->did_threeway = 1;
if (status) {
@@ -3157,6 +3162,9 @@ static int try_threeway_fallback(struct image *image, struct patch *patch,
hashcpy(patch->threeway_stage[0], pre_sha1);
hashcpy(patch->threeway_stage[1], our_sha1);
hashcpy(patch->threeway_stage[2], post_sha1);
+ fprintf(stderr, "Applied patch to '%s' with conflicts.\n", patch->new_name);
+ } else {
+ fprintf(stderr, "Applied patch to '%s' cleanly.\n", patch->new_name);
}
return 0;
}
@@ -3702,6 +3710,27 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned
die_errno(_("unable to write file '%s' mode %o"), path, mode);
}
+static void add_conflicted_stages_file(struct patch *patch)
+{
+ int stage, namelen;
+ unsigned ce_size, mode;
+
+ if (!update_index)
+ return;
+ namelen = strlen(patch->new_name);
+ ce_size = cache_entry_size(namelen);
+ mode = patch->new_mode ? patch->new_mode : (S_IFREG | 0644);
+ for (stage = 1; stage < 4; stage++) {
+ struct cache_entry *ce = xcalloc(1, ce_size);
+ memcpy(ce->name, patch->new_name, namelen);
+ ce->ce_mode = create_ce_mode(mode);
+ ce->ce_flags = create_ce_flags(namelen, stage);
+ hashcpy(ce->sha1, patch->threeway_stage[stage - 1]);
+ if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
+ die(_("unable to add cache entry for %s"), patch->new_name);
+ }
+}
+
static void create_file(struct patch *patch)
{
char *path = patch->new_name;
@@ -3712,7 +3741,11 @@ static void create_file(struct patch *patch)
if (!mode)
mode = S_IFREG | 0644;
create_one_file(path, mode, buf, size);
- add_index_file(path, mode, buf, size);
+
+ if (patch->conflicted_threeway)
+ add_conflicted_stages_file(patch);
+ else
+ add_index_file(path, mode, buf, size);
}
/* phase zero is to remove, phase one is to create */
@@ -3814,6 +3847,7 @@ static int write_out_results(struct patch *list)
int phase;
int errs = 0;
struct patch *l;
+ struct string_list cpath = STRING_LIST_INIT_DUP;
for (phase = 0; phase < 2; phase++) {
l = list;
@@ -3822,12 +3856,28 @@ static int write_out_results(struct patch *list)
errs = 1;
else {
write_out_one_result(l, phase);
- if (phase == 1 && write_out_one_reject(l))
- errs = 1;
+ if (phase == 1) {
+ if (write_out_one_reject(l))
+ errs = 1;
+ if (l->conflicted_threeway) {
+ string_list_append(&cpath, l->new_name);
+ errs = 1;
+ }
+ }
}
l = l->next;
}
}
+
+ if (cpath.nr) {
+ struct string_list_item *item;
+
+ sort_string_list(&cpath);
+ for_each_string_list_item(item, &cpath)
+ fprintf(stderr, "U %s\n", item->string);
+ string_list_clear(&cpath, 0);
+ }
+
return errs;
}
@@ -3950,8 +4000,11 @@ static int apply_patch(int fd, const char *filename, int options)
!apply_with_reject)
exit(1);
- if (apply && write_out_results(list))
- exit(1);
+ if (apply && write_out_results(list)) {
+ if (apply_with_reject)
+ exit(1);
+ return 1;
+ }
if (fake_ancestor)
build_fake_ancestor(list, fake_ancestor);
diff --git a/t/t4108-apply-threeway.sh b/t/t4108-apply-threeway.sh
new file mode 100755
index 0000000..521c114
--- /dev/null
+++ b/t/t4108-apply-threeway.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git apply --threeway'
+
+. ./test-lib.sh
+
+create_file () {
+ for i
+ do
+ echo "$i"
+ done
+}
+
+sanitize_conflicted_diff () {
+ sed -e '
+ /^index /d
+ s/^\(+[<>][<>][<>][<>]*\) .*/\1/
+ '
+}
+
+test_expect_success setup '
+ test_tick &&
+ create_file >one 1 2 3 4 5 6 7 &&
+ cat one >two &&
+ git add one two &&
+ git commit -m initial &&
+
+ git branch side &&
+
+ test_tick &&
+ create_file >one 1 two 3 4 5 six 7 &&
+ create_file >two 1 two 3 4 5 6 7 &&
+ git commit -a -m master &&
+
+ git checkout side &&
+ create_file >one 1 2 3 4 five 6 7 &&
+ create_file >two 1 2 3 4 five 6 7 &&
+ git commit -a -m side &&
+
+ git checkout master
+'
+
+test_expect_success 'apply without --threeway' '
+ git diff side^ side >P.diff &&
+
+ # should fail to apply
+ git reset --hard &&
+ git checkout master^0 &&
+ test_must_fail git apply --index P.diff &&
+ # should leave things intact
+ git diff-files --exit-code &&
+ git diff-index --exit-code --cached HEAD
+'
+
+test_expect_success 'apply with --threeway' '
+ # Merging side should be similar to applying this patch
+ git diff ...side >P.diff &&
+
+ # The corresponding conflicted merge
+ git reset --hard &&
+ git checkout master^0 &&
+ test_must_fail git merge --no-commit side &&
+ git ls-files -s >expect.ls &&
+ git diff HEAD | sanitize_conflicted_diff >expect.diff &&
+
+ # should fail to apply
+ git reset --hard &&
+ git checkout master^0 &&
+ test_must_fail git apply --index --threeway P.diff &&
+ git ls-files -s >actual.ls &&
+ git diff HEAD | sanitize_conflicted_diff >actual.diff &&
+
+ # The result should resemble the corresponding merge
+ test_cmp expect.ls actual.ls &&
+ test_cmp expect.diff actual.diff
+'
+
+test_done
--
1.7.10.1.562.gfc79b1c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/8] "git apply --threeway"
2012-05-10 6:02 [PATCH 0/8] "git apply --threeway" Junio C Hamano
` (7 preceding siblings ...)
2012-05-10 6:02 ` [PATCH 8/8] apply: register conflicted stages to the index Junio C Hamano
@ 2012-05-10 7:31 ` Matthieu Moy
2012-05-10 15:26 ` Junio C Hamano
8 siblings, 1 reply; 19+ messages in thread
From: Matthieu Moy @ 2012-05-10 7:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> - After trying and failing to apply the patch text, we inspect our object
> database and see if there is a blob object that matches what is
> recorded on the "index" line of the patch as its preimage.
Just being curious: why is it called "index"?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 19+ messages in thread