On Sun, Apr 15, 2012 at 12:42:12PM +0200, Clemens Buchacher wrote:
> On Fri, Apr 13, 2012 at 02:45:05PM -0400, Neil Horman wrote:
> >
> > + OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
>
> For consistency, I'd prefer if the variable name were the same as the
> option. Then I wouldn't have to keep the option<->variable translation
> in mind.
>
Ok
> >
> > + if (!empty && !opts->keep_if_made_empty) {
> [...]
> > + return 0;
> [...]
> > if (opts->allow_empty)
> > + argv_array_push(&array, "--allow-empty");
> > +
> > + rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
>
> I find it a bit strange, that if we cherry-pick a commit that was
> already empty, we _do_ call git commit (and error out), but if we find a
> commit that is made empty, we do _not_ call git commit and quietly
> succeed (in not doing anything). But I suppose that is the legacy
> behavior?
>
Correct, more or less. The legacy behavior is to call git commit unilaterally.
With this change, we still do that, but we pass --allow-empty to the commit
command, allowing it to preserve the empty commit. If we specify
keep-redundant-commits, we skip the check to see if the new commit is empty and
create the new (now empty) commit. The only change is that if we do not specify
keep-redundant-commits, we check to see if the commit is made empty in
git-cherry-pick and skip it if it is. We could, instead of returning prior to
calling git-commit, use that test to override the keep_empty option below, so
that we don't pass --allow-empty to git-commit instead. That would preserve the
prior code path, but for no real advatage, as the outcome is the same, and this
way saves us having to fork the git-commit command, which I think is
adventageous.
> > +
> > + rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> > +
> > + if (rc)
>
> rc is short for run_command, for which it stores the return value, no?
> Let's not abuse the variable like this and instead use the result
> directly:
>
No. rc is short for return code, generically used to represent the value
to be returned from a function. Its used in this fashion in any number of
places, both in git and any other project.
> if (!hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1))
>
> If you make the entire paragraph leading up to this a separate function,
> say index_is_empty(), then the above would read more naturally like
> this:
>
> if (!opts->keep_if_made_empty && !empty && index_is_empty())
>
> > + /*
> > + * The head tree and the parent tree match
> > + * meaning the commit is empty. Since it wasn't created
>
> Don't you mean the head tree and the index?
>
yeah.
> > + * empty (based on the previous test), we can conclude
> > + * the commit has been made redundant. Since we don't
> > + * want to keep redundant commits, just skip this one
> > + */
> > + return 0;
> > + }
> > +
> > + argv_array_init(&array);
> > + argv_array_push(&array, "commit");
> > + argv_array_push(&array, "-n");
> >
> > - args[i++] = "commit";
> > - args[i++] = "-n";
> > if (opts->signoff)
> > - args[i++] = "-s";
> > + argv_array_push(&array, "-s");
> > if (!opts->edit) {
> > - args[i++] = "-F";
> > - args[i++] = defmsg;
> > + argv_array_push(&array, "-F");
> > + argv_array_push(&array, defmsg);
> > }
> > +
> > if (opts->allow_empty)
> > - args[i++] = "--allow-empty";
> > + argv_array_push(&array, "--allow-empty");
> > +
> > +
>
> Why two newlines?
>
> > + rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> > + argv_array_clear(&array);
> > + return rc;
> > +}
>
> Looks good to me otherwise.
>
--
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