threads / patch / 14776

patchgit-name-rev: allow --name-only in combination with --stdin

Subject: [PATCH] git-name-rev: allow --name-only in combination with --stdin

## tl;dr

8 messages between Jul 31, 2008 and Aug 3, 2008. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Pieter de Bie· Jul 31, 2008, 13:20 UTC · lore
Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
	Or was there a specific reason not to allow this?
 Documentation/git-name-rev.txt |    3 +--
 builtin-name-rev.c             |   10 ++++++++--
 2 files changed, 9 insertions(+), 4 deletions(-)
Show changes to 2 files +9 −4

Documentation/git-name-rev.txt, builtin-name-rev.c

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 6e77ab1..c8a72dd 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -38,8 +38,7 @@ OPTIONS
 	Instead of printing both the SHA-1 and the name, print only
 	the name.  If given with --tags the usual tag prefix of
 	"tags/" is also omitted from the name, matching the output
-	of 'git-describe' more closely.  This option
-	cannot be combined with --stdin.
+	of 'git-describe' more closely.
 
 --no-undefined::
 	Die with error code != 0 when a reference is undefined,
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 85612c4..0536af4 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 					if (!name)
 						continue;
 
-					fwrite(p_start, p - p_start + 1, 1, stdout);
-					printf(" (%s)", name);
+					if (data.name_only) {
+						fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
+						printf(name);
+					}
+					else {
+						fwrite(p_start, p - p_start + 1, 1, stdout);
+						printf(" (%s)", name);
+					}
 					p_start = p + 1;
 				}
 			}

-- 
1.6.0.rc1.163.gc85c5.dirty
Junio C Hamano· Aug 1, 2008, 07:23 UTC · re: Pieter de Bie · lore

Re: [PATCH] git-name-rev: allow --name-only in combination with --stdin

Pieter de Bie <pdebie@ai.rug.nl> writes:
> Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
> ---
>
> 	Or was there a specific reason not to allow this?
I'll let Dscho answer that one.
Show 21 quoted lines
> diff --git a/builtin-name-rev.c b/builtin-name-rev.c
> index 85612c4..0536af4 100644
> --- a/builtin-name-rev.c
> +++ b/builtin-name-rev.c
> @@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>  					if (!name)
>  						continue;
>  
> -					fwrite(p_start, p - p_start + 1, 1, stdout);
> -					printf(" (%s)", name);
> +					if (data.name_only) {
> +						fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
> +						printf(name);
> +					}
> +					else {
> +						fwrite(p_start, p - p_start + 1, 1, stdout);
> +						printf(" (%s)", name);
> +					}
>  					p_start = p + 1;
>  				}
>  			}

Is it just me to find that this part is getting indented too deeply to be readable?

Johannes Schindelin· Aug 1, 2008, 10:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-name-rev: allow --name-only in combination with --stdin

Hi,
On Fri, 1 Aug 2008, Junio C Hamano wrote:
Show 8 quoted lines
> Pieter de Bie <pdebie@ai.rug.nl> writes:
> 
> > Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
> > ---
> >
> > 	Or was there a specific reason not to allow this?
> 
> I'll let Dscho answer that one.
... who let's Shawn answer that one.
Show 24 quoted lines
> > diff --git a/builtin-name-rev.c b/builtin-name-rev.c
> > index 85612c4..0536af4 100644
> > --- a/builtin-name-rev.c
> > +++ b/builtin-name-rev.c
> > @@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> >  					if (!name)
> >  						continue;
> >  
> > -					fwrite(p_start, p - p_start + 1, 1, stdout);
> > -					printf(" (%s)", name);
> > +					if (data.name_only) {
> > +						fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
> > +						printf(name);
> > +					}
> > +					else {
> > +						fwrite(p_start, p - p_start + 1, 1, stdout);
> > +						printf(" (%s)", name);
> > +					}
> >  					p_start = p + 1;
> >  				}
> >  			}
> 
> Is it just me to find that this part is getting indented too deeply to be
> readable?
No.

Ciao, Dscho

Pieter de Bie· Aug 1, 2008, 11:16 UTC · re: Johannes Schindelin · lore

[PATCH] builtin-name-rev: refactor stdin handling to its own function

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
    On 1 aug 2008, at 09:23, Junio C Hamano wrote:
    >Is it just me to find that this part is getting indented too deeply to be
    >readable?
    How about something like this then?
 builtin-name-rev.c |   93 ++++++++++++++++++++++++++++------------------------
 1 files changed, 50 insertions(+), 43 deletions(-)
Show changes to builtin-name-rev.c +50 −43
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 0536af4..057172d 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -176,6 +176,52 @@ static char const * const name_rev_usage[] = {
 	NULL
 };
 
