patch, 6 partsTrivial cleanups and fixes

> 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")

> 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));

> 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);

> 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/}

> @@ -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

> 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?

>> 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.

> 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.

> 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.

> 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.

> 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.

>> 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.

> 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.

>> 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 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.

> 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.

>> 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.