Hi,
Christian Couder wrote:
> This patch refactors the code that prints a message like
> "Automatic cherry-pick failed. <help message>".
>From the point of view of refactoring, it does not seem
so promising: the patch adds more code than it removes.
$ git diff --numstat HEAD^
29 22 builtin/revert.c
25 1 t/t3508-cherry-pick-many-commits.sh
So I am hoping there is some other reason.
> To do that, now do_recursive_merge() returns an int to signal
> success or failure. And in case of failure we just return 1
> from do_pick_commit() instead of doing "exit(1)" from either
> do_recursive_merge() or do_pick_commit().
This part sounds like libification...
> In case of successful merge, do_recursive_merge() used to print
> something like "Finished one cherry-pick." but nothing was
> printed in case a special strategy like "resolve" was used.
> Now in this case a message like "Finished one cherry-pick with
> strategy resolve." is printed.
>
> So the command behavior should be more consistent wether we are
> using a special strategy or not.
This part sounds like improving output in the cherry-pick --strategy
success case.
> I also wonder why messages like "Finished one cherry-pick."
> are printed on stderr instead of stdout.
Progress reports tend to go to stderr, since they are not what the
user wanted to learn from the command but side-effects. In other
words, I think the general principle is that
$ git <foo> 2>/dev/null
should output whatever a traditional Unix command would (usually
nothing).
> And while at making a backward incompatible change, maybe
> we could change the message to something like:
>
> "Finished cherry-pick <sha1>"
Does <sha1> represent the old commit or the new one?
I can’t come up with a reason to worry about backward compatibility in
this case. Messages to stderr from porcelain are not guaranteed to be
stable.
> +++ b/builtin/revert.c
> @@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base, struct commit *next,
> i++;
> }
> }
> - write_message(msgbuf, defmsg);
> - fprintf(stderr, "Automatic %s failed.%s\n",
> - me, help_msg());
> - rerere(allow_rerere_auto);
> - exit(1);
> }
> - write_message(msgbuf, defmsg);
> - fprintf(stderr, "Finished one %s.\n", me);
> +
> + return !clean;
> }
The return value is 1 if dirty, 0 if clean. In any other
situation (e.g., the index locking or the merge machinery breaks)
it will still die, so this is not about libification. Is it
is for unification with the try_merge_command case?
> @@ -470,30 +466,41 @@ static int do_pick_commit(void)
[...]
> + if (res) {
> + fprintf(stderr, "Automatic %s failed.%s\n",
> + mebuf.buf, help_msg());
> + rerere(allow_rerere_auto);
> + } else {
> + fprintf(stderr, "Finished one %s.\n", mebuf.buf);
> + }
> +
> + strbuf_release(&mebuf);
Yep, looks like it is.
[...]
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -23,12 +23,36 @@ test_expect_success setup '
New tests for the success output.
> '
>
> test_expect_success 'cherry-pick first..fourth works' '
> + cat <<-EOF > expected &&
> + Finished one cherry-pick.
> + Finished one cherry-pick.
> + Finished one cherry-pick.
> + EOF
> +
> + git checkout -f master &&
> + git reset --hard first &&
> + test_tick &&
> + git cherry-pick first..fourth 2>actual &&
> + git diff --quiet other &&
> + git diff --quiet HEAD other &&
> + test_cmp expected actual &&
This breaks test runs with sh -x or GIT_TRACE=1 (I am not sure
whether we support them, but I think it is worth avoiding
problems where possible). Maybe:
...
grep "Finished one cherry-pick\." actual >filtered &&
n=$(wc -l <filtered) &&
... &&
test $n -eq 3
Summary: I was misled by the commit message. Maybe something
like
Subject: cherry-pick --strategy: report success
"git cherry-pick foo" has always reported success
with "Finished one cherry-pick" and cherry-pick
--strategy is starting to feel left out. Move the
code to write that message from do_recursive_merge
to do_cherry_pick so other strategies can share it.
would have set me straight.
--
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