[PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Matthias Urlichs]

[-- Attachment #1: Type: text/plain, Size: 4052 bytes --]

Junio C Hamano wrote:

> +		else if (!strcmp(arg, "--name-only-z"))
> +			diff_output_format = DIFF_FORMAT_NAME_Z;

Speaking as a user, I would get rather frustrated when I try to do that
and it doesn't work... so the attached patch allows that usage.
Please apply.

---
diff --git a/diff-cache.c b/diff-cache.c
--- a/diff-cache.c
+++ b/diff-cache.c
@@ -222,11 +222,17 @@ int main(int argc, const char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "-z")) {
-			diff_output_format = DIFF_FORMAT_MACHINE;
+			if (diff_output_format == DIFF_FORMAT_NAME)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_MACHINE;
 			continue;
 		}
 		if (!strcmp(arg, "--name-only")) {
-			diff_output_format = DIFF_FORMAT_NAME;
+			if (diff_output_format == DIFF_FORMAT_MACHINE)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_NAME;
 			continue;
 		}
 		if (!strcmp(arg, "--name-only-z")) {
diff --git a/diff-files.c b/diff-files.c
--- a/diff-files.c
+++ b/diff-files.c
@@ -55,11 +55,17 @@ int main(int argc, const char **argv)
 			; /* no-op */
 		else if (!strcmp(argv[1], "-s"))
 			; /* no-op */
-		else if (!strcmp(argv[1], "-z"))
-			diff_output_format = DIFF_FORMAT_MACHINE;
-		else if (!strcmp(argv[1], "--name-only"))
-			diff_output_format = DIFF_FORMAT_NAME;
-		else if (!strcmp(argv[1], "--name-only-z"))
+		else if (!strcmp(argv[1], "-z")) {
+			if (diff_output_format == DIFF_FORMAT_NAME)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_MACHINE;
+		} else if (!strcmp(argv[1], "--name-only")) {
+			if (diff_output_format == DIFF_FORMAT_MACHINE)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_NAME;
+		} else if (!strcmp(argv[1], "--name-only-z"))
 			diff_output_format = DIFF_FORMAT_NAME_Z;
 		else if (!strcmp(argv[1], "-R"))
 			diff_setup_opt |= DIFF_SETUP_REVERSE;
diff --git a/diff-stages.c b/diff-stages.c
--- a/diff-stages.c
+++ b/diff-stages.c
@@ -86,11 +86,17 @@ int main(int ac, const char **av)
 		}
 		else if (!strcmp(arg, "--find-copies-harder"))
 			find_copies_harder = 1;
-		else if (!strcmp(arg, "-z"))
-			diff_output_format = DIFF_FORMAT_MACHINE;
-		else if (!strcmp(arg, "--name-only"))
-			diff_output_format = DIFF_FORMAT_NAME;
-		else if (!strcmp(arg, "--name-only-z"))
+		else if (!strcmp(arg, "-z")) {
+			if (diff_output_format == DIFF_FORMAT_NAME)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_MACHINE;
+		} else if (!strcmp(arg, "--name-only")) {
+			if (diff_output_format == DIFF_FORMAT_MACHINE)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_NAME;
+		} else if (!strcmp(arg, "--name-only-z"))
 			diff_output_format = DIFF_FORMAT_NAME_Z;
 		else if (!strcmp(arg, "-R"))
 			diff_setup_opt |= DIFF_SETUP_REVERSE;
diff --git a/diff-tree.c b/diff-tree.c
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -483,7 +483,10 @@ int main(int argc, const char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "--name-only")) {
-			diff_output_format = DIFF_FORMAT_NAME;
+			if (diff_output_format == DIFF_FORMAT_MACHINE)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_NAME;
 			continue;
 		}
 		if (!strcmp(arg, "--name-only-z")) {
@@ -491,7 +494,10 @@ int main(int argc, const char **argv)
 			continue;
 		}
 		if (!strcmp(arg, "-z")) {
-			diff_output_format = DIFF_FORMAT_MACHINE;
+			if (diff_output_format == DIFF_FORMAT_NAME)
+				diff_output_format = DIFF_FORMAT_NAME_Z;
+			else
+				diff_output_format = DIFF_FORMAT_MACHINE;
 			continue;
 		}
 		if (!strcmp(arg, "-m")) {

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
When The Religious Right Takes Over, We'll All Live In Iran

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Junio C Hamano]

I've considered it, but what happens if you give -z first and
then name-only?

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Matthias Urlichs]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

Hi,

Junio C Hamano:
> I've considered it, but what happens if you give -z first and
> then name-only?
> 
Exactly the same thing as vice versa.
Or, even more exactly, my patch *makes* that happen. ;-)

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Isn't this a beautiful day! Just watch some bastard louse it up.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Junio C Hamano]

