Re: [PATCH 1/2] revert: refactor code that prints success or failure message

[Christian Couder <chriscool@tuxfamily.org>]

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.