[PATCH] Warn the users when more than 3 '-C' given.

Bo Yang· Apr 10, 2010, 11:51 UTC · lore

Output a warning message to users when there are more than 3 '-C' options given. And ignore the numeric argument value provided by the additional '-C' options.

Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
---
 builtin/blame.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

Show changes to builtin/blame.c +9 −0

diff --git a/builtin/blame.c b/builtin/blame.c
index fc15863..e8ed547 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2165,6 +2165,15 @@ static int blame_copy_callback(const struct option *option, const char *arg, int
 	int *opt = option->value;
 
 	/*
+	 * Warn the users when more than 3 '-C' options are given and
+	 * ignore the corresponding numeric argument of it.
+	 */
+	if (*opt & PICKAXE_BLAME_COPY_HARDEST) {
+		warning("The additional '-C' above 3 is not supported.");
+		return 0;
+	}
+
+	/*
 	 * -C enables copy from removed files;
 	 * -C -C enables copy from existing files, but only
 	 *       when blaming a new file;

-- 
1.7.0.2.273.gc2413.dirty

Junio C Hamano· Apr 10, 2010, 19:12 UTC · re: Bo Yang · lore

Re: [PATCH] Warn the users when more than 3 '-C' given.

Bo Yang <struggleyb.nku@gmail.com> writes:

> Output a warning message to users when there are more than
> 3 '-C' options given. And ignore the numeric argument value
> provided by the additional '-C' options.

How were you bitten by the lack of this warning? You gave four or five to see how output would change, spent sleepless nights but couldn't figure out what the differences between third and fourth levels are, and wasted too much time?

IOW, what does this fix?

I personally do not see much value in this patch. It would be just a hindrance to remember to remove this hunk when somebody improves the algorithm to add fourth level of detail to the inspection logic.

Yann Dirson· Apr 12, 2010, 06:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] Warn the users when more than 3 '-C' given.

Le Sat, 10 Apr 2010 12:12:58 -0700, Junio C Hamano <gitster@pobox.com> a écrit :

Show 10 quoted lines

> Bo Yang <struggleyb.nku@gmail.com> writes:
> 
> > Output a warning message to users when there are more than
> > 3 '-C' options given. And ignore the numeric argument value
> > provided by the additional '-C' options.
> 
> How were you bitten by the lack of this warning?  You gave four or
> five to see how output would change, spent sleepless nights but
> couldn't figure out what the differences between third and fourth
> levels are, and wasted too much time?

That sounding a bit harsh, I guess it is my turn to take the blame for suggesting this in last week's thread :)

> IOW, what does this fix?

One practical advantage of this warning would be, in the very case of adding meaning to an additional -C, that a user trying to use it on an older version of git would get a warning that the program might not indeed to what the user requested.

However, my first feeling was simply that, while it is usually harmless to let the user specify a flag several time, when it changes nothing, the situation is different when repetition of the flag is important - it is closer to an invalid flag combination.

In fact, I even dislike that use of repetitive -C. One could argue that it is much like repetition of -v used in various programs to raise verbosity. But well, in our case, it is much more than just increasing the level of details, it makes it use a different mechanism - even if each time it is a superset of the previous one.

And what if someone comes with an idea of a "level of -C" that indeed lays between two existing ones ? Will we shift the meaning of the existing ones ? And what about one "level" that would not strictly fit in the existing "superset" chain ?

What about instead using a more descriptive flag ? That would be more verbose typing, but then we can still keep the existing flags for backward compatibility, and we also have shell command-line completion.

I'd think about something like: -C -C -> -Cunmodified (that one also for diff) -C -C -C -> -Chistory

I could also argue that "blame -M" could also be better placed as a -C variant (it is also supposed to detect some copies), and could have as fullname something like "blame -Csamefile".

> I personally do not see much value in this patch.  It would be just a
> hindrance to remember to remove this hunk when somebody improves the
> algorithm to add fourth level of detail to the inspection logic.

Well, the warning should trigger the 1st time that somebody tests his fourth -C, right ?

-- 
Yann

patchWarn the users when more than 3 '-C' given.