Re: [PATCH/RFC v2 2/2] git-p4: warn if git authorship won't be retained

From: Pete Wyckoff <pw@padd.com>
Date: May 9, 2011, 22:39 UTC
Message-ID: <20110509223928.GB1716@arf.padd.com>
In-Reply-To: <1304923203-26278-3-git-send-email-luke@diamand.org>

luke@diamand.org wrote on Mon, 09 May 2011 07:40 +0100:

Show 9 quoted lines

> If the git commits you are submitting contain changes made by
> other people, the authorship will not be retained. Change git-p4
> to warn of this and to note that --preserve-user can be used
> to solve the problem (if you have suitable permissions).
> The warning can be disabled.
> 
> Add a test case and update documentation.
> 
> Signed-off-by: Luke Diamand <luke@diamand.org>

This addresses Junio's concerns and makes more sense to me too. It is odd for someone to submit commits not their own, but if they want to, at least the warning can be hushed.

Two teensy fixes:

Show 5 quoted lines

> +            if self.checkAuthorship and not self.p4UserIsMe(p4User):
> +                submitTemplate += "######## git author %s does not match your p4 account.\n" % gitEmail
> +                submitTemplate += "######## Use git-p4 option --preserve-user to modify authorship\n"
> +                submitTemplate += "######## Use git-p4 config git-p4.skipUserNameCheck hides this messsage.\n"
> +

s/messsage/message/

Show 12 quoted lines

>              separatorLine = "######## everything below this line is just the diff #######\n"
>  
>              [handle, fileName] = tempfile.mkstemp()
> @@ -998,6 +1022,11 @@ class P4Submit(Command, P4UserMap):
>              commits.append(line.strip())
>          commits.reverse()
>  
> +        if not self.preserveUser and (gitConfig("git-p4.skipUserNameCheck") != "true"):
> +            self.checkAuthorship = True
> +        else:
> +            self.checkAuthorship = False
> +

Can we have a bit clearer logic? I get confused with too many negatives.

	#
	# Issue a warning in the submission template if the
	# author of the patch is different from the submitter.
	# But if preserveUser is used, the authorship will be
	# adjusted, hence no warning.  Also this message can be
	# hushed with a config option.
	#
	self.checkAuthorship = True
	if self.preserveUser:
	    self.checkAuthorship = False
	if gitConfig("git-p4.skipUserNameCheck") != "true":
	    self.checkAuthorship = False

Show 8 quoted lines

> +make_change_by_user() {
> +    file=$1
> +    name=$2
> +    email=$3
> +	echo "username: a change by $name" >> $file &&
> +	git add $file &&
> +	git commit --author "$name <$email>" -m "a change by $name"
> +}

Tab/space oddities.

Show 8 quoted lines

> +# If we're *not* using --preserve-user, git-p4 should warn if we're submitting
> +# changes that are not all ours.
> +# Test: user in p4 and user unknown to p4.
> +# Test: warning disabled and user is the same.
> +test_expect_success 'not preserving user with mixed authorship' '
> +	"$GITP4" clone --dest="$git" //depot &&
> +	cd "$git" &&
> +	git config git-p4.skipSubmitEditCheck true

Missing &&. These are hard to remember, and I've been corrected before too.

Show 22 quoted lines

> +	p4_add_user derek Derek &&
> +
> +	make_change_by_user usernamefile3 Derek derek@localhost &&
> +	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author derek@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	make_change_by_user usernamefile3 Charlie charlie@localhost &&
> +	(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author charlie@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	make_change_by_user usernamefile3 alice alice@localhost &&
> +	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "does not match") &&
> +	git diff --exit-code HEAD..p4/master &&
> +
> +	git config git-p4.skipUserNameCheck true &&
> +	make_change_by_user usernamefile3 Charlie charlie@localhost &&
> +	!(P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |
> +		grep "git author charlie@localhost does not match") &&
> +	git diff --exit-code HEAD..p4/master &&

Maybe -q on the greps.  Not a big deal.

> +	p4_check_commit_author usernamefile3 alice &&
> +	cd "$TRASH_DIRECTORY" &&
> +	rm -rf "$git" && mkdir "$git"
> +'

If you're okay on these fixes, go ahead and add my Acked-By for the resend to Junio.

		-- Pete