{"thread":{"id":"34811","subject":"[PATCH 0/6] Trivial cleanups and fixes","startedAt":"2013-08-30T21:56:44Z","lastAt":"2013-09-15T13:02:31Z","messageCount":26,"participants":["Felipe Contreras","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"226372","messageId":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":null,"subject":"[PATCH 0/6] Trivial cleanups and fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:44Z","receivedAt":"2013-08-30T21:56:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras (6):\n reset: trivial refactoring\n branch: trivial style fix\n rebase: trivial style fixes\n reset: trivial style cleanup\n add: trivial style cleanup\n pull: trivial cleanup\n\n branch.c | 2 +-\n builtin/add.c | 10 +++++-----\n builtin/reset.c | 11 ++++-------\n git-pull.sh | 3 +--\n git-rebase.sh | 4 ++--\n 5 files changed, 13 insertions(+), 17 deletions(-)\n\n-- \n1.8.4-fc\n"},{"id":"226377","messageId":"1377899810-1818-2-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/6] reset: trivial refactoring","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:45Z","receivedAt":"2013-08-30T21:56:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"After commit 3fde386 (reset [--mixed]: use diff-based reset whether or\nnot pathspec was given), some code can be moved to the 'reset_type ==\nMIXED' check.\n\nLet's move the code that is specific to MIXED.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/reset.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex afa6e02..225e3f1 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -326,8 +326,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \t\tint newfd = hold_locked_index(lock, 1);\n \t\tif (reset_type == MIXED) {\n+\t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n \t\t\tif (read_from_tree(pathspec, sha1))\n \t\t\t\treturn 1;\n+\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n+\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t} else {\n \t\t\tint err = reset_index(sha1, reset_type, quiet);\n \t\t\tif (reset_type == KEEP && !err)\n@@ -336,12 +339,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\tdie(_(\"Could not reset index file to revision '%s'.\"), rev);\n \t\t}\n \n-\t\tif (reset_type == MIXED) { /* Report what has not been updated. */\n-\t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n-\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n-\t\t\t\t _(\"Unstaged changes after reset:\"));\n-\t\t}\n-\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t commit_locked_index(lock))\n \t\t\tdie(_(\"Could not write new index file.\"));\n-- \n1.8.4-fc\n"},{"id":"226374","messageId":"1377899810-1818-3-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/6] branch: trivial style fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:46Z","receivedAt":"2013-08-30T21:56:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex c5c6984..546c4b4 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -307,7 +307,7 @@ void create_branch(const char *head,\n \t\t\t start_name);\n \n \tif (real_ref && track)\n-\t\tsetup_tracking(ref.buf+11, real_ref, track, quiet);\n+\t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n \n \tif (!dont_change_ref)\n \t\tif (write_ref_sha1(lock, sha1, msg) < 0)\n-- \n1.8.4-fc\n"},{"id":"226373","messageId":"1377899810-1818-4-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/6] rebase: trivial style fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:47Z","receivedAt":"2013-08-30T21:56:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n git-rebase.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 8d7659a..2c02853 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -324,7 +324,7 @@ done\n test $# -gt 2 && usage\n \n if test -n \"$cmd\" &&\n- test \"$interactive_rebase\" != explicit\n+\ttest \"$interactive_rebase\" != explicit\n then\n \tdie \"$(gettext \"The --exec option must be used with the --interactive option\")\"\n fi\n@@ -486,7 +486,7 @@ case \"$#\" in\n \tswitch_to=\"$1\"\n \n \tif git show-ref --verify --quiet -- \"refs/heads/$1\" &&\n-\t orig_head=$(git rev-parse -q --verify \"refs/heads/$1\")\n+\t\torig_head=$(git rev-parse -q --verify \"refs/heads/$1\")\n \tthen\n \t\thead_name=\"refs/heads/$1\"\n \telif orig_head=$(git rev-parse -q --verify \"$1\")\n-- \n1.8.4-fc\n"},{"id":"226378","messageId":"1377899810-1818-5-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/6] reset: trivial style cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:48Z","receivedAt":"2013-08-30T21:56:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n builtin/reset.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 225e3f1..7e65934 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -323,7 +323,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\tdie_if_unmerged_cache(reset_type);\n \n \tif (reset_type != SOFT) {\n-\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\t\tstruct lock_file *lock = xcalloc(1, sizeof(*lock));\n \t\tint newfd = hold_locked_index(lock, 1);\n \t\tif (reset_type == MIXED) {\n \t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n-- \n1.8.4-fc\n"},{"id":"226375","messageId":"1377899810-1818-6-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 5/6] add: trivial style cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:49Z","receivedAt":"2013-08-30T21:56:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n builtin/add.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 8266a9c..a1e1e0e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -336,7 +336,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \n \tif (read_cache() < 0)\n-\t\tdie (_(\"Could not read the index\"));\n+\t\tdie(_(\"Could not read the index\"));\n \n \tinit_revisions(&rev, prefix);\n \trev.diffopt.context = 7;\n@@ -347,11 +347,11 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n \tout = open(file, O_CREAT | O_WRONLY, 0666);\n \tif (out < 0)\n-\t\tdie (_(\"Could not open '%s' for writing.\"), file);\n+\t\tdie(_(\"Could not open '%s' for writing.\"), file);\n \trev.diffopt.file = xfdopen(out, \"w\");\n \trev.diffopt.close_file = 1;\n \tif (run_diff_files(&rev, 0))\n-\t\tdie (_(\"Could not write patch\"));\n+\t\tdie(_(\"Could not write patch\"));\n \n \tlaunch_editor(file, NULL, NULL);\n \n@@ -364,7 +364,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tchild.git_cmd = 1;\n \tchild.argv = apply_argv;\n \tif (run_command(&child))\n-\t\tdie (_(\"Could not apply '%s'\"), file);\n+\t\tdie(_(\"Could not apply '%s'\"), file);\n \n \tunlink(file);\n \tfree(file);\n@@ -598,7 +598,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tunplug_bulk_checkin();\n \n- finish:\n+finish:\n \tif (active_cache_changed) {\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t commit_locked_index(&lock_file))\n-- \n1.8.4-fc\n"},{"id":"226376","messageId":"1377899810-1818-7-git-send-email-felipe.contreras@gmail.com","threadId":"34811","inReplyTo":"1377899810-1818-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 6/6] pull: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-30T21:56:50Z","receivedAt":"2013-08-30T21:56:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to remove 'refs/heads/' yet again.\n\nSigned-off-by: Felipe Contreras \n---\n git-pull.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex f0df41c..3bdcbfd 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -166,7 +166,6 @@ error_on_no_merge_candidates () {\n \t\top_prep=with\n \tfi\n \n-\tcurr_branch=${curr_branch#refs/heads/}\n \tupstream=$(git config \"branch.$curr_branch.merge\")\n \tremote=$(git config \"branch.$curr_branch.remote\")\n \n@@ -183,7 +182,7 @@ error_on_no_merge_candidates () {\n \t\techo \"You asked to pull from the remote '$1', but did not specify\"\n \t\techo \"a branch. Because this is not the default configured remote\"\n \t\techo \"for your current branch, you must specify a branch on the command line.\"\n-\telif [ -z \"$curr_branch\" -o -z \"$upstream\" ]; then\n+\telif [ -z \"$curr_branch_short\" -o -z \"$upstream\" ]; then\n \t\t. git-parse-remote\n \t\terror_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n \t\t\t\"git pull \"\n-- \n1.8.4-fc\n"},{"id":"226393","messageId":"xmqqppsuwmz7.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/6] reset: trivial refactoring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:36:12Z","receivedAt":"2013-08-31T03:36:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> After commit 3fde386 (reset [--mixed]: use diff-based reset whether or\n> not pathspec was given), some code can be moved to the 'reset_type ==\n> MIXED' check.\n\nMakes sense.\n"},{"id":"226394","messageId":"xmqqli3iwmyq.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/6] branch: trivial style fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:36:29Z","receivedAt":"2013-08-31T03:36:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Good. Thanks.\n"},{"id":"226395","messageId":"xmqqhae6wmxk.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-6-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 5/6] add: trivial style cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:37:11Z","receivedAt":"2013-08-31T03:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/add.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 8266a9c..a1e1e0e 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -336,7 +336,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n> \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> \n> \tif (read_cache() < 0)\n> -\t\tdie (_(\"Could not read the index\"));\n> +\t\tdie(_(\"Could not read the index\"));\n> \n> \tinit_revisions(&rev, prefix);\n> \trev.diffopt.context = 7;\n> @@ -347,11 +347,11 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n> \tDIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);\n> \tout = open(file, O_CREAT | O_WRONLY, 0666);\n> \tif (out < 0)\n> -\t\tdie (_(\"Could not open '%s' for writing.\"), file);\n> +\t\tdie(_(\"Could not open '%s' for writing.\"), file);\n> \trev.diffopt.file = xfdopen(out, \"w\");\n> \trev.diffopt.close_file = 1;\n> \tif (run_diff_files(&rev, 0))\n> -\t\tdie (_(\"Could not write patch\"));\n> +\t\tdie(_(\"Could not write patch\"));\n> \n> \tlaunch_editor(file, NULL, NULL);\n> \n> @@ -364,7 +364,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n> \tchild.git_cmd = 1;\n> \tchild.argv = apply_argv;\n> \tif (run_command(&child))\n> -\t\tdie (_(\"Could not apply '%s'\"), file);\n> +\t\tdie(_(\"Could not apply '%s'\"), file);\n> \n> \tunlink(file);\n> \tfree(file);\n\nGood. These often bothered me.\n"},{"id":"226396","messageId":"xmqqbo4ewmd4.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/6] rebase: trivial style fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:49:27Z","receivedAt":"2013-08-31T03:49:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Signed-off-by: Felipe Contreras \n> ---\n> git-rebase.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 8d7659a..2c02853 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -324,7 +324,7 @@ done\n> test $# -gt 2 && usage\n> \n> if test -n \"$cmd\" &&\n> - test \"$interactive_rebase\" != explicit\n> +\ttest \"$interactive_rebase\" != explicit\n> then\n> \tdie \"$(gettext \"The --exec option must be used with the --interactive option\")\"\n> fi\n> @@ -486,7 +486,7 @@ case \"$#\" in\n> \tswitch_to=\"$1\"\n> \n> \tif git show-ref --verify --quiet -- \"refs/heads/$1\" &&\n> -\t orig_head=$(git rev-parse -q --verify \"refs/heads/$1\")\n> +\t\torig_head=$(git rev-parse -q --verify \"refs/heads/$1\")\n> \tthen\n> \t\thead_name=\"refs/heads/$1\"\n> \telif orig_head=$(git rev-parse -q --verify \"$1\")\n\nI am not sure about this change. I do not personally have strong\npreference on this, but it would be better to be consistent.\n\nThe style of the original we see above seems to be the one that is\nconsistently used in this file for conditionals that span multiple\nlines. That is, to align the beginning of subsequent lines with the\nbeginning of the conditional (i.e. the \"g\" in \"git show-ref\" on the\nfirst line)---which happens to be in line with what we use in our C\nsources, too.\n\nI see there is one oddball in that file, though.\n\n git-rebase.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 8d7659a..187793e 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -422,7 +422,7 @@ then\n \tcase \"$#\" in\n \t0)\n \t\tif ! upstream_name=$(git rev-parse --symbolic-full-name \\\n-\t\t\t--verify -q @{upstream} 2>/dev/null)\n+\t\t --verify -q @{upstream} 2>/dev/null)\n \t\tthen\n \t\t\t. git-parse-remote\n \t\t\terror_on_missing_default_upstream \"rebase\" \"rebase\" \\\n"},{"id":"226397","messageId":"xmqq7gf2wmcf.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/6] reset: trivial style cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:49:52Z","receivedAt":"2013-08-31T03:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/reset.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 225e3f1..7e65934 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -323,7 +323,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n> \t\tdie_if_unmerged_cache(reset_type);\n> \n> \tif (reset_type != SOFT) {\n> -\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n> +\t\tstruct lock_file *lock = xcalloc(1, sizeof(*lock));\n\nGood. Thanks.\n"},{"id":"226398","messageId":"xmqq38pqwlyl.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"1377899810-1818-7-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 6/6] pull: trivial cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-31T03:58:10Z","receivedAt":"2013-08-31T03:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> There's no need to remove 'refs/heads/' yet again.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> git-pull.sh | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/git-pull.sh b/git-pull.sh\n> index f0df41c..3bdcbfd 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -166,7 +166,6 @@ error_on_no_merge_candidates () {\n> \t\top_prep=with\n> \tfi\n> \n> -\tcurr_branch=${curr_branch#refs/heads/}\n\nThe code assumes that at this point $curr_branch has the result of\ngit symbolic-ref -q HEAD it did at the beginning, before it entered\nin the command line parsing loop. But immediately after it, the\ncode sets up $curr_branch_short for the value this code computes.\n\n> \tupstream=$(git config \"branch.$curr_branch.merge\")\n> \tremote=$(git config \"branch.$curr_branch.remote\")\n\nSo it appears to me that the above two lines that are not updated\nwould introduce a regression. Am I missing something trivial?\n\nPuzzled.\n\n\n> @@ -183,7 +182,7 @@ error_on_no_merge_candidates () {\n> \t\techo \"You asked to pull from the remote '$1', but did not specify\"\n> \t\techo \"a branch. Because this is not the default configured remote\"\n> \t\techo \"for your current branch, you must specify a branch on the command line.\"\n> -\telif [ -z \"$curr_branch\" -o -z \"$upstream\" ]; then\n> +\telif [ -z \"$curr_branch_short\" -o -z \"$upstream\" ]; then\n\nIf $curr_branch in the original code was (wasn't) an empty string,\nthen with the updated code that does not strip refs/heads/ from the\nbeginning of it after applying the first hunk of this patch, the\nvariable is (isn't) an empty string, respectively. So there is no\nneed for this hunk, I think.\n\n> \t\t. git-parse-remote\n> \t\terror_on_missing_default_upstream \"pull\" $op_type $op_prep \\\n> \t\t\t\"git pull \"\n"},{"id":"226408","messageId":"CAMP44s3X6au7iG3O31HZn_kvcjU-AJa+pnMci+FQ0vaDPvjOsA@mail.gmail.com","threadId":"34811","inReplyTo":"xmqq38pqwlyl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/6] pull: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-31T07:56:08Z","receivedAt":"2013-08-31T07:56:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Aug 30, 2013 at 10:58 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> There's no need to remove 'refs/heads/' yet again.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> git-pull.sh | 3 +--\n>> 1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/git-pull.sh b/git-pull.sh\n>> index f0df41c..3bdcbfd 100755\n>> --- a/git-pull.sh\n>> +++ b/git-pull.sh\n>> @@ -166,7 +166,6 @@ error_on_no_merge_candidates () {\n>> op_prep=with\n>> fi\n>>\n>> - curr_branch=${curr_branch#refs/heads/}\n>\n> The code assumes that at this point $curr_branch has the result of\n> git symbolic-ref -q HEAD it did at the beginning, before it entered\n> in the command line parsing loop. But immediately after it, the\n> code sets up $curr_branch_short for the value this code computes.\n>\n>> upstream=$(git config \"branch.$curr_branch.merge\")\n>> remote=$(git config \"branch.$curr_branch.remote\")\n>\n> So it appears to me that the above two lines that are not updated\n> would introduce a regression. Am I missing something trivial?\n\nYes, I'm not exactly sure where from which branch I got this change,\nbut those lines were removed. They should be updated.\n\n-- \nFelipe Contreras\n"},{"id":"226410","messageId":"5221A510.2020206@web.de","threadId":"34811","inReplyTo":"xmqq38pqwlyl.fsf@gitster.dls.corp.google.com","subject":"[PATCH] branch: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-08-31T08:10:56Z","receivedAt":"2013-08-31T08:10:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"One of the first things git-pull.sh does is setting $curr_branch to\nthe target of HEAD and $curr_branch_short to the same but with the\nleading \"refs/heads/\" removed. Use $curr_branch_short in the function\nerror_on_no_merge_candidates instead of removing the prefix from\n$curr_branch directly.\n\nThe only other use of $curr_branch in that function doesn't have to\nbe replaced with $curr_branch_short because it just checks if the\nstring is empty. That property is the same with or without the prefix\nunless HEAD points to \"refs/heads/\" alone, which is invalid.\n\nNoticed-by: Felipe Contreras \nSigned-off-by: Rene Scharfe \n---\n git-pull.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex f0df41c..d8b2708 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -166,9 +166,8 @@ error_on_no_merge_candidates () {\n \t\top_prep=with\n \tfi\n \n-\tcurr_branch=${curr_branch#refs/heads/}\n-\tupstream=$(git config \"branch.$curr_branch.merge\")\n-\tremote=$(git config \"branch.$curr_branch.remote\")\n+\tupstream=$(git config \"branch.$curr_branch_short.merge\")\n+\tremote=$(git config \"branch.$curr_branch_short.remote\")\n \n \tif [ $# -gt 1 ]; then\n \t\tif [ \"$rebase\" = true ]; then\n-- \n1.8.4\n"},{"id":"226411","messageId":"CAMP44s1qyF70eSCM4L1Yr=U02AwFZ0TNqiDvag8Qdzzi9GS1Mw@mail.gmail.com","threadId":"34811","inReplyTo":"5221A510.2020206@web.de","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-31T08:22:24Z","receivedAt":"2013-08-31T08:22:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"> Subject: branch: use $curr_branch_short more\n\nWhy? I don't think that summary explains the reason for being for this\npatch, also, it starts with branch: instead of pull:\n\nSubject: pull: trivial simplification\n\nWith that summary, people would have an easier time figuring out if\nthey need to read more about the patch or not.\n\n-- \nFelipe Contreras\n"},{"id":"226412","messageId":"5221B324.7020908@web.de","threadId":"34811","inReplyTo":"CAMP44s1qyF70eSCM4L1Yr=U02AwFZ0TNqiDvag8Qdzzi9GS1Mw@mail.gmail.com","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-08-31T09:11:00Z","receivedAt":"2013-08-31T09:11:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.2013 10:22, schrieb Felipe Contreras:\n>> Subject: branch: use $curr_branch_short more\n>\n> Why? I don't think that summary explains the reason for being for this\n> patch, also, it starts with branch: instead of pull:\n\nYou're right about \"branch\" vs. \"pull\". I'll better go back to bed. ~_~\n\n> Subject: pull: trivial simplification\n>\n> With that summary, people would have an easier time figuring out if\n> they need to read more about the patch or not.\n\n\"trivial simplification\" is too generic; we could have lots of them. A \nsummary should describe the change. Its low complexity can be derived \nfrom it -- using an existing variable a bit more is not very exciting.\n\nBut I wouldn't call that patch trivial because its correctness depends \non code outside of its shown context.\n\nThe reason for the patch isn't mentioned explicitly. Perhaps it should \nbe. I felt that using something that's already there instead of \nrecreating it is motivation alone.\n\nRené\n"},{"id":"226413","messageId":"CAMP44s0et9_e5XH-4aydrM-i_+ORampdsitJzK-rSj+4dwPUKw@mail.gmail.com","threadId":"34811","inReplyTo":"5221B324.7020908@web.de","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-31T09:22:44Z","receivedAt":"2013-08-31T09:22:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe wrote:\n\n>> Subject: pull: trivial simplification\n>>\n>> With that summary, people would have an easier time figuring out if\n>> they need to read more about the patch or not.\n>\n>\n> \"trivial simplification\" is too generic; we could have lots of them.\n\nNo, we can have only one, otherwise it would say simplificationS.\n\n> A summary should describe the change.\n\nYou can never fully describe the change, only the diff does that.\n\nFor example \"use $curr_branch_short more\" does not tell me anything\nabout the extent of the changes, is it used in one more place? two?\none hundred? Moreover, how exactly is it used more? Is some\nrefactoring needed?\n\nAnd it still doesn't answer the most important question any summary\nshould answer: why? Why use $curr_branch_short more?\n\n> Its low complexity can be derived from\n> it -- using an existing variable a bit more is not very exciting.\n\nYou didn't say \"a bit more\" you said \"more\". And yes, the complexity\ncan be derived from the summary, but not from this one.\n\n> But I wouldn't call that patch trivial because its correctness depends on\n> code outside of its shown context.\n\nCorrectness is a separate question from triviality, and the\ncorrectness can only be assessed by looking at the actual patch.\n\nThe patch can be both trivial and wrong.\n\n> The reason for the patch isn't mentioned explicitly. Perhaps it should be.\n> I felt that using something that's already there instead of recreating it is\n> motivation alone.\n\nWhy? Because it simplifies the code. That's the real answer.\n\n-- \nFelipe Contreras\n"},{"id":"226414","messageId":"5221C533.1070109@web.de","threadId":"34811","inReplyTo":"CAMP44s0et9_e5XH-4aydrM-i_+ORampdsitJzK-rSj+4dwPUKw@mail.gmail.com","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-08-31T10:28:03Z","receivedAt":"2013-08-31T10:28:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.2013 11:22, schrieb Felipe Contreras:\n> On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe wrote:\n>\n>>> Subject: pull: trivial simplification\n>>>\n>>> With that summary, people would have an easier time figuring out if\n>>> they need to read more about the patch or not.\n>>\n>>\n>> \"trivial simplification\" is too generic; we could have lots of them.\n>\n> No, we can have only one, otherwise it would say simplificationS.\n\nI was too terse again, let me rephrase that: We could have lots of \ncommits that fit the same description if we used such a generic one.\n\n>> A summary should describe the change.\n>\n> You can never fully describe the change, only the diff does that.\n>\n> For example \"use $curr_branch_short more\" does not tell me anything\n> about the extent of the changes, is it used in one more place? two?\n> one hundred? Moreover, how exactly is it used more? Is some\n> refactoring needed?\n>\n> And it still doesn't answer the most important question any summary\n> should answer: why? Why use $curr_branch_short more?\n\nA summary doesn't have to contain lots of details. The what is \nimportant, the why can be explained in the commit message.\n\n>> Its low complexity can be derived from\n>> it -- using an existing variable a bit more is not very exciting.\n>\n> You didn't say \"a bit more\" you said \"more\". And yes, the complexity\n> can be derived from the summary, but not from this one.\n>\n>> But I wouldn't call that patch trivial because its correctness depends on\n>> code outside of its shown context.\n>\n> Correctness is a separate question from triviality, and the\n> correctness can only be assessed by looking at the actual patch.\n>\n> The patch can be both trivial and wrong.\n\nProbably too terse again, let's say it differently: Only a patch whose \ncorrectness can be judged without looking outside of the three lines of \ncontext it includes qualifies as trivial in my book. The patch in \nquestion is not trivial because you can't see the value of \n$curr_branch_short just by looking at the diff.\n\n>> The reason for the patch isn't mentioned explicitly. Perhaps it should be.\n>> I felt that using something that's already there instead of recreating it is\n>> motivation alone.\n>\n> Why? Because it simplifies the code. That's the real answer.\n\nI don't disagree.\n\nRené\n"},{"id":"226424","messageId":"CAMP44s39X2SsZC7f6=j=CX+9wky7YpiJUz3itCiqeLScD0TbNA@mail.gmail.com","threadId":"34811","inReplyTo":"5221C533.1070109@web.de","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-31T17:20:53Z","receivedAt":"2013-08-31T17:20:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Aug 31, 2013 at 5:28 AM, René Scharfe wrote:\n> Am 31.08.2013 11:22, schrieb Felipe Contreras:\n>\n>> On Sat, Aug 31, 2013 at 4:11 AM, René Scharfe wrote:\n>>\n>>>> Subject: pull: trivial simplification\n>>>>\n>>>> With that summary, people would have an easier time figuring out if\n>>>> they need to read more about the patch or not.\n>>>\n>>>\n>>>\n>>> \"trivial simplification\" is too generic; we could have lots of them.\n>>\n>>\n>> No, we can have only one, otherwise it would say simplificationS.\n>\n>\n> I was too terse again, let me rephrase that: We could have lots of commits\n> that fit the same description if we used such a generic one.\n\nYes, but they are all trivial, and they all simply the code.\n\n>>> A summary should describe the change.\n>>\n>>\n>> You can never fully describe the change, only the diff does that.\n>>\n>> For example \"use $curr_branch_short more\" does not tell me anything\n>> about the extent of the changes, is it used in one more place? two?\n>> one hundred? Moreover, how exactly is it used more? Is some\n>> refactoring needed?\n>>\n>> And it still doesn't answer the most important question any summary\n>> should answer: why? Why use $curr_branch_short more?\n>\n> A summary doesn't have to contain lots of details. The what is important,\n> the why can be explained in the commit message.\n\nA summary should contain as much information that would allow me to\nskip the commit message as possible.\n\nIf I can't tell from the summary if I can safely skip the commit\nmessage, the summary is not doing a good job.\n\n\"trivial simplification\" explains the \"what\", and the \"why\" at the\nsame time, and allows most people to skip the commit message, thus is\na good summary.\n\n>>> Its low complexity can be derived from\n>>> it -- using an existing variable a bit more is not very exciting.\n>>\n>>\n>> You didn't say \"a bit more\" you said \"more\". And yes, the complexity\n>> can be derived from the summary, but not from this one.\n>>\n>>> But I wouldn't call that patch trivial because its correctness depends on\n>>> code outside of its shown context.\n>>\n>>\n>> Correctness is a separate question from triviality, and the\n>> correctness can only be assessed by looking at the actual patch.\n>>\n>> The patch can be both trivial and wrong.\n>\n>\n> Probably too terse again, let's say it differently: Only a patch whose\n> correctness can be judged without looking outside of the three lines of\n> context it includes qualifies as trivial in my book. The patch in question\n> is not trivial because you can't see the value of $curr_branch_short just by\n> looking at the diff.\n\nAgain, triviality and correctness are two separate different things.\nThe patch is trivial even if you can't judge it's correctness.\n\nTo me, what you are describing is an obvious patch, not a trivial one.\nAn obvious patch is so obvious that you can judge it's correctness\neasily by looking at the diff, a trivial one is of little importance.\n\n>>> The reason for the patch isn't mentioned explicitly. Perhaps it should\n>>> be.\n>>> I felt that using something that's already there instead of recreating it\n>>> is\n>>> motivation alone.\n>>\n>>\n>> Why? Because it simplifies the code. That's the real answer.\n>\n> I don't disagree.\n\nYet your commit message doesn't explain that anywhere.\n\n-- \nFelipe Contreras\n"},{"id":"226630","messageId":"xmqqr4d5u9mk.fsf@gitster.dls.corp.google.com","threadId":"34811","inReplyTo":"CAMP44s1qyF70eSCM4L1Yr=U02AwFZ0TNqiDvag8Qdzzi9GS1Mw@mail.gmail.com","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T16:56:35Z","receivedAt":"2013-09-03T16:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> Subject: branch: use $curr_branch_short more\n>\n> Why? I don't think that summary explains the reason for being for this\n> patch, also, it starts with branch: instead of pull:\n>\n> Subject: pull: trivial simplification\n>\n> With that summary, people would have an easier time figuring out if\n> they need to read more about the patch or not.\n\nPeople, other than the author of the patch, use the subject line in\ndifferent ways. It is unclear to me which usage the \"trivial lets\nthem decide if it needs further reading\" is trying to help.\n\n 1. Those running \"shortlog\" to see how much impact each contributor\n had.\n\n 2. Those viewing list of Subjects in their MUA to see which is\n worth reading and commenting on.\n\n 3. Those viewing list of Subjects in their MUA to see which is\n worth reading and applying to their trees.\n\n 4. Those trying to resolve conflicts during \"git am -3\" and \"git\n merge\" [*1*].\n\n 5. Those who find that a commit broke the build after bisecting,\n and want to see what is the reasoning behind the broken change.\n\n 6. Those who find an interesting section of the code, blamed its\n origin to a commit, and want to see what is the reasoning behind\n the broken change.\n\nThere may be others, but the above should cover most of the cases, I\nthink.\n\nFor 1., they may discount anything that says \"trivial\" as \"not of\nhigh impact\". There may be trivial but high impact changes, but I\ndo not know how much this mischaracterization hurts. Probably not\nthat much.\n\nFor 2., they may either skip \"trivial\", thinking \"if it is trivial,\nit must be correct\", or read \"trivial\", thinking \"the author thinks\ntrivial, it is likely there are holes in the author's thinking\".\nAmong the 6 patches in $gmane/233472 \"Trivial cleanups and fixes\",\n\n - 1, 2, 4 and 5 were trivially correct and good,\n - 3 was not a clear improvement,\n - 6 was wrong.\n\nThis is totally unscientific but if we take them as a representative\nset of \"trivial\" patches, a \"trivial\" patch is correct only about\n4.5/6 = 75% of the time. As a consequence, people who do want to\nhelp the project are better off reading \"trivial\" just like others,\nso that breakages in the 25% do not go unnoticed. The label does\nnot let them skip, and wastes one line that possibly could have\ngiven them more information with non-descriptive word \"trivial\".\n\nFor 3., unless the author wants such a patch skipped and dropped on\nthe floor, \"marking it 'trivial' to allow them skip\" would not make\nmuch sense, and with 75% success rate, it would be irresponsible for\nthose who apply patches not to read a \"trivial\" and blindly apply\nit. Labelling it \"trivial\" only wastes one line that possibly could\nhave given them more information with non-descriptive word \"trivial\".\n\nFor 4., 5., and 6., \"allow them to skip\" will not be in the picture,\nas they already know they are interested in that particular change.\nLabelling it \"trivial\" only wastes one line that possibly could have\ngiven them more information with non-descriptive word \"trivial\"\n\nSo it seems to me that a title \"trivial\" only helps those running\n\"shortlog\" to discount weight of contributor's contribution.\n\nAnd the author who does not want to spend time on coming up with a\ngood title, but for each patch, there are a lot more readers than a\nsingle author of that patch, so the benefit to the author does not\ncount much.\n\n\n[Footnote]\n\n*1* The former shows the title of the patch and the latter shows the\n branch name after a conflict marker, and it helps to have as\n much clue as possible to remind what is attempted on each side\n of the change. It is a responsibility for the person who does\n the merge to give the branch a descriptive name, and the branch\n that queued a \"trivial\" patch does not have to be named\n \"trivial\", but the title of the patch is a large part of the\n input to naming the branch.\n"},{"id":"227138","messageId":"522C95F3.5030308@web.de","threadId":"34811","inReplyTo":"CAMP44s39X2SsZC7f6=j=CX+9wky7YpiJUz3itCiqeLScD0TbNA@mail.gmail.com","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-08T15:21:23Z","receivedAt":"2013-09-08T15:21:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.08.2013 19:20, schrieb Felipe Contreras:\n> A summary should contain as much information that would allow me to\n> skip the commit message as possible.\n>\n> If I can't tell from the summary if I can safely skip the commit\n> message, the summary is not doing a good job.\n>\n> \"trivial simplification\" explains the \"what\", and the \"why\" at the\n> same time, and allows most people to skip the commit message, thus is\n> a good summary.\n\nNo patch should be skipped on the mailing list. As you wrote, trivial \npatches can still be wrong.\n\nWhen going through the history I can see that quickly recognizing \ninsubstantial changes is useful, but if I see a summary twice then in my \nmind forms a big question mark -- why did the same thing had to be done \nyet again?\n\nAs an example, both 0d12e59f (pull: replace unnecessary sed invocation) \nand bc2bbc45 (pull, rebase: simplify to use die()) could arguably have \nhad the summary \"trivial simplification\", but I'm glad the author went \nwith something a bit more specific.\n\nI agree that some kind of tagging with keywords like \"trivial\", \"typo\" \nand so on can be helpful, though.\n\n> Again, triviality and correctness are two separate different things.\n> The patch is trivial even if you can't judge it's correctness.\n\nWell, in terms of impact I agree.\n\n> To me, what you are describing is an obvious patch, not a trivial one.\n> An obvious patch is so obvious that you can judge it's correctness\n> easily by looking at the diff, a trivial one is of little importance.\n\nThat's one definition; I think I had the mathematical notion in mind \nthat calls proofs trivial which are immediately evident.\n\nRené\n"},{"id":"227139","messageId":"522C9608.8010304@web.de","threadId":"34811","inReplyTo":"5221A510.2020206@web.de","subject":"[PATCH] pull: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-08T15:21:44Z","receivedAt":"2013-09-08T15:21:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"One of the first things git-pull.sh does is setting $curr_branch to\nthe target of HEAD and $curr_branch_short to the same but with the\nleading \"refs/heads/\" removed. Simplify the code by using\n$curr_branch_short instead of setting $curr_branch to the same\nshortened value.\n\nThe only other use of $curr_branch in that function doesn't have to\nbe replaced with $curr_branch_short because it just checks if the\nstring is empty. That property is the same with or without the prefix\nunless HEAD points to \"refs/heads/\" alone, which is invalid.\n\nNoticed-by: Felipe Contreras \nSigned-off-by: Rene Scharfe \n---\nReplacement patch. Corrected the command in the summary and\nchanged the first part of the description slightly.\n\n git-pull.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex f0df41c..d8b2708 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -166,9 +166,8 @@ error_on_no_merge_candidates () {\n \t\top_prep=with\n \tfi\n \n-\tcurr_branch=${curr_branch#refs/heads/}\n-\tupstream=$(git config \"branch.$curr_branch.merge\")\n-\tremote=$(git config \"branch.$curr_branch.remote\")\n+\tupstream=$(git config \"branch.$curr_branch_short.merge\")\n+\tremote=$(git config \"branch.$curr_branch_short.remote\")\n \n \tif [ $# -gt 1 ]; then\n \t\tif [ \"$rebase\" = true ]; then\n-- 1.8.4 \n"},{"id":"227168","messageId":"CAMP44s0LOhGWpgy6iy==WC7tBtjUjG-CTJ6jXNm+Jumu-5OkXw@mail.gmail.com","threadId":"34811","inReplyTo":"522C95F3.5030308@web.de","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-08T23:13:00Z","receivedAt":"2013-09-08T23:13:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe wrote:\n> Am 31.08.2013 19:20, schrieb Felipe Contreras:\n>\n>> A summary should contain as much information that would allow me to\n>> skip the commit message as possible.\n>>\n>> If I can't tell from the summary if I can safely skip the commit\n>> message, the summary is not doing a good job.\n>>\n>> \"trivial simplification\" explains the \"what\", and the \"why\" at the\n>> same time, and allows most people to skip the commit message, thus is\n>> a good summary.\n>\n>\n> No patch should be skipped on the mailing list. As you wrote, trivial\n> patches can still be wrong.\n\nWhat patches each persons skips is each person's own decision, don't\nbe patronizing, if I want to skip a trivial patch, I will, I can't\nread each and every patch from the dozens of mailing lists I'm\nsubscribed to, and there's no way each and every reader is going to\nread each and every patch. They should be prioritized, and trivial\nones can be safely skipped by most people.\n\nHere's a good example from a simple summary that I didn't write:\n\nt2024: Fix inconsequential typos\nhttp://article.gmane.org/gmane.comp.version-control.git/234038\n\nDo I have to read this patch? No. I know it's inconsequential, I can\nsafely skip it, the key word being *I*. *Somebody* should read it,\nsure, and I'm sure at least Junio would, but I don't need to.\n\n> When going through the history I can see that quickly recognizing\n> insubstantial changes is useful, but if I see a summary twice then in my\n> mind forms a big question mark -- why did the same thing had to be done yet\n> again?\n>\n> As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and\n> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the\n> summary \"trivial simplification\", but I'm glad the author went with\n> something a bit more specific.\n\nWell I wont. Because it takes long to read, and after reading I still\ndon't don't if they are trivial or not, I might actually have to read\nthe commit message, but to be honest, I would probably go right into\nthe diff itself, because judging from Git's history, chances are that\nsomebody wrote a novel there with the important bit I'm looking for\njust at the end, to don't ruin the suspense.\n\nIn the first commit, it's saying it's a single invocation, so I take\nit it's trivial, but what is it replaced with? Is the code simpler, is\nit more complex? I don't know, I'm still not being told *why* that\npatch is made. It says 'unnecessary' but why is it unnecessary?\n\nIn the second commit, it's saying it's a simplification, but I don't\nknow if it's just one instance, or a thousand, so I don't know what\nwould be the impact of the patch.\n\nEither way I'm forced to read more just to know if it was safe for me\nto skip them, at which point the whole purpose of a summary is\ndefeated.\n\n>> Again, triviality and correctness are two separate different things.\n>> The patch is trivial even if you can't judge it's correctness.\n>\n> Well, in terms of impact I agree.\n\nNo, in all terms. A patch can be:\n\nTrivial and correct\nTrivial and incorrect\nNon-trivial and correct\nNon-trivial and incorrect\n\n>> To me, what you are describing is an obvious patch, not a trivial one.\n>> An obvious patch is so obvious that you can judge it's correctness\n>> easily by looking at the diff, a trivial one is of little importance.\n>\n> That's one definition; I think I had the mathematical notion in mind that\n> calls proofs trivial which are immediately evident.\n\nWe are not talking about mathematics, we are talking about the English\nhuman language.\n\n-- \nFelipe Contreras\n"},{"id":"227692","messageId":"52359D33.508@web.de","threadId":"34811","inReplyTo":"CAMP44s0LOhGWpgy6iy==WC7tBtjUjG-CTJ6jXNm+Jumu-5OkXw@mail.gmail.com","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2013-09-15T11:42:43Z","receivedAt":"2013-09-15T11:42:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.09.2013 01:13, schrieb Felipe Contreras:\n> On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe wrote:\n>> Am 31.08.2013 19:20, schrieb Felipe Contreras:\n>>\n>>> A summary should contain as much information that would allow me to\n>>> skip the commit message as possible.\n>>>\n>>> If I can't tell from the summary if I can safely skip the commit\n>>> message, the summary is not doing a good job.\n>>>\n>>> \"trivial simplification\" explains the \"what\", and the \"why\" at the\n>>> same time, and allows most people to skip the commit message, thus is\n>>> a good summary.\n>>\n>>\n>> No patch should be skipped on the mailing list. As you wrote, trivial\n>> patches can still be wrong.\n>\n> What patches each persons skips is each person's own decision, don't\n> be patronizing, if I want to skip a trivial patch, I will, I can't\n> read each and every patch from the dozens of mailing lists I'm\n> subscribed to, and there's no way each and every reader is going to\n> read each and every patch. They should be prioritized, and trivial\n> ones can be safely skipped by most people.\n\nYes, of course; someone needs to review every patch in the end, but each \nreader decides for themselves which ones to skip. I can't keep up with \nthe traffic either.\n\nBy the way, the bikeshedding phenomenon probably causes trivial patches \nto receive the most attention. :)\n\n>> When going through the history I can see that quickly recognizing\n>> insubstantial changes is useful, but if I see a summary twice then in my\n>> mind forms a big question mark -- why did the same thing had to be done yet\n>> again?\n>>\n>> As an example, both 0d12e59f (pull: replace unnecessary sed invocation) and\n>> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had the\n>> summary \"trivial simplification\", but I'm glad the author went with\n>> something a bit more specific.\n>\n> Well I wont. Because it takes long to read, and after reading I still\n> don't don't if they are trivial or not, I might actually have to read\n> the commit message, but to be honest, I would probably go right into\n> the diff itself, because judging from Git's history, chances are that\n> somebody wrote a novel there with the important bit I'm looking for\n> just at the end, to don't ruin the suspense.\n\nHa! It's better to write it down at all than to miss it years later, \nwhen even the original author has forgotten all about it.\n\n> In the first commit, it's saying it's a single invocation, so I take\n> it it's trivial, but what is it replaced with? Is the code simpler, is\n> it more complex? I don't know, I'm still not being told *why* that\n> patch is made. It says 'unnecessary' but why is it unnecessary?\n\nThe sed call is unnecessary because of the fact that it can be replaced. \n:) I'm sure I would have understood it to mean a conversion to Shell \nbuiltin functionality in order to avoid forking and executing an \nexternal command, even if I hadn't read the patch.\n\n> In the second commit, it's saying it's a simplification, but I don't\n> know if it's just one instance, or a thousand, so I don't know what\n> would be the impact of the patch.\n>\n> Either way I'm forced to read more just to know if it was safe for me\n> to skip them, at which point the whole purpose of a summary is\n> defeated.\n\nI don't see how using \"trivial simplification\" as the summary for both \ncould have improved matters.\n\n>>> Again, triviality and correctness are two separate different things.\n>>> The patch is trivial even if you can't judge it's correctness.\n>>\n>> Well, in terms of impact I agree.\n>\n> No, in all terms. A patch can be:\n>\n> Trivial and correct\n> Trivial and incorrect\n> Non-trivial and correct\n> Non-trivial and incorrect\n\nWell, yes, but I thought your statement that \"The patch is trivial\" was \nreferring to my actual patch which started this sub-thread. And I meant \nthat the benefit of that patch to users and programmers was small.\n\n>>> To me, what you are describing is an obvious patch, not a trivial one.\n>>> An obvious patch is so obvious that you can judge it's correctness\n>>> easily by looking at the diff, a trivial one is of little importance.\n>>\n>> That's one definition; I think I had the mathematical notion in mind that\n>> calls proofs trivial which are immediately evident.\n>\n> We are not talking about mathematics, we are talking about the English\n> human language.\n\nHere we were talking about source code patches. As formal descriptions \nof changes to (mostly) programming language text they are closer to \nmathematics than English. Using math terms when talking about them is \nnot too far of a stretch.\n\nRené\n"},{"id":"227694","messageId":"CAMP44s3f=TzDGkMZawKm-KZozCfSvLQbhvQ83KrM9dqbNBm-pg@mail.gmail.com","threadId":"34811","inReplyTo":"52359D33.508@web.de","subject":"Re: [PATCH] branch: use $curr_branch_short more","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-09-15T13:02:31Z","receivedAt":"2013-09-15T13:02:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Sep 15, 2013 at 6:42 AM, René Scharfe wrote:\n> Am 09.09.2013 01:13, schrieb Felipe Contreras:\n>\n>> On Sun, Sep 8, 2013 at 10:21 AM, René Scharfe wrote:\n>>>\n>>> Am 31.08.2013 19:20, schrieb Felipe Contreras:\n>>>\n>>>> A summary should contain as much information that would allow me to\n>>>> skip the commit message as possible.\n>>>>\n>>>> If I can't tell from the summary if I can safely skip the commit\n>>>> message, the summary is not doing a good job.\n>>>>\n>>>> \"trivial simplification\" explains the \"what\", and the \"why\" at the\n>>>> same time, and allows most people to skip the commit message, thus is\n>>>> a good summary.\n>>>\n>>>\n>>>\n>>> No patch should be skipped on the mailing list. As you wrote, trivial\n>>> patches can still be wrong.\n>>\n>>\n>> What patches each persons skips is each person's own decision, don't\n>> be patronizing, if I want to skip a trivial patch, I will, I can't\n>> read each and every patch from the dozens of mailing lists I'm\n>> subscribed to, and there's no way each and every reader is going to\n>> read each and every patch. They should be prioritized, and trivial\n>> ones can be safely skipped by most people.\n>\n>\n> Yes, of course; someone needs to review every patch in the end, but each\n> reader decides for themselves which ones to skip. I can't keep up with the\n> traffic either.\n>\n> By the way, the bikeshedding phenomenon probably causes trivial patches to\n> receive the most attention. :)\n\nExactly, so if the summary of the commit message allows people to skip\na patch, that is fine.\n\n>>> When going through the history I can see that quickly recognizing\n>>> insubstantial changes is useful, but if I see a summary twice then in my\n>>> mind forms a big question mark -- why did the same thing had to be done\n>>> yet\n>>> again?\n>>>\n>>> As an example, both 0d12e59f (pull: replace unnecessary sed invocation)\n>>> and\n>>> bc2bbc45 (pull, rebase: simplify to use die()) could arguably have had\n>>> the\n>>> summary \"trivial simplification\", but I'm glad the author went with\n>>> something a bit more specific.\n>>\n>>\n>> Well I wont. Because it takes long to read, and after reading I still\n>> don't don't if they are trivial or not, I might actually have to read\n>> the commit message, but to be honest, I would probably go right into\n>> the diff itself, because judging from Git's history, chances are that\n>> somebody wrote a novel there with the important bit I'm looking for\n>> just at the end, to don't ruin the suspense.\n>\n>\n> Ha! It's better to write it down at all than to miss it years later, when\n> even the original author has forgotten all about it.\n\nYes, of course, but that still means the commit message summary failed\nit's purpose.\n\n>> In the first commit, it's saying it's a single invocation, so I take\n>> it it's trivial, but what is it replaced with? Is the code simpler, is\n>> it more complex? I don't know, I'm still not being told *why* that\n>> patch is made. It says 'unnecessary' but why is it unnecessary?\n>\n>\n> The sed call is unnecessary because of the fact that it can be replaced. :)\n> I'm sure I would have understood it to mean a conversion to Shell builtin\n> functionality in order to avoid forking and executing an external command,\n> even if I hadn't read the patch.\n\nThe problem is that the commit message is not for you, it's for every\nreader, so the fact that you would have understood it that way is\nirrelevant.\n\nMaybe this is an exercise in the lack of empathy, and an example of\nmono-culture.\n\n>> In the second commit, it's saying it's a simplification, but I don't\n>> know if it's just one instance, or a thousand, so I don't know what\n>> would be the impact of the patch.\n>>\n>> Either way I'm forced to read more just to know if it was safe for me\n>> to skip them, at which point the whole purpose of a summary is\n>> defeated.\n>\n>\n> I don't see how using \"trivial simplification\" as the summary for both could\n> have improved matters.\n\nIt would say \"trivial\", which allows me and a lot of other people to\nsafely skip them, it's as simple as that.\n\n>>>> Again, triviality and correctness are two separate different things.\n>>>> The patch is trivial even if you can't judge it's correctness.\n>>>\n>>>\n>>> Well, in terms of impact I agree.\n>>\n>>\n>> No, in all terms. A patch can be:\n>>\n>> Trivial and correct\n>> Trivial and incorrect\n>> Non-trivial and correct\n>> Non-trivial and incorrect\n>\n>\n> Well, yes, but I thought your statement that \"The patch is trivial\" was\n> referring to my actual patch which started this sub-thread. And I meant\n> that the benefit of that patch to users and programmers was small.\n\nI don't understand what you are trying to say, the point remains; a\npatch being trivial says nothing about its correctness.\n\n>>>> To me, what you are describing is an obvious patch, not a trivial one.\n>>>> An obvious patch is so obvious that you can judge it's correctness\n>>>> easily by looking at the diff, a trivial one is of little importance.\n>>>\n>>>\n>>> That's one definition; I think I had the mathematical notion in mind that\n>>> calls proofs trivial which are immediately evident.\n>>\n>>\n>> We are not talking about mathematics, we are talking about the English\n>> human language.\n>\n>\n> Here we were talking about source code patches. As formal descriptions of\n> changes to (mostly) programming language text they are closer to mathematics\n> than English. Using math terms when talking about them is not too far of a\n> stretch.\n\nNo, we are not. Commit messages have nothing formal about them, they\nare human oriented and colloquial.\n\n-- \nFelipe Contreras\n"}]}