Matthias Urlichs <smurf@smurf.noris.de> writes:

> Exactly the same thing as vice versa.
> Or, even more exactly, my patch *makes* that happen. ;-)

Ah, I was not being careful enough.  Sorry.

That said, I have been hating that diff options parsing for
quite a while, and I've been thinking about cleaning it up along
the lines I'll outline here, but have not done anything about
it.  Care to help me out?

 - In diff.h introduce these new stuff:

     struct diff_opts {
     int output_format;
     int detect_rename;
     ...
     };
     void diff_opts_init(struct diff_opts *);
     int diff_opts_parse(const char *, struct diff_opts *);
     int diff_opts_final(struct diff_opts *);

 - In diff-* brothers:

   - replace individual diff option variables with a single
     "static struct diff_opts diff_opts";

   - change the argument parsing code to do the following:

     diff_opts_init(&diff_opts);
     for each arg {
         /* common options to diff brothers are handled by
          * diff_opts_parse()
          */
         switch (diff_opts_parse(arg, &diff_opts)) {
         case 1: /* was a diff option and was parsed successfully */
         	continue;
         case -1: /* error */
                usage(diff_*_usage);
         }
         if (!strcmp())
              ... parsing of other options
     }
     if (diff_opts_final(&diff_opts))
         /* defaulting to HUMAN format when nothing specified,
          * complaining if find-copies-harder is specified but
          * -C was not, etc. is done in diff_opts_final().
          *
          * The complex if() chains that checks if we are in
          * name or in raw mode and switch output_format around
          * properly is what I missed in your patch, but I think
          * you can lose that by recording z-ness of the output
          * independently from the output format in diff_opts_parse()
          * and combining diff-raw vs diff-name and z vs non-z
          * in diff_opts_final().  That would make the code much
          * simpler.
          */
         usage(diff_*_usage);

 - In diff.h and diff.c, replace individual option parameters
   for the following functions to a single pointer to struct
   diff_opts:

     diff_setup(), diffcore_std(), diffcore_std_no_resolve(), diff_flush().  

We probably can make diff_scoreopt_parse() function static to
diff.c once this is done.

We may want to rip out the independeant pickaxe, orderfile and
filter support for diff-helper while we are at it, making it
truly just a "diff-raw to diff-patch" converter.

Hmm?

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Matthias Urlichs]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

Hi,

Junio C Hamano:
> That said, I have been hating that diff options parsing for
> quite a while, and I've been thinking about cleaning it up along
> the lines I'll outline here, but have not done anything about
> it.  Care to help me out?
> 
I saw the problem...
> 
> Hmm?
> 
Sure -- assuming I find some time to actually do it over the next few days.

The problem is that this has been a problem lately. :-/

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Illiterate?  Write today, for free help!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Linus Torvalds]

On Thu, 14 Jul 2005, Junio C Hamano wrote:
> 
> That said, I have been hating that diff options parsing for
> quite a while, and I've been thinking about cleaning it up along
> the lines I'll outline here, but have not done anything about
> it.  Care to help me out?

I didn't do what you suggested, but I _did_ split the "format" up into 
"format + line_termination", which in my opinion cleaned up part of it a 
_lot_.

So now "-z" only sets "line_termination" to NUL. "format" starts out as 
"DIFF_FORMAT_RAW" (which is the old HUMAN/MACHINE format - the difference 
between those two are now the line termination) but can be "PATCH" and 
"NAME".

Now, DIFF_FORMAT_PATCH + -z wouldn't seem to make any sense at all, but
you can actually do so, and it actually makes some amount of sense for the 
case of

	git-diff-tree -v -p -z HEAD

where the "-z" means that the commit _message_ will be terminated by a NUL 
character, while the "-v" obviously means that the commit message will be 
printed at all, and the "-p" means that the diff gets printed as a patch.

But the diff obviously gets printed with newlines (as does any newlines
_within_ the commit message), not with lines terminated by NUL's.

So "--name-only-z" no longer exists. It's "-z --name-only" (in any order,
quite naturally).

			Linus

Re: [PATCH] git-diff-*: Allow "--name-only -z" as alias for "--name-only-z"

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 14 Jul 2005, Junio C Hamano wrote:
>> 
>> That said, I have been hating that diff options parsing for
>> quite a while, and I've been thinking about cleaning it up along
>> the lines I'll outline here, but have not done anything about
>> it.  Care to help me out?
>
> I didn't do what you suggested, but I _did_ split the "format" up into 
> "format + line_termination", which in my opinion cleaned up part of it a 
> _lot_.

Agreed 100%.  Regardless of the further cleanup I suggested,
what you did was something I should have done in the first
place.  Thanks for the cleanup.