[PATCH 0/6] Trivial cleanups and fixes

[PATCH 0/6] Trivial cleanups and fixes

[Felipe Contreras]

Felipe Contreras (6):
  reset: trivial refactoring
  branch: trivial style fix
  rebase: trivial style fixes
  reset: trivial style cleanup
  add: trivial style cleanup
  pull: trivial cleanup

 branch.c        |  2 +-
 builtin/add.c   | 10 +++++-----
 builtin/reset.c | 11 ++++-------
 git-pull.sh     |  3 +--
 git-rebase.sh   |  4 ++--
 5 files changed, 13 insertions(+), 17 deletions(-)

-- 
1.8.4-fc

[PATCH 1/6] reset: trivial refactoring

[Felipe Contreras]

After commit 3fde386 (reset [--mixed]: use diff-based reset whether or
not pathspec was given), some code can be moved to the 'reset_type ==
MIXED' check.

Let's move the code that is specific to MIXED.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/reset.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index afa6e02..225e3f1 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -326,8 +326,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 		int newfd = hold_locked_index(lock, 1);
 		if (reset_type == MIXED) {
+			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
 			if (read_from_tree(pathspec, sha1))
 				return 1;
+			refresh_index(&the_index, flags, NULL, NULL,
+				      _("Unstaged changes after reset:"));
 		} else {
 			int err = reset_index(sha1, reset_type, quiet);
 			if (reset_type == KEEP && !err)
@@ -336,12 +339,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				die(_("Could not reset index file to revision '%s'."), rev);
 		}
 
-		if (reset_type == MIXED) { /* Report what has not been updated. */
-			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-			refresh_index(&the_index, flags, NULL, NULL,
-				      _("Unstaged changes after reset:"));
-		}
-
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(lock))
 			die(_("Could not write new index file."));
-- 
1.8.4-fc

[PATCH 2/6] branch: trivial style fix

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/branch.c b/branch.c
index c5c6984..546c4b4 100644
--- a/branch.c
+++ b/branch.c
@@ -307,7 +307,7 @@ void create_branch(const char *head,
 			 start_name);
 
 	if (real_ref && track)
-		setup_tracking(ref.buf+11, real_ref, track, quiet);
+		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
 	if (!dont_change_ref)
 		if (write_ref_sha1(lock, sha1, msg) < 0)
-- 
1.8.4-fc

[PATCH 3/6] rebase: trivial style fixes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8d7659a..2c02853 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -324,7 +324,7 @@ done
 test $# -gt 2 && usage
 
 if test -n "$cmd" &&
-   test "$interactive_rebase" != explicit
+	test "$interactive_rebase" != explicit
 then
 	die "$(gettext "The --exec option must be used with the --interactive option")"
 fi
@@ -486,7 +486,7 @@ case "$#" in
 	switch_to="$1"
 
 	if git show-ref --verify --quiet -- "refs/heads/$1" &&
-	   orig_head=$(git rev-parse -q --verify "refs/heads/$1")
+		orig_head=$(git rev-parse -q --verify "refs/heads/$1")
 	then
 		head_name="refs/heads/$1"
 	elif orig_head=$(git rev-parse -q --verify "$1")
-- 
1.8.4-fc

[PATCH 4/6] reset: trivial style cleanup

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 225e3f1..7e65934 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -323,7 +323,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		die_if_unmerged_cache(reset_type);
 
 	if (reset_type != SOFT) {
-		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+		struct lock_file *lock = xcalloc(1, sizeof(*lock));
 		int newfd = hold_locked_index(lock, 1);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-- 
1.8.4-fc

[PATCH 5/6] add: trivial style cleanup

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/add.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 8266a9c..a1e1e0e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -336,7 +336,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
 	if (read_cache() < 0)
-		die (_("Could not read the index"));
+		die(_("Could not read the index"));
 
 	init_revisions(&rev, prefix);
 	rev.diffopt.context = 7;
@@ -347,11 +347,11 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
 	out = open(file, O_CREAT | O_WRONLY, 0666);
 	if (out < 0)
-		die (_("Could not open '%s' for writing."), file);
+		die(_("Could not open '%s' for writing."), file);
 	rev.diffopt.file = xfdopen(out, "w");
 	rev.diffopt.close_file = 1;
 	if (run_diff_files(&rev, 0))
-		die (_("Could not write patch"));
+		die(_("Could not write patch"));
 
 	launch_editor(file, NULL, NULL);
 
@@ -364,7 +364,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	child.git_cmd = 1;
 	child.argv = apply_argv;
 	if (run_command(&child))
-		die (_("Could not apply '%s'"), file);
+		die(_("Could not apply '%s'"), file);
 
 	unlink(file);
 	free(file);
@@ -598,7 +598,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	unplug_bulk_checkin();
 
- finish:
+finish:
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))
-- 
1.8.4-fc

[PATCH 6/6] pull: trivial cleanup

