threads / patch / 13332

patch, 3 partsShow author and/or committer in some cases

Subject: [PATCH 0/3] Show author and/or committer in some cases

## tl;dr

15 messages between Apr 30, 2008 and May 2, 2008. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Santi Béjar· Apr 30, 2008, 08:47 UTC · lore
Hi *,
  This patch series tries to make more evident the author and committer ident
used in a commit trying to be minimize when it is shown.
Santi
Santi Béjar (3):
  Preparation to call determine_author_info from prepare_to_commit
  commit: Show author if different from committer
  commit: Show the committer ident when is different from the parent
 builtin-commit.c  |  127 ++++++++++++++++++++++++++++++++++++-----------------
 t/t7502-commit.sh |   22 +++++++++
 2 files changed, 109 insertions(+), 40 deletions(-)
Santi Béjar· Apr 30, 2008, 08:47 UTC · re: Santi Béjar · lore

[PATCH 1/3] Preparation to call determine_author_info from prepare_to_commit

Reorder functions definitions such that determine_author_info is defined before prepare_to_commit. No code changes.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 builtin-commit.c |   78 +++++++++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 39 deletions(-)
Show changes to builtin-commit.c +39 −39
diff --git a/builtin-commit.c b/builtin-commit.c
index 256181a..a37d8c3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -395,6 +395,45 @@ static int is_a_merge(const unsigned char *sha1)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
+static void determine_author_info(struct strbuf *sb)
+{
+	char *name, *email, *date;
+
+	name = getenv("GIT_AUTHOR_NAME");
+	email = getenv("GIT_AUTHOR_EMAIL");
+	date = getenv("GIT_AUTHOR_DATE");
+
+	if (use_message) {
+		const char *a, *lb, *rb, *eol;
+
+		a = strstr(use_message_buffer, "\nauthor ");
+		if (!a)
+			die("invalid commit: %s", use_message);
+
+		lb = strstr(a + 8, " <");
+		rb = strstr(a + 8, "> ");
+		eol = strchr(a + 8, '\n');
+		if (!lb || !rb || !eol)
+			die("invalid commit: %s", use_message);
+
+		name = xstrndup(a + 8, lb - (a + 8));
+		email = xstrndup(lb + 2, rb - (lb + 2));
+		date = xstrndup(rb + 2, eol - (rb + 2));
+	}
+
+	if (force_author) {
+		const char *lb = strstr(force_author, " <");
+		const char *rb = strchr(force_author, '>');
+
+		if (!lb || !rb)
+			die("malformed --author parameter");
+		name = xstrndup(force_author, lb - force_author);
+		email = xstrndup(lb + 2, rb - (lb + 2));
+	}
+
+	strbuf_addf(sb, "author %s\n", fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME));
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix)
 {
 	struct stat statbuf;
@@ -622,45 +661,6 @@ static int message_is_empty(struct strbuf *sb, int start)
 	return 1;
 }
 
-static void determine_author_info(struct strbuf *sb)
-{
-	char *name, *email, *date;
-
-	name = getenv("GIT_AUTHOR_NAME");
-	email = getenv("GIT_AUTHOR_EMAIL");
-	date = getenv("GIT_AUTHOR_DATE");
-
-	if (use_message) {
-		const char *a, *lb, *rb, *eol;
-
-		a = strstr(use_message_buffer, "\nauthor ");
-		if (!a)
-			die("invalid commit: %s", use_message);
-
-		lb = strstr(a + 8, " <");
-		rb = strstr(a + 8, "> ");
-		eol = strchr(a + 8, '\n');
-		if (!lb || !rb || !eol)
-			die("invalid commit: %s", use_message);
-
-		name = xstrndup(a + 8, lb - (a + 8));
-		email = xstrndup(lb + 2, rb - (lb + 2));
-		date = xstrndup(rb + 2, eol - (rb + 2));
-	}
-
-	if (force_author) {
-		const char *lb = strstr(force_author, " <");
-		const char *rb = strchr(force_author, '>');
-
-		if (!lb || !rb)
-			die("malformed --author parameter");
-		name = xstrndup(force_author, lb - force_author);
-		email = xstrndup(lb + 2, rb - (lb + 2));
-	}
-
-	strbuf_addf(sb, "author %s\n", fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME));
-}
-
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const char * const usage[])
 {

-- 
1.5.5.1.148.g37726
Santi Béjar· Apr 30, 2008, 08:47 UTC · re: Santi Béjar · lore

[PATCH 2/3] commit: Show author if different from committer

That would help reassure anybody while committing other's changes.
Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 builtin-commit.c  |   23 ++++++++++++++++++++---
 t/t7502-commit.sh |   10 ++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)
