On Fri, Dec 6, 2013 at 1:11 PM, Junio C Hamano gits...@pobox.com wrote:
Samuel Bronson naes...@gmail.com 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[]
-Oorderfile::
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
