[PATCH] TIG: Fix to reinstate proper operation with no arguments

[PATCH] TIG: Fix to reinstate proper operation with no arguments

[Drew Northup]

Since c7d67ab running "tig" with no options has failed with the
error "tig: No revisions match the given arguments." This was due
to a change in how the arguments for the back-end git call was
being constructed. This change caused the blank field left in
place of "(encoding_arg)" when it is empty to not overwrite
"buf" which then caused the value in "buf" to be copied into
dst_argv twice. The resulting git command failed if there was no
available revision named "log" as shown in the trace.

>From the TIG_TRACE log:
git log log --no-color --pretty=raw --parents --parents --
fatal: bad revision 'log'

This fix works by teaching tig that when it is supplied with a
blank field in the source argument buffer that it should skip
over that field and continue instead of copying the previous
field value into the destination buffer a second time.

github issue # 167

Signed-off-by: Drew Northup <n1xim.email@gmail.com>
---

This should apply cleanly to the tig public master whether the
mkstemps() patch I wrote has been applied or not.

 tig.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tig.c b/tig.c
index ba9ba98..1016cfe 100644
--- a/tig.c
+++ b/tig.c
@@ -3105,10 +3105,11 @@ static bool
 format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
 {
 	format->bufpos = 0;
+	int len = 0;
 
 	while (arg) {
 		char *next = strstr(arg, "%(");
-		int len = next ? next - arg : strlen(arg);
+		len = next ? next - arg : strlen(arg);
 
 		if (len && !string_format_from(format->buf, &format->bufpos, "%.*s", len, arg))
 			return FALSE;
@@ -3119,7 +3120,11 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
 		arg = next ? strchr(next, ')') + 1 : NULL;
 	}
 
-	return argv_append(dst_argv, format->buf);
+	if(len){
+		return argv_append(dst_argv, format->buf);
+	} else {
+		return TRUE;
+	}
 }
 
 static bool
-- 
1.8.0

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

[Drew Northup]

Somehow this patch breaks the main view to not open the correct commit 
in diff view when <enter> is pressed. Back to the debugger...

On 07/18/2013 12:51 AM, Drew Northup wrote:
> Since c7d67ab running "tig" with no options has failed with the
> error "tig: No revisions match the given arguments." This was due
> to a change in how the arguments for the back-end git call was
> being constructed. This change caused the blank field left in
> place of "(encoding_arg)" when it is empty to not overwrite
> "buf" which then caused the value in "buf" to be copied into
> dst_argv twice. The resulting git command failed if there was no
> available revision named "log" as shown in the trace.
>
>  From the TIG_TRACE log:
> git log log --no-color --pretty=raw --parents --parents --
> fatal: bad revision 'log'
>
> This fix works by teaching tig that when it is supplied with a
> blank field in the source argument buffer that it should skip
> over that field and continue instead of copying the previous
> field value into the destination buffer a second time.
>
> github issue # 167
>
> Signed-off-by: Drew Northup<n1xim.email@gmail.com>
> ---
>
> This should apply cleanly to the tig public master whether the
> mkstemps() patch I wrote has been applied or not.
>
>   tig.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tig.c b/tig.c
> index ba9ba98..1016cfe 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3105,10 +3105,11 @@ static bool
>   format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
>   {
>   	format->bufpos = 0;
> +	int len = 0;
>
>   	while (arg) {
>   		char *next = strstr(arg, "%(");
> -		int len = next ? next - arg : strlen(arg);
> +		len = next ? next - arg : strlen(arg);
>
>   		if (len&&  !string_format_from(format->buf,&format->bufpos, "%.*s", len, arg))
>   			return FALSE;
> @@ -3119,7 +3120,11 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
>   		arg = next ? strchr(next, ')') + 1 : NULL;
>   	}
>
> -	return argv_append(dst_argv, format->buf);
> +	if(len){
> +		return argv_append(dst_argv, format->buf);
> +	} else {
> +		return TRUE;
> +	}
>   }
>
>   static bool


-- 
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

[Jonas Fonseca]

On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup <n1xim.email@gmail.com> wrote:
>
> Somehow this patch breaks the main view to not open the correct commit in diff view when <enter> is pressed. Back to the debugger...

Does this (possibly white-space damaged) patch work for you?

