Re: [PATCH 1/2] revert: refactor code that prints success or failure message
- From
Christian Couder <chriscool@tuxfamily.org>
- Date
- Jul 13, 2010, 21:02 UTC
- Message-ID
- <201007132302.59324.chriscool@tuxfamily.org>
- In-Reply-To
- <20100712163043.GA1531@burratino>
On Monday 12 July 2010 18:30:43 Jonathan Nieder wrote:
Show 14 quoted lines
> 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.
If it makes the code easier to maintain it's still a refactoring even if it adds some lines, but anyway as I say below I will improve the commit message.
Show 6 quoted lines
> > 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...
Yes, but it's just because it's needed for the refactoring.
Show 11 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.
Yes.
Show 11 quoted lines
> > 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).
Ok.
Show 6 quoted lines
> > 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?
The old 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.
Ok, so I think I will propose a patch to do that.
Show 29 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?
Yes.
Show 15 quoted lines
> > @@ -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.Right.
Show 29 quoted lines
> [...] > > > --- 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:
I don't know about sh -x but there is this code in test-lib.sh to warn about GIT_TRACE use:
case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in 1|2|true) echo "* warning: Some tests will not work if GIT_TRACE" \ "is set as to trace on STDERR ! *" echo "* warning: Please set GIT_TRACE to something" \ "other than 1, 2 or true ! *" ;; esac
Show 18 quoted lines
> ... > 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.
Ok, I will improve it.
Thanks, Christian.