Re: Re: [PATCH v2 4/4] implement automatic fast forward merge for submodules

Hi,

On Wed, Jul 07, 2010 at 01:52:16AM +0200, Johan Herland wrote:

On Tuesday 06 July 2010, Heiko Voigt wrote:
[...]

quoted

+	/* get all revisions that merge commit a */
+	snprintf(merged_revision, sizeof(merged_revision), "^%s",
+			find_unique_abbrev(a->object.sha1, 40));

Why do you call find_unique_abbrev(..., 40) here?
Isn't sha1_to_hex(a->object.sha1) a better solution?

Because I did not know it better at the time I wrote this :) Will be
corrected.

quoted

+	init_revisions(&revs, NULL);
+	rev_opts.submodule = path;
+	setup_revisions(sizeof(rev_args)/sizeof(char *)-1, rev_args, &revs, &rev_opts);
+
+	/* save all revisions from the above list that contain b */
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+	while ((commit = get_revision(&revs)) != NULL) {
+		struct object *o = &(commit->object);
+		if (in_merge_bases(b, (struct commit **) &o, 1)) {

Why not s/(struct commit **) &o/&commit/ ?

Similar, because I needed objects for the array. Will be corrected.

quoted

+	 (cd sub &&
+	  git rev-parse sub-d > ../expect) &&
+	 test_cmp actual expect)
+'
+
+test_expect_success 'merging should conflict for non fast-forward' '
+	(cd merge-search &&
+	 git checkout -b test-nonforward b &&
+	 (cd sub &&
+	  git rev-parse sub-d > ../expect) &&
+	 test_must_fail 	git merge c 2> actual  &&
+	 grep $(<expect) actual > /dev/null &&

I cannot find the "$(<expect)" construct anywhere else in out test scripts.
Is it portable? Should we maybe use "$(cat expect)" instead?

I do not know. Just to be sure I will change it to cat.

Otherwise, it looks good to me. Thanks for the effort!

Thank you as well. The rest of the comments are already incorporated.

cheers Heiko