diff --git a/tig.c b/tig.c
index ba9ba98..74a2928 100644
--- a/tig.c
+++ b/tig.c
@@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
const char *name)
 static bool
 format_append_arg(struct format_context *format, const char
***dst_argv, const char *arg)
 {
- format->bufpos = 0;
+ format->buf[0] = format->bufpos = 0;

  while (arg) {
  char *next = strstr(arg, "%(");


--
Jonas Fonseca

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

[Drew Northup]

On Fri, Jul 19, 2013 at 12:07 AM, Jonas Fonseca <fonseca@diku.dk> wrote:
> On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup <n1xim.email@gmail.com> wrote:
>>
>> Somehow this patch breaks the main view to not open the correct commit in diff view when <enter> is pressed. Back to the debugger...
>
> Does this (possibly white-space damaged) patch work for you?

I did look back at that sort of fix, but I kept getting compiler
warnings about a pointer conversion. Yes, it does work (I was actually
initializing the whole buffer to NULL), but I'd prefer doing so
without the implicit int* conversion warnings.

> diff --git a/tig.c b/tig.c
> index ba9ba98..74a2928 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
> const char *name)
>  static bool
>  format_append_arg(struct format_context *format, const char
> ***dst_argv, const char *arg)
>  {
> - format->bufpos = 0;
> + format->buf[0] = format->bufpos = 0;
>
>   while (arg) {
>   char *next = strstr(arg, "%(");

--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

[Drew Northup]

On 07/19/2013 12:07 AM, Jonas Fonseca wrote:
> On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup<n1xim.email@gmail.com>  wrote:
>>
>> Somehow this patch breaks the main view to not open the correct commit in diff view when<enter>  is pressed. Back to the debugger...
>
> Does this (possibly white-space damaged) patch work for you?

It does, but I prefer to actually fix the problem this highlights and 
not to paper it over. Patch V2 will be on the way shortly.

> diff --git a/tig.c b/tig.c
> index ba9ba98..74a2928 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
> const char *name)
>   static bool
>   format_append_arg(struct format_context *format, const char
> ***dst_argv, const char *arg)
>   {
> - format->bufpos = 0;
> + format->buf[0] = format->bufpos = 0;
>
>    while (arg) {
>    char *next = strstr(arg, "%(");
>
>
> --
> Jonas Fonseca


-- 
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

[PATCH V2] TIG: Fix to reinstate proper operation with no arguments

[Drew Northup]

Since c7d67ab running "tig" with no options has failed with the
error "tig: No revisions match the given arguments." This was due
to a change in how the arguments for the back-end git call was
being constructed. This change caused the blank field left in
place of "(encoding_arg)" when it is empty to not overwrite
"buf" which then caused the value in "buf" to be copied into
dst_argv twice. The resulting git command failed if there was no
available revision named "log" as shown in the trace.

>From the TIG_TRACE log:
git log log --no-color --pretty=raw --parents --parents --
fatal: bad revision 'log'

This fix works by properly and fully initializing "buf" before each use.

github issue # 167

Helped-by: Jonas Fonseca <fonseca@diku.dk>
Signed-off-by: Drew Northup <n1xim.email@gmail.com>
---

This should apply cleanly to the tig public master whether the
mkstemps() patch I wrote has been applied or not. (As of the time I
am sending this for review I haven't yet applied the latest fix to
my mkstemps() patch that has been submitted for git itself.)

This time, knowing for sure now that format->buf is not being used in
the extant code path for any other purpose, I went ahead and
initialized the whole thing to be sure that we don't find any other
ghosts hiding in that buffer between uses. Just initializing the
first byte fixes the near term problem but does not prevent the buffer
initialization issue that this bug highlighted from rising up again
later on.

 tig.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tig.c b/tig.c
index ba9ba98..c65bc43 100644
--- a/tig.c
+++ b/tig.c
@@ -3104,8 +3104,12 @@ format_expand_arg(struct format_context *format, const char *name)
 static bool
 format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
 {
+	int i;
 	format->bufpos = 0;
 
+	for (i = 0; i < SIZEOF_STR; i++)
+		format->buf[i] = 0;
+
 	while (arg) {
 		char *next = strstr(arg, "%(");
 		int len = next ? next - arg : strlen(arg);
-- 
1.8.0

Re: [PATCH V2] TIG: Fix to reinstate proper operation with no arguments

[Jonas Fonseca]

On Wed, Jul 24, 2013 at 8:50 AM, Drew Northup <n1xim.email@gmail.com> wrote:
> This time, knowing for sure now that format->buf is not being used in
> the extant code path for any other purpose, I went ahead and
> initialized the whole thing to be sure that we don't find any other
> ghosts hiding in that buffer between uses. Just initializing the
> first byte fixes the near term problem but does not prevent the buffer
> initialization issue that this bug highlighted from rising up again
> later on.

Thanks applied with minor tidyup.

> diff --git a/tig.c b/tig.c
> index ba9ba98..c65bc43 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,8 +3104,12 @@ format_expand_arg(struct format_context *format, const char *name)
>  static bool
>  format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
>  {
> +       int i;

Added space after the declaration.

>         format->bufpos = 0;
>
> +       for (i = 0; i < SIZEOF_STR; i++)

Changed this to use sizeof(format->buf) instead.

> +               format->buf[i] = 0;
> +
>         while (arg) {
>                 char *next = strstr(arg, "%(");
>                 int len = next ? next - arg : strlen(arg);
> --
> 1.8.0
>

-- 
Jonas Fonseca