Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
- Original Message - > From: "Junio C Hamano" > Sent: Wednesday, September 12, 2012 1:01:30 PM > Subject: Re: [PATCH 2/2] attr: "binary" attribute should choose built-in > "binary" merge driver > > >> Perhaps something like this makes it better. > > > > Patch didn't apply on top of the previous two for me,... > > Look at 'pu' and see how it applies. Ah, that seems to work better. > > ... The only remaining > > question for me is should -Xtheirs resolve "deleted by them" > > conflicts? > > I do not know, and I do not care to worry about it too deeply, which > means I would rather err on the safe side and have users inspect the > situation, make the decision when such a conflict happens and > resolve them themselves, instead of claiming a clean resolution that > is possibly wrong. A reasonable approach. Thanks for clarifying. Stephen -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
Stephen Bash writes: >> Perhaps something like this makes it better. > > Patch didn't apply on top of the previous two for me,... Look at 'pu' and see how it applies. > ... The only remaining > question for me is should -Xtheirs resolve "deleted by them" > conflicts? I do not know, and I do not care to worry about it too deeply, which means I would rather err on the safe side and have users inspect the situation, make the decision when such a conflict happens and resolve them themselves, instead of claiming a clean resolution that is possibly wrong. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
On Wed, Sep 12, 2012 at 01:55:53AM -0700, Junio C Hamano wrote:
> > Yeah, that seems like the obviously correct thing to do. In practice,
> > most files would end up in the first few lines of ll_xdl_merge checking
> > buffer_is_binary anyway, so I think this would really only make a
> > difference when our "is it binary?" heuristic guesses wrong.
>
> You made me look at that part again and then made me notice
> something unrelated.
>
> if (buffer_is_binary(orig->ptr, orig->size) ||
> buffer_is_binary(src1->ptr, src1->size) ||
> buffer_is_binary(src2->ptr, src2->size)) {
> warning("Cannot merge binary files: %s (%s vs. %s)",
> path, name1, name2);
> return ll_binary_merge(drv_unused, result,
> path,
> orig, orig_name,
> src1, name1,
> src2, name2,
> opts, marker_size);
> }
>
> Given that we now may know how to merge these things, the
> unconditional warning feels very wrong.
>
> Perhaps something like this makes it better.
>
> A path that is explicitly marked as binary did not get any such
> warning, but it will start to get warned just like a path that was
> auto-detected to be a binary.
>
> It is a behaviour change, but I think it is a good one that makes
> two cases more consistent.
>
> And we won't see the warning when -Xtheirs/-Xours large sledgehammer
> is in use, which tells us how to resolve these things "cleanly".
Yeah, I think it is the right thing to do. I noticed that the warning
would not trigger in the "-merge" case and wondered if it should, but
figured it was not a big deal either way.
However, I agree it is very bad for it to trigger with -Xours/theirs,
and that is worth fixing. That it triggers in the "-merge" case
afterwards is a slight bonus.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
- Original Message -
> From: "Junio C Hamano"
> Sent: Wednesday, September 12, 2012 4:55:53 AM
> Subject: Re: [PATCH 2/2] attr: "binary" attribute should choose built-in
> "binary" merge driver
>
> Jeff King writes:
>
> > On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:
> >
> >> The built-in "binary" attribute macro expands to "-diff -text", so
> >> that textual diff is not produced, and the contents will not go
> >> through any CR/LF conversion ever. During a merge, it should also
> >> choose the "binary" low-level merge driver, but it didn't.
> >>
> >> Make it expand to "-diff -merge -text".
> >
> > Yeah, that seems like the obviously correct thing to do. In
> > practice,
> > most files would end up in the first few lines of ll_xdl_merge
> > checking
> > buffer_is_binary anyway, so I think this would really only make a
> > difference when our "is it binary?" heuristic guesses wrong.
>
> You made me look at that part again and then made me notice
> something unrelated.
>
> if (buffer_is_binary(orig->ptr, orig->size) ||
> buffer_is_binary(src1->ptr, src1->size) ||
> buffer_is_binary(src2->ptr, src2->size)) {
> warning("Cannot merge binary files: %s (%s vs. %s)",
> path, name1, name2);
> return ll_binary_merge(drv_unused, result,
> path,
> orig, orig_name,
> src1, name1,
> src2, name2,
> opts, marker_size);
> }
>
> Given that we now may know how to merge these things, the
> unconditional warning feels very wrong.
>
> Perhaps something like this makes it better.
Patch didn't apply on top of the previous two for me, but after making the
edits manually does what it claims to do (and makes the merge output much nicer
to read, thanks!). The only remaining question for me is should -Xtheirs
resolve "deleted by them" conflicts?
Thanks,
Stephen
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
Jeff King writes:
> On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:
>
>> The built-in "binary" attribute macro expands to "-diff -text", so
>> that textual diff is not produced, and the contents will not go
>> through any CR/LF conversion ever. During a merge, it should also
>> choose the "binary" low-level merge driver, but it didn't.
>>
>> Make it expand to "-diff -merge -text".
>
> Yeah, that seems like the obviously correct thing to do. In practice,
> most files would end up in the first few lines of ll_xdl_merge checking
> buffer_is_binary anyway, so I think this would really only make a
> difference when our "is it binary?" heuristic guesses wrong.
You made me look at that part again and then made me notice
something unrelated.
if (buffer_is_binary(orig->ptr, orig->size) ||
buffer_is_binary(src1->ptr, src1->size) ||
buffer_is_binary(src2->ptr, src2->size)) {
warning("Cannot merge binary files: %s (%s vs. %s)",
path, name1, name2);
return ll_binary_merge(drv_unused, result,
path,
orig, orig_name,
src1, name1,
src2, name2,
opts, marker_size);
}
Given that we now may know how to merge these things, the
unconditional warning feels very wrong.
Perhaps something like this makes it better.
A path that is explicitly marked as binary did not get any such
warning, but it will start to get warned just like a path that was
auto-detected to be a binary.
It is a behaviour change, but I think it is a good one that makes
two cases more consistent.
And we won't see the warning when -Xtheirs/-Xours large sledgehammer
is in use, which tells us how to resolve these things "cleanly".
ll-merge.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git i/ll-merge.c w/ll-merge.c
index 8535e2d..307315b 100644
--- i/ll-merge.c
+++ w/ll-merge.c
@@ -35,7 +35,7 @@ struct ll_merge_driver {
*/
static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
mmbuffer_t *result,
- const char *path_unused,
+ const char *path,
mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
@@ -53,6 +53,9 @@ static int ll_binary_merge(const struct ll_merge_driver
*drv_unused,
} else {
switch (opts->variant) {
default:
+ warning("Cannot merge binary files: %s (%s vs. %s)\n",
+ path, name1, name2);
+ /* fallthru */
case XDL_MERGE_FAVOR_OURS:
stolen = src1;
break;
@@ -88,8 +91,6 @@ static int ll_xdl_merge(const struct ll_merge_driver
*drv_unused,
if (buffer_is_binary(orig->ptr, orig->size) ||
buffer_is_binary(src1->ptr, src1->size) ||
buffer_is_binary(src2->ptr, src2->size)) {
- warning("Cannot merge binary files: %s (%s vs. %s)\n",
- path, name1, name2);
return ll_binary_merge(drv_unused, result,
path,
orig, orig_name,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote: > The built-in "binary" attribute macro expands to "-diff -text", so > that textual diff is not produced, and the contents will not go > through any CR/LF conversion ever. During a merge, it should also > choose the "binary" low-level merge driver, but it didn't. > > Make it expand to "-diff -merge -text". Yeah, that seems like the obviously correct thing to do. In practice, most files would end up in the first few lines of ll_xdl_merge checking buffer_is_binary anyway, so I think this would really only make a difference when our "is it binary?" heuristic guesses wrong. > Documentation/gitattributes.txt | 2 +- > attr.c | 2 +- > t/t6037-merge-ours-theirs.sh| 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) Patch itself looks good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
The built-in "binary" attribute macro expands to "-diff -text", so
that textual diff is not produced, and the contents will not go
through any CR/LF conversion ever. During a merge, it should also
choose the "binary" low-level merge driver, but it didn't.
Make it expand to "-diff -merge -text".
Signed-off-by: Junio C Hamano
---
Documentation/gitattributes.txt | 2 +-
attr.c | 2 +-
t/t6037-merge-ours-theirs.sh| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index a85b187..ead7254 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -904,7 +904,7 @@ file at the toplevel (i.e. not in any subdirectory). The
built-in
macro attribute "binary" is equivalent to:
-[attr]binary -diff -text
+[attr]binary -diff -merge -text
diff --git a/attr.c b/attr.c
index 303751f..3f581b3 100644
--- a/attr.c
+++ b/attr.c
@@ -306,7 +306,7 @@ static void free_attr_elem(struct attr_stack *e)
}
static const char *builtin_attr[] = {
- "[attr]binary -diff -text",
+ "[attr]binary -diff -merge -text",
NULL,
};
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 8d05671..3889eca 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -54,7 +54,7 @@ test_expect_success 'recursive favouring ours' '
'
test_expect_success 'binary file with -Xours/-Xtheirs' '
- echo "file -merge" >.gitattributes &&
+ echo file binary >.gitattributes &&
git reset --hard master &&
git merge -s recursive -X theirs side &&
--
1.7.12.322.g2c7d289
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
