[PATCH 0/2] Fix windows portability issues in en/d-f-conflict-fix series

[PATCH 0/2] Fix windows portability issues in en/d-f-conflict-fix series

[Elijah Newren]

This fixes two issues in next for Windows reported by Hannes, both due
to my en/d-f-conflict-fix patch series:

On Thu, Aug 12, 2010 at 3:23 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Today's next produces this warning:
>
> merge-recursive.c: In function 'process_df_entry':
> merge-recursive.c:1246: warning: unused variable 'o_sha'
>
> (line number may be different) because o_sha is only used inside assert().
> I don't know how you would like this fixed.


>> * en/d-f-conflict-fix (2010-07-27) 7 commits
>>   (merged to 'next' on 2010-08-03 at 7f78604)
>>  + t/t6035-merge-dir-to-symlink.sh: Remove TODO on passing test
>>  + fast-import: Improve robustness when D->F changes provided in wrong order
>>  + fast-export: Fix output order of D/F changes
>>  + merge_recursive: Fix renames across paths below D/F conflicts
>>  + merge-recursive: Fix D/F conflicts
>>  + Add a rename + D/F conflict testcase
>>  + Add additional testcases for D/F conflicts
>
> The new tests in t/t3509-cherry-pick-merge-df.sh and t9350-fast-export.sh
> need SYMLINKS prerequisite.

Elijah Newren (2):
  merge-recursive: Workaround unused variable warning
  Mark tests that use symlinks as needing SYMLINKS prerequisite

 merge-recursive.c               |    1 +
 t/t3509-cherry-pick-merge-df.sh |    6 ++++++
 t/t9350-fast-export.sh          |    2 +-
 3 files changed, 8 insertions(+), 1 deletions(-)

-- 
1.7.2.1.119.gca9fe.dirty

[PATCH 1/2] merge-recursive: Workaround unused variable warning

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9678c1d..7e32498 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,
 	/* We currently only handle D->F cases */
 	assert((!o_sha && a_sha && !b_sha) ||
 	       (!o_sha && !a_sha && b_sha));
+	(void)o_sha;
 
 	entry->processed = 1;
 
-- 
1.7.2.1.119.gca9fe.dirty

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Elijah Newren]

Hi,

On Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren <newren@gmail.com> wrote:
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 9678c1d..7e32498 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,
>        /* We currently only handle D->F cases */
>        assert((!o_sha && a_sha && !b_sha) ||
>               (!o_sha && !a_sha && b_sha));
> +       (void)o_sha;
>
>        entry->processed = 1;

It appears that this patch did not get included, though 2/2 from this
series did.  I'm not sure whether that was intentional or accidental.
If it was intentional, would a different method of fixing warnings
when NDEBUG is defined be preferred?  (Maybe changing the
"assert(foo)" into "if (!foo) die..." instead?)

If you'd rather just leave it as is, that's fine too.  I just wanted
to make sure it was addressed to Johannes' satisfaction, since he
brought it up as an issue for the Windows build caused by my previous
patches.

Thanks,
Elijah

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Jonathan Nieder]

Elijah Newren wrote:
> On Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren <newren@gmail.com> wrote:

>> +++ b/merge-recursive.c
>> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,
>>        /* We currently only handle D->F cases */
>>        assert((!o_sha && a_sha && !b_sha) ||
>>               (!o_sha && !a_sha && b_sha));
>> +       (void)o_sha;
[...]
>                        would a different method of fixing warnings
> when NDEBUG is defined be preferred?  (Maybe changing the
> "assert(foo)" into "if (!foo) die..." instead?)