Show changes to 2 files +30 −3

builtin-commit.c, t/t7502-commit.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index a37d8c3..c51c70f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,6 +47,7 @@ static enum {
 
 static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
+static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
 /*
@@ -395,7 +396,7 @@ static int is_a_merge(const unsigned char *sha1)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
-static void determine_author_info(struct strbuf *sb)
+static void determine_author_info()
 {
 	char *name, *email, *date;
 
@@ -431,7 +432,9 @@ static void determine_author_info(struct strbuf *sb)
 		email = xstrndup(lb + 2, rb - (lb + 2));
 	}
 
-	strbuf_addf(sb, "author %s\n", fmt_ident(name, email, date, IDENT_ERROR_ON_NO_NAME));
+	author_name = name;
+	author_email = email;
+	author_date = date;
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix)
@@ -443,6 +446,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 	FILE *fp;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
+	const char *author_ident;
+	const char *committer_ident;
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -522,6 +527,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
 	strbuf_release(&sb);
 
+	determine_author_info();
+
 	if (use_editor) {
 		if (in_merge)
 			fprintf(fp,
@@ -545,6 +552,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 		if (only_include_assumed)
 			fprintf(fp, "# %s\n", only_include_assumed);
 
+		author_ident = xstrdup(fmt_name(author_name, author_email));
+		committer_ident =
+			xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					 getenv("GIT_COMMITTER_EMAIL")));
+		if (strcmp(author_ident, committer_ident))
+			fprintf(fp,
+				"# Author:    %s\n",
+				fmt_name(author_name, author_email));
+
 		saved_color_setting = wt_status_use_color;
 		wt_status_use_color = 0;
 		commitable = run_status(fp, index_file, prefix, 1);
@@ -920,7 +936,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
 	}
 
-	determine_author_info(&sb);
+	strbuf_addf(&sb, "author %s\n",
+		    fmt_ident(author_name, author_email, author_date, IDENT_ERROR_ON_NO_NAME));
 	strbuf_addf(&sb, "committer %s\n", git_committer_info(IDENT_ERROR_ON_NO_NAME));
 	if (!is_encoding_utf8(git_commit_encoding))
 		strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 284c941..95acf4c 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -154,6 +154,16 @@ test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 
 '
 
+echo "# Author:    $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >> expect
+
+test_expect_success 'author different from committer' '
+
+	echo >>negative &&
+	git commit -e -m "sample"
+	head -n 5 .git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 pwd=`pwd`
 cat >> .git/FAKE_EDITOR << EOF
 #! /bin/sh

-- 
1.5.5.1.148.g37726
Santi Béjar· Apr 30, 2008, 08:47 UTC · re: Santi Béjar · lore

[PATCH 3/3] commit: Show the committer ident when is different from the parent

To remind the committer ident in case it is not what you want (taken from the gecos field, want to use different ident in different repositories).

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
Hi,
  it does not work with the initial commit, I don't know why.
  Even with this:
	if (!commit)
		return NULL;
  Can someone help me? Thanks
 builtin-commit.c  |   30 ++++++++++++++++++++++++++++++
 t/t7502-commit.sh |   12 ++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
Show changes to 2 files +42 −0

builtin-commit.c, t/t7502-commit.sh

diff --git a/builtin-commit.c b/builtin-commit.c
index c51c70f..06582ef 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -437,6 +437,29 @@ static void determine_author_info()
 	author_date = date;
 }
 
+const char *get_parent_ident()
+{
+	unsigned char sha1[20];
+	struct commit *commit;
+	const char *a, *lb, *rb, *eol;
+
+
+	get_sha1("HEAD", sha1);
+	commit = lookup_commit_reference(sha1);
+	if (!commit)
+		return NULL;
+
+	a = strstr(commit->buffer, "\ncommitter ");
+
+	lb = strstr(a + 11, " <");
+	rb = strstr(a + 11, "> ");
+	eol = strchr(a + 11, '\n');
+	if (!lb || !rb || !eol)
+		return NULL;
+
+	return xstrndup(a + 11, rb + 1 - (a + 11));
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix)
 {
 	struct stat statbuf;
@@ -448,6 +471,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 	const char *hook_arg2 = NULL;
 	const char *author_ident;
 	const char *committer_ident;
+	const char *parent_ident;
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -561,6 +585,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 				"# Author:    %s\n",
 				fmt_name(author_name, author_email));
 
+		parent_ident = xstrdup(get_parent_ident());
+		if (strcmp(parent_ident, committer_ident))
+			fprintf(fp,
+				"# Committer: %s\n",
+				committer_ident);
+
 		saved_color_setting = wt_status_use_color;
 		wt_status_use_color = 0;
 		commitable = run_status(fp, index_file, prefix, 1);
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 95acf4c..96b5dcb 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -164,6 +164,18 @@ test_expect_success 'author different from committer' '
 	test_cmp expect actual
 '
 
+GIT_COMMITTER_NAME="C. O. Mitter"
+export GIT_COMMITTER_NAME
+echo "# Committer: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >> expect
+
+test_expect_success 'committer different from the parent committer' '
+
+	echo >>negative &&
+	git commit -e -m "sample"
+	head -n 6 .git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 pwd=`pwd`
 cat >> .git/FAKE_EDITOR << EOF
 #! /bin/sh

-- 
1.5.5.1.148.g37726
Jeff King· Apr 30, 2008, 14:50 UTC · re: Santi Béjar · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 10:47:16AM +0200, Santi Béjar wrote:
Show 6 quoted lines
>   it does not work with the initial commit, I don't know why.
>   Even with this:
> 	if (!commit)
> 		return NULL;
> 
>   Can someone help me? Thanks

It segfaults for me, since you try to strdup NULL. I think you want to return "" if there is no initial commit, so you can strcmp against the current committer (or alternatively, explicitly check for NULL).

Show 6 quoted lines
> +		parent_ident = xstrdup(get_parent_ident());
> +		if (strcmp(parent_ident, committer_ident))
> +			fprintf(fp,
> +				"# Committer: %s\n",
> +				committer_ident);
> +

Why strdup at all here, which leaks? Nobody else uses this variable, so it should be sufficient to strcmp(get_parent_ident(), committer_ident) once get_parent_ident promises to always return a valid string. Or alternatively:

  parent_ident = get_parent_ident();
  if (!parent_ident || strcmp(parent_ident, committer_ident))
A few other comments (I like the idea overall):
  - I haven't looked at the code organization, so maybe it is not
    feasible, but it seems like this stuff should go into wt-status's
    implementation, which would show up in a git-status.
  - The output looks very cluttered. It would be nice to have
    # enter your commit message...
    #
    # Author: whatever
    # Committer: whatever
    #
    # other stuff
    IOW, put in a blank line on either side, but not between the two
    identities if both are shown.
-Peff
Jeff King· Apr 30, 2008, 15:10 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 10:50:17AM -0400, Jeff King wrote:
Show 6 quoted lines
> It segfaults for me, since you try to strdup NULL. I think you want to
> return "" if there is no initial commit, so you can strcmp against the
> current committer (or alternatively, explicitly check for NULL).
> [...]
>     IOW, put in a blank line on either side, but not between the two
>     identities if both are shown.

Fixes for these are below (no commit message; I think you should respin the series rather than making this a separate patch). I didn't look at putting it into wt-status.c, but I think that is worth pursuing.

---
 builtin-commit.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
Show changes to builtin-commit.c +10 −4
diff --git a/builtin-commit.c b/builtin-commit.c
index 30828c5..4e62510 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -472,6 +472,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 	const char *author_ident;
 	const char *committer_ident;
 	const char *parent_ident;
+	int showed_ident = 0;
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -582,15 +583,20 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 					 getenv("GIT_COMMITTER_EMAIL")));
 		if (strcmp(author_ident, committer_ident))
 			fprintf(fp,
-				"# Author:    %s\n",
+				"%s# Author:    %s\n",
+				showed_ident++ ? "" : "#\n",
 				fmt_name(author_name, author_email));
 
-		parent_ident = xstrdup(get_parent_ident());
-		if (strcmp(parent_ident, committer_ident))
+		parent_ident = get_parent_ident();
+		if (!parent_ident || strcmp(parent_ident, committer_ident))
 			fprintf(fp,
-				"# Committer: %s\n",
+				"%s# Committer: %s\n",
+				showed_ident++ ? "" : "#\n",
 				committer_ident);
 
+		if (showed_ident)
+			fprintf(fp, "#\n");
+
 		saved_color_setting = wt_status_use_color;
 		wt_status_use_color = 0;
 		commitable = run_status(fp, index_file, prefix, 1);

-- 
1.5.5.1.183.g593d89.dirty
Jeff King· Apr 30, 2008, 16:37 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 10:50:17AM -0400, Jeff King wrote:
> A few other comments (I like the idea overall):

Actually, thinking about this a bit more (and using it), I'm not sure that the best rule for showing the committer is "differs from the previous committer." For example, if I am building on Junio's git.git, the committer name is shown, even though I have made many commits in this repo already.

So if the goal is to show the committer name only when it "matters", I don't think you have succeeded.

-Peff
Santi Béjar· Apr 30, 2008, 19:23 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 6:37 PM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
>  Actually, thinking about this a bit more (and using it), I'm not sure
>  that the best rule for showing the committer is "differs from the
>  previous committer." For example, if I am building on Junio's git.git,
>  the committer name is shown, even though I have made many commits in
>  this repo already.
>
>  So if the goal is to show the committer name only when it "matters", I
>  don't think you have succeeded.
>
>  -Peff
>
On Wed, Apr 30, 2008 at 7:52 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>  I like the idea in general, but the implementation could be refined.
>  Maybe consider looking at user.name (whether the repo has an ident
>  set)? This is usually done conciously, and the user wont need this
>  reminder.
Maybe only show the committer ident when both happens:
1) the committer ident is different from the parent
2) the committer ident is set automatically
Santi
P.D.: Jeff, thanks for the amending patch.
Jeff King· Apr 30, 2008, 19:26 UTC · re: Santi Béjar · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 09:23:43PM +0200, Santi Béjar wrote:
Show 9 quoted lines
> >  I like the idea in general, but the implementation could be refined.
> >  Maybe consider looking at user.name (whether the repo has an ident
> >  set)? This is usually done conciously, and the user wont need this
> >  reminder.
> 
> Maybe only show the committer ident when both happens:
> 
> 1) the committer ident is different from the parent
> 2) the committer ident is set automatically

Honestly, I think just "2)" is probably fine (where automatically presumably means "from GECOS"). I see what you are trying to accomplish with "1)", but it's so workflow specific as to be useless.

> P.D.: Jeff, thanks for the amending patch.
No problem.
-Peff
Santi Béjar· May 1, 2008, 08:34 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Wed, Apr 30, 2008 at 9:26 PM, Jeff King <peff@peff.net> wrote:
Show 15 quoted lines
> On Wed, Apr 30, 2008 at 09:23:43PM +0200, Santi Béjar wrote:
>
>  > >  I like the idea in general, but the implementation could be refined.
>  > >  Maybe consider looking at user.name (whether the repo has an ident
>  > >  set)? This is usually done conciously, and the user wont need this
>  > >  reminder.
>  >
>  > Maybe only show the committer ident when both happens:
>  >
>  > 1) the committer ident is different from the parent
>  > 2) the committer ident is set automatically
>
>  Honestly, I think just "2)" is probably fine (where automatically
>  presumably means "from GECOS"). I see what you are trying to accomplish
>  with "1)", but it's so workflow specific as to be useless.

I don't see "1) as workflow specific, it is there to minimize the number of time it is shown, ie somebody might prefer the automatic ident, so as long as it does not change it is not shown.

Santi
Jeff King· May 1, 2008, 13:51 UTC · re: Santi Béjar · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Thu, May 01, 2008 at 10:34:44AM +0200, Santi Béjar wrote:
Show 10 quoted lines
> >  > 1) the committer ident is different from the parent
> >  > 2) the committer ident is set automatically
> >
> >  Honestly, I think just "2)" is probably fine (where automatically
> >  presumably means "from GECOS"). I see what you are trying to accomplish
> >  with "1)", but it's so workflow specific as to be useless.
> 
> I don't see "1) as workflow specific, it is there to minimize the
> number of time it is shown, ie somebody might prefer the automatic
> ident, so as long as it does not change it is not shown.

Yes, but whether it actually succeeds in minimizing is dependent on the workflow of the user. I.e., it works great if I tend to build on small, personal repositories (or topic branches), or if I tend to receive other people's work by patches that I commit myself. But if I am pulling from somebody else's repo (as many of us do from git.git), or if I use a shared repo with the other committers, then I am very likely to be building on a parent that was committed by somebody else (and consequently will see the committer mentioned way more often than was intended).

-Peff
Junio C Hamano· May 1, 2008, 19:02 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Thu, May 01, 2008 at 10:34:44AM +0200, Santi Béjar wrote:
>
>> >  > 1) the committer ident is different from the parent
>> >  > 2) the committer ident is set automatically
>> >
>> >  Honestly, I think just "2)" is probably fine (where automatically
>> >  presumably means "from GECOS"). I see what you are trying to accomplish
>> >  with "1)", but it's so workflow specific as to be useless.
>> 
>> I don't see "1) as workflow specific, it is there to minimize the
>> number of time it is shown, ie somebody might prefer the automatic
>> ident, so as long as it does not change it is not shown.
>
> Yes, but whether it actually succeeds in minimizing is dependent on the
> workflow of the user. I.e., it works great if I tend to build on small,
> personal repositories (or topic branches), or if I tend to receive other
> people's work by patches that I commit myself. But if I am pulling from
> somebody else's repo (as many of us do from git.git), or if I use a
> shared repo with the other committers, then I am very likely to be
> building on a parent that was committed by somebody else (and
> consequently will see the committer mentioned way more often than was
> intended).

The goal of the patch as I understand it is to prevent mistakes of committing under a wrong committer ident, isn't it? Why does "minimizing" come into the picture?

Once you have a good algorithm to see when to trigger the warning that the user might be using an unintended committer identity, I do not think you should refrain from issuing the warning when you see the offending committer ident and whose commit you are building on top of should not affect it. Otherwise, the user will get the warning once (or not even get the warning because the commit was made with a "commit -s -m 'typofix'" one liner), and keep using the wrong ident without noticing before building up a long chain of commits. If the identification algorithm is bogus, that would result in too many false hits to be annoying, and in that case, (1) I would not apply such a bogus algorithm until it gets into reasonable shape, (2) we would improve it after it gets applied and if still makes noise on a hopefully rarer false positive cases, and/or (3) the warning will be accompanied with a suggestion to explicitly use user.name and/or user.email in the configuration file to allow the user to squelch it.

I think the other patch about showing the author when you are committing other's changes is a good move, by the way.

Jeff King· May 1, 2008, 22:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Thu, May 01, 2008 at 12:02:51PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Once you have a good algorithm to see when to trigger the warning that the
> user might be using an unintended committer identity, I do not think you
> should refrain from issuing the warning when you see the offending
> committer ident and whose commit you are building on top of should not
> affect it.  Otherwise, the user will get the warning once (or not even get

I think I must not be writing very clearly, because that is basically the same point I have been trying to make in this thread. Santi's original algorithm for warning about the wrong committer ident was to check whether it matched the parent commit. But my point was that is a bad algorithm, because it has way too many false positives (i.e., you will end up showing the committer _all the time_ in many workflows). So the followup was "check this other thing, and also look at whether it matches the parent" to which my response was "why bother checking the parent match then?"

I also think you could argue that we should just show the committer all the time. But I don't think anyone has made that argument.

> I think the other patch about showing the author when you are committing
> other's changes is a good move, by the way.

I agree; it has a very straightforward and sensible algorithm for when to show.

-Peff
Santi Béjar· May 2, 2008, 09:25 UTC · re: Jeff King · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

On Fri, May 2, 2008 at 12:28 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> On Thu, May 01, 2008 at 12:02:51PM -0700, Junio C Hamano wrote:
>
>  > Once you have a good algorithm to see when to trigger the warning that the
>  > user might be using an unintended committer identity, I do not think you
>  > should refrain from issuing the warning when you see the offending
>  > committer ident and whose commit you are building on top of should not
>  > affect it.  Otherwise, the user will get the warning once (or not even get
>
>  I think I must not be writing very clearly,

It was clear, but I think there is more than this, especially the definition of what is a wrong committer depends on the user. I was trying to not annoy too much to some users (those who like the automatic committer).

Let's find the definition of "wrong committer":
1) user.{name,email} or GIT_COMMITTER_{NAME,EMAIL} is not a wrong committer.
2) automatic without a domain name (user@hostname.(none)) is a wrong committer.
2) automatic or partially set ident:
    a) wrong committer for some users
    b) right committer for others
I see different strategies. Show the committer:
1) always
2) when user.warn = yes (defaulting to yes)
3) when it is automatic
    a) always
    b) and different from parent
    c) and different from a list of valid committer idents
    d) and when user.WarnAutomatic = yes (defaulting to yes)
(the names of the configs are just ideas)
I prefer in order: 3a, 3d, 2, 1.
>
>  I also think you could argue that we should just show the committer all
>  the time. But I don't think anyone has made that argument.
>From past threads and the "precious screen state" I thought it was
totally discarded.
Santi
Alex Riesen· Apr 30, 2008, 17:52 UTC · re: Santi Béjar · lore

Re: [PATCH 3/3] commit: Show the committer ident when is different from the parent

Santi Béjar, Wed, Apr 30, 2008 10:47:16 +0200:
> To remind the committer ident in case it is not what you want
> (taken from the gecos field, want to use different ident in
> different repositories).
Like: show committer id always if the repo is any kind of a public
project. Which maybe ok, I'd better always see what id I use to work
on, for example, Git.

I like the idea in general, but the implementation could be refined. Maybe consider looking at user.name (whether the repo has an ident set)? This is usually done conciously, and the user wont need this reminder.

← back to recent threads