* Re: [PATCH] Documentation/bisect: improve on (bad|new) and (good|bad)
From: Christian Couder @ 2017-01-15 14:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Manuel Ullmann, Matthieu Moy, Christian Couder
In-Reply-To: <xmqqinpihiwz.fsf@gitster.mtv.corp.google.com>
On Fri, Jan 13, 2017 at 8:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> The following part of the description:
>>
>> git bisect (bad|new) [<rev>]
>> git bisect (good|old) [<rev>...]
>>
>> may be a bit confusing, as a reader may wonder if instead it should be:
>>
>> git bisect (bad|good) [<rev>]
>> git bisect (old|new) [<rev>...]
>>
>> Of course the difference between "[<rev>]" and "[<rev>...]" should hint
>> that there is a good reason for the way it is.
>>
>> But we can further clarify and complete the description by adding
>> "<term-new>" and "<term-old>" to the "bad|new" and "good|old"
>> alternatives.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> Documentation/git-bisect.txt | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Thanks. The patch looks good.
>
> A related tangent.
>
> Last night, I was trying to think if there is a fundamental reason
> why "bad/new/term-new" cannot take more than one <rev>s on the newer
> side of the bisection, and couldn't quite think of any before
> falling asleep.
>
> Currently we keep track of a single bisect/bad, while marking all the
> revs given as good previously as bisect/good-<SHA-1>.
>
> Because the next "bad" is typically chosen from the region of the
> commit DAG that is bounded by bad and good commits, i.e. "rev-list
> bisect/bad --not bisect/good-*", the current bisect/bad will always
> be an ancestor of all bad commits that used to be bisect/bad, and
> keeping previous bisect/bad as bisect/bad-<SHA-1> won't change the
> region of the commit DAG yet to be explored.
>
> As a reason why we need to use only a single bisect/bad, the above
> description is understandable. But as a reason why we cannot have
> more than one, it is tautological. It merely says "if we start from
> only one and dig history to find older culprit, we need only one
> bad".
>
> I fell asleep last night without thinking further than that.
>
> I think the answer to the question "why do we think we need a single
> bisect/bad?" is "because bisection is about assuming that there is
> only one commit that flips the tree state from 'old' to 'new' and
> finding that single commit". That would mean that even if we had
> bisect/bad-A and bisect/bad-B, e.g.
>
> o---o---o---bad-A
> /
> -----Good---o---o---o
> \
> o---o---o---bad-B
>
>
> where 'o' are all commits whose goodness is not yet known, because
> bisection is valid only when we are hunting for a single commit that
> flips the state from good to bad, that commit MUST be at or before
> the merge base of bad-A and bad-B. So even if we allowed
>
> $ git bisect bad bad-A bad-B
>
> on the command line, we won't have to set bisect/bad-A and
> bisect/bad-B. We only need a single bisect/bad that points at the
> merge base of these two.
>
> But what if bad-A and bad-B have more than one merge bases? We
> won't know which side the badness came from.
>
> o---o---o---bad-A
> / \ /
> -----Good---o---o---o /
> \ / \
> o---o---o---bad-B
>
> Being able to bisect the region of DAG bound by "^Good bad-A bad-B"
> may have value in such a case. I dunno.
I agree that there could be improvements in this area. Though from my
experience with special cases, like when a good commit is not an
ancestor of the bad commit (where there are probably bugs still
lurking), I think it could be tricky to implement correctly in all
cases, and it could make it even more difficult, than it sometimes
already is, to explain the resulting behavior to users.
^ permalink raw reply
* Re: feature request: allow to stash changed files
From: Thomas Gummerer @ 2017-01-15 14:26 UTC (permalink / raw)
To: KES; +Cc: git
In-Reply-To: <2141311484484121@web16g.yandex.ru>
On 01/15, KES wrote:
> http://stackoverflow.com/questions/3040833/stash-only-one-file-out-of-multiple-files-that-have-changed-with-git#comment32451416_3040833
>
> Sometimes poople are forced to save stash for changed files. But there is no such option (
You may just be lucky. I've been wishing for something like that for
a while now as well, and finally got around to write a patch this
weekend.
^ permalink raw reply
* [RFC] stash: support filename argument
From: Thomas Gummerer @ 2017-01-15 14:25 UTC (permalink / raw)
To: git; +Cc: kes-kes, Thomas Gummerer
While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone. Unfortunately
git currently offers no such option. git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.
Add a --file option to git stash save, which allows for stashing a
single file. Specifying the --file argument multiple times allows
stashing more than one file at a time.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Marked as RFC and without documentation updates to first get a feeling
for the user interface, and whether people are interested in this
change.
Ideally I wanted the the user interface to look like something like:
git stash save -- [<filename1,...>], but unfortunately that's already
taken up by the stash message. So to preserve backward compatibility
I used the new --file argument.
git-stash.sh | 45 +++++++++++++++++++++++++++++++++++++--------
t/t3903-stash.sh | 27 +++++++++++++++++++++++++++
2 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..0ef1b5b56e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt
+ git ls-files -o -z $excl_opt -- $1
}
clear_stash () {
@@ -56,6 +56,23 @@ clear_stash () {
}
create_stash () {
+ files=
+ while test $# != 0
+ do
+ case "$1" in
+ --)
+ shift
+ break
+ ;;
+ --files)
+ ;;
+ *)
+ files="$1 $files"
+ ;;
+ esac
+ shift
+ done
+
stash_msg="$1"
untracked="$2"
@@ -92,7 +109,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files | (
+ untracked_files $files | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -115,7 +132,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- $files >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -129,7 +146,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- &&
+ git add--interactive --patch=stash -- $files &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -193,6 +210,7 @@ save_stash () {
keep_index=
patch_mode=
untracked=
+ files=
while test $# != 0
do
case "$1" in
@@ -216,6 +234,10 @@ save_stash () {
-a|--all)
untracked=all
;;
+ --file)
+ shift
+ files="$files $1"
+ ;;
--help)
show_help
;;
@@ -262,18 +284,25 @@ save_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash "$stash_msg" $untracked
+ create_stash --files $files -- "$stash_msg" "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
if test -z "$patch_mode"
then
- git reset --hard ${GIT_QUIET:+-q}
+ if test -n "$files"
+ then
+ git reset -- $files
+ git checkout HEAD -- $(git ls-files --modified -- $files)
+ git clean --force --quiet -- $(git ls-files --others -- $files)
+ else
+ git reset --hard ${GIT_QUIET:+-q}
+ fi
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION
+ git clean --force --quiet -d $CLEAN_X_OPTION -- $files
fi
if test "$keep_index" = "t" && test -n "$i_tree"
@@ -627,7 +656,7 @@ clear)
;;
create)
shift
- create_stash "$*" && echo "$w_commit"
+ create_stash -- "$*" && echo "$w_commit"
;;
store)
shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..42bfca873b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,31 @@ test_expect_success 'stash is not confused by partial renames' '
test_path_is_missing file
'
+test_expect_success 'stash --file <filename> stashes and restores the file' '
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ git stash save --file foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+ >foo &&
+ >bar &&
+ >extra &&
+ git add foo bar &&
+ git stash save --file foo --file bar &&
+ test_path_is_missing bar &&
+ test_path_is_missing foo &&
+ test_path_is_file extra &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ test_path_is_file extra
+'
+
test_done
--
2.11.0.258.ge05806da9e.dirty
^ permalink raw reply related
* feature request: allow to stash changed files
From: KES @ 2017-01-15 12:42 UTC (permalink / raw)
To: git
http://stackoverflow.com/questions/3040833/stash-only-one-file-out-of-multiple-files-that-have-changed-with-git#comment32451416_3040833
Sometimes poople are forced to save stash for changed files. But there is no such option (
^ permalink raw reply
* Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
From: Vegard Nossum @ 2017-01-15 10:06 UTC (permalink / raw)
To: Junio C Hamano, René Scharfe; +Cc: git
In-Reply-To: <xmqqy3ydcaia.fsf@gitster.mtv.corp.google.com>
On 15/01/2017 03:39, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>>> I am also more focused on keeping the codebase maintainable in good
>>> health by making sure that we made an effort to find a solution that
>>> is general-enough before solving a single specific problem you have
>>> today. We may end up deciding that a blank-line heuristics gives us
>>> good enough tradeoff, but I do not want us to make a decision before
>>> thinking.
You are right; I appreciate this approach.
>> How about extending the context upward only up to and excluding a line
>> that is either empty *or* a function line? That would limit the extra
>> context to a single function in the worst case.
>>
>> Reducing context at the bottom with the aim to remove comments for the
>> next section is more tricky as it could remove part of the function
>> that we'd like to show if we get the boundary wrong. How bad would it
>> be to keep the southern border unchanged?
>
> I personally do not think there is any robust heuristic other than
> Vegard's "a blank line may be a signal enough that lines before that
> are not part of the beginning of the function", and I think your
> "hence we look for a blank line but if there is a line that matches
> the function header, stop there as we know we came too far back"
> will be a good-enough safety measure.
>
> I also agree with you that we probably do not want to futz with the
> southern border.
You are right, trying to change the southern border in this way is not
quite reliable if there are no empty lines whatsoever and can
erroneously cause the function context to not include the bottom of the
function being changed.
I'm splitting the function boundary detection logic into separate
functions and trying to solve the above case without breaking the tests
(and adding a new test for the above case too).
I'll see if I can additionally provide some toggles (flags or config
variables) to control the new behaviour, what I had in mind was:
-W[=preamble,=no-preamble]
--function-context[=preamble,=no-preamble]
diff.functionContextPreamble = <bool>
(where the new logic is controlled by the new config variable and
overridden by the presence of =preamble or =no-preamble).
Then it also shouldn't be too difficult to add
diff.<driver>.preamble = <regex>
diff.<driver>.xpreamble = <regex>
to override the heuristic used for function border detection in
exceptional cases.
You can argue about the naming now ;-) But I will use this for a start,
renaming/reworking it (or throwing it away) afterwards should be easy
once the code has been written.
Vegard
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: Elia Pinto @ 2017-01-15 8:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqqr345cacz.fsf@gitster.mtv.corp.google.com>
2017-01-15 3:42 GMT+01:00 Junio C Hamano <gitster@pobox.com>:
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> Ok. I agree. But is it strictly necessary to resend for this ?
>
> FWIW, the attacched is what I queued locally, after complaining
> "both have the same title? They need to be explained better."
>
> In any case, I sense that 2/2 will be redone using strbuf, from the
> looks of what is discussed in a subthread nearby?
Yes, it seems to me correct, I resend the patch 2/2 based on the review
thank you all
>
> Thanks.
>
> commit 8d7aa4ba6a00b3ff69261e88b4842c0df5662125
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date: Fri Jan 13 17:58:00 2017 +0000
>
> builtin/commit.c: remove the PATH_MAX limitation via dynamic allocation
>
> Remove the PATH_MAX limitation from the environment setting that
> points to a filename by switching to dynamic allocation.
>
> As a side effect of this change, we also reduce the snprintf()
> calls, that may silently truncate results if the programmer is not
> careful.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> commit 2a7e328877982557d921a398af9442093290c613
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date: Fri Jan 13 17:58:01 2017 +0000
>
> builtin/commit.c: switch to xstrfmt(), instead of snprintf()
>
> Switch to dynamic allocation with xstrfmt(), so we can avoid dealing
> with magic numbers in the code and reduce the cognitive burden from
> the programmers. The original code is correct, but programmers no
> longer have to count bytes needed for static allocation to know that.
>
> As a side effect of this change, we also reduce the snprintf()
> calls, that may silently truncate results if the programmer is not
> careful.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-15 7:47 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1701141904390.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> You stated elsewhere that converting a script into a builtin should focus
> on a faithful conversion.
>
> The original code is:
>
> . "$author_script"
>
> Granted, this *cannot* be converted faithfully without reimplementing a
> shell interpreter. So I did the next best thing: I converted it into code
> that reads a block of environment variable settings.
It is unfortunate that you took "faithful" too literally. While I
do appreciate it if your conversion aims to faithfully replicate the
original, even to be bug-to-bug compatible, but obviously we cannot
replicate everything the above original _could_ do.
By "a faithful conversion", I meant that the behaviour of the
reimplementation should be as faithful to the original's intent as
possible. Nothing more.
The intent of the above original is to read back what we wrote to
preserve the author identity we learned earlier when we wrote the
file. We read the "author" line from the commit object header, and
write assignments to GIT_AUTHOR_{NAME,EMAIL,DATE} variables. Nothing
more is intended.
The end-users COULD abuse the original code to cause it to do a lot
more than that, e.g. by adding "export FOO=BAR" at the end and have
the value of the new environment variable propagate throughout the
code and even down to subprocesses. They can even assign to some
variables we use internally for bookkeeping and break "rebase -i"
machinery. But that is outside the intent of the original code--we
do not need to or want to replicate that faithfully.
I also need to react to the "environment variable settings" at the
end of the quoted paragraph.
If the code in the sequencer.c reads things other than the three
variables we ourselves set, and make them into environment variables
and propagate to subprocesses (hooks and editors), it would be a
bug. The original did not intend to do that (the dot-sourcing is
overly loose than reading three known variables and nothing else,
but is OK because we do not support the case where end users muck
with the file). Also, writing FOO=BAR alone (not "export FOO=BAR"
or "FOO=BAR; export FOO") to the file wouldn't have exported FOO to
subprocesses anyway.
^ permalink raw reply
* Re: merge maintaining history
From: Jacob Keller @ 2017-01-15 6:24 UTC (permalink / raw)
To: David J. Bakeman; +Cc: Git mailing list
In-Reply-To: <58798686.5050401@comcast.net>
On Fri, Jan 13, 2017 at 6:01 PM, David J. Bakeman <nakuru@comcast.net> wrote:
> History
>
> git cloned a remote repository and made many changes pushing them all to
> said repository over many months.
>
> The powers that be then required me to move project to new repository
> server did so by pushing local version to new remote saving all history!
>
> Now have to merge back to original repository(which has undergone many
> changes since I split off) but how do I do that without loosing the
> history of all the commits since the original move? Note I need to push
> changes to files that are already in existence. I found on the web a
> bunch of ways to insert a whole new directory structure into an existing
> repository but as I said I need to do it on top of existing files. Of
> course I can copy all the files from my local working repository to the
> cloned remote repository and commit any changes but I loose all the
> history that way.
>
> Thanks.
If I understand it.. you have two remotes now:
The "origin" remote, which was the original remote you started with.
You have now a "new" remote which you created and pushed to.
So you want to merge the "new" history into the original tree now, so
you checkout the original tree, then "git merge <new-remote>/<branch>"
and then fix up any conflicts, and then git commit to create a merge
commit that has the new history. Then you could push that to both
trees.
I would want a bit more information about your setup before providing
actual commands.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v10 19/20] branch: use ref-filter printing APIs
From: Jacob Keller @ 2017-01-15 5:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list, Junio C Hamano
In-Reply-To: <CAOLa=ZQR9ksPtRw_9FneN26Mjq1TVYx7o=YOM4cDNgrDbuQtXg@mail.gmail.com>
On Sat, Jan 14, 2017 at 2:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Hello,
>
> On Thu, Jan 12, 2017 at 5:17 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Jan 10, 2017 at 12:49 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> diff --git a/builtin/branch.c b/builtin/branch.c
>>> index 34cd61cd9..f293ee5b0 100644
>>> --- a/builtin/branch.c
>>> +++ b/builtin/branch.c
>>> @@ -37,11 +37,11 @@ static unsigned char head_sha1[20];
>>> static int branch_use_color = -1;
>>> static char branch_colors[][COLOR_MAXLEN] = {
>>> GIT_COLOR_RESET,
>>> - GIT_COLOR_NORMAL, /* PLAIN */
>>> - GIT_COLOR_RED, /* REMOTE */
>>> - GIT_COLOR_NORMAL, /* LOCAL */
>>> - GIT_COLOR_GREEN, /* CURRENT */
>>> - GIT_COLOR_BLUE, /* UPSTREAM */
>>> + GIT_COLOR_NORMAL, /* PLAIN */
>>> + GIT_COLOR_RED, /* REMOTE */
>>> + GIT_COLOR_NORMAL, /* LOCAL */
>>> + GIT_COLOR_GREEN, /* CURRENT */
>>> + GIT_COLOR_BLUE, /* UPSTREAM */
>>> };
>>
>>
>> What's... actually changing here? It looks like just white space? Is
>> there a strong reason for why this is changing?
>>
>> Thanks,
>> Jake
>
> None, I'm not sure how this ended up being added too.
>
> --
> Regards,
> Karthik Nayak
It looks like it might just be reformatting of some spaces or stray
end-spaces or something. Hard to see in an email.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-15 2:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1701141857270.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The problem in this instance was that the authorship is no longer retained
> when continuing after resolving a conflict. Let me stress again that this
> has not been a problem with v1 of sequencer-i, nor with v2. The regression
> was caused by changes required by the code review.
>
> In case you wonder: Yes, I am upset by this.
>
> -- snipsnap --
> Subject: [PATCH] fixup! sequencer: make reading author-script more elegant
I do not think anybody asked to make the code "more elegant". Quite
frankly, I do not expect elegance in your code (or any of the code
in our codebase, for that matter). What we want is readable code
that does not make the overall codebase less maintainable that is
correct. Not reinveting a new codepath when there is already code
that does the thing is one of the things that we may need to do, but
that was not done between these rerolls.
Of course, when trying to share code, the existing code we have that
the new codepath needs to borrow would have to be refactored and
extended, and a new bug can sneak in during the process. If that
were what happened, I would be a bit more sympathetic, but I suspect
that this "more elegant" thing that needed fix-up is not that.
You may be upset, but I cannot quite bring myself to feel sympathy
in this particular case.
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: Junio C Hamano @ 2017-01-15 2:42 UTC (permalink / raw)
To: Elia Pinto; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CA+EOSBm_ciQ-7bXuzn4Ba7Q5qqihaYH3Sdkkv+0M0VKWbhk=7w@mail.gmail.com>
Elia Pinto <gitter.spiros@gmail.com> writes:
> Ok. I agree. But is it strictly necessary to resend for this ?
FWIW, the attacched is what I queued locally, after complaining
"both have the same title? They need to be explained better."
In any case, I sense that 2/2 will be redone using strbuf, from the
looks of what is discussed in a subthread nearby?
Thanks.
commit 8d7aa4ba6a00b3ff69261e88b4842c0df5662125
Author: Elia Pinto <gitter.spiros@gmail.com>
Date: Fri Jan 13 17:58:00 2017 +0000
builtin/commit.c: remove the PATH_MAX limitation via dynamic allocation
Remove the PATH_MAX limitation from the environment setting that
points to a filename by switching to dynamic allocation.
As a side effect of this change, we also reduce the snprintf()
calls, that may silently truncate results if the programmer is not
careful.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
commit 2a7e328877982557d921a398af9442093290c613
Author: Elia Pinto <gitter.spiros@gmail.com>
Date: Fri Jan 13 17:58:01 2017 +0000
builtin/commit.c: switch to xstrfmt(), instead of snprintf()
Switch to dynamic allocation with xstrfmt(), so we can avoid dealing
with magic numbers in the code and reduce the cognitive burden from
the programmers. The original code is correct, but programmers no
longer have to count bytes needed for static allocation to know that.
As a side effect of this change, we also reduce the snprintf()
calls, that may silently truncate results if the programmer is not
careful.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
From: Junio C Hamano @ 2017-01-15 2:39 UTC (permalink / raw)
To: René Scharfe; +Cc: Vegard Nossum, git
In-Reply-To: <48bdfd94-2fd4-bd55-d78b-2877e195fb82@web.de>
René Scharfe <l.s.r@web.de> writes:
>> I am also more focused on keeping the codebase maintainable in good
>> health by making sure that we made an effort to find a solution that
>> is general-enough before solving a single specific problem you have
>> today. We may end up deciding that a blank-line heuristics gives us
>> good enough tradeoff, but I do not want us to make a decision before
>> thinking.
>
> How about extending the context upward only up to and excluding a line
> that is either empty *or* a function line? That would limit the extra
> context to a single function in the worst case.
>
> Reducing context at the bottom with the aim to remove comments for the
> next section is more tricky as it could remove part of the function
> that we'd like to show if we get the boundary wrong. How bad would it
> be to keep the southern border unchanged?
I personally do not think there is any robust heuristic other than
Vegard's "a blank line may be a signal enough that lines before that
are not part of the beginning of the function", and I think your
"hence we look for a blank line but if there is a line that matches
the function header, stop there as we know we came too far back"
will be a good-enough safety measure.
I also agree with you that we probably do not want to futz with the
southern border.
Thanks.
^ permalink raw reply
* Re: gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions
From: Junio C Hamano @ 2017-01-15 2:35 UTC (permalink / raw)
To: David Aguilar
Cc: Stefan Beller, Andrew Janke, Paul Mackerras, git@vger.kernel.org
In-Reply-To: <20170114084825.lcecrbtxny3ntulf@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> On Fri, Jan 13, 2017 at 03:20:43AM -0800, David Aguilar wrote:
>>
>> Ping.. it would be nice to get this patch applied.
>
> Sorry for the noise, and thank you Paul for the fix.
> This was already fixed by Paul in gitk@22a713c72df.
>
> I'm sure Junio will merge gitk.git into git.git soon enough so I
> can sit tight until then, but while I'm here I might as well
> send out a pull request:
>
> The following changes since commit 22a713c72df8b6799c59287c50cee44c4a6db51e:
>
> gitk: Follow themed bgcolor in help dialogs (2016-03-19 14:12:21 +1100)
>
> are available in the git repository at:
>
> git://ozlabs.org/~paulus/gitk.git
>
> for you to fetch changes up to fbf426478e540f4737860dae622603cc0daba3d2:
>
> gitk: Update copyright notice to 2016 (2016-12-12 20:46:42 +1100)
Pinging Paul to signal me that his tree is ready to pull from is
appreciated, and asking Paul if his tree is ready to be pulled and
then relaying his answer to me is also fine, but I am sensing that
this message is neither. So let me double check.
Paul, is it a good time to pull, or do you still have something not
published yet that should go together with what you have already
queued?
Thanks.
>
> ----------------------------------------------------------------
> Markus Hitter (3):
> gitk: Turn off undo manager in the text widget
> gitk: Remove closed file descriptors from $blobdifffd
> gitk: Clear array 'commitinfo' on reload
>
> Paul Mackerras (2):
> gitk: Use explicit RGB green instead of "lime"
> gitk: Update copyright notice to 2016
>
> Rogier Goossens (3):
> gitk: Add a 'rename' option to the branch context menu
> gitk: Allow checking out a remote branch
> gitk: Include commit title in branch dialog
>
> Satoshi Yasushima (1):
> gitk: Fix Japanese translation for "marked commit"
>
> Stefan Dotterweich (1):
> gitk: Fix missing commits when using -S or -G
>
> Vasco Almeida (2):
> gitk: Makefile: create install bin directory
> gitk: Add Portuguese translation
>
> Makefile | 1 +
> gitk | 166 +++++--
> po/bg.po | 4 +-
> po/ca.po | 6 +-
> po/de.po | 4 +-
> po/es.po | 4 +-
> po/fr.po | 4 +-
> po/hu.po | 4 +-
> po/it.po | 4 +-
> po/ja.po | 13 +-
> po/pt_br.po | 4 +-
> po/pt_pt.po | 1376 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> po/ru.po | 4 +-
> po/sv.po | 8 +-
> po/vi.po | 4 +-
> 15 files changed, 1549 insertions(+), 57 deletions(-)
> create mode 100644 po/pt_pt.po
>
> Thanks,
^ permalink raw reply
* Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path
From: brian m. carlson @ 2017-01-14 21:57 UTC (permalink / raw)
To: Eric Wong
Cc: Jeff King, Junio C Hamano, Pat Pannuto, Johannes Schindelin,
Johannes Sixt, git
In-Reply-To: <20170114103134.GA586@untitled>
[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]
On Sat, Jan 14, 2017 at 10:31:34AM +0000, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
> > Just as a devil's advocate, why do we care about warnings in third-party
> > modules? Or more specifically, why do _users_ who are running Git care
> > about them? We cannot fix them in Git. A user may report the error to
> > the module author, but the module author may not be responsive, or even
> > may not be inclined to fix the problem (because they have a particular
> > opinion on that warning).
>
> Every user is a potential developer(*). And I do feel
> we (git developers) should be at least somewhat responsible
> for helping maintain and fix the projects we depend on;
> or moving to alternatives if we can't fix them.
>
> There is a chance a newly-introduced warning in a 3rd-party
> module points to a real problem with the way git uses it, too.
> Having that warning would help us fix or workaround the bug
> (either in git or the module).
>
> I doubt any module author would be unresponsive to having a
> localized "no warnings" for special cases. AFAIK, "-w" is
> widespread amongst Perl users (unlike Ruby in my experience).
My experience is that using strict and warnings is so common in Perl
code that absent a compelling documented reason, most Perl developers
would consider it a bug not to use them. Consequently, using -w, while
a good practice, is unlikely to have any practical effect on external
modules.
What is more likely to occur is that as newer versions of Perl come out,
we'll get warnings about questionable constructs that Perl, in its
extensive flexibility, has permitted for a long time, but should really
be fixed. Most distributors of Git will have fixed any affected
third-party modules as part of the Perl upgrade,
> I'm not saying we blindly start using '-w' everywhere today.
> But we may at least try it and see if it introduces new
> warnings, first, and only enable '-w' when it it looks quiet
> (and perhaps start working with module authors to fix warnings
> if not).
>
> As a user, I'd rather have some indication of where something
> might be wrong, than no warning at all when something does go
> wrong.
I'm working on patches for USE_ASCIIDOCTOR=Yes and I found at least two
bugs by enabling warnings.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: problem with insider build for windows and git
From: Johannes Schindelin @ 2017-01-14 18:09 UTC (permalink / raw)
To: Michael Gooch; +Cc: git
In-Reply-To: <1bddbd50-86ea-6c38-6ab8-08336de2ba72@gmail.com>
Hi Michael,
On Thu, 12 Jan 2017, Michael Gooch wrote:
> when running commands like pull and clone I get the following message:
>
> Cygwin WARNING:
> Couldn't compute FAST_CWD pointer. This typically occurs if you're using
> an older Cygwin version on a newer Windows. Please update to the latest
> available Cygwin version from https://cygwin.com/. If the problem
> persists,
> please see https://cygwin.com/problems.html
>
> Windows build is version 1607, OS BUILD 15002.1001
>
> I assume they broke something that cygwin was depending on.
This is not only a known problem, we already have a fix for it, too.
Please note that the recommended way [*1*] to report bugs in Git for
Windows would have led you to this ticket:
https://github.com/git-for-windows/git/issues/1029
Please also note that this is only a warning, not an error. Even so, the
problem has been reported independently several times...
There have been enough real fixes have been accumulated in Git for
Windows' `master` branch, and the next official Git version is far enough
in the future [*2*] that it was time for a Git for Windows version,
including the fix for the FAST_CWD warning.
Ciao,
Johannes
Footnote *1*: https://git-for-windows.github.io/#contribute
Footnote *2*: http://tinyurl.com/gitCal suggests that Git v2.12.0 will be
released early February soon after Git Merge (and Git for Windows v2.12.0
will follow soon thereafter), and with no patches applied to the `maint`
branch since v2.11.0, I do actually not expect any v2.11.1 to happen
before v2.12.0 comes out.
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Johannes Schindelin @ 2017-01-14 18:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqinpnuaxl.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 9 Jan 2017, Junio C Hamano wrote:
> I however think that the renaming of read_author_script() is totally
> backwards from maintainability's point of view.
You stated elsewhere that converting a script into a builtin should focus
on a faithful conversion.
The original code is:
. "$author_script"
Granted, this *cannot* be converted faithfully without reimplementing a
shell interpreter. So I did the next best thing: I converted it into code
that reads a block of environment variable settings.
What you asked for is totally unreasonable: you ask me to make this
conversion *even less faithful*.
What is worse: you argue from the "maintainability point of view", when it
is pretty obvious that *adding validation that was not there before* can
in no way make the code more maintainable, as it *adds new logic*.
And what is the worst: over all these discussions about a nothingburger
(you simply cannot convince me that I should introduce validating code
that has not been there before in the same patch series that simply tries
to recreate existing functionality), the most important part of a code
review was forgotten: to make sure that the changes are correct.
The worst direction a code review can take is to introduce regressions,
and that is exactly what happened.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: Jeff King @ 2017-01-14 18:08 UTC (permalink / raw)
To: René Scharfe; +Cc: Elia Pinto, git
In-Reply-To: <3165803b-6073-de97-bf33-71ad21108d6f@web.de>
On Sat, Jan 14, 2017 at 05:31:39PM +0100, René Scharfe wrote:
> Perhaps I missed it from the discussion, but why not use strbuf? It
> would avoid counting the generated string's length. That's probably
> not going to make a measurable difference performance-wise, but it's
> easy to avoid and doesn't even take up more lines:
It was mentioned in the review of the very first patch, but I think it
just got dropped. I agree the strbuf way is nicer (not because of
efficiency, but because it's simply easier to read and follow).
-Peff
^ permalink raw reply
* Re: [PATCH v3 01/38] sequencer: avoid unnecessary curly braces
From: Jeff King @ 2017-01-14 18:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, Kevin Daudt, Dennis Kaarsemaker,
Stephan Beyer
In-Reply-To: <alpine.DEB.2.20.1701141856240.3469@virtualbox>
On Sat, Jan 14, 2017 at 06:57:13PM +0100, Johannes Schindelin wrote:
> On Thu, 12 Jan 2017, Junio C Hamano wrote:
>
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> > >
> > > - if (!commit->parents) {
> > > + if (!commit->parents)
> > > parent = NULL;
> > > - }
> > > else if (commit->parents->next) {
> > > /* Reverting or cherry-picking a merge commit */
> > > int cnt;
> >
> > The result becomes
> >
> > if (...)
> > single statement;
> > else if (...) {
> > multiple;
> > statements;
> > }
> >
> > which is not quite an improvement.
>
> Yet, this used to be the coding style of Git, and your statement comes
> quite as a surprise to me.
Yeah, I thought we were OK with:
if (cond)
single statement;
else {
multiple;
statements;
}
but not the other way around:
if (cond) {
multiple;
statements;
} else
single statement;
I don't know if the "else if" changes that or not, but I certainly have
written things like your patch does.
-Peff
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Johannes Schindelin @ 2017-01-14 18:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqinpnuaxl.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 9 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Changes since v2:
> >
> > - fixed a TRANSLATORS: comment
> > ...
> > - replaced a spawned `diff-tree` command by code using the diff functions
> > directly
>
> I just finished skimming the interdiff (the difference between the
> result of merging the v2 into 'master' and the result of applying
> this series on 'master').
I wish you would not have skimmed it, but provided a thorough review.
There was a rather serious bug in this (not the first problem introduced
into one of my patch series *because of* code review, unhidden-git and
mmap-regexec are also very recent examples, I really should learn to
resist the prodding to replace well-tested code with code of unknown
correctness).
The problem in this instance was that the authorship is no longer retained
when continuing after resolving a conflict. Let me stress again that this
has not been a problem with v1 of sequencer-i, nor with v2. The regression
was caused by changes required by the code review.
In case you wonder: Yes, I am upset by this.
The required fixup patch is:
-- snipsnap --
Subject: [PATCH] fixup! sequencer: make reading author-script more elegant
An unfortunate regression of formerly battle-tested code sadly crept
into Git for Windows v2.11.0(2): authorship was not retained in case of
conflicts during picks.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 2 +-
t/t3404-rebase-interactive.sh | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 73b2ec6894..8ecab02291 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -612,7 +612,7 @@ static int read_env_script(struct argv_array *env)
count++;
}
- for (i = 0; i < count; i++) {
+ for (i = 0, p = script.buf; i < count; i++) {
argv_array_push(env, p);
p += strlen(p) + 1;
}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 71b9c8ef8b..61113be08a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -237,6 +237,22 @@ test_expect_success 'retain authorship' '
git show HEAD | grep "^Author: Twerp Snog"
'
+test_expect_success 'retain authorship w/ conflicts' '
+ git reset --hard twerp &&
+ test_commit a conflict a conflict-a &&
+ git reset --hard twerp &&
+ GIT_AUTHOR_NAME=AttributeMe \
+ test_commit b conflict b conflict-b &&
+ set_fake_editor &&
+ test_must_fail git rebase -i conflict-a &&
+ echo resolved >conflict &&
+ git add conflict &&
+ git rebase --continue &&
+ test $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&
+ git show >out &&
+ grep AttributeMe out
+'
+
test_expect_success 'squash' '
git reset --hard twerp &&
echo B > file7 &&
--
2.11.0.windows.3
^ permalink raw reply related
* Re: [PATCH v3 01/38] sequencer: avoid unnecessary curly braces
From: Johannes Schindelin @ 2017-01-14 17:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqk2a0ktxr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 12 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> >
> > - if (!commit->parents) {
> > + if (!commit->parents)
> > parent = NULL;
> > - }
> > else if (commit->parents->next) {
> > /* Reverting or cherry-picking a merge commit */
> > int cnt;
>
> The result becomes
>
> if (...)
> single statement;
> else if (...) {
> multiple;
> statements;
> }
>
> which is not quite an improvement.
Yet, this used to be the coding style of Git, and your statement comes
quite as a surprise to me.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 06/38] sequencer (rebase -i): implement the 'edit' command
From: Johannes Schindelin @ 2017-01-14 17:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqq7f60kssh.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 12 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +static int make_patch(struct commit *commit, struct replay_opts *opts)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + struct rev_info log_tree_opt;
> > + const char *subject, *p;
> > + int res = 0;
> > +
> > + p = short_commit_name(commit);
> > + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> > + return -1;
> > +
> > + strbuf_addf(&buf, "%s/patch", get_dir(opts));
> > + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> > + init_revisions(&log_tree_opt, NULL);
> > + log_tree_opt.abbrev = 0;
> > + log_tree_opt.diff = 1;
> > + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> > + log_tree_opt.disable_stdin = 1;
> > + log_tree_opt.no_commit_id = 1;
> > + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> > + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> > + if (!log_tree_opt.diffopt.file)
> > + res |= error_errno(_("could not open '%s'"), buf.buf);
> > + else {
> > + res |= log_tree_commit(&log_tree_opt, commit);
> > + fclose(log_tree_opt.diffopt.file);
> > + }
> > + strbuf_reset(&buf);
> > +
> > + strbuf_addf(&buf, "%s/message", get_dir(opts));
> > + if (!file_exists(buf.buf)) {
> > + const char *commit_buffer = get_commit_buffer(commit, NULL);
> > + find_commit_subject(commit_buffer, &subject);
> > + res |= write_message(subject, strlen(subject), buf.buf, 1);
> > + unuse_commit_buffer(commit, commit_buffer);
> > + }
> > + strbuf_release(&buf);
> > +
> > + return res;
> > +}
>
> Unlike the scripted version, where a merge is shown with "diff --cc"
> and a root commit is shown as "Root commit", this only deals with a
> single-parent commit.
Indeed. The reason is that we never encounter a merge commit (as we
explicitly do not handle --preserve-merges) nor root commits (as we
explicitly do not handle --root)
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: René Scharfe @ 2017-01-14 16:31 UTC (permalink / raw)
To: Elia Pinto, git
In-Reply-To: <20170113175801.39468-2-gitter.spiros@gmail.com>
Am 13.01.2017 um 18:58 schrieb Elia Pinto:
> In this patch, instead of using xnprintf instead of snprintf, which asserts
> that we don't truncate, we are switching to dynamic allocation with xstrfmt(),
> , so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from
> the programmers, because they no longer have to count bytes needed for static allocation.
> As a side effect of this patch we have also reduced the snprintf() calls, that may silently truncate
> results if the programmer is not careful.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is the third version of the patch.
>
> Changes from the first version: I have split the original commit in two, as discussed here
> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
>
> Changes from the second version:
> - Changed the commit message to clarify the purpose of the patch (
> as suggested by Junio)
> https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
>
>
>
> builtin/commit.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 09bcc0f13..37228330c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
> static int run_rewrite_hook(const unsigned char *oldsha1,
> const unsigned char *newsha1)
> {
> - /* oldsha1 SP newsha1 LF NUL */
> - static char buf[2*40 + 3];
> + char *buf;
> struct child_process proc = CHILD_PROCESS_INIT;
> const char *argv[3];
> int code;
> - size_t n;
>
> argv[0] = find_hook("post-rewrite");
> if (!argv[0])
> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
> code = start_command(&proc);
> if (code)
> return code;
> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> sigchain_push(SIGPIPE, SIG_IGN);
> - write_in_full(proc.in, buf, n);
> + write_in_full(proc.in, buf, strlen(buf));
> close(proc.in);
> + free(buf);
> sigchain_pop(SIGPIPE);
> return finish_command(&proc);
> }
>
Perhaps I missed it from the discussion, but why not use strbuf? It
would avoid counting the generated string's length. That's probably
not going to make a measurable difference performance-wise, but it's
easy to avoid and doesn't even take up more lines:
---
builtin/commit.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 711f96cc43..73bb72016f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1525,12 +1525,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
{
- /* oldsha1 SP newsha1 LF NUL */
- static char buf[2*40 + 3];
+ struct strbuf sb = STRBUF_INIT;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
- size_t n;
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,11 +1544,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
code = start_command(&proc);
if (code)
return code;
- n = snprintf(buf, sizeof(buf), "%s %s\n",
- sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+ strbuf_addf(&sb, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(proc.in, buf, n);
+ write_in_full(proc.in, sb.buf, sb.len);
close(proc.in);
+ strbuf_release(&sb);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
From: René Scharfe @ 2017-01-14 14:58 UTC (permalink / raw)
To: Junio C Hamano, Vegard Nossum; +Cc: git
In-Reply-To: <xmqqbmvaecpl.fsf@gitster.mtv.corp.google.com>
Am 14.01.2017 um 00:56 schrieb Junio C Hamano:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
>
>> The patch will work as intended and as expected for 95% of the users out
>> there (javadoc, Doxygen, kerneldoc, etc. all have the comment
>> immediately preceding the function) and fixes a very real problem for me
>> (and I expect many others) _today_; for the remaining 5% (who put a
>> blank line between their comment and the start of the function) it will
>> revert back to the current behaviour, so there should be no regression
>> for them.
>
> I notice your 95% are all programming languages, but I am more
> worried about the contents written in non programming languages
> (René gave HTML an an example--there may be other types of contents
> that we programmer types do not deal with every day, but Git users
> depend on).
>
> I am also more focused on keeping the codebase maintainable in good
> health by making sure that we made an effort to find a solution that
> is general-enough before solving a single specific problem you have
> today. We may end up deciding that a blank-line heuristics gives us
> good enough tradeoff, but I do not want us to make a decision before
> thinking.
How about extending the context upward only up to and excluding a line
that is either empty *or* a function line? That would limit the extra
context to a single function in the worst case.
Reducing context at the bottom with the aim to remove comments for the
next section is more tricky as it could remove part of the function that
we'd like to show if we get the boundary wrong. How bad would it be to
keep the southern border unchanged?
René
^ permalink raw reply
* Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
From: Elia Pinto @ 2017-01-14 12:42 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20170113183309.GA28002@google.com>
Ok. I agree. But is it strictly necessary to resend for this ?
Thanks
2017-01-13 19:33 GMT+01:00 Brandon Williams <bmwill@google.com>:
> On 01/13, Elia Pinto wrote:
>> In this patch, instead of using xnprintf instead of snprintf, which asserts
>> that we don't truncate, we are switching to dynamic allocation with xstrfmt(),
>> , so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from
>> the programmers, because they no longer have to count bytes needed for static allocation.
>> As a side effect of this patch we have also reduced the snprintf() calls, that may silently truncate
>> results if the programmer is not careful.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>
> Small nit's with the commit message:
> * Stray comma ',' of on its own
> * lines are longer than 80 characters
>
>> ---
>> This is the third version of the patch.
>>
>> Changes from the first version: I have split the original commit in two, as discussed here
>> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
>>
>> Changes from the second version:
>> - Changed the commit message to clarify the purpose of the patch (
>> as suggested by Junio)
>> https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
>>
>>
>>
>> builtin/commit.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 09bcc0f13..37228330c 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>> static int run_rewrite_hook(const unsigned char *oldsha1,
>> const unsigned char *newsha1)
>> {
>> - /* oldsha1 SP newsha1 LF NUL */
>> - static char buf[2*40 + 3];
>> + char *buf;
>> struct child_process proc = CHILD_PROCESS_INIT;
>> const char *argv[3];
>> int code;
>> - size_t n;
>>
>> argv[0] = find_hook("post-rewrite");
>> if (!argv[0])
>> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>> code = start_command(&proc);
>> if (code)
>> return code;
>> - n = snprintf(buf, sizeof(buf), "%s %s\n",
>> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> sigchain_push(SIGPIPE, SIG_IGN);
>> - write_in_full(proc.in, buf, n);
>> + write_in_full(proc.in, buf, strlen(buf));
>> close(proc.in);
>> + free(buf);
>> sigchain_pop(SIGPIPE);
>> return finish_command(&proc);
>> }
>> --
>> 2.11.0.154.g5f5f154
>>
>
> --
> Brandon Williams
^ permalink raw reply
* [ANNOUNCE] Git for Windows 2.11.0(3)
From: Johannes Schindelin @ 2017-01-14 12:09 UTC (permalink / raw)
To: git-for-windows, git; +Cc: Johannes Schindelin
MIME-Version: 1.0
Fcc: Sent
Dear Git users,
It is my pleasure to announce that Git for Windows 2.11.0(3) is available from:
https://git-for-windows.github.io/
Changes since Git for Windows v2.11.0(2) (January 13th 2017)
Bug Fixes
• Fixed an off-by-two bug in the POSIX emulation layer that possibly
affected third-party Perl scripts that load native libraries
dynamically.
• A regression in rebase -i, introduced into v2.11.0(2), which caused
commit attribution to be mishandled after resolving conflicts, was
fixed.
Filename | SHA-256
-------- | -------
Git-2.11.0.3-64-bit.exe | c3897e078cd7f7f496b0e4ab736ce144c64696d3dbee1e5db417ae047ca3e27f
Git-2.11.0.3-32-bit.exe | dff9bec9c4e21eaba5556fe4a7b1071d1f18e3a8b9645bffb48fda9eaee37e62
PortableGit-2.11.0.3-64-bit.7z.exe | 41a4ab3a1f0c88a3254b5a30d49c0c6e4ef06c204ddce53e23fc15d6e56f8d24
PortableGit-2.11.0.3-32-bit.7z.exe | 8bf3769c37945e991903dd1b988c6b1d97bbf0f3afc9851508974f38bf94dc01
MinGit-2.11.0.3-64-bit.zip | bf3714e04bcbafb464353235a27c328c43d40568d6b2e9064f1a63444b8236c5
MinGit-2.11.0.3-32-bit.zip | db05d5e98ef1017dd07f27fa4641dc8c0d66ba09da5a196ef656e7f1d7c078e2
Git-2.11.0.3-64-bit.tar.bz2 | 65296c54f0b8294374547cc6a169d6ea95178e12dee04cd2d4632f39d8fe7852
Git-2.11.0.3-32-bit.tar.bz2 | 0f0e2f78fc9b91d6c860eb7de742f3601b0ccd13c5c61444c7cf55b00bcb4ed4
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox