Re: [PATCH v2 1/8] t4051: rewrite, add more tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 30.05.2016 um 01:55 schrieb Junio C Hamano:
René Scharfe <l.s.r@xxxxxx> writes:

+commit_and_tag () {
+	message=$1 &&
+	shift &&
+	git add $@ &&

Lack of dq around $@ makes me wonder if there is something funny
going on (looking at the callers, there isn't, so we'd better quote
it to avoid wasting time, I think).

OK.

+	test_tick &&
+	git commit -m $message &&
+	git tag $message
  }

The use of $message as the sole argument to "git tag" makes the
readers guess that it must be a single token without any funny
character, so the readers would probably do not waste too much time
wondreing if the lack of dq around $message in the last two is
problematic.

Well, let's call it $tag; $message is a bit misleading here. The saved letters can be invested in quotes. ;)

+last_context_line () {
+	sed -n '$ p'
  }

I have a vague recollection that some implementations of sed are
unhappy to see that space between the address and the operation; I'd
feel safer without it.

Indeed most sed calls in t/ have no space there (found counter-examples only in annotate-tests.sh, t4201-shortlog.sh, t9824-git-p4-git-lfs.sh).

+check_diff () {
+	name=$1
+	desc=$2
+	options="-W $3"
+
+	test_expect_success "$desc" '
+		git diff $options "$name^" "$name" >"$name.diff"
+	'
+
+	test_expect_success ' diff applies' '
+		test_when_finished "git reset --hard" &&
+		git checkout --detach "$name^" &&

With the presence of ^ there, --detach is unnecessary; it would not
hurt, though.

Right.  It's just there to make that intent clear.

+		git apply "$name.diff" &&
+		git diff --exit-code "$name"

Even though we may know that $name.diff" will never have a creation
of new paths, I'd feel safer if "apply" is run with "--index".

Makes sense; the less we assume about the diff to be checked the better.

Thanks a lot!

René
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]