On Wed, Apr 11, 2012 at 08:15:15PM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@xxxxxxxxxxxxx> writes:
>
> >> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> >> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> >> > {
> >> > - /* 7 is max possible length of our args array including NULL */
> >> > - const char *args[7];
> >> > - int i = 0;
> >> > + struct argv_array array;
> >> > + int rc;
> >> > +
> >> > + if (!empty && !opts->keep_if_made_empty) {
> >> > + unsigned char head_sha1[20];
> >> > + struct commit *head_commit;
> >> > + int need_free = 0;
> >> > +
> >> > + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
> >> > + head_commit = lookup_commit(head_sha1);
> >>
> >> No error checking whatsoever? HEAD might be pointing at a branch that
> >> hasn't been born, for example.
> >>
> > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that
> > we validate it in rollback_single_pick via read_ref_full. But I can add
> > additional checking here.
>
> As long as it is clear that we always have "HEAD" here, it is not strictly
> necessary, but we'd probably feel better, especially now that the function
> is (for some reason) made into a global one and may gain other new callers
> outside this file scope.
>
Gah! Thanks for that, its not meant to be static. I made that global so that
gdb could see it and set a breakpoint on it, while I was figuring out the
segfault I was getting when active_cache_tree was NULL. I'll fix that.
> >> > if (opts->allow_empty)
> >> > - args[i++] = "--allow-empty";
> >> > + argv_array_push(&array, "--allow-empty");
> >>
> >> If --keep-if-made-empty is not given but --allow-empty was, it is fine to
> >> give --allow-empty here, but otherwise, it logically is iffy and is likely
> >> to become a cause of future bugs to pass --allow-empty to "git commit",
> >> even though the earlier check _ought_ to catch problematic cases, no?
> >>
> > Not sure I follow your reasoning here. We need to pass allow-empty to git
> > commit here if git cherry-pick set either allow-empty or keep-redundant-commits
> > (the latter setting opts->empty), Can you give an example of what might be
> > problematic in the future?
>
> Thinking about it again, I think this part of your patch is correct.
>
> We may pass an index that yields the same tree as "HEAD" if the original
> was empty, or the original was not empty but the merge found the commit
> unneeded. In either case, if "--keep-unnecessary" or "--allow-empty" was
> given, we want to record the empty commit, and opts->allow_empty is set
> when either option was given from the command line.
>
Yes, exactly.
Thanks.
> Thanks.
>
--
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