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

[Jonathan Nieder <jrnieder@gmail.com>]

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.