On Thu, Apr 05, 2012 at 02:01:43PM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@xxxxxxxxxxxxx> writes:
>
> > Subject: Re: [PATCH 3/5] git-cherry-pick: Add ignore-if-made-empty option [v2]
>
> Please don't do this. The tools do not strip the garbage at the end.
>
> Instead, do it like either one of these:
>
> Subject: [PATCH 3/5 (v2)] mumble mumble...
> Subject: [PATCH v2 3/5] mumble mumble...
>
> The latter is more appropriate when resending everything, including the
> ones that did not change from the earlier round.
>
Thats fine, I typically use tools that strip everything inside all braces, I'll
update this on my next re-roll.
> > Since we'll be using git-cherry-pick to enhance git-rebase's ability to preserve
> > empty commits, we open ourselves to the possibility of preserving commits that
> > are made empty by a previous merge as well, which is almost certainly not what
> > we want (most of the time). To handle this, we can add the ignore-if-made-empty
> > option. If enabled, it will look at cherry-picked commits, and if the origional
> > sha1 has the same tree as its parent, then the cherry-pick is comitted as an
> > empty commit, otherwise the commit is skipped (because it previously made
> > changes to the tree, but no longer does).
> >
> > Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
>
> > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> > index c283d8c..bb7eb4a 100644
> > --- a/Documentation/git-cherry-pick.txt
> > +++ b/Documentation/git-cherry-pick.txt
> > @@ -103,7 +103,7 @@ effect to your index in a row.
> > cherry-pick'ed commit, then a fast forward to this commit will
> > be performed.
> >
> > ---allow-empty:
> > +--allow-empty:::
>
> A single colon is already buggy, but three colons is equally bad. Please
> fix that in the original point this bug was introduced. I guess it was
> 2/5 of this series?
>
Crap, thanks, I missed that. I'll square that away
> > @@ -112,6 +112,14 @@ effect to your index in a row.
> > commits that meet the "fast-forward" requirement will be kept
> > even without this option.
> >
> > +--ignore-if-made-empty::
> > + If the --allow-empty option is used, all empty commits are kept,
>
> Literal strings the user may type are typically set in tt font
> (i.e. `--allow-empty`).
>
Ack, ok.
> > + including those which were made empty due to a previous change.
> > + While this may be desireable, likely it is not.
>
> Mental note. Up to this point, the reader is told that "--allow-empty"
> alone is likely to do a wrong thing.
>
> > + This option
> > + restricts the scope of --allow-empty to only those commits which
> > + were created as empty commits ...
>
> And the user is asked to give "--ignore-if-made-empty" in addition to
> "--allow-empty" to get a saner and more likely to be useful behaviour.
> Isn't that backwards? This "the other option is insane, and please make
> it saner" option needs a lot more typing than the more insane option.
>
> I would expect that "--allow-empty" would by default filter ones that are
> originally non-empty but are made unnecessary (we are allowing empty
> commits in the original history to be cherry-picked, but the general
> principle that unnecessary commits must not be picked still is in effect).
> If you want to give a user the other more insane mode of operation, it is
> OK to let the user give a different option *instead* *of* the saner
> "--allow-empty".
>
> Perhaps name that "--keep-unnecessary-commit" (it is no longer about
> allowing empty commits in the original history to be picked; it is about
> keeping unnecessary and irrelevant commits in the resulting history).
>
> And error out if both options are given.
>
Yeah, ok. I wasn't really thinking about the direct use case, as I had expected
the typical use to be in the rebase scripts, where this is already wired up, but
I see your point. I'll reverse the logic.
> > +static int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
> > +{
> > + struct argv_array array;
> > + int rc;
> > +
> > + argv_array_init(&array);
> > + argv_array_push(&array, "commit");
> > + argv_array_push(&array, "-n");
> > +
> > + if ((!empty) && (opts->ignore_if_made_empty)) {
>
> Style: lose the extra and unnecessary parentheses, especially when the
> expression inside them are so trivial, i.e.
>
Ok
> if (!empty && opts->ignore_if_made_empty) {
>
> although I suspect that redesign of the command line interface might make
> it a moot point to polish this part of the code without major rethinking.
>
> > + /* Note: This implies --dry-run */
> > + argv_array_push(&array, "--porcelain");
> > + if (run_command_v_opt(array.argv, RUN_GIT_CMD) == 1) {
>
> The only thing you want to check at this point is if the contents of the
> index being committed is the same as the tree of HEAD. Why do we even
> want to incur this much overhead?
>
> Isn't running "diff-index --cached HEAD" sufficient?
Well, I was looking at the git commit code to see what exactly --dry-run did,
and I thought that it was roughly equivalent to diff-index. But if its lower
overhead to do a diff-index instead, I'll happily change it.
>
> > + /* The dry run exit code of 1 tells us this is
> > + * an empty commit, just skip it.
> > + */
>
> /*
> * Our multi-line comments are
> * formatted this way.
> */
>
Ok.
> > + argv_array_clear(&array);
> > + return 0;
> > + }
> > + argv_array_pop(&array, 1);
> > + }
> > +
> >
> > if (opts->signoff)
> > + argv_array_push(&array, "-s");
> > if (!opts->edit) {
> > + argv_array_push(&array, "-F");
> > + argv_array_push(&array, defmsg);
> > }
> > +
> > if (opts->allow_empty)
> > + argv_array_push(&array, "--allow-empty");
> > +
> > +
> > + rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> > + argv_array_clear(&array);
> > + return rc;
> > +}
> > +
> > +static int is_origional_commit_empty(struct commit *commit)
>
> What is origional?
>
A spelling error :). Thanks.
> Is there a reason why this series is not marked as WIP or RFC?
>
Because its not. It works perfectly well, and its fairly close to done. So far
your review has turned up one significant operational change, which is easily
re-worked and a few stylistic nits. Thats how review works, I don't see whats
wrong with that.
> I am starting to wonder if it is worth spending time on careful reviewing,
> or it would be sufficient to give a cursory review quickly to give you
> more time to polish your re-roll.
>
I'm not sure what you think is so egregious about this changeset, but if
you have a specific problem, please let me know. We all make errors, thats why
we review work like this. All your comment above does is toss a purposeless
insult into the conversation.
> > +{
> > + struct argv_array argv_array;
> > + struct child_process cp;
> > + char ptree[41], pptree[41];
> > + int pipefd[2];
> > + FILE *output;
> > + int ret = 0;
> > +
> > + if (pipe2(pipefd, 0) < 0)
> > + return 0;
> > +
> > + output = xfdopen(pipefd[0], "r");
> > +
> > + argv_array_init(&argv_array);
> > + memset(&cp, 0, sizeof(struct child_process));
> >
> > - args[i] = NULL;
> > + argv_array_push(&argv_array, "rev-parse");
> > + argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));
> >
> > - return run_command_v_opt(args, RUN_GIT_CMD);
> > + cp.git_cmd = 1;
> > + cp.no_stdin = 1;
> > + cp.no_stderr = 1;
> > + cp.out = pipefd[1];
> > + cp.argv = argv_array.argv;
> > +
> > + if (start_command(&cp) < 0)
> > + goto out;
> > +
> > + if (fscanf(output, "%s\n", ptree) < 1)
> > + goto out;
> > +
> > + finish_command(&cp);
> > +
> > + fclose(output);
> > + close(pipefd[0]);
> > + argv_array_clear(&argv_array);
> > +
> > + if (pipe2(pipefd, 0) < 0)
>
> Huh? "man pipe2" and see if it is portable.
>
Crud, I completely forgot that pipe2 was _GNU_SOURCE only. I'll rework that,
thanks.
Regards
Neil
--
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