On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Samuel Bronson <naesten@xxxxxxxxx> writes:
> Thanks for reviving a stalled topic.
I was asking about such a feature in #git and jrnieder was nice enough
to point me at the stalled patch.
>> *I* even verified that the tests do fail properly when the feature is
>> sabotaged.
>
> Sabotaged in what way?
I commented out the "options->orderfile = diff_order_file_cfg;" line.
>> @@ -432,6 +432,8 @@ endif::git-format-patch[]
>> -O<orderfile>::
>> Output the patch in the order specified in the
>> <orderfile>, which has one shell glob pattern per line.
>> + This overrides the `diff.orderfile' configuration variable
>> + ((see linkgit:git-config[1]).
>
> Double opening parenthesis?
Oops, and it looks like I messed up the quoting on diff.orderfile too ...
> If somebody has diff.orderfile configuration that points at a custom
> ordering, and wants to send out a patch (or show a diff) with the
> standard order, how would the "overriding" command line look like?
> Would it be "git diff -O/dev/null"?
It looks like that works ... and so do files that don't exist. What
do you think should happen with -O file-that-does-not-exist, and how
do you suppose it should be tested?
After having fixed this, will /dev/null still work everywhere, or will
we want a new diff flag to unset the option? (I see that "git diff
/dev/null some-file" works fine with msysgit, which doesn't seem to
actually be linked with MSYS, but I don't know *why* it works, and I
don't know what other non-POSIXoid ports exist.)
For the moment, I've added this to "for" loop (after some changes
based on some of your other suggestions):
# I don't think this should just pretend the orderfile was empty?
test_expect_failure "override with bogus orderfile ($i)" '
test_might_fail git -c diff.orderfile=order_file_$i diff
-Obogus_file --name-only HEAD^..HEAD >actual_diff_filenames &&
! test_cmp expect_diff_filenames_none actual_diff_filenames
'
Does this look (modulo gmail's stupid indentation) anything like a
reasonable approach to testing that? (Of course, you can't actually
test it because it depends on other changes I haven't posted yet ...)
Also, I'm starting to wonder if I shouldn't split this into two patches:
1. diff: Add tests for -O flag
2. diff: Add diff.orderfile configuration variable
(If so, I would obviously want to rewrite the above test to avoid the
configuration option.)
>> + return $?
>> +}
>
> That return looks somewhat strange. Does it even need to be there?
I'm certainly no great expert at shell functions, so I expect it
isn't. I'm not really sure what possessed me to think it might be
needed.
> EOF
> cat >order_file_2 <<-\EOF &&
I'd kind of prefer to keep a blank line between one EOF and the next
cat, if that's okay with you.
>
> Quoting the EOF like the above will help the readers by signaling
> them that they do not have to wonder if there is some substitution
> going on in the here text.
Perhaps, but probably only after they've scrutinized their shell
manuals to figure out what the - and the \ are for. (I had to check
two: dash(1) wasn't clear enough for me about the quoting ...)
>> +test_expect_success "no order (=tree object order)" '
>> + git diff HEAD^..HEAD >patch &&
>> + grep ^diff patch >actual_diff_headers &&
>> + test_cmp expect_diff_headers_none actual_diff_headers
>> +'
>
> Instead of grepping, "git diff --name-only" would be far easier to
> check, no?
It certainly makes for less-cluttered expected output. (I guess
jrnieder didn't know about that trick when he suggested using the
intermediate file?)
> Points to note:
>
> * We eval the scriptlets inside test framework, so using $i as a
> variable inside the single quotes will have the expected result.
> You do not have to worry about extra quoting inside dq pair.
Hmm. I'm obviously not used to things getting eval'd in the same
shell instance as my script ...
(Thanks for the review!)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html