Re: [PATCH 1/2] revert: refactor code that prints success or failure message
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 12, 2010, 16:30 UTC
- Message-ID
- <20100712163043.GA1531@burratino>
- In-Reply-To
- <20100712115455.12251.53947.chriscool@tuxfamily.org>
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...
Show 8 quoted lines
> 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.
Show 16 quoted lines
> +++ 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)
[...]
Show 9 quoted lines
> + 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.
Show 16 quoted lines
> ' > > 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.