On Mon, Jun 30, 2008 at 11:23:17PM -0700, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Miklos Vajna <vmiklos@xxxxxxxxxxxxxx> writes:
>
> > +static int read_tree_trivial(unsigned char *common, unsigned char *head,
> > + unsigned char *one)
> > +{
> > + int i, nr_trees = 0;
> > + struct tree *trees[MAX_UNPACK_TREES];
> > + struct tree_desc t[MAX_UNPACK_TREES];
> > + struct unpack_trees_options opts;
> > +
> > + memset(&opts, 0, sizeof(opts));
> > + opts.head_idx = 2;
>
> Do you still need this assignment here?
No, it was duplicated.
> > +int cmd_merge(int argc, const char **argv, const char *prefix)
> > +{
> > + unsigned char result_tree[20];
> > + struct strbuf buf;
> > + const char *head_arg;
> > + int flag, head_invalid = 0, i;
> > + int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
> > + struct commit_list *common = NULL;
> > + struct path_list_item *best_strategy = NULL, *wt_strategy = NULL;
> > + struct commit_list **remotes = &remoteheads;
> > +
> > + setup_work_tree();
> > + if (unmerged_cache())
> > + die("You are in the middle of a conflicted merge.");
> > +
> > + /*
> > + * Check if we are _not_ on a detached HEAD, i.e. if there is a
> > + * current branch.
> > + */
> > + branch = resolve_ref("HEAD", head, 0, &flag);
> > + if (branch && flag & REF_ISSYMREF) {
> > + const char *ptr = skip_prefix(branch, "refs/heads/");
> > + if (ptr)
> > + branch = ptr;
> > + } else
> > + head_invalid = 1;
>
> Wait a minute... Are you calling a detached HEAD as head_invalid here? I
> am not too much worried about variable naming, but you later do ...
>
> > + if (!have_message && is_old_style_invocation(argc, argv)) {
> > ...
> > + } else if (head_invalid) {
> > + struct object *remote_head;
> > + /*
> > + * If the merged head is a valid one there is no reason
> > + * to forbid "git merge" into a branch yet to be born.
> > + * We do the same for "git pull".
> > + */
> > + if (argc != 1)
> > + die("Can merge only exactly one commit into "
> > + "empty head");
>
> Which is about HEAD pointing at a branch that isn't born yet. They are
> two very different concepts.
>
> Either the above "else if (head_invalid)" is wrong, or more likely the
> setting of head_invalid we saw earlier is wrong.
>
> Probably what you meant was:
>
> - "char *branch" points at either "HEAD" (when detached) or
> the name of the branch (e.g. "master" when "refs/heads/master");
>
> - "unsigned char head[]" stores the commit object name of the
> current HEAD (or 0{40} if the current branch is unborn);
>
> - set head_invalid to true only when the current branch is unborn.
>
> So perhaps...
>
> branch = resolve_ref("HEAD", head, 0, &flag);
> if (branch && (flag & REF_ISSYMREF) && !prefixcmp(branch, "refs/heads/"))
> branch += 11;
> head_invalid = is_null_sha1(head);
>
> And probably we can even drop (flag & REF_ISSYMREF) from the above safely.
> Do we even care if the head is detached in this program? I doubt it.
You are right. I though we care about it, but now I realized such a
comment was only in git-merge.annotated (in Dscho's fork), not in
git-merge.sh. And yes, I mixed up head_invalid for two purposes. Thanks
for clearing the situation.
> > + strbuf_init(&msg, 0);
> > + strbuf_addstr(&msg, "Fast forward");
> > + if (have_message)
> > + strbuf_addstr(&msg,
> > + " (no commit created; -m option ignored)");
> > + o = peel_to_type(sha1_to_hex(remoteheads->item->object.sha1),
> > + 0, NULL, OBJ_COMMIT);
> > + if (!o)
> > + return 1;
> > +
> > + if (checkout_fast_forward(head, remoteheads->item->object.sha1))
> > + return 1;
>
> Not exiting with 0 status from around here upon error is an improvement,
> but does the user see sensible error messages in addition to the exit
> status?
Now he/she does. :-)
(I updated my working branch on repo.or.cz, will send a patch soon, as
well.)
Attachment:
pgpSU2WhupmbI.pgp
Description: PGP signature
