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.
>
> + 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?
> +
> + 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:
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?
> + * 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