* Inconsistent results of git blame --porcelain when detecting copies from other files
From: Sokolov, Konstantin @ 2017-02-20 17:59 UTC (permalink / raw)
To: git@vger.kernel.org
Hi Folks!
The issue is best explained on an example. You can reproduce it using the Lucene repo https://github.com/apache/lucene-solr.git. Tested with the following versions: 1.8.1.6 (Ubuntu), 2.11.0.windows.1, 2.11.1.windows.1.
First, let's produce the correct results without using --procelain:
> git blame --show-name --show-number -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java
The following excerpt shows lines 501-505 from the output. In particular we can see that lines 502-503 originate from IndexReader.java.
10ba9abeb208d37df985e95a742f756de067353f lucene/src/java/org/apache/lucene/index/DirectoryReader.java 501 501) * <p>This method
^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java 496 502) * returns the version recorded in the commit that the
^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java 497 503) * reader opened. This version is advanced every time
^d1165b19726fa0cd13a539827a7cd43237a4fee lucene/src/java/org/apache/lucene/index/IndexReader.java 498 504) * a change is made with {@link IndexWriter}.</p>
10ba9abeb208d37df985e95a742f756de067353f lucene/src/java/org/apache/lucene/index/DirectoryReader.java 505 505) */
The same information can be obtained as well by using --line-porcelain:
> git blame --show-name --show-number --line-porcelain -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java
Here is the output for line 502:
d1165b19726fa0cd13a539827a7cd43237a4feef 496 502 3
author Michael McCandless
author-mail <mikemccand@apache.org>
author-time 1327877325
author-tz +0000
committer Michael McCandless
committer-mail <mikemccand@apache.org>
committer-time 1327877325
committer-tz +0000
summary LUCENE-3725: add optional packing to FSTs
boundary
filename lucene/src/java/org/apache/lucene/index/IndexReader.java
* returns the version recorded in the commit that the
However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:
> git blame --show-name --show-number --porcelain -s -w --abbrev=40 -C -C -C d1165b19726fa0cd13a539827a7cd43237a4feef..10ba9abeb208d37df985e95a742f756de067353f --not f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 -- lucene/src/java/org/apache/lucene/index/DirectoryReader.java
10ba9abeb208d37df985e95a742f756de067353f 501 501 1
author Uwe Schindler
author-mail <uschindler@apache.org>
author-time 1327879145
author-tz +0000
committer Uwe Schindler
committer-mail <uschindler@apache.org>
committer-time 1327879145
committer-tz +0000
summary Reverse merged revision(s) from lucene/dev/trunk up to 1237502
previous f5dba8b76709ff0ef8715b8b288a4b64d4993fa3 lucene/src/java/org/apache/lucene/index/DirectoryReader.java
filename lucene/src/java/org/apache/lucene/index/DirectoryReader.java
* <p>This method
d1165b19726fa0cd13a539827a7cd43237a4feef 496 502 3
* returns the version recorded in the commit that the
d1165b19726fa0cd13a539827a7cd43237a4feef 497 503
* reader opened. This version is advanced every time
d1165b19726fa0cd13a539827a7cd43237a4feef 498 504
This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.
Thanks for any feedback.
Kind Regards
Konstantin Sokolov
^ permalink raw reply
* Sending informational messages from upload-pack
From: Lukas Fleischer @ 2017-02-20 18:38 UTC (permalink / raw)
To: git
Hi,
It would be handy to be able to show a message to the user when
cloning/fetching from a repository (e.g. to show a warning if a
repository is deprecated). This should technically already be possible
using the current pack protocol and sidebands. However, to my knowledge,
there is no easy way to configure this on the server side; writing a
wrapper around git-upload-pack(1) or replacing git-upload-pack(1) seem
to be the only options.
What I have in mind is something like a post-upload hook whose stdout
and stderr are redirected to sideband 2 and 3, respectively. The commit
message of 20b20a22f (upload-pack: provide a hook for running
pack-objects, 2016-05-18) suggests that such a hook should be
implemented as a "config variable hook" rather than a regular hook.
One could think of additional parameters passed to such a hook. For the
purposes I intend to use this, no parameters are needed. However, a
fixed per-repository MOTD would be too inflexible since we are using
namespaces and database accesses to determine whether a repository is
"deprecated".
Am I missing any "easy" already supported way to add such messages
without patching Git or writing a git-upload-pack(1) wrapper? If not,
does this sound general and useful enough to become an official feature?
Are there any alternative suggestions on how to display such messages?
Regards,
Lukas
^ permalink raw reply
* Re: Sending informational messages from upload-pack
From: Jeff King @ 2017-02-20 19:21 UTC (permalink / raw)
To: Lukas Fleischer; +Cc: git
In-Reply-To: <148761588216.31363.15518967793189077139@typhoon>
On Mon, Feb 20, 2017 at 07:38:02PM +0100, Lukas Fleischer wrote:
> It would be handy to be able to show a message to the user when
> cloning/fetching from a repository (e.g. to show a warning if a
> repository is deprecated). This should technically already be possible
> using the current pack protocol and sidebands. However, to my knowledge,
> there is no easy way to configure this on the server side; writing a
> wrapper around git-upload-pack(1) or replacing git-upload-pack(1) seem
> to be the only options.
I wouldn't recommend wrapping upload-pack. You don't know you have a
sideband until partway through the upload-pack conversation. And clients
do not expect sideband at all until we get to the pack-sending part of
the protocol (I think; I just quickly verified the location of the
demuxer async code in fetch-pack.c, but I didn't dig into it in depth).
So I don't think you can do a MOTD or similar in a backwards-compatible
way. You're only allowed to talk if the conversation results in an
actual pack being sent.
> What I have in mind is something like a post-upload hook whose stdout
> and stderr are redirected to sideband 2 and 3, respectively. The commit
> message of 20b20a22f (upload-pack: provide a hook for running
> pack-objects, 2016-05-18) suggests that such a hook should be
> implemented as a "config variable hook" rather than a regular hook.
Yeah, because of the "upload-pack is special and untrusted" rule, this
can't be a regular hook. I think the config mechanism used by 20b20a22f
would be the right approach.
If my fetch-pack assertion above is right, technically the hook added by
20b20a22f is sufficient for your purposes, if your hook looks like:
echo >&2 "pre-pack message"
git pack-objects "$@"
echo >72 "post-pack message"
but I would not be opposed to having pre-/post- hooks that run
separately, if only for the convenience of the admin.
> One could think of additional parameters passed to such a hook. For the
> purposes I intend to use this, no parameters are needed. However, a
> fixed per-repository MOTD would be too inflexible since we are using
> namespaces and database accesses to determine whether a repository is
> "deprecated".
There was a proposed post-upload-pack hook a long time ago that
collected clone/fetch stats, and we used it at GitHub for many years.
These days we use something much more invasive that dumps stats from
every git invocation over a Unix socket.
> Am I missing any "easy" already supported way to add such messages
> without patching Git or writing a git-upload-pack(1) wrapper? If not,
> does this sound general and useful enough to become an official feature?
> Are there any alternative suggestions on how to display such messages?
I don't think there's any other mechanism to do what you're asking,
aside from the hook in 20b20a22f.
-Peff
^ permalink raw reply
* Re: Cross-referencing the Git mailing list archive with their corresponding commits in `pu`
From: Junio C Hamano @ 2017-02-20 19:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Josh Triplett, git
In-Reply-To: <alpine.DEB.2.20.1702171841450.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> There is a third category, and this one *does* come as a surprise to me.
> It appears that at least *some* patches' Date: lines are either ignored or
> overridden or changed on their way from the mailing list into Git's commit
> history. There was only one commit in that commit range:
>
> 3c0cb0c (read_loose_refs(): read refs using resolve_ref_recursively(),
> Michael Haggerty 2017-02-09)
>
> This one was committed with an author date "Thu, 09 Feb 2017 21:53:52
> +0100" but it appears that there was no mail sent to the Git mailing list
I think this is this one:
<ff0b0df6-9aed-9417-d9d4-1234d53f05c3@alum.mit.edu>
Recent "What's cooking" lists the topic this one is part with this
comment:
The tip one is newer than the one posted to the list but was sent
privately by the author via his GitHub repository.
^ permalink raw reply
* Re: [PATCH v6 0/6] stash: support pathspec argument
From: Junio C Hamano @ 2017-02-20 19:39 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, Øyvind A Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> @@ -55,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
>
> Save your local modifications to a new 'stash', and run `git reset
> --hard` to revert them. The <message> part is optional and gives
I didn't notice this during v5 review, but the above seems to be
based on the codebase before your documentation update (which used
to be part of the series in older iteration). I had to tweak the
series to apply on top of your 20a7e06172 ("Documentation/stash:
remove mention of git reset --hard", 2017-02-12) while queuing, so
please double check the result when it is pushed out to 'pu'.
Thanks.
^ permalink raw reply
* Re: Cross-referencing the Git mailing list archive with their corresponding commits in `pu`
From: Junio C Hamano @ 2017-02-20 20:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Josh Triplett, git
In-Reply-To: <xmqqefysk67v.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> There is a third category, and this one *does* come as a surprise to me.
>> It appears that at least *some* patches' Date: lines are either ignored or
>> overridden or changed on their way from the mailing list into Git's commit
>> history. There was only one commit in that commit range:
>>
>> 3c0cb0c (read_loose_refs(): read refs using resolve_ref_recursively(),
>> Michael Haggerty 2017-02-09)
>>
>> This one was committed with an author date "Thu, 09 Feb 2017 21:53:52
>> +0100" but it appears that there was no mail sent to the Git mailing list
>
> I think this is this one:
>
> <ff0b0df6-9aed-9417-d9d4-1234d53f05c3@alum.mit.edu>
>
> Recent "What's cooking" lists the topic this one is part with this
> comment:
>
> The tip one is newer than the one posted to the list but was sent
> privately by the author via his GitHub repository.
We didn't have any pull from sub-maintainers during the period you
checked, but when we do, those could also fall into the category.
Even though I see some l10n patches Cc'ed to the list, I won't be
surprised if not everything that is sent to Jiang Xin (i18n/l10n
coordinator) is, for example. It also is OK for sub-maintainers to
have their own commit to describe or otherwise improve their area
and without sending a patch before doing so if they deem it
appropriate [*1*].
I actually think automation like yours would help another category:
There is a newer version of the series or an entirely new series on
the list, but the project's tree has not picked them up (yet).
I from time to time sweep my inbox in an attempt to find and pick up
leftover bits. Sometimes the authors remind me by pinging [*2*],
which greatly helps. But another set of eyeballs that may be
enhanced with a mechanised filter that catches "messages without
corresponding commits", which is the opposite of this "third"
category, would be of great help, too [*3*].
[Footnote]
*1* ... like trivial fixes, for example, at their discretion. After
all we entrusted their own area and we should give them the
flexibility they can exercise with good taste ;-).
*2* e.g. <2f67fc21-92f9-a03e-1b09-a237af6dbc46@alum.mit.edu>
*3* ... even if a mechanised filter alone might strike too many
false positives.
^ permalink raw reply
* Re: slightly confusing message
From: Junio C Hamano @ 2017-02-20 20:19 UTC (permalink / raw)
To: Leon George; +Cc: git, Brandon Williams
In-Reply-To: <7cca3326-d6b5-4669-7256-ab275567b72e@georgemail.eu>
Leon George <leon@georgemail.eu> writes:
> Every once in a while this:
>
> £ git add -p .
> warning: empty strings as pathspecs will be made invalid in upcoming
> releases. please use . instead if you meant to match all paths
> No changes.
>
> It seems to happen only when there are no more modified files and git
> still works wonderfully otherwise - just wanted to let you know.
This sounds vaguely familiar and indeed I think it is this one:
https://public-inbox.org/git/CAEnOLdvG=SoKFxeJ_pLmamGj_8osC+28TSg+pbFLLTr+ZLcpQA@mail.gmail.com/
which was from late last year.
I suspect that the issue may already be fixed at the tip of 'master'
(aka Git 2.12-rc2).
Thanks.
^ permalink raw reply
* Re: [PATCH 2/5] hashmap: allow memihash computation to be continued
From: Junio C Hamano @ 2017-02-20 20:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702201342020.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> If an extra call level really matters, its "inline" equivalent in
>> the header would probably be good.
>
> Well, the hashing is supposed to be as fast as possible, so I would like
> to avoid that extra call level. However, the end result is not so pretty
> because FNV32_BASE needs to be made public (OTOH it removes more lines
> than it adds):
I think our usual answer is "can we measure the difference to
demonstrate that the overhead for an extra call matter?"
As two functions sit next to each other in a single file, the code
duplication does not bother me _that_ much. A single liner
/* keep implementations of these two in sync */
in front of these two functions would not hurt, but whoever attempts
to come up with a better hash needs to stare at this file carefully
anyway, so lack of such carefulness probably wouldn't be too big an
issue, either.
But the above 8 lines are something we need to worry about after we
definitely know that we MUST have two independent functions that are
supposed to be kept in sync; a patch that makes us worry them before
we know is a premature optimization, and that bothers me even more
than the actual code duplication that can drift apart.
^ permalink raw reply
* Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Junio C Hamano @ 2017-02-20 20:30 UTC (permalink / raw)
To: Siddharth Kannan
Cc: Git List, Matthieu Moy, Pranit Bauva, Jeff King, pclouds,
brian m. carlson
In-Reply-To: <CAN-3QhoXBnLWyfuUsuvvRMYNnoupMrQHxE_G=ysyA_14KX4Yrw@mail.gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> On 17 February 2017 at 00:38, Junio C Hamano <gitster@pobox.com> wrote:
>> Having said all that, I do not think the remainder of the code is
>> prepared to take "-", not yet anyway [*1*], so turning "-" into
>> "@{-1}" this patch does before it calls get_sha1_basic(), while it
>> is not an ideal final state, is probably an acceptable milestone to
>> stop at.
>
> So, is it okay to stop with just supporting "-" and not support things
> like "-@{yesterday}"?
If the approach to turn "-" into "@{-1}" at that spot you did will
cause "-@{yesterday}" to barf, then I'd say so be it for now ;-).
We can later spread the understanding of "-" to functions deeper in
the callchain and add support for that, no?
>> It is a separate matter if this patch is sufficient to produce
>> correct results, though. I haven't studied the callers of this
>> change to make sure yet, and may find bugs in this approach later.
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Alex Hoffman @ 2017-02-20 20:31 UTC (permalink / raw)
To: Oleg Taranenko
Cc: Jakub Narębski, Jacob Keller, Johannes Sixt,
Christian Couder, Stephan Beyer, git
In-Reply-To: <CABEd3j8m5D=bBbUD+uzvE9c8AwdBEM79Np7Pnin-RLL=Hjq06A@mail.gmail.com>
I see two different problems each with a different assumption (see the
definition of "bisectable" in the email of Junio C Hamano):
1. (Current) Assume the entire history graph is bisectable. DO: Search
where in the entire graph the first 'trait'/transition occurs.
2. (New) Assume only the graph between one good commit and one bad
commit is bisectable. DO: Search where the first transition occurs in
the graph between these two commits.
It seems that the real world needs a solution also for the second
problem (example if the good commit is the FIRST good commit of a
feature or if the good commit is not the first good commit, but you
definitely know, that it broke first somewhere in between the good and
bad commit).
I find the way to go as Oleg proposed is gittish enough (with a new
parameter --strategy). Beside I would underline that also the second
problem is a bisect problem, just for another graph, thus it makes
perfect sense to extend 'git bisect' for this.
Does that look reasonably?
^ permalink raw reply
* Re: url.<base>.insteadOf vs. submodules
From: Toolforger @ 2017-02-20 20:31 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170220090115.6kfzwl62opj4q7k7@sigill.intra.peff.net>
On 20.02.2017 10:01, Jeff King wrote:
> On Sun, Feb 19, 2017 at 10:12:28PM +0100, Toolforger wrote:
>
>> I am trying to make url.<base>.insteadOf work on the URLs inside
>> .gitmodules, but it won't work (applying it to the repo itself works fine,
>> to the config setting seems to be fine).
>
> The submodule operations happen in their own processes, and do not look
> at the config of the parent repo.
Ah, then we have a docbug.
git help config has this to say:
url.<base>.insteadOf
Any URL that starts with this value will be rewritten to start,
instead, with <base>.
The "Any" here is wrong, it would be "any except submodule" (possibly
other exceptions).
> Are you setting the config in
> .git/config of the super-project?
Exactly.
My thinking was that since the submodule URLs are specified in the super
project's .gitmodules, that setting should apply.
> I don't know if there plans to make that work,
It would certainly help me out, though I guess it's going to be too late
for my current project :-)
> but one workaround is to set the config in ~/.gitconfig.
No can do - that's under version control.
My personal setup does not belong there I think ;-)
I am currently trying to write a shell script that
- does git submodule init
- pulls submodule configuration out of git config -l
- configures each submodule with insteadOf
It fits with my workflow because setting up the repositories is going to
be done via script anyway.
I'm neither a shell nor a git expert, so any advice still appreciated.
Regards,
Jo
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Jakub Narębski @ 2017-02-20 20:35 UTC (permalink / raw)
To: Alex Hoffman, Oleg Taranenko
Cc: Jacob Keller, Johannes Sixt, Christian Couder, Stephan Beyer, git
In-Reply-To: <CAMX8fZUNHmouUsgEY3+0CmTaEp+y5b1-Cp8Nk3OttTc30v0R5A@mail.gmail.com>
W dniu 20.02.2017 o 21:31, Alex Hoffman pisze:
> I see two different problems each with a different assumption (see the
> definition of "bisectable" in the email of Junio C Hamano):
>
> 1. (Current) Assume the entire history graph is bisectable. DO: Search
> where in the entire graph the first 'trait'/transition occurs.
>
> 2. (New) Assume only the graph between one good commit and one bad
> commit is bisectable. DO: Search where the first transition occurs in
> the graph between these two commits.
If `git bisect` is/would be affected by `git log` history-related options
then this is what `--strict-ancestor` option gives/would give.
--
Jakub Narębski
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Alex Hoffman @ 2017-02-20 20:39 UTC (permalink / raw)
To: Jakub Narębski
Cc: Oleg Taranenko, Jacob Keller, Johannes Sixt, Christian Couder,
Stephan Beyer, git
In-Reply-To: <ca1ef8f6-58cf-4994-d1bf-39e04b42dd4c@gmail.com>
> If `git bisect` is/would be affected by `git log` history-related options
> then this is what `--strict-ancestor` option gives/would give.
Exactly my thoughts. All that needs to be changed in the 2nd problem
is the graph where to search.
But first we must agree about the usefulness of the 2nd problem. Any
thoughts/comments/votes for/against?
^ permalink raw reply
* Re: url.<base>.insteadOf vs. submodules
From: Jeff King @ 2017-02-20 20:52 UTC (permalink / raw)
To: Toolforger; +Cc: git
In-Reply-To: <404d109f-e5a7-85a3-e64c-ab1b21c3045d@durchholz.org>
On Mon, Feb 20, 2017 at 09:31:40PM +0100, Toolforger wrote:
> > The submodule operations happen in their own processes, and do not look
> > at the config of the parent repo.
>
> Ah, then we have a docbug.
> git help config has this to say:
>
> url.<base>.insteadOf
> Any URL that starts with this value will be rewritten to start,
> instead, with <base>.
>
> The "Any" here is wrong, it would be "any except submodule" (possibly other
> exceptions).
I'm not sure that "any" is wrong here. Repository-specific config does
not cross repository boundaries. That applies to this config value, and
to all the others, too (e.g., if you set "diff.renames" in the
super-project, it would not have an effect in the submodule).
I think if there is a doc bug, it is that the repo boundary between the
submodule and the super-project is not made more clear.
That said, I do think it would be a useful feature for the super-project
to rewrite URLs before handing them off to the submodule. But I do not
really work on submodules nor use them myself, so there may be
complications.
I suppose you could argue that failing to rewrite violates the "any" in
the quoted text. It doesn't say when the rewriting occurs, but it is
essentially "when the URL is accessed". So the super-project feeds the
raw URL to the submodule `git clone`, which then applies any URL
rewriting.
> > but one workaround is to set the config in ~/.gitconfig.
>
> No can do - that's under version control.
> My personal setup does not belong there I think ;-)
I'm not sure I understand. You have a project policy to use certain
URLs. But you, the user, want to override that. Why isn't the
user-specific config file the right place to put that?
(I think there _is_ a mismatch, in that the change is specific not just
to your user, but to the repo. So you would not want to rewrite other
references to the same URL in other repos. But that does not seem to be
your objection).
-Peff
^ permalink raw reply
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-02-20 21:25 UTC (permalink / raw)
To: Mike Crowe; +Cc: git
In-Reply-To: <20170220153322.GA8352@mcrowe.com>
Mike Crowe <mac@mcrowe.com> writes:
> I think that if there's a risk that file contents will undergo conversion
> then this should force the diff to check the file contents. Something like:
>
> diff --git a/diff.c b/diff.c
> index 051761b..bee1662 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3413,13 +3413,14 @@ void diff_setup_done(struct diff_options *options)
> /*
> * Most of the time we can say "there are changes"
> * only by checking if there are changed paths, but
> - * --ignore-whitespace* options force us to look
> - * inside contents.
> + * --ignore-whitespace* options or text conversion
> + * force us to look inside contents.
> */
>
> if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
> DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
> - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
> + DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) ||
> + DIFF_OPT_TST(options, ALLOW_TEXTCONV))
> DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
> else
> DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
Thanks.
You may be on the right track to find FROM_CONTENTS bit, but
I think TEXTCONV bit is a red-herring.
This part of diff.c caught my attention, while thinking about this
topic:
if (output_format & DIFF_FORMAT_NO_OUTPUT &&
DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
/*
* run diff_flush_patch for the exit status. setting
* options->file to /dev/null should be safe, because we
* aren't supposed to produce any output anyway.
*/
and the body of this "if" statement loops over q->queue[]. It is
about the "even though we prefer not having to format the patch
because we are doing --quiet, we need to see if contents of one and
two that we _know_ are different are made into the same thing when
we compare them while ignoring various forms of whitespace changes".
So one and two that are removed in earlier step because they were
truly identical may not be penalized if you flip FROM_CONTENTS bit
early on.
Having said that, DIFF_FROM_CONTENTS is about all paths this options
structure governs, not just paths that have eol conversion defined.
When you say "diff --ignore-whitespace-change", that applies to all
paths. The eol conversion is specified per-path, so ideally the
FROM_CONTENTS bit should be flipped if and only if one or more of
the paths would need the conversion (i.e. does the helper function
would_convert_to_git() say "yes" to the path?). I however suspect
that necessary information to do so (i.e. "which paths we are
looking at?") has not been generated yet at the point of the code
you quoted. setup comes (and must come) very early, and then
q->queue[] is populated by different front-end functions that
compare trees, the index, and the working tree, depending on the
"git diff" option or "git diff-{tree,index,files}" plumbing command,
and you can ask "would one of these paths need conversion?" only
after q->queue[] is populated. Hmm.....
Another thing is that ALLOW_TEXTCONV is not a right bit to check for
your example. It is "do we use textconv filters to turn binary
files into a (phony) text representation before comparing?". People
use the mechanism to throw JPEG photos in Git and have textconv
filter to extract only EXIF data, and "diff --textconv" would let us
textually compare only the EXIF data (which is the only human
readable part of the contents anyway).
It might be a good idea to also flip FROM_CONTENTS bit under "diff
--textconv", and ...
> This solves the problem for me and my test case now passes.
... but I suspect that you were misled to think it fixes your issue,
only because "--textconv" is by default enabled for "git diff" and
"git log" (see "git diff --help"). I think you are saying that if
you always set FROM_CONTENTS bit on, you get what you want. But
that is to be expected and it unfortunately penalizes everybody by
turning an obvious optimization permanently off.
Also "--textconv" is not on by default for the plumbing "git
diff-index" command and its friends, so it is likely that "git
diff-index HEAD" with your change will still not work as you expect.
A cheap (from code-change point of view) band-aid might be to flip
FROM_CONTENTS on if we know the repository has _some_ paths that
need eol conversion, even when the particular "diff" we are taking
does not involve any eol conversion (e.g. "is core.crlf set?").
While it may be better than "if we are porcelain (aka ALLOW_TEXTCONV
is set), unconditionally flip FROM_CONTENTS on", it is not ideal,
either.
This almost makes me suspect that the place that checks lengths of
one and two in order to refrain from running more expensive content
comparison you found earlier need to ask would_convert_to_git()
before taking the short-cut, something along the lines of this (for
illustration purposes only, not even compile-tested). The "almost"
comes to me because I do not offhand know the performance implications
of making calls to would_convert_to_git() here.
diff.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index 051761be40..094d5913da 100644
--- a/diff.c
+++ b/diff.c
@@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
* differences.
*
* 2. At this point, the file is known to be modified,
- * with the same mode and size, and the object
- * name of one side is unknown. Need to inspect
- * the identical contents.
+ * with the same mode and size, the object
+ * name of one side is unknown, or size comparison
+ * cannot be depended upon. Need to inspect the
+ * contents.
*/
if (!DIFF_FILE_VALID(p->one) || /* (1) */
!DIFF_FILE_VALID(p->two) ||
@@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
(p->one->mode != p->two->mode) ||
diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
- (p->one->size != p->two->size) ||
+
+ /*
+ * only if eol and other conversions are not involved,
+ * we can say that two contents of different sizes
+ * cannot be the same without checking their contents.
+ */
+ (!would_convert_to_git(p->one->path) &&
+ !would_convert_to_git(p->two->path) &&
+ (p->one->size != p->two->size)) ||
+
!diff_filespec_is_identical(p->one, p->two)) /* (2) */
p->skip_stat_unmatch_result = 1;
return p->skip_stat_unmatch_result;
^ permalink raw reply related
* Re: Inconsistent results of git blame --porcelain when detecting copies from other files
From: Junio C Hamano @ 2017-02-20 21:30 UTC (permalink / raw)
To: Sokolov, Konstantin; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <71BF70CE41AEE741896AF3A5450D86F11F4268FF@DEFTHW99EH3MSX.ww902.siemens.net>
"Sokolov, Konstantin" <konstantin.sokolov.ext@siemens.com> writes:
> However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:
> ...
> This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.
Hmph, this sounds vaguely familiar with
http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net
which is part of Git 2.12-rc0
^ permalink raw reply
* Re: Inconsistent results of git blame --porcelain when detecting copies from other files
From: Jeff King @ 2017-02-20 22:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sokolov, Konstantin, git@vger.kernel.org
In-Reply-To: <xmqqd1ecim8a.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 20, 2017 at 01:30:29PM -0800, Junio C Hamano wrote:
> "Sokolov, Konstantin" <konstantin.sokolov.ext@siemens.com> writes:
>
> > However, when using --porcelain DirectoryReader.java is reported as the origin of lines 502-504:
> > ...
> > This is not only inconsistent with the other outputs but the output is also inconsistent in itself because lines 496 -498 do not even exist in a previous version of DirectoryReader.java.
>
> Hmph, this sounds vaguely familiar with
>
> http://public-inbox.org/git/20170106042051.nwjiuyyp7ljhs4sr@sigill.intra.peff.net
>
> which is part of Git 2.12-rc0
Yeah, I had the same thought while reading Konstantin's report.
I'm not sure Git is wrong, though. I think it's just the way the
porcelain output works.
Here's a minimal reproduction; the interesting thing is when a commit is
mentioned twice (as happens on lines 1 and 5 here):
git init repo
cd repo
# use long lines to make sure we trigger content-movement detection
for i in $(seq 1 5); do
echo this is really long line number $i
done >file
git add file
git commit -m initial
sed 's/1/one/; s/5/five/' <file >renamed
git rm file
git add renamed
git commit -m 'rename and use english'
git blame renamed
git blame --line-porcelain renamed
git blame --porcelain renamed
The first blame output looks something like this:
bab03701 renamed ... line number 1
^dda1349 file ... line number 2
^dda1349 file ... line number 3
^dda1349 file ... line number 4
bab03701 renamed ... line number 5
so we can see it's the same case. The --line-porcelain similarly matches
the commits and filenames.
But the --porcelain output is:
bab037010dcabaf0509db27bf232d25659b180fa 1 1 1
...
filename renamed
this is really long line number one
dda1349d41da859f4c37e018dbed714ba6c1aa18 2 2 3
...
filename file
this is really long line number 2
dda1349d41da859f4c37e018dbed714ba6c1aa18 3 3
this is really long line number 3
dda1349d41da859f4c37e018dbed714ba6c1aa18 4 4
this is really long line number 4
bab037010dcabaf0509db27bf232d25659b180fa 5 5 1
this is really long line number five
You might be tempted to say that the fifth line comes from "filename
file", because that was the last "filename" entry we saw. But that's
_not_ how the porcelain output works. That "filename" entry was
associated with dda1349, but the line comes from bab0370 here.
The simplest way (IMHO) to parse --porcelain output is:
- maintain a mapping of commit sha1s to the commit's details
- whenever you see a "<sha1> <line_nr> <orig_nr> [<size-of-hunk>]"
line, any key-value fields which follow impact _only_ that sha1, and
you should update the details for that map entry
- when you see the actual tab-indented line content, you have gotten
all of the key-value updates for that sha1. You can now safely do
what you like with the line entry.
Another way, if you don't want to update your mapping, is to actually
pay attention to the size-of-hunk headers. In this case the middle three
lines come in their own hunk (which you can see from the "2 2 3" header
on the second line). The "filename" field we get applies to that hunk,
but once we switch to a different one, the filename field needs to be
looked up in the commit mapping.
But it's definitely not correct to blindly apply one "filename" field to
subsequent lines in other hunks.
And yes, I do think this is probably more complex than it needs to be.
I didn't write it. And I don't think it is worth the backwards
compatibility headache of trying to change it now. It's possible this
could be better documented (I didn't look at the documentation to write
that explanation; I happened to puzzle it out for somebody else
recently who had a similar case. That's what led to the bug-fix in the
message you linked).
-Peff
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Philip Oakley @ 2017-02-20 22:24 UTC (permalink / raw)
To: Alex Hoffman, Oleg Taranenko, Jakub Narębski
Cc: Jacob Keller, Johannes Sixt, Christian Couder, Stephan Beyer, git
In-Reply-To: <ca1ef8f6-58cf-4994-d1bf-39e04b42dd4c@gmail.com>
From: "Jakub Narębski" <jnareb@gmail.com>
>W dniu 20.02.2017 o 21:31, Alex Hoffman pisze:
>> I see two different problems each with a different assumption (see the
>> definition of "bisectable" in the email of Junio C Hamano):
>>
>> 1. (Current) Assume the entire history graph is bisectable. DO: Search
>> where in the entire graph the first 'trait'/transition occurs.
>>
>> 2. (New) Assume only the graph between one good commit and one bad
>> commit is bisectable. DO: Search where the first transition occurs in
>> the graph between these two commits.
>
> If `git bisect` is/would be affected by `git log` history-related options
> then this is what `--strict-ancestor` option gives/would give.
>
isn't that spelt `--ancestry-path` ?
(--ancestry-path has it's own issues such as needing an --first-parent-show
option, but that's possibly a by the by)
--
Philip
^ permalink raw reply
* Re: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
From: brian m. carlson @ 2017-02-21 0:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Junio C Hamano, Ramsay Jones
In-Reply-To: <20170220080902.vkexezd5solnhrhb@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 501 bytes --]
On Mon, Feb 20, 2017 at 03:09:02AM -0500, Jeff King wrote:
> It's a little disturbing that we do not seem to have even a basic test
> of:
>
> git rev-list --parents HEAD | git diff-tree --stdin
>
> which would exercise this code.
I'm unsure, so I'll add a test. The only way to know if it works is to
test it.
--
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: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
From: Jeff King @ 2017-02-21 1:08 UTC (permalink / raw)
To: brian m. carlson, git, Michael Haggerty, Junio C Hamano,
Ramsay Jones
In-Reply-To: <20170221002519.55d4mlljia3mposi@genre.crustytoothpaste.net>
On Tue, Feb 21, 2017 at 12:25:19AM +0000, brian m. carlson wrote:
> On Mon, Feb 20, 2017 at 03:09:02AM -0500, Jeff King wrote:
> > It's a little disturbing that we do not seem to have even a basic test
> > of:
> >
> > git rev-list --parents HEAD | git diff-tree --stdin
> >
> > which would exercise this code.
>
> I'm unsure, so I'll add a test. The only way to know if it works is to
> test it.
Not to spoil the ending, but I did test and it does not work. :)
-Peff
^ permalink raw reply
* [PATCH v2 0/4] delete_ref: support reflog messages
From: Kyle Meyer @ 2017-02-21 1:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
Thanks to Junio and Peff for their feedback on v1.
https://public-inbox.org/git/20170217035800.13214-1-kyle@kyleam.com/T/#u
Changes from v1:
* "msg" is now positioned as the first argument to delete_ref() to
match update_ref().
20170217081205.zn7j6d5cffgdvfbn@sigill.intra.peff.net
* A "delete by head" test case has been added for the update-ref
change.
20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net
* The added tests no longer send grep's output to /dev/null.
20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net
* Renaming the current branch is represented by two entries in HEAD's
log, both which reuse the log message passed to rename_ref().
20170217083112.vn7m4udsopmlvnn5@sigill.intra.peff.net,
20170217195549.z6uyy7hbbhj5avh7@sigill.intra.peff.net
* Corrected a few places in the commit messages where "delete_refs"
was written instead of "delete_ref".
Kyle Meyer (4):
delete_ref: accept a reflog message argument
update-ref: pass reflog message to delete_ref()
rename_ref: replace empty message in HEAD's log
branch: record creation of renamed branch in HEAD's log
branch.c | 5 +++--
branch.h | 3 ++-
builtin/am.c | 4 ++--
builtin/branch.c | 7 ++++---
builtin/notes.c | 4 ++--
builtin/remote.c | 4 ++--
builtin/replace.c | 2 +-
builtin/reset.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/tag.c | 2 +-
builtin/update-ref.c | 2 +-
fast-import.c | 2 +-
refs.c | 6 +++---
refs.h | 7 ++++---
refs/files-backend.c | 10 +++++-----
t/t1400-update-ref.sh | 18 ++++++++++++++++++
t/t3200-branch.sh | 6 ++++++
transport.c | 2 +-
18 files changed, 58 insertions(+), 30 deletions(-)
--
2.11.1
^ permalink raw reply
* [PATCH v2 1/4] delete_ref: accept a reflog message argument
From: Kyle Meyer @ 2017-02-21 1:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>
When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
with an empty message. In preparation for adding a more meaningful
message to HEAD's log in these cases, update delete_ref() to take a
message argument and pass it along to ref_transaction_delete().
Modify all callers to pass NULL for the new message argument; no
change in behavior is intended.
Note that this is relevant for HEAD's log but not for the deleted
ref's log, which is currently deleted along with the ref. Even if it
were not, an entry for the deletion wouldn't be present in the deleted
ref's log. files_transaction_commit() writes to the log if
REF_NEEDS_COMMIT or REF_LOG_ONLY are set, but lock_ref_for_update()
doesn't set REF_NEEDS_COMMIT for the deleted ref because REF_DELETING
is set. In contrast, the update for HEAD has REF_LOG_ONLY set by
split_head_update(), resulting in the deletion being logged.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
builtin/am.c | 4 ++--
builtin/branch.c | 2 +-
builtin/notes.c | 4 ++--
builtin/remote.c | 4 ++--
builtin/replace.c | 2 +-
builtin/reset.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/tag.c | 2 +-
builtin/update-ref.c | 2 +-
fast-import.c | 2 +-
refs.c | 6 +++---
refs.h | 4 ++--
refs/files-backend.c | 6 +++---
transport.c | 2 +-
14 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..f7a7a971f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
} else {
write_state_text(state, "abort-safety", "");
if (!state->rebasing)
- delete_ref("ORIG_HEAD", NULL, 0);
+ delete_ref(NULL, "ORIG_HEAD", NULL, 0);
}
/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
has_curr_head ? &curr_head : NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
- delete_ref(curr_branch, NULL, REF_NODEREF);
+ delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..8f8242e07 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,7 +251,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
goto next;
}
- if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
+ if (delete_ref(NULL, name, is_null_sha1(sha1) ? NULL : sha1,
REF_NODEREF)) {
error(remote_branch
? _("Error deleting remote-tracking branch '%s'")
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..4b492abd4 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
* notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
*/
- if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+ if (delete_ref(NULL, "NOTES_MERGE_PARTIAL", NULL, 0))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
- if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+ if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NODEREF))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..2b415911b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, &flag);
if (!(flag & REF_ISSYMREF))
continue;
- if (delete_ref(item->string, NULL, REF_NODEREF))
+ if (delete_ref(NULL, item->string, NULL, REF_NODEREF))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
head_name = xstrdup(states.heads.items[0].string);
free_remote_ref_states(&states);
} else if (opt_d && !opt_a && argc == 1) {
- if (delete_ref(buf.buf, NULL, REF_NODEREF))
+ if (delete_ref(NULL, buf.buf, NULL, REF_NODEREF))
result |= error(_("Could not delete %s"), buf.buf);
} else
usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..226d0f952 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
static int delete_replace_ref(const char *name, const char *ref,
const unsigned char *sha1)
{
- if (delete_ref(ref, sha1, 0))
+ if (delete_ref(NULL, ref, sha1, 0))
return 1;
printf("Deleted replace ref '%s'\n", name);
return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfc..fc3b906c4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -256,7 +256,7 @@ static int reset_refs(const char *rev, const struct object_id *oid)
update_ref_oid(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
- delete_ref("ORIG_HEAD", old_orig->hash, 0);
+ delete_ref(NULL, "ORIG_HEAD", old_orig->hash, 0);
set_reflog_message(&msg, "updating HEAD", rev);
update_ref_status = update_ref_oid(msg.buf, "HEAD", oid, orig, 0,
UPDATE_REFS_MSG_ON_ERR);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 96eed9446..70addef15 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -58,7 +58,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
die("Cannot delete %s, not a symbolic ref", argv[0]);
if (!strcmp(argv[0], "HEAD"))
die("deleting '%s' is not allowed", argv[0]);
- return delete_ref(argv[0], NULL, REF_NODEREF);
+ return delete_ref(NULL, argv[0], NULL, REF_NODEREF);
}
switch (argc) {
diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a967..c23d6d4bc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -97,7 +97,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
static int delete_tag(const char *name, const char *ref,
const unsigned char *sha1, const void *cb_data)
{
- if (delete_ref(ref, sha1, 0))
+ if (delete_ref(NULL, ref, sha1, 0))
return 1;
printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7f30d3a76..86d006d36 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -433,7 +433,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
* For purposes of backwards compatibility, we treat
* NULL_SHA1 as "don't care" here:
*/
- return delete_ref(refname,
+ return delete_ref(NULL, refname,
(oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
flags);
else
diff --git a/fast-import.c b/fast-import.c
index 64fe602f0..6c13472c4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1752,7 +1752,7 @@ static int update_branch(struct branch *b)
if (is_null_sha1(b->sha1)) {
if (b->delete)
- delete_ref(b->name, NULL, 0);
+ delete_ref(NULL, b->name, NULL, 0);
return 0;
}
if (read_ref(b->name, old_sha1))
diff --git a/refs.c b/refs.c
index cd36b64ed..053d23a90 100644
--- a/refs.c
+++ b/refs.c
@@ -591,8 +591,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
return 0;
}
-int delete_ref(const char *refname, const unsigned char *old_sha1,
- unsigned int flags)
+int delete_ref(const char *msg, const char *refname,
+ const unsigned char *old_sha1, unsigned int flags)
{
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -603,7 +603,7 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
- flags, NULL, &err) ||
+ flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
diff --git a/refs.h b/refs.h
index 9fbff90e7..5880886a7 100644
--- a/refs.h
+++ b/refs.h
@@ -276,8 +276,8 @@ int reflog_exists(const char *refname);
* exists, regardless of its old value. It is an error for old_sha1 to
* be NULL_SHA1. flags is passed through to ref_transaction_delete().
*/
-int delete_ref(const char *refname, const unsigned char *old_sha1,
- unsigned int flags);
+int delete_ref(const char *msg, const char *refname,
+ const unsigned char *old_sha1, unsigned int flags);
/*
* Delete the specified references. If there are any problems, emit
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c041d4ba2..299eb4d8a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,7 +2489,7 @@ static int files_delete_refs(struct ref_store *ref_store,
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
- if (delete_ref(refname, NULL, flags))
+ if (delete_ref(NULL, refname, NULL, flags))
result |= error(_("could not remove reference %s"), refname);
}
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
+ if (delete_ref(NULL, oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
}
@@ -2630,7 +2630,7 @@ static int files_rename_ref(struct ref_store *ref_store,
*/
if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
sha1, NULL) &&
- delete_ref(newrefname, NULL, REF_NODEREF)) {
+ delete_ref(NULL, newrefname, NULL, REF_NODEREF)) {
if (errno==EISDIR) {
struct strbuf path = STRBUF_INIT;
int result;
diff --git a/transport.c b/transport.c
index d72e08948..269352b22 100644
--- a/transport.c
+++ b/transport.c
@@ -299,7 +299,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
if (verbose)
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
if (ref->deletion) {
- delete_ref(rs.dst, NULL, 0);
+ delete_ref(NULL, rs.dst, NULL, 0);
} else
update_ref("update by push", rs.dst,
ref->new_oid.hash, NULL, 0, 0);
--
2.11.1
^ permalink raw reply related
* [PATCH v2 2/4] update-ref: pass reflog message to delete_ref()
From: Kyle Meyer @ 2017-02-21 1:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>
Now that delete_ref() accepts a reflog message, pass the user-provided
message to delete_ref() rather than silently dropping it.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
builtin/update-ref.c | 2 +-
t/t1400-update-ref.sh | 18 ++++++++++++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 86d006d36..0b2ecf41a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -433,7 +433,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
* For purposes of backwards compatibility, we treat
* NULL_SHA1 as "don't care" here:
*/
- return delete_ref(NULL, refname,
+ return delete_ref(msg, refname,
(oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
flags);
else
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b0ffc0b57..6e112fb5f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,6 +85,24 @@ test_expect_success "delete $m (by HEAD)" '
'
rm -f .git/$m
+test_expect_success "deleting current branch adds message to HEAD's log" '
+ git update-ref $m $A &&
+ git symbolic-ref HEAD $m &&
+ git update-ref -m delete-$m -d $m &&
+ ! test -f .git/$m &&
+ grep "delete-$m$" .git/logs/HEAD
+'
+rm -f .git/$m
+
+test_expect_success "deleting by HEAD adds message to HEAD's log" '
+ git update-ref $m $A &&
+ git symbolic-ref HEAD $m &&
+ git update-ref -m delete-by-head -d HEAD &&
+ ! test -f .git/$m &&
+ grep "delete-by-head$" .git/logs/HEAD
+'
+rm -f .git/$m
+
test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
--
2.11.1
^ permalink raw reply related
* [PATCH v2 4/4] branch: record creation of renamed branch in HEAD's log
From: Kyle Meyer @ 2017-02-21 1:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>
Renaming the current branch adds an event to the current branch's log
and to HEAD's log. However, the logged entries differ. The entry in
the branch's log represents the entire renaming operation (the old and
new hash are identical), whereas the entry in HEAD's log represents
the deletion only (the new sha1 is null).
Extend replace_each_worktree_head_symref(), whose only caller is
branch_rename(), to take a reflog message argument. This allows the
creation of the new ref to be recorded in HEAD's log. As a result,
the renaming event is represented by two entries (a deletion and a
creation entry) in HEAD's log.
It's a bit unfortunate that the branch's log and HEAD's log now
represent the renaming event in different ways. Given that the
renaming operation is not atomic, the two-entry form is a more
accurate representation of the operation and is more useful for
debugging purposes if a failure occurs between the deletion and
creation events. It would make sense to move the branch's log to the
two-entry form, but this would involve changes to how the rename is
carried out and to how the update flags and reflogs are processed for
deletions, so it may not be worth the effort.
Based-on-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
branch.c | 5 +++--
branch.h | 3 ++-
builtin/branch.c | 5 +++--
refs.h | 3 ++-
refs/files-backend.c | 4 ++--
t/t3200-branch.sh | 5 +++--
6 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
branch, wt->path);
}
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg)
{
int ret = 0;
struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
continue;
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
- newref)) {
+ newref, logmsg)) {
ret = -1;
error(_("HEAD of working tree %s is not updated"),
worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int ignore_current_worktree);
* This will be used when renaming a branch. Returns 0 if successful, non-zero
* otherwise.
*/
-extern int replace_each_worktree_head_symref(const char *oldref, const char *newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg);
#endif
diff --git a/builtin/branch.c b/builtin/branch.c
index 8f8242e07..e1f97dcfc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -579,14 +579,15 @@ static void rename_branch(const char *oldname, const char *newname, int force)
if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
- strbuf_release(&logmsg);
if (recovery)
warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 11);
- if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+ if (replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ strbuf_release(&logmsg);
+
strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
strbuf_release(&oldref);
strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 5880886a7..e529f4c3a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, const char *logmsg);
* $GIT_DIR points to.
* Return 0 if successful, non-zero otherwise.
* */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+ const char *logmsg);
enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f6e7c192c..42b137bb1 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3055,7 +3055,7 @@ static int files_create_symref(struct ref_store *ref_store,
return ret;
}
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
{
static struct lock_file head_lock;
struct ref_lock *lock;
@@ -3083,7 +3083,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
lock->lk = &head_lock;
lock->ref_name = xstrdup(head_rel);
- ret = create_symref_locked(lock, head_rel, target, NULL);
+ ret = create_symref_locked(lock, head_rel, target, logmsg);
unlock_ref(lock); /* will free lock */
strbuf_release(&head_path);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 0dbc54003..d84837d08 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,9 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
'
-test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD' '
msg="Branch: renamed refs/heads/baz to refs/heads/bam" &&
- grep " 0\{40\}.*$msg$" .git/logs/HEAD
+ grep " 0\{40\}.*$msg$" .git/logs/HEAD &&
+ grep "^0\{40\}.*$msg$" .git/logs/HEAD
'
test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
--
2.11.1
^ permalink raw reply related
* [PATCH v2 3/4] rename_ref: replace empty message in HEAD's log
From: Kyle Meyer @ 2017-02-21 1:10 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170221011035.847-1-kyle@kyleam.com>
When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message. Now that delete_ref()
accepts a reflog message, provide a more descriptive message by
passing along the log message that is given to rename_ref().
The next step will be to extend HEAD's log to also include the second
part of the rename, the creation of the new branch.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
refs/files-backend.c | 2 +-
t/t3200-branch.sh | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 299eb4d8a..f6e7c192c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
- if (delete_ref(NULL, oldrefname, orig_sha1, REF_NODEREF)) {
+ if (delete_ref(logmsg, oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..0dbc54003 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,11 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
'
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+ msg="Branch: renamed refs/heads/baz to refs/heads/bam" &&
+ grep " 0\{40\}.*$msg$" .git/logs/HEAD
+'
+
test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
--
2.11.1
^ permalink raw reply related
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