Yes, that sounds like a good idea.  The user would probably benefit
from knowing what happened.

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Elijah Newren wrote:
>> On Thu, Aug 12, 2010 at 8:09 PM, Elijah Newren <newren@gmail.com> wrote:
>
>>> +++ b/merge-recursive.c
>>> @@ -1214,6 +1214,7 @@ static int process_df_entry(struct merge_options *o,
>>>        /* We currently only handle D->F cases */
>>>        assert((!o_sha && a_sha && !b_sha) ||
>>>               (!o_sha && !a_sha && b_sha));
>>> +       (void)o_sha;
> [...]
>>                        would a different method of fixing warnings
>> when NDEBUG is defined be preferred?  (Maybe changing the
>> "assert(foo)" into "if (!foo) die..." instead?)
>
> Yes, that sounds like a good idea.  The user would probably benefit
> from knowing what happened.

I'd agree.  This assert() is not about protecting new callers from making
obvious programming error by passing wrong parameters, but about Elijah
not being confident enough that the changes made to process_entry() with
this series really covers all the cases so that only D/F cases are left
unprocessed.

Another possibility is to move this assert() out of process_df_entry() and
have it on the calling side.  Perhaps something like the attached.

BTW, it is not so obvious if (!o_sha && !!a_sha != !!b_sha) is equivalent
to "we are handling a D-F case".  Can you explain why?


diff --git a/merge-recursive.c b/merge-recursive.c
index b0f055e..7bab728 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1208,9 +1208,8 @@ static int process_df_entry(struct merge_options *o,
 	const char *conf;
 	struct stat st;
 
-	/* We currently only handle D->F cases */
-	assert((!o_sha && a_sha && !b_sha) ||
-	       (!o_sha && !a_sha && b_sha));
+	if (! ((!o_sha && a_sha && !b_sha) || (!o_sha && !a_sha && b_sha)))
+		return 1; /* we don't handle non D-F cases */
 
 	entry->processed = 1;
 
@@ -1321,6 +1320,12 @@ int merge_trees(struct merge_options *o,
 				&& !process_df_entry(o, path, e))
 				clean = 0;
 		}
+		for (i = 0; i < entries->nr; i++) {
+			struct stage_data *e = entries->items[i].util;
+			if (!e->processed)
+				die("Unprocessed path??? %s", 
+				    entries->items[i].string);
+		}
 
 		string_list_clear(re_merge, 0);
 		string_list_clear(re_head, 0);

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Elijah Newren]

Hi,

On Wed, Aug 18, 2010 at 12:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
<snip>
>> Yes, that sounds like a good idea.  The user would probably benefit
>> from knowing what happened.
>
> I'd agree.  This assert() is not about protecting new callers from making
> obvious programming error by passing wrong parameters, but about Elijah
> not being confident enough that the changes made to process_entry() with
> this series really covers all the cases so that only D/F cases are left
> unprocessed.

Actually, it is pretty clear right now that only D/F cases are left
unprocessed, and in particular D->F cases.  This is because
process_entry() starts with unconditionally setting "entry->processed
= 1" and only unsets it in the one 'if' block where we know that
(!o_sha && !!a_sha != !!b_sha &&
string_list_has_string(&o->current_directory_set, path)).

So, I'd say it is more about programming errors, in particular ones
where people modify the code to make process_entry() leave more cases
unprocessed than what is currently possible without also making the
necessary modifications to process_df_entry().

> Another possibility is to move this assert() out of process_df_entry() and
> have it on the calling side.  Perhaps something like the attached.
>
> BTW, it is not so obvious if (!o_sha && !!a_sha != !!b_sha) is equivalent
> to "we are handling a D-F case".  Can you explain why?

It's not equivalent.  Things are slightly confusing, because !<sha>
can mean either (a) there is nothing at the given path, or (b) there
is a directory at the given path.  The only way to tell which of the
two it means is to look up the path in o->current_directory_set.

A directory/file conflict ("D-F" in my shorthand) implies !!a_sha !=
!!b_sha (but not vice versa).

