Trivial cleanups and fixes

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.

Signed-off-by Felipe Contreras <[email protected]>

---
 git-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

git-rebase.sh +1 -1

index 8d7659a..2c02853 100755
@@ -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

Signed-off-by Felipe Contreras <[email protected]>

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/reset.c +0 -0

index 225e3f1..7e65934 100644

Signed-off-by Felipe Contreras <[email protected]>

---
 builtin/add.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

builtin/add.c +3 -3

index 8266a9c..a1e1e0e 100644
@@ -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);
 

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

Signed-off-by Felipe Contreras <[email protected]>

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

git-pull.sh +0 -0

index f0df41c..3bdcbfd 100755

git-pull.sh +0 -0

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

Subject: branch: use $curr_branch_short more

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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:

Quoted 9 lines · René Scharfe

Quoted 8 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

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

Quoted 10 lines

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 14 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 8 lines

Quoted 5 lines

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.

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.

Am 31.08.2013 19:20, schrieb Felipe Contreras:

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Am 31.08.2013 19:20, schrieb Felipe Contreras:

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 9 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 3 lines

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:

Quoted 20 lines · René Scharfe

Quoted 9 lines

Am 31.08.2013 19:20, schrieb Felipe Contreras:

Quoted 6 lines

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.

Quoted 8 lines

Quoted 3 lines

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

Quoted 6 lines

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

Quoted 14 lines

Quoted 6 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 12 lines · Felipe Contreras

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

Signed-off-by Felipe Contreras <[email protected]>

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

git-pull.sh +0 -0

index f0df41c..3bdcbfd 100755

git-pull.sh error_on_no_merge_candidates () { · line 169

Quoted 4 lines

git-pull.sh +0 -1

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

git-pull.sh error_on_no_merge_candidates () { · line 170

Quoted 2 lines

git-pull.sh +0 -0

      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?