[Felipe Contreras]

There's no need to remove 'refs/heads/' yet again.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-pull.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..3bdcbfd 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -166,7 +166,6 @@ error_on_no_merge_candidates () {
 		op_prep=with
 	fi
 
-	curr_branch=${curr_branch#refs/heads/}
 	upstream=$(git config "branch.$curr_branch.merge")
 	remote=$(git config "branch.$curr_branch.remote")
 
@@ -183,7 +182,7 @@ error_on_no_merge_candidates () {
 		echo "You asked to pull from the remote '$1', but did not specify"
 		echo "a branch. Because this is not the default configured remote"
 		echo "for your current branch, you must specify a branch on the command line."
-	elif [ -z "$curr_branch" -o -z "$upstream" ]; then
+	elif [ -z "$curr_branch_short" -o -z "$upstream" ]; then
 		. git-parse-remote
 		error_on_missing_default_upstream "pull" $op_type $op_prep \
 			"git pull <remote> <branch>"
-- 
1.8.4-fc

Re: [PATCH 1/6] reset: trivial refactoring

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> After commit 3fde386 (reset [--mixed]: use diff-based reset whether or
> not pathspec was given), some code can be moved to the 'reset_type ==
> MIXED' check.

Makes sense.

Re: [PATCH 2/6] branch: trivial style fix

[Junio C Hamano]

Good.  Thanks.

Re: [PATCH 5/6] add: trivial style cleanup

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/add.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 8266a9c..a1e1e0e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -336,7 +336,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  
>  	if (read_cache() < 0)
> -		die (_("Could not read the index"));
> +		die(_("Could not read the index"));
>  
>  	init_revisions(&rev, prefix);
>  	rev.diffopt.context = 7;
> @@ -347,11 +347,11 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
>  	out = open(file, O_CREAT | O_WRONLY, 0666);
>  	if (out < 0)
> -		die (_("Could not open '%s' for writing."), file);
> +		die(_("Could not open '%s' for writing."), file);
>  	rev.diffopt.file = xfdopen(out, "w");
>  	rev.diffopt.close_file = 1;
>  	if (run_diff_files(&rev, 0))
> -		die (_("Could not write patch"));
> +		die(_("Could not write patch"));
>  
>  	launch_editor(file, NULL, NULL);
>  
> @@ -364,7 +364,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  	child.git_cmd = 1;
>  	child.argv = apply_argv;
>  	if (run_command(&child))
> -		die (_("Could not apply '%s'"), file);
> +		die(_("Could not apply '%s'"), file);
>  
>  	unlink(file);
>  	free(file);

Good. These often bothered me.

Re: [PATCH 3/6] rebase: trivial style fixes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 8d7659a..2c02853 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -324,7 +324,7 @@ done
>  test $# -gt 2 && usage
>  
>  if test -n "$cmd" &&
> -   test "$interactive_rebase" != explicit
> +	test "$interactive_rebase" != explicit
>  then
>  	die "$(gettext "The --exec option must be used with the --interactive option")"
>  fi
> @@ -486,7 +486,7 @@ case "$#" in
>  	switch_to="$1"
>  
>  	if git show-ref --verify --quiet -- "refs/heads/$1" &&
> -	   orig_head=$(git rev-parse -q --verify "refs/heads/$1")
> +		orig_head=$(git rev-parse -q --verify "refs/heads/$1")
>  	then
>  		head_name="refs/heads/$1"
>  	elif orig_head=$(git rev-parse -q --verify "$1")

I am not sure about this change.  I do not personally have strong
preference on this, but it would be better to be consistent.

The style of the original we see above seems to be the one that is
consistently used in this file for conditionals that span multiple
lines.  That is, to align the beginning of subsequent lines with the
beginning of the conditional (i.e. the "g" in "git show-ref" on the
first line)---which happens to be in line with what we use in our C
sources, too.

I see there is one oddball in that file, though.

 git-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8d7659a..187793e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -422,7 +422,7 @@ then
 	case "$#" in
 	0)
 		if ! upstream_name=$(git rev-parse --symbolic-full-name \
-			--verify -q @{upstream} 2>/dev/null)
+		   --verify -q @{upstream} 2>/dev/null)
 		then
 			. git-parse-remote
 			error_on_missing_default_upstream "rebase" "rebase" \

