Hi,
On Wed, Jul 07, 2010 at 01:52:16AM +0200, Johan Herland wrote:
> On Tuesday 06 July 2010, Heiko Voigt wrote:
> [...]
>
> > + /* 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.
> > + 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.
> > + (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
--
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