On Monday 12 July 2010 18:30:43 Jonathan Nieder wrote:
> 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.
> > 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.
> > 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.
> > 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.
> > 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.
> > +++ 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.
> > @@ -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.
> [...]
>
> > --- 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
> ...
> 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.
--
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