Re: [PATCH 13/13] Build in merge

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@frugalware.org> writes:

>> > +		if (!remote_head)
>> > +			die("%s - not something we can merge", argv[0]);
>> > +		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
>> > +				DIE_ON_ERR);
>> > +		reset_hard(remote_head->sha1, 0);
>> > +		return 0;
>> 
>> Makes one wonder reset_hard() (aka "read-tree --reset -u HEAD") ever fail
>> and return here (iow, without calling die()).  The answer is luckily no
>> in this case, but it is somewhat unnerving to reviewers.
>
> Actually reset_hard does not return if an error occures:

I know that; didn't I already say "Luckily no"?  The point was it was not
apparent from the above 6 lines alone.

>> > +	for (i = 0; i < use_strategies.nr; i++) {
>> > +		if ((unsigned int)use_strategies.items[i].util &
>> > +			NO_FAST_FORWARD)
>> > +			allow_fast_forward = 0;
>> > +		if ((unsigned int)use_strategies.items[i].util & NO_TRIVIAL)
>> > +			allow_trivial = 0;
>> 
>> Can we abstract out these ugly casts?  Any code that use path_list to
>> store anything but list of paths (i.e. some value keyed with string) tends
>> to have this readability issue.
>
> If you don't cast, you can't use the & operator. If I change the
> path_list_item's util to be an unsigned number then I break fast-export.
> I think if we _really_ want to get rid of those casts, we could have
> something like:

No, no, no.  That is not what I meant.

The places that use use_strategies in your code knows too much about the
internal implementation detail of path_list, while path_list pretends to
be a general purpose "table keyed with string" facility.  The fact is that
the table is not a very useful general purpose abstraction unless you are
pointing at some structures that exist regardless of your use of path_list
(e.g. you have some "struct object" and you hold pointers in a path_list).
It does not work very well as an abstraction for use case like yours.

With something like:

        static inline unsigned nth_strategy_flags(struct path_list *s, int nth)
        {
                return (unsigned) s->items[nth].util;
        }

the checks would be more like:

	if (nth_strategy_flags(&use_strategies, i) & NO_FAST_FORWARD)
		...

or even:

        static inline check_nth_strategy_flags(struct path_list_item *i, unsigned flags)
        {
        	return !((unsigned) i->util & flags);
        }

        if (check_nth_strategy_flags(&use_strategies,items[i], NO_FAST_FORWARD)
        	...	

either of which would be much easier on the eye.


> Actually the shell version did not check here, either, but yes, I would
> have to. Now I do.

The scripted did not have to do so explicitly, as the one before that have
already caught "more than one common" case in the case arm before this
part.  It is already known that "have only one common" condition is true
when you reached the corresponding part of the scripted version.