+static void handle_stdin_line(char *p_start, int name_only)
+{
+	int forty = 0;
+	char *p;
+	for (p = p_start; *p; p++) {
+#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
+		if (!ishex(*p))
+			forty = 0;
+		else if (++forty == 40 && !ishex(*(p+1))) {
+			unsigned char sha1[40];
+			const char *name = NULL;
+			char c = *(p+1);
+
+			forty = 0;
+
+			*(p+1) = 0;
+			if (!get_sha1(p - 39, sha1)) {
+				struct object *o =
+					lookup_object(sha1);
+				if (o)
+					name = get_rev_name(o);
+			}
+			*(p+1) = c;
+
+			if (!name)
+				continue;
+
+			if (name_only) {
+				fwrite(p_start, p - p_start + 1 - 40,
+					1, stdout);sssss
+				printf(name);
+			}
+			else {
+				fwrite(p_start, p - p_start + 1, 1, stdout);
+				printf(" (%s)", name);
+			}
+			p_start = p + 1;
+		}
+	}
+
+	/* flush */
+	if (p != p_start)
+		fwrite(p_start, p - p_start, 1, stdout);
+
+}
+
 int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = { 0, 0, NULL };
@@ -234,53 +280,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
 	if (transform_stdin) {
 		char buffer[2048];
-		char *p, *p_start;
+		char *p_start;
 
 		while (!feof(stdin)) {
-			int forty = 0;
-			p = fgets(buffer, sizeof(buffer), stdin);
-			if (!p)
+			p_start = fgets(buffer, sizeof(buffer), stdin);
+			if (!p_start)
 				break;
 
-			for (p_start = p; *p; p++) {
-#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
-				if (!ishex(*p))
-					forty = 0;
-				else if (++forty == 40 &&
-						!ishex(*(p+1))) {
-					unsigned char sha1[40];
-					const char *name = NULL;
-					char c = *(p+1);
-
-					forty = 0;
-
-					*(p+1) = 0;
-					if (!get_sha1(p - 39, sha1)) {
-						struct object *o =
-							lookup_object(sha1);
-						if (o)
-							name = get_rev_name(o);
-					}
-					*(p+1) = c;
-
-					if (!name)
-						continue;
-
-					if (data.name_only) {
-						fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
-						printf(name);
-					}
-					else {
-						fwrite(p_start, p - p_start + 1, 1, stdout);
-						printf(" (%s)", name);
-					}
-					p_start = p + 1;
-				}
-			}
-
-			/* flush */
-			if (p_start != p)
-				fwrite(p_start, p - p_start, 1, stdout);
+			handle_stdin_line(p_start, data.name_only);
 		}
 	} else if (all) {
 		int i, max;

-- 
1.6.0.rc1.214.g5f0bd
Junio C Hamano· Aug 1, 2008, 19:07 UTC · re: Pieter de Bie · lore

Re: [PATCH] builtin-name-rev: refactor stdin handling to its own function

Pieter de Bie <pdebie@ai.rug.nl> writes:
Show 8 quoted lines
> Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
> ---
>
>     On 1 aug 2008, at 09:23, Junio C Hamano wrote:
>     >Is it just me to find that this part is getting indented too deeply to be
>     >readable?
>
>     How about something like this then?

Much nicer, except that this refactoring should come first and then a new feature. Dropping those extra five 's' so that it would compile would be a nice bonus as well ;-)

Show 7 quoted lines
> ...
> +
> +			if (name_only) {
> +				fwrite(p_start, p - p_start + 1 - 40,
> +					1, stdout);sssss
> +				printf(name);
> +			}
Junio C Hamano· Aug 2, 2008, 18:12 UTC · re: Pieter de Bie · lore

Re: [PATCH] git-name-rev: allow --name-only in combination with --stdin

Pieter de Bie <pdebie@ai.rug.nl> writes:
> 	Or was there a specific reason not to allow this?

The --name-only option to the command was added in 1.5.3 cycle, but the original focus was only to support describe. I do not see any fundamental reason to disallow it --- we could even say this is a bug.

I've applied the "split single line processing to a separate function" and then this patch to 'maint' as a bugfix.

René Scharfe· Aug 3, 2008, 13:44 UTC · re: Junio C Hamano · lore

[PATCH] git-name-rev: don't use printf without format

printf() without an explicit format string is not a good coding practise, unless the printed string is guaranteed to not contain percent signs. While fixing this, we might as well combine the calls to fwrite() and printf().

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin-name-rev.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)
Show changes to builtin-name-rev.c +5 −7
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 7055ac3..08c8aab 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -189,6 +189,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
 			unsigned char sha1[40];
 			const char *name = NULL;
 			char c = *(p+1);
+			int p_len = p - p_start + 1;
 
 			forty = 0;
 
@@ -204,13 +205,10 @@ static void name_rev_line(char *p, struct name_ref_data *data)
 			if (!name)
 				continue;
 
-			if (data->name_only) {
-				fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
-				printf(name);
-			} else {
-				fwrite(p_start, p - p_start + 1, 1, stdout);
-				printf(" (%s)", name);
-			}
+			if (data->name_only)
+				printf("%.*s%s", p_len - 40, p_start, name);
+			else
+				printf("%.*s (%s)", p_len, p_start, name);
 			p_start = p + 1;
 		}
 	}
Junio C Hamano· Aug 3, 2008, 20:44 UTC · re: René Scharfe · lore

Re: [PATCH] git-name-rev: don't use printf without format

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> printf() without an explicit format string is not a good coding practise,
> unless the printed string is guaranteed to not contain percent signs.  While
> fixing this, we might as well combine the calls to fwrite() and printf().

Good catch; I should have caught it when I applied the "split overlong function" patch, but I apparently was blind.

Thanks.

← back to recent threads