git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Pratyush Yadav <me@yadavpratyush.com>
To: Serg Tereshchenko <serg.partizan@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v2] style(git-gui): Fix mixed tabs & spaces; Prefer tabs.
Date: Wed, 9 Sep 2020 10:21:08 +0530	[thread overview]
Message-ID: <20200909045108.j5ovnbk35cmghgcz@yadavpratyush.com> (raw)
In-Reply-To: <20200822222431.35027-1-serg.partizan@gmail.com>

Hi Serg,

Thanks for the patch.

> Subject: [PATCH v2] style(git-gui): Fix mixed tabs & spaces; Prefer tabs.

s/style(git-gui)/git-gui/

On 23/08/20 01:24AM, Serg Tereshchenko wrote:
> Here is cleaned up version of the patch.

A line like this should not be a part of the actual commit message. It 
is some extra commentary for the reviewers. The way you have submitted 
this patch, this line would end up in the commit message. The usual way 
of doing something like this is to use "scissors".

You can add this line:

--- 8< ---

This "scissors" line can tell git-am (when the option --scissors) to 
ignore everything above it. That makes my job a little bit easier when 
applying your patch :-)
 
> Spaces replaced with tabs when possible. In some cases just replacing
> spaces with tabs would break readability, so it was left as it is.
> 
> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  git-gui.sh | 154 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 77 insertions(+), 77 deletions(-)

Most of the changes here look good to me. One comment below.

> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 49bd86e..847c3c9 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -947,15 +947,15 @@ if {![regsub {^git version } $_git_version {} _git_version]} {
>  }
>  
>  proc get_trimmed_version {s} {
> -    set r {}
> -    foreach x [split $s -._] {
> -        if {[string is integer -strict $x]} {
> -            lappend r $x
> -        } else {
> -            break
> -        }
> -    }
> -    return [join $r .]
> +	set r {}
> +	foreach x [split $s -._] {
> +		if {[string is integer -strict $x]} {
> +			lappend r $x
> +		} else {
> +			break
> +		}
> +	}
> +	return [join $r .]
>  }
>  set _real_git_version $_git_version
>  set _git_version [get_trimmed_version $_git_version]
> @@ -967,7 +967,7 @@ if {![regexp {^[1-9]+(\.[0-9]+)+$} $_git_version]} {
>  		-type yesno \
>  		-default no \
>  		-title "[appname]: warning" \
> -		 -message [mc "Git version cannot be determined.
> +		-message [mc "Git version cannot be determined.
>  
>  %s claims it is version '%s'.
>  
> @@ -1181,44 +1181,44 @@ enable_option transport
>  disable_option bare
>  
>  switch -- $subcommand {
> -browser -
> -blame {
> -	enable_option bare
> -
> -	disable_option multicommit
> -	disable_option branch
> -	disable_option transport
> -}
> -citool {
> -	enable_option singlecommit
> -	enable_option retcode
> -
> -	disable_option multicommit
> -	disable_option branch
> -	disable_option transport
> +	browser -
> +	blame {
> +		enable_option bare
> +
> +		disable_option multicommit
> +		disable_option branch
> +		disable_option transport
> +	}
> +	citool {
> +		enable_option singlecommit
> +		enable_option retcode
> +
> +		disable_option multicommit
> +		disable_option branch
> +		disable_option transport
> +
> +		while {[llength $argv] > 0} {
> +			set a [lindex $argv 0]
> +			switch -- $a {
> +				--amend {
> +					enable_option initialamend
> +				}
> +				--nocommit {
> +					enable_option nocommit
> +					enable_option nocommitmsg
> +				}
> +				--commitmsg {
> +					disable_option nocommitmsg
> +				}
> +				default {
> +					break
> +				}
> +			}
>  
> -	while {[llength $argv] > 0} {
> -		set a [lindex $argv 0]
> -		switch -- $a {
> -		--amend {
> -			enable_option initialamend
> -		}
> -		--nocommit {
> -			enable_option nocommit
> -			enable_option nocommitmsg
> +			set argv [lrange $argv 1 end]
>  		}
> -		--commitmsg {
> -			disable_option nocommitmsg
> -		}
> -		default {
> -			break
> -		}
> -		}
> -
> -		set argv [lrange $argv 1 end]
>  	}
>  }
> -}

I'm not on board with this entire hunk. In many C projects (like Linux, 
Git, etc) the "switch" and the "case" are on the same indent level. I 
can see instances of this in almost every switch-case block in 
git-gui.sh as well. We should stick to the local convention here and 
drop this hunk.

I can make these changes locally and merge them so no need to re-roll... 
unless you have any counter points that is.

-- 
Regards,
Pratyush Yadav

  reply	other threads:[~2020-09-09  4:51 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-22 10:56 style(git-gui): Fix mixed tabs & spaces; Always use tabs Serg Tereshchenko
2020-08-22 19:26 ` Junio C Hamano
2020-08-22 22:24   ` [PATCH v2] style(git-gui): Fix mixed tabs & spaces; Prefer tabs Serg Tereshchenko
2020-09-09  4:51     ` Pratyush Yadav [this message]
2020-09-09 13:01       ` Serg Tereshchenko
2020-09-22  9:52         ` Pratyush Yadav

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200909045108.j5ovnbk35cmghgcz@yadavpratyush.com \
    --to=me@yadavpratyush.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=serg.partizan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).