A directory/file conflict where the relevant path was a file in the
merge-base ("F->D" in my shorthand) implies (o_sha && !!a_sha !=
!!b_sha).  This case is handled just fine by process_entry() (if the
file was unchanged, the correct resolution is to delete it, allowing
paths beneath the directory of the same name to be handled by later
process_entry() calls -- although this silently relies on the order of
entries from get_unmerged() to be such that things do operate in this
order.  That seems to be correct for the cases I've seen).

A directory/file conflict where the path was a directory in the
merge-base ("D->F" in my shorthand) implies (!o_sha && !!a_sha !=
!!b_sha).  This is the case the process_df_entry needs to be invoked
to handle.  That function was explicitly written explicitly for that
one case, hence the assert.  The assert might be triggered, for
example, if get_unmerged() were changed to return entries in a
different order and someone decides to make the F->D case be
unprocessed by process_entry() as well (and forgets to update
process_df_entry).

> diff --git a/merge-recursive.c b/merge-recursive.c
> index b0f055e..7bab728 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1208,9 +1208,8 @@ static int process_df_entry(struct merge_options *o,
>        const char *conf;
>        struct stat st;
>
> -       /* We currently only handle D->F cases */
> -       assert((!o_sha && a_sha && !b_sha) ||
> -              (!o_sha && !a_sha && b_sha));
> +       if (! ((!o_sha && a_sha && !b_sha) || (!o_sha && !a_sha && b_sha)))
> +               return 1; /* we don't handle non D-F cases */
>
>        entry->processed = 1;
>
> @@ -1321,6 +1320,12 @@ int merge_trees(struct merge_options *o,
>                                && !process_df_entry(o, path, e))
>                                clean = 0;
>                }
> +               for (i = 0; i < entries->nr; i++) {
> +                       struct stage_data *e = entries->items[i].util;
> +                       if (!e->processed)
> +                               die("Unprocessed path??? %s",
> +                                   entries->items[i].string);
> +               }
>
>                string_list_clear(re_merge, 0);
>                string_list_clear(re_head, 0);
>

Other than possible wording of the comment ("we only handle
directory/file conflicts where the path was not a directory in the
merge-base"? "we don't currently handle any other cases"? something
else?), the patch looks good to me.

Elijah

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> So, I'd say it is more about programming errors, in particular ones
> where people modify the code to make process_entry() leave more cases
> unprocessed than what is currently possible without also making the
> necessary modifications to process_df_entry().

Yeah.  But they do not need to touch process_df_entry().

I actually was hoping that my weatherbaloon patch will illustrate that a
new special case these people may make to process_entry() to leave other
cases unprocessed do _NOT_ have to be handled by process_df_entry().

The "if" statement in process_df_entry() would check if the entry is
something the function is ready to resolve, and otherwise punts.  A new
exception they add to process_entry() can introduce a separate phase (just
like process_df_entry() is not done in parallel with other kinds of
entries inside the process_entry() but as a separate post-processing
phase) between the loop that calls process_df_entry() and the loop that
checks if there is a remaining entry.  And it probably should, as such a
new exception may not have anything to do with "df", and adding such a
logic to process_df_entry() would be wrong ;-).

Re: [PATCH 1/2] merge-recursive: Workaround unused variable warning

[Elijah Newren]

On Wed, Aug 18, 2010 at 4:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I actually was hoping that my weatherbaloon patch will illustrate that a
> new special case these people may make to process_entry() to leave other
> cases unprocessed do _NOT_ have to be handled by process_df_entry().
>
> The "if" statement in process_df_entry() would check if the entry is
> something the function is ready to resolve, and otherwise punts.  A new
> exception they add to process_entry() can introduce a separate phase (just
> like process_df_entry() is not done in parallel with other kinds of
> entries inside the process_entry() but as a separate post-processing
> phase) between the loop that calls process_df_entry() and the loop that
> checks if there is a remaining entry.  And it probably should, as such a
> new exception may not have anything to do with "df", and adding such a
> logic to process_df_entry() would be wrong ;-).

That makes sense and sounds like a good idea to me.  I think we should
go with your patch, modulo possibly modifying the comment's wording.

[PATCH 2/2] Mark tests that use symlinks as needing SYMLINKS prerequisite

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3509-cherry-pick-merge-df.sh |    6 ++++++
 t/t9350-fast-export.sh          |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh
index 6e7ef84..93ad20d 100755
--- a/t/t3509-cherry-pick-merge-df.sh
+++ b/t/t3509-cherry-pick-merge-df.sh
@@ -3,6 +3,12 @@
 test_description='Test cherry-pick with directory/file conflicts'
 . ./test-lib.sh
 
+if ! test_have_prereq SYMLINKS
+then
+	skip_all="symbolic links not supported - skipping tests"
+	test_done
+fi
+
 test_expect_success 'Setup rename across paths each below D/F conflicts' '
 	mkdir a &&
 	>a/f &&
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 1ee1461..27aea5c 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -376,7 +376,7 @@ test_expect_success 'tree_tag-obj'    'git fast-export tree_tag-obj'
 test_expect_success 'tag-obj_tag'     'git fast-export tag-obj_tag'
 test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'
 
-test_expect_success 'directory becomes symlink'        '
+test_expect_success SYMLINKS 'directory becomes symlink'        '
 	git init dirtosymlink &&
 	git init result &&
 	(
-- 
1.7.2.1.119.gca9fe.dirty