Re: [PATCH 4/6] reset: trivial style cleanup

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/reset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 225e3f1..7e65934 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -323,7 +323,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  		die_if_unmerged_cache(reset_type);
>  
>  	if (reset_type != SOFT) {
> -		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
> +		struct lock_file *lock = xcalloc(1, sizeof(*lock));

Good.  Thanks.

Re: [PATCH 6/6] pull: trivial cleanup

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no need to remove 'refs/heads/' yet again.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-pull.sh | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index f0df41c..3bdcbfd 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -166,7 +166,6 @@ error_on_no_merge_candidates () {
>  		op_prep=with
>  	fi
>  
> -	curr_branch=${curr_branch#refs/heads/}

The code assumes that at this point $curr_branch has the result of
git symbolic-ref -q HEAD it did at the beginning, before it entered
in the command line parsing loop.  But immediately after it, the
code sets up $curr_branch_short for the value this code computes.

>  	upstream=$(git config "branch.$curr_branch.merge")
>  	remote=$(git config "branch.$curr_branch.remote")

So it appears to me that the above two lines that are not updated
would introduce a regression.  Am I missing something trivial?

Puzzled.


> @@ -183,7 +182,7 @@ error_on_no_merge_candidates () {
>  		echo "You asked to pull from the remote '$1', but did not specify"
>  		echo "a branch. Because this is not the default configured remote"
>  		echo "for your current branch, you must specify a branch on the command line."
> -	elif [ -z "$curr_branch" -o -z "$upstream" ]; then
> +	elif [ -z "$curr_branch_short" -o -z "$upstream" ]; then

If $curr_branch in the original code was (wasn't) an empty string,
then with the updated code that does not strip refs/heads/ from the
beginning of it after applying the first hunk of this patch, the
variable is (isn't) an empty string, respectively. So there is no
need for this hunk, I think.

>  		. git-parse-remote
>  		error_on_missing_default_upstream "pull" $op_type $op_prep \
>  			"git pull <remote> <branch>"

Re: [PATCH 6/6] pull: trivial cleanup

[Felipe Contreras]

On Fri, Aug 30, 2013 at 10:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> There's no need to remove 'refs/heads/' yet again.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  git-pull.sh | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/git-pull.sh b/git-pull.sh
>> index f0df41c..3bdcbfd 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -166,7 +166,6 @@ error_on_no_merge_candidates () {
>>               op_prep=with
>>       fi
>>
>> -     curr_branch=${curr_branch#refs/heads/}
>
> The code assumes that at this point $curr_branch has the result of
> git symbolic-ref -q HEAD it did at the beginning, before it entered
> in the command line parsing loop.  But immediately after it, the
> code sets up $curr_branch_short for the value this code computes.
>
>>       upstream=$(git config "branch.$curr_branch.merge")
>>       remote=$(git config "branch.$curr_branch.remote")
>
> So it appears to me that the above two lines that are not updated
> would introduce a regression.  Am I missing something trivial?

Yes, I'm not exactly sure where from which branch I got this change,
but those lines were removed. They should be updated.

-- 
Felipe Contreras

[PATCH] branch: use $curr_branch_short more

[René Scharfe]

One of the first things git-pull.sh does is setting $curr_branch to
the target of HEAD and $curr_branch_short to the same but with the
leading "refs/heads/" removed.  Use $curr_branch_short in the function
error_on_no_merge_candidates instead of removing the prefix from
$curr_branch directly.

The only other use of $curr_branch in that function doesn't have to
be replaced with $curr_branch_short because it just checks if the
string is empty.  That property is the same with or without the prefix
unless HEAD points to "refs/heads/" alone, which is invalid.

Noticed-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-pull.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..d8b2708 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -166,9 +166,8 @@ error_on_no_merge_candidates () {
 		op_prep=with
 	fi
 
-	curr_branch=${curr_branch#refs/heads/}
-	upstream=$(git config "branch.$curr_branch.merge")
-	remote=$(git config "branch.$curr_branch.remote")
+	upstream=$(git config "branch.$curr_branch_short.merge")
+	remote=$(git config "branch.$curr_branch_short.remote")
 
 	if [ $# -gt 1 ]; then
 		if [ "$rebase" = true ]; then
-- 
1.8.4

Re: [PATCH] branch: use $curr_branch_short more

[Felipe Contreras]

> Subject: branch: use $curr_branch_short more

Why? I don't think that summary explains the reason for being for this
patch, also, it starts with branch: instead of pull:

Subject: pull: trivial simplification

With that summary, people would have an easier time figuring out if
they need to read more about the patch or not.

-- 
Felipe Contreras

Re: [PATCH] branch: use $curr_branch_short more

[René Scharfe]

Am 31.08.2013 10:22, schrieb Felipe Contreras:
>> Subject: branch: use $curr_branch_short more
>
> Why? I don't think that summary explains the reason for being for this
> patch, also, it starts with branch: instead of pull:

You're right about "branch" vs. "pull".  I'll better go back to bed. ~_~

> Subject: pull: trivial simplification
>
> With that summary, people would have an easier time figuring out if
> they need to read more about the patch or not.

"trivial simplification" is too generic; we could have lots of them.  A 
summary should describe the change.  Its low complexity can be derived 
from it -- using an existing variable a bit more is not very exciting.

But I wouldn't call that patch trivial because its correctness depends 
on code outside of its shown context.

The reason for the patch isn't mentioned explicitly.  Perhaps it should 
be.  I felt that using something that's already there instead of 
recreating it is motivation alone.

René

Re: [PATCH] branch: use $curr_branch_short more

[Felipe Contreras]

On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe <l.s.r@web.de> wrote:

>> Subject: pull: trivial simplification
>>
>> With that summary, people would have an easier time figuring out if
>> they need to read more about the patch or not.
>
>
> "trivial simplification" is too generic; we could have lots of them.

No, we can have only one, otherwise it would say simplificationS.

> A summary should describe the change.

You can never fully describe the change, only the diff does that.

For example "use $curr_branch_short more" does not tell me anything
about the extent of the changes, is it used in one more place? two?
one hundred? Moreover, how exactly is it used more? Is some
refactoring needed?

And it still doesn't answer the most important question any summary
should answer: why? Why use $curr_branch_short more?

> Its low complexity can be derived from
> it -- using an existing variable a bit more is not very exciting.

You didn't say "a bit more" you said "more". And yes, the complexity
can be derived from the summary, but not from this one.

> But I wouldn't call that patch trivial because its correctness depends on
> code outside of its shown context.

Correctness is a separate question from triviality, and the
correctness can only be assessed by looking at the actual patch.

The patch can be both trivial and wrong.

> The reason for the patch isn't mentioned explicitly.  Perhaps it should be.
> I felt that using something that's already there instead of recreating it is
> motivation alone.

Why? Because it simplifies the code. That's the real answer.

-- 
Felipe Contreras

Re: [PATCH] branch: use $curr_branch_short more

[René Scharfe]

Am 31.08.2013 11:22, schrieb Felipe Contreras:
> On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe <l.s.r@web.de> wrote:
>
>>> Subject: pull: trivial simplification
>>>
>>> With that summary, people would have an easier time figuring out if
>>> they need to read more about the patch or not.
>>
>>
>> "trivial simplification" is too generic; we could have lots of them.
>
> No, we can have only one, otherwise it would say simplificationS.

I was too terse again, let me rephrase that: We could have lots of 
commits that fit the same description if we used such a generic one.

>> A summary should describe the change.
>
> You can never fully describe the change, only the diff does that.
>
> For example "use $curr_branch_short more" does not tell me anything
> about the extent of the changes, is it used in one more place? two?
> one hundred? Moreover, how exactly is it used more? Is some
> refactoring needed?
>
> And it still doesn't answer the most important question any summary
> should answer: why? Why use $curr_branch_short more?

A summary doesn't have to contain lots of details.  The what is 
important, the why can be explained in the commit message.

>> Its low complexity can be derived from
>> it -- using an existing variable a bit more is not very exciting.
>
> You didn't say "a bit more" you said "more". And yes, the complexity
> can be derived from the summary, but not from this one.
>
>> But I wouldn't call that patch trivial because its correctness depends on
>> code outside of its shown context.
>
> Correctness is a separate question from triviality, and the
> correctness can only be assessed by looking at the actual patch.
>
> The patch can be both trivial and wrong.

Probably too terse again, let's say it differently: Only a patch whose 
correctness can be judged without looking outside of the three lines of 
context it includes qualifies as trivial in my book.  The patch in 
question is not trivial because you can't see the value of 
$curr_branch_short just by looking at the diff.

>> The reason for the patch isn't mentioned explicitly.  Perhaps it should be.
>> I felt that using something that's already there instead of recreating it is
>> motivation alone.
>
> Why? Because it simplifies the code. That's the real answer.

I don't disagree.

René

Re: [PATCH] branch: use $curr_branch_short more

[Felipe Contreras]

On Sat, Aug 31, 2013 at 5:28 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 31.08.2013 11:22, schrieb Felipe Contreras:
>
>> On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe <l.s.r@web.de> wrote:
>>
>>>> Subject: pull: trivial simplification
>>>>
>>>> With that summary, people would have an easier time figuring out if
>>>> they need to read more about the patch or not.
>>>
>>>
>>>
>>> "trivial simplification" is too generic; we could have lots of them.
>>
>>
>> No, we can have only one, otherwise it would say simplificationS.
>
>
> I was too terse again, let me rephrase that: We could have lots of commits
> that fit the same description if we used such a generic one.

Yes, but they are all trivial, and they all simply the code.

>>> A summary should describe the change.
>>
>>
>> You can never fully describe the change, only the diff does that.
>>
>> For example "use $curr_branch_short more" does not tell me anything
>> about the extent of the changes, is it used in one more place? two?
>> one hundred? Moreover, how exactly is it used more? Is some
>> refactoring needed?
>>
>> And it still doesn't answer the most important question any summary
>> should answer: why? Why use $curr_branch_short more?
>
> A summary doesn't have to contain lots of details.  The what is important,
> the why can be explained in the commit message.

A summary should contain as much information that would allow me to
skip the commit message as possible.

If I can't tell from the summary if I can safely skip the commit
message, the summary is not doing a good job.

"trivial simplification" explains the "what", and the "why" at the
same time, and allows most people to skip the commit message, thus is
a good summary.

>>> Its low complexity can be derived from
>>> it -- using an existing variable a bit more is not very exciting.
>>
>>
>> You didn't say "a bit more" you said "more". And yes, the complexity
>> can be derived from the summary, but not from this one.
>>
>>> But I wouldn't call that patch trivial because its correctness depends on
>>> code outside of its shown context.
>>
>>
>> Correctness is a separate question from triviality, and the
>> correctness can only be assessed by looking at the actual patch.
>>
>> The patch can be both trivial and wrong.
>
>
> Probably too terse again, let's say it differently: Only a patch whose
> correctness can be judged without looking outside of the three lines of
> context it includes qualifies as trivial in my book.  The patch in question
> is not trivial because you can't see the value of $curr_branch_short just by
> looking at the diff.

Again, triviality and correctness are two separate different things.
The patch is trivial even if you can't judge it's correctness.

To me, what you are describing is an obvious patch, not a trivial one.
An obvious patch is so obvious that you can judge it's correctness
easily by looking at the diff, a trivial one is of little importance.

>>> The reason for the patch isn't mentioned explicitly.  Perhaps it should
>>> be.
>>> I felt that using something that's already there instead of recreating it
>>> is
>>> motivation alone.
>>
>>
>> Why? Because it simplifies the code. That's the real answer.
>
> I don't disagree.

Yet your commit message doesn't explain that anywhere.

-- 
Felipe Contreras

Re: [PATCH] branch: use $curr_branch_short more

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> Subject: branch: use $curr_branch_short more
>
> Why? I don't think that summary explains the reason for being for this
> patch, also, it starts with branch: instead of pull:
>
> Subject: pull: trivial simplification
>
> With that summary, people would have an easier time figuring out if
> they need to read more about the patch or not.

People, other than the author of the patch, use the subject line in
different ways.  It is unclear to me which usage the "trivial lets
them decide if it needs further reading" is trying to help.

 1. Those running "shortlog" to see how much impact each contributor
    had.

 2. Those viewing list of Subjects in their MUA to see which is
    worth reading and commenting on.

 3. Those viewing list of Subjects in their MUA to see which is
    worth reading and applying to their trees.

 4. Those trying to resolve conflicts during "git am -3" and "git
    merge" [*1*].

 5. Those who find that a commit broke the build after bisecting,
    and want to see what is the reasoning behind the broken change.

 6. Those who find an interesting section of the code, blamed its
    origin to a commit, and want to see what is the reasoning behind
    the broken change.

There may be others, but the above should cover most of the cases, I
think.

For 1., they may discount anything that says "trivial" as "not of
high impact".  There may be trivial but high impact changes, but I
do not know how much this mischaracterization hurts.  Probably not
that much.

For 2., they may either skip "trivial", thinking "if it is trivial,
it must be correct", or read "trivial", thinking "the author thinks
trivial, it is likely there are holes in the author's thinking".
Among the 6 patches in $gmane/233472 "Trivial cleanups and fixes",

 - 1, 2, 4 and 5 were trivially correct and good,
 - 3 was not a clear improvement,
 - 6 was wrong.

This is totally unscientific but if we take them as a representative
set of "trivial" patches, a "trivial" patch is correct only about
4.5/6 = 75% of the time.  As a consequence, people who do want to
help the project are better off reading "trivial" just like others,
so that breakages in the 25% do not go unnoticed.  The label does
not let them skip, and wastes one line that possibly could have
given them more information with non-descriptive word "trivial".

For 3., unless the author wants such a patch skipped and dropped on
the floor, "marking it 'trivial' to allow them skip" would not make
much sense, and with 75% success rate, it would be irresponsible for
those who apply patches not to read a "trivial" and blindly apply
it.  Labelling it "trivial" only wastes one line that possibly could
have given them more information with non-descriptive word "trivial".

For 4., 5., and 6., "allow them to skip" will not be in the picture,
as they already know they are interested in that particular change.
Labelling it "trivial" only wastes one line that possibly could have
given them more information with non-descriptive word "trivial"

So it seems to me that a title "trivial" only helps those running
"shortlog" to discount weight of contributor's contribution.

And the author who does not want to spend time on coming up with a
good title, but for each patch, there are a lot more readers than a
single author of that patch, so the benefit to the author does not
count much.


[Footnote]

*1* The former shows the title of the patch and the latter shows the
    branch name after a conflict marker, and it helps to have as
    much clue as possible to remind what is attempted on each side
    of the change.  It is a responsibility for the person who does
    the merge to give the branch a descriptive name, and the branch
    that queued a "trivial" patch does not have to be named
    "trivial", but the title of the patch is a large part of the
    input to naming the branch.

Re: [PATCH] branch: use $curr_branch_short more

[René Scharfe]

Am 31.08.2013 19:20, schrieb Felipe Contreras:
> A summary should contain as much information that would allow me to
> skip the commit message as possible.
>
> If I can't tell from the summary if I can safely skip the commit
> message, the summary is not doing a good job.
>
> "trivial simplification" explains the "what", and the "why" at the
> same time, and allows most people to skip the commit message, thus is
> a good summary.

No patch should be skipped on the mailing list.  As you wrote, trivial 
patches can still be wrong.

When going through the history I can see that quickly recognizing 
insubstantial changes is useful, but if I see a summary twice then in my 
mind forms a big question mark -- why did the same thing had to be done 
yet again?

As an example, both 0d12e59f (pull: replace unnecessary sed invocation) 
and bc2bbc45 (pull, rebase: simplify to use die()) could arguably have 
had the summary "trivial simplification", but I'm glad the author went 
with something a bit more specific.

I agree that some kind of tagging with keywords like "trivial", "typo" 
and so on can be helpful, though.

> Again, triviality and correctness are two separate different things.
> The patch is trivial even if you can't judge it's correctness.

Well, in terms of impact I agree.

> To me, what you are describing is an obvious patch, not a trivial one.
> An obvious patch is so obvious that you can judge it's correctness
> easily by looking at the diff, a trivial one is of little importance.

That's one definition; I think I had the mathematical notion in mind 
that calls proofs trivial which are immediately evident.

René

[PATCH] pull: use $curr_branch_short more

[René Scharfe]

One of the first things git-pull.sh does is setting $curr_branch to
the target of HEAD and $curr_branch_short to the same but with the
leading "refs/heads/" removed.  Simplify the code by using
$curr_branch_short instead of setting $curr_branch to the same
shortened value.

The only other use of $curr_branch in that function doesn't have to
be replaced with $curr_branch_short because it just checks if the
string is empty.  That property is the same with or without the prefix
unless HEAD points to "refs/heads/" alone, which is invalid.

Noticed-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Replacement patch.  Corrected the command in the summary and
changed the first part of the description slightly.

 git-pull.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index f0df41c..d8b2708 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -166,9 +166,8 @@ error_on_no_merge_candidates () {
 		op_prep=with
 	fi
 
-	curr_branch=${curr_branch#refs/heads/}
-	upstream=$(git config "branch.$curr_branch.merge")
-	remote=$(git config "branch.$curr_branch.remote")
+	upstream=$(git config "branch.$curr_branch_short.merge")
+	remote=$(git config "branch.$curr_branch_short.remote")
 
 	if [ $# -gt 1 ]; then
 		if [ "$rebase" = true ]; then
-- 1.8.4 

Re: [PATCH] branch: use $curr_branch_short more

[Felipe Contreras]

On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 31.08.2013 19:20, schrieb Felipe Contreras:
>
>> A summary should contain as much information that would allow me to
>> skip the commit message as possible.
>>
>> If I can't tell from the summary if I can safely skip the commit
>> message, the summary is not doing a good job.
>>
>> "trivial simplification" explains the "what", and the "why" at the
>> same time, and allows most people to skip the commit message, thus is
>> a good summary.
>
>
> No patch should be skipped on the mailing list.  As you wrote, trivial
> patches can still be wrong.

What patches each persons skips is each person's own decision, don't
be patronizing, if I want to skip a trivial patch, I will, I can't
read each and every patch from the dozens of mailing lists I'm
subscribed to, and there's no way each and every reader is going to
read each and every patch. They should be prioritized, and trivial
ones can be safely skipped by most people.

Here's a good example from a simple summary that I didn't write:

t2024: Fix inconsequential typos
http://article.gmane.org/gmane.comp.version-control.git/234038

Do I have to read this patch? No. I know it's inconsequential, I can
safely skip it, the key word being *I*. *Somebody* should read it,
sure, and I'm sure at least Junio would, but I don't need to.

> When going through the history I can see that quickly recognizing
> insubstantial changes is useful, but if I see a summary twice then in my
> mind forms a big question mark -- why did the same thing had to be done yet
> again?
>
> As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and
> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the
> summary "trivial simplification", but I'm glad the author went with
> something a bit more specific.

Well I wont. Because it takes long to read, and after reading I still
don't don't if they are trivial or not, I might actually have to read
the commit message, but to be honest, I would probably go right into
the diff itself, because judging from Git's history, chances are that
somebody wrote a novel there with the important bit I'm looking for
just at the end, to don't ruin the suspense.

In the first commit, it's saying it's a single invocation, so I take
it it's trivial, but what is it replaced with? Is the code simpler, is
it more complex? I don't know, I'm still not being told *why* that
patch is made. It says 'unnecessary' but why is it unnecessary?

In the second commit, it's saying it's a simplification, but I don't
know if it's just one instance, or a thousand, so I don't know what
would be the impact of the patch.

Either way I'm forced to read more just to know if it was safe for me
to skip them, at which point the whole purpose of a summary is
defeated.

>> Again, triviality and correctness are two separate different things.
>> The patch is trivial even if you can't judge it's correctness.
>
> Well, in terms of impact I agree.

No, in all terms. A patch can be:

Trivial and correct
Trivial and incorrect
Non-trivial and correct
Non-trivial and incorrect

>> To me, what you are describing is an obvious patch, not a trivial one.
>> An obvious patch is so obvious that you can judge it's correctness
>> easily by looking at the diff, a trivial one is of little importance.
>
> That's one definition; I think I had the mathematical notion in mind that
> calls proofs trivial which are immediately evident.

We are not talking about mathematics, we are talking about the English
human language.

-- 
Felipe Contreras

Re: [PATCH] branch: use $curr_branch_short more

[René Scharfe]

Am 09.09.2013 01:13, schrieb Felipe Contreras:
> On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe <l.s.r@web.de> wrote:
>> Am 31.08.2013 19:20, schrieb Felipe Contreras:
>>
>>> A summary should contain as much information that would allow me to
>>> skip the commit message as possible.
>>>
>>> If I can't tell from the summary if I can safely skip the commit
>>> message, the summary is not doing a good job.
>>>
>>> "trivial simplification" explains the "what", and the "why" at the
>>> same time, and allows most people to skip the commit message, thus is
>>> a good summary.
>>
>>
>> No patch should be skipped on the mailing list.  As you wrote, trivial
>> patches can still be wrong.
>
> What patches each persons skips is each person's own decision, don't
> be patronizing, if I want to skip a trivial patch, I will, I can't
> read each and every patch from the dozens of mailing lists I'm
> subscribed to, and there's no way each and every reader is going to
> read each and every patch. They should be prioritized, and trivial
> ones can be safely skipped by most people.

Yes, of course; someone needs to review every patch in the end, but each 
reader decides for themselves which ones to skip.  I can't keep up with 
the traffic either.

By the way, the bikeshedding phenomenon probably causes trivial patches 
to receive the most attention. :)

>> When going through the history I can see that quickly recognizing
>> insubstantial changes is useful, but if I see a summary twice then in my
>> mind forms a big question mark -- why did the same thing had to be done yet
>> again?
>>
>> As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and
>> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the
>> summary "trivial simplification", but I'm glad the author went with
>> something a bit more specific.
>
> Well I wont. Because it takes long to read, and after reading I still
> don't don't if they are trivial or not, I might actually have to read
> the commit message, but to be honest, I would probably go right into
> the diff itself, because judging from Git's history, chances are that
> somebody wrote a novel there with the important bit I'm looking for
> just at the end, to don't ruin the suspense.

Ha!  It's better to write it down at all than to miss it years later, 
when even the original author has forgotten all about it.

> In the first commit, it's saying it's a single invocation, so I take
> it it's trivial, but what is it replaced with? Is the code simpler, is
> it more complex? I don't know, I'm still not being told *why* that
> patch is made. It says 'unnecessary' but why is it unnecessary?

The sed call is unnecessary because of the fact that it can be replaced. 
:)  I'm sure I would have understood it to mean a conversion to Shell 
builtin functionality in order to avoid forking and executing an 
external command, even if I hadn't read the patch.

> In the second commit, it's saying it's a simplification, but I don't
> know if it's just one instance, or a thousand, so I don't know what
> would be the impact of the patch.
>
> Either way I'm forced to read more just to know if it was safe for me
> to skip them, at which point the whole purpose of a summary is
> defeated.

I don't see how using "trivial simplification" as the summary for both 
could have improved matters.

>>> Again, triviality and correctness are two separate different things.
>>> The patch is trivial even if you can't judge it's correctness.
>>
>> Well, in terms of impact I agree.
>
> No, in all terms. A patch can be:
>
> Trivial and correct
> Trivial and incorrect
> Non-trivial and correct
> Non-trivial and incorrect

Well, yes, but I thought your statement that "The patch is trivial" was 
referring to my actual patch which started this sub-thread.  And I meant 
that the benefit of that patch to users and programmers was small.

>>> To me, what you are describing is an obvious patch, not a trivial one.
>>> An obvious patch is so obvious that you can judge it's correctness
>>> easily by looking at the diff, a trivial one is of little importance.
>>
>> That's one definition; I think I had the mathematical notion in mind that
>> calls proofs trivial which are immediately evident.
>
> We are not talking about mathematics, we are talking about the English
> human language.

Here we were talking about source code patches.  As formal descriptions 
of changes to (mostly) programming language text they are closer to 
mathematics than English.  Using math terms when talking about them is 
not too far of a stretch.

René

Re: [PATCH] branch: use $curr_branch_short more

[Felipe Contreras]

On Sun, Sep 15, 2013 at 6:42 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 09.09.2013 01:13, schrieb Felipe Contreras:
>
>> On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe <l.s.r@web.de> wrote:
>>>
>>> Am 31.08.2013 19:20, schrieb Felipe Contreras:
>>>
>>>> A summary should contain as much information that would allow me to
>>>> skip the commit message as possible.
>>>>
>>>> If I can't tell from the summary if I can safely skip the commit
>>>> message, the summary is not doing a good job.
>>>>
>>>> "trivial simplification" explains the "what", and the "why" at the
>>>> same time, and allows most people to skip the commit message, thus is
>>>> a good summary.
>>>
>>>
>>>
>>> No patch should be skipped on the mailing list.  As you wrote, trivial
>>> patches can still be wrong.
>>
>>
>> What patches each persons skips is each person's own decision, don't
>> be patronizing, if I want to skip a trivial patch, I will, I can't
>> read each and every patch from the dozens of mailing lists I'm
>> subscribed to, and there's no way each and every reader is going to
>> read each and every patch. They should be prioritized, and trivial
>> ones can be safely skipped by most people.
>
>
> Yes, of course; someone needs to review every patch in the end, but each
> reader decides for themselves which ones to skip.  I can't keep up with the
> traffic either.
>
> By the way, the bikeshedding phenomenon probably causes trivial patches to
> receive the most attention. :)

Exactly, so if the summary of the commit message allows people to skip
a patch, that is fine.

>>> When going through the history I can see that quickly recognizing
>>> insubstantial changes is useful, but if I see a summary twice then in my
>>> mind forms a big question mark -- why did the same thing had to be done
>>> yet
>>> again?
>>>
>>> As an example, both 0d12e59f (pull: replace unnecessary sed invocation)
>>> and
>>> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had
>>> the
>>> summary "trivial simplification", but I'm glad the author went with
>>> something a bit more specific.
>>
>>
>> Well I wont. Because it takes long to read, and after reading I still
>> don't don't if they are trivial or not, I might actually have to read
>> the commit message, but to be honest, I would probably go right into
>> the diff itself, because judging from Git's history, chances are that
>> somebody wrote a novel there with the important bit I'm looking for
>> just at the end, to don't ruin the suspense.
>
>
> Ha!  It's better to write it down at all than to miss it years later, when
> even the original author has forgotten all about it.

Yes, of course, but that still means the commit message summary failed
it's purpose.

>> In the first commit, it's saying it's a single invocation, so I take
>> it it's trivial, but what is it replaced with? Is the code simpler, is
>> it more complex? I don't know, I'm still not being told *why* that
>> patch is made. It says 'unnecessary' but why is it unnecessary?
>
>
> The sed call is unnecessary because of the fact that it can be replaced. :)
> I'm sure I would have understood it to mean a conversion to Shell builtin
> functionality in order to avoid forking and executing an external command,
> even if I hadn't read the patch.

The problem is that the commit message is not for you, it's for every
reader, so the fact that you would have understood it that way is
irrelevant.

Maybe this is an exercise in the lack of empathy, and an example of
mono-culture.

>> In the second commit, it's saying it's a simplification, but I don't
>> know if it's just one instance, or a thousand, so I don't know what
>> would be the impact of the patch.
>>
>> Either way I'm forced to read more just to know if it was safe for me
>> to skip them, at which point the whole purpose of a summary is
>> defeated.
>
>
> I don't see how using "trivial simplification" as the summary for both could
> have improved matters.

It would say "trivial", which allows me and a lot of other people to
safely skip them, it's as simple as that.

>>>> Again, triviality and correctness are two separate different things.
>>>> The patch is trivial even if you can't judge it's correctness.
>>>
>>>
>>> Well, in terms of impact I agree.
>>
>>
>> No, in all terms. A patch can be:
>>
>> Trivial and correct
>> Trivial and incorrect
>> Non-trivial and correct
>> Non-trivial and incorrect
>
>
> Well, yes, but I thought your statement that "The patch is trivial" was
> referring to my actual patch which started this sub-thread.  And I meant
> that the benefit of that patch to users and programmers was small.

I don't understand what you are trying to say, the point remains; a
patch being trivial says nothing about its correctness.

>>>> To me, what you are describing is an obvious patch, not a trivial one.
>>>> An obvious patch is so obvious that you can judge it's correctness
>>>> easily by looking at the diff, a trivial one is of little importance.
>>>
>>>
>>> That's one definition; I think I had the mathematical notion in mind that
>>> calls proofs trivial which are immediately evident.
>>
>>
>> We are not talking about mathematics, we are talking about the English
>> human language.
>
>
> Here we were talking about source code patches.  As formal descriptions of
> changes to (mostly) programming language text they are closer to mathematics
> than English.  Using math terms when talking about them is not too far of a
> stretch.

No, we are not. Commit messages have nothing formal about them, they
are human oriented and colloquial.

-- 
Felipe Contreras