* 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: 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: [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: [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: 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: 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: [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 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: 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
* 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
* 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
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Lars Schneider @ 2017-02-20 17:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jonathan Tan, git, sbeller
In-Reply-To: <xmqqino5jia8.fsf@gitster.mtv.corp.google.com>
> On 20 Feb 2017, at 10:58, Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I still haven't queued any of the variants I posted (and I do not
>> think other people sent their own versions, either). I need to pick
>> one and queue, with a test or two. Perhaps after -rc2.
>>
>> Others are welcome to work on it while I cut -rc2 tomorrow, so that
>> by the time I see their patch all that is left for me to do is to
>> apply it ;-)
>
> Since nothing seems to have happened in the meantime, here is what
> I'll queue so that we won't forget for now. Lars's tests based on
> how the scripted "git submodule" uses "git config" may still be
> valid, but it is somewhat a roundabout way to demonstrate the
> breakage and not very effective way to protect the fix, so I added a
> new test that directly tests "git -c <var>=<val> <command>".
Agreed. Please ignore my tests.
If you want to you could queue this tiny cleanup, though:
http://public-inbox.org/git/20170215113325.14393-1-larsxschneider@gmail.com/
> ...
>
> +/*
> + * downcase the <section> and <variable> in <section>.<variable> or
> + * <section>.<subsection>.<variable> and do so in place. <subsection>
> + * is left intact.
> + */
> +static void canonicalize_config_variable_name(char *varname)
> +{
> + char *cp, *last_dot;
What does cp stand for? "char pointer"?
> +
> + /* downcase the first segment */
> + for (cp = varname; *cp; cp++) {
> + if (*cp == '.')
> + break;
> + *cp = tolower(*cp);
> + }
> + if (!*cp)
> + return;
> +
> + /* scan for the last dot */
> + for (last_dot = cp; *cp; cp++)
> + if (*cp == '.')
> + last_dot = cp;
> +
> + /* downcase the last segment */
> + for (cp = last_dot; *cp; cp++)
> + *cp = tolower(*cp);
> +}
> +
> int git_config_parse_parameter(const char *text,
> config_fn_t fn, void *data)
> {
> @@ -221,7 +249,7 @@ int git_config_parse_parameter(const char *text,
> strbuf_list_free(pair);
> return error("bogus config parameter: %s", text);
> }
> - strbuf_tolower(pair[0]);
> + canonicalize_config_variable_name(pair[0]->buf);
> if (fn(pair[0]->buf, value, data) < 0) {
> strbuf_list_free(pair);
> return -1;
> diff --git a/t/t1351-config-cmdline.sh b/t/t1351-config-cmdline.sh
> new file mode 100755
> index 0000000000..acb8dc3b15
> --- /dev/null
> +++ b/t/t1351-config-cmdline.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description='git -c var=val'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'last one wins: two level vars' '
> + echo VAL >expect &&
> +
> + # sec.var and sec.VAR are the same variable, as the first
> + # and the last level of a configuration variable name is
> + # case insensitive. Test both setting and querying.
> +
> + git -c sec.var=val -c sec.VAR=VAL config --get sec.var >actual &&
> + test_cmp expect actual &&
> + git -c SEC.var=val -c sec.var=VAL config --get sec.var >actual &&
> + test_cmp expect actual &&
> +
> + git -c sec.var=val -c sec.VAR=VAL config --get SEC.var >actual &&
> + test_cmp expect actual &&
> + git -c SEC.var=val -c sec.var=VAL config --get sec.VAR >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'last one wins: three level vars' '
> + echo val >expect &&
> +
> + # v.a.r and v.A.r are not the same variable, as the middle
> + # level of a three-level configuration variable name is
> + # case sensitive. Test both setting and querying.
> +
> + git -c v.a.r=val -c v.A.r=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c v.A.r=VAL config --get V.a.R >actual &&
> + test_cmp expect actual &&
> +
> + echo VAL >expect &&
> + git -c v.a.r=val -c v.a.R=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c V.a.r=VAL config --get v.a.r >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c v.a.R=VAL config --get V.a.R >actual &&
> + test_cmp expect actual &&
> + git -c v.a.r=val -c V.a.r=VAL config --get V.a.R >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
> --
> 2.12.0-rc2-221-g8fa194a99f
>
Looks good to me!
Thank you,
Lars
^ permalink raw reply
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-02-20 15:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20170217221958.GA12163@mcrowe.com>
On Friday 17 February 2017 at 22:19:58 +0000, Mike Crowe wrote:
> On Friday 17 February 2017 at 14:05:17 -0800, Junio C Hamano wrote:
> > Mike Crowe <mac@mcrowe.com> writes:
> >
> > > If "git diff --quiet" finds it necessary to compare actual file contents,
> > > and a file requires CRLF conversion, then it incorrectly exits with an exit
> > > code of 1 even if there have been no changes.
> > >
> > > The patch below adds a test file that shows the problem.
> >
> > If "git diff" does not show any output and "git diff --exit-code" or
> > "git diff --quiet" says there are differences, then it is a bug.
> >
> > I would however have expected that any culprit would involve a code
> > that says "under QUICK option, we do not have to bother doing
> > this". The part you quoted:
> >
> > > if (!DIFF_FILE_VALID(p->one) || /* (1) */
> > > !DIFF_FILE_VALID(p->two) ||
> > > (p->one->oid_valid && p->two->oid_valid) ||
> > > (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) ||
> > > !diff_filespec_is_identical(p->one, p->two)) /* (2) */
> > > p->skip_stat_unmatch_result = 1;
> >
> > is used by "git diff" with and without "--quiet", afacr, so I
> > suspect that the bug lies somewhere else.
>
> I can't say that I really understand the code fully, but it appears that
> the first pass generates a queue of files that contain differences. The
> result of the quiet diff comes from the size of that queue,
> diff_queued_diff.nr, being non-zero in diffcore_std. I'm assuming that the
> result of the noisy diff comes from actually comparing the files.
>
> But, I've only spent a short while looking so I may have got the wrong end
> of the stick.
Tricking Git into checking the actual file contents (by passing
--ignore-space-change for example) is sufficient to ensure that the exit
status is never 1 when it should be zero. (Of course that option has other
unwanted effects in this case.)
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);
This solves the problem for me and my test case now passes. Unfortunately
it breaks the 'removing and adding subproject' test case in
t3040-subprojects-basic at the line:
test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD
presumably because after the rename has been detected the file contents are
identical. :( A rename of a single file appears to still be handled
correctly.
Mike.
^ permalink raw reply related
* Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-20 14:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Matthieu Moy, Pranit Bauva, Jeff King, pclouds,
brian m. carlson
In-Reply-To: <xmqq8tp6x8b6.fsf@gitster.mtv.corp.google.com>
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}"?
Matthieu's comments on the matter:
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> As per Matthieu's comments, I have updated the tests, but there
is still one
> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
Note that I did not request that these things work, just that they seem
to be relevant tests: IMHO it's OK to reject them, but for example we
don't want them to segfault. And having a test is a good hint that you
thought about what could happen and to document it.
[Quoted from email <vpqa89mnl4z.fsf@anie.imag.fr>]
>
> 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.
>
--
Best Regards,
- Siddharth Kannan.
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Oleg Taranenko @ 2017-02-20 13:50 UTC (permalink / raw)
To: Jakub Narębski
Cc: Jacob Keller, Alex Hoffman, Johannes Sixt, Christian Couder,
Stephan Beyer, git
In-Reply-To: <58d25138-de2e-6995-444f-79c3ac0bbad2@gmail.com>
> Anyway, I don't think it is feasible to weaken the assumption that there
> is only one transition from 'old' ('good') to 'new' ('bad'); this is
> what allows O(log(N)) operations. See bisection method of root finding,
> that is finding zeros of a continuous function.
I don't intended to change default behaviour of git bisect, I can
estimate what drawback it could bring.
I'd rather implement something like
git bisect start --bisect-strategy=missing-feature
by default current state is being used.
git config value would be also useful.
Oleg
^ permalink raw reply
* Re: [PATCH v4 00/15] Remove submodule from files-backend.c
From: Duy Nguyen @ 2017-02-20 12:47 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <cdf01f55-f3df-d3ce-149c-0249a03d27bf@alum.mit.edu>
On Mon, Feb 20, 2017 at 7:42 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/18/2017 02:32 PM, Nguyễn Thái Ngọc Duy wrote:
>> v4:
>
> I very much like the direction of this patch series. I reviewed the
> design pretty carefully and left some comments about it, and skimmed
> through the code changes. But I haven't yet reviewed the code in detail.
> I'll wait for your reaction to my design comments before doing so.
Unless there are some mails in transit, thanks for the review. The
comments I haven't replied to usually mean "I agree" (but not writing
unless I could add anything after).
--
Duy
^ permalink raw reply
* Re: [PATCH v4 00/15] Remove submodule from files-backend.c
From: Michael Haggerty @ 2017-02-20 12:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170218133303.3682-1-pclouds@gmail.com>
On 02/18/2017 02:32 PM, Nguyễn Thái Ngọc Duy wrote:
> v4:
I very much like the direction of this patch series. I reviewed the
design pretty carefully and left some comments about it, and skimmed
through the code changes. But I haven't yet reviewed the code in detail.
I'll wait for your reaction to my design comments before doing so.
Thanks,
Michael
^ permalink raw reply
* Re: [PATCH 2/5] hashmap: allow memihash computation to be continued
From: Johannes Schindelin @ 2017-02-20 12:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff Hostetler
In-Reply-To: <xmqq7f4onjrs.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 17 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/hashmap.c b/hashmap.c
> > index b10b642229c..061b7d61da6 100644
> > --- a/hashmap.c
> > +++ b/hashmap.c
> > @@ -50,6 +50,20 @@ unsigned int memihash(const void *buf, size_t len)
> > return hash;
> > }
> >
> > +/* Incoporate another chunk of data into a memihash computation. */
> > +unsigned int memihash_continue(unsigned int hash,
> > + const void *buf, size_t len)
> > +{
> > + const unsigned char *p = buf;
> > + while (len--) {
> > + unsigned int c = *p++;
> > + if (c >= 'a' && c <= 'z')
> > + c -= 'a' - 'A';
> > + hash = (hash * FNV32_PRIME) ^ c;
> > + }
> > + return hash;
> > +}
>
> This makes me wonder if we want to reduce the duplication (primarily
> to avoid risking the loop body to go out of sync) by doing:
>
> unsigned int memihash(const void *buf, size_t len)
> {
> return memihash_continue(buf, len, FNV32_BASE);
> }
>
> 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):
-- snipsnap --
diff --git a/hashmap.c b/hashmap.c
index 061b7d61da6..470a0832688 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -4,7 +4,6 @@
#include "cache.h"
#include "hashmap.h"
-#define FNV32_BASE ((unsigned int) 0x811c9dc5)
#define FNV32_PRIME ((unsigned int) 0x01000193)
unsigned int strhash(const char *str)
@@ -37,19 +36,6 @@ unsigned int memhash(const void *buf, size_t len)
return hash;
}
-unsigned int memihash(const void *buf, size_t len)
-{
- unsigned int hash = FNV32_BASE;
- unsigned char *ucbuf = (unsigned char *) buf;
- while (len--) {
- unsigned int c = *ucbuf++;
- if (c >= 'a' && c <= 'z')
- c -= 'a' - 'A';
- hash = (hash * FNV32_PRIME) ^ c;
- }
- return hash;
-}
-
/* Incoporate another chunk of data into a memihash computation. */
unsigned int memihash_continue(unsigned int hash,
const void *buf, size_t len)
diff --git a/hashmap.h b/hashmap.h
index 78e14dfde71..a1a8fd7dc54 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -8,12 +8,17 @@
/* FNV-1 functions */
+#define FNV32_BASE ((unsigned int) 0x811c9dc5)
+
extern unsigned int strhash(const char *buf);
extern unsigned int strihash(const char *buf);
extern unsigned int memhash(const void *buf, size_t len);
-extern unsigned int memihash(const void *buf, size_t len);
extern unsigned int memihash_continue(unsigned int hash_seed,
const void *buf, size_t len);
+static inline unsigned int memihash(const void *buf, size_t len)
+{
+ return memihash_continue(FNV32_BASE, buf, len);
+}
static inline unsigned int sha1hash(const unsigned char *sha1)
{
^ permalink raw reply related
* slightly confusing message
From: Leon George @ 2017-02-20 12:35 UTC (permalink / raw)
To: git
Hey, you lovely people <3
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.
enjoy your weeks :-)
^ permalink raw reply
* Re: [PATCH v4 11/15] refs.c: make get_main_ref_store() public and use it
From: Michael Haggerty @ 2017-02-20 12:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170218133303.3682-12-pclouds@gmail.com>
On 02/18/2017 02:32 PM, Nguyễn Thái Ngọc Duy wrote:
> get_ref_store() will soon be renamed to get_submodule_ref_store().
> Together with future get_worktree_ref_store(), the three functions
> provide an appropriate ref store for different operation modes. New APIs
> will be added to operate directly on ref stores.
I see where you're going with this, but as of the end of this patch
series, there is still nothing that a caller outside of the refs module
can do with a `struct ref_store *`. This means that it would be enough
to put this declaration (and that of `get_submodule_ref_store()`, added
in a later patch) in refs/refs-internal.h for now.
If you want to move the declarations straight to `refs.h` now to avoid
code churn in some later patch series, then please mention that fact in
the commit message.
Michael
^ permalink raw reply
* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Michael Haggerty @ 2017-02-20 12:38 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <CACsJy8ChAwwPawTrq4gqZqAO3k6PrgzzvceVFjbJmxkE0Rn8SQ@mail.gmail.com>
On 02/20/2017 01:33 PM, Duy Nguyen wrote:
> On Mon, Feb 20, 2017 at 7:30 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/20/2017 01:21 PM, Duy Nguyen wrote:
>>> On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>> [...]
>>>> These limitations, I think, imply that submodule references have to be
>>>> treated as read-only.
>>>
>>> Behind the scene submodule does add_submodule_odb(), which basically
>>> makes the submodule's odb an alternate of in-core odb. So odb access
>>> works. I was puzzled how submodules could by pass odb access at the
>>> beginning only to find that out. technical/api-ref-iteration.txt also
>>> mentions that you need to add_submodule_odb(), so I think it's
>>> deliberate (albeit hacky) design.
>>
>> That's interesting. I didn't know it before.
>>
>> But I still don't think that means that reference writing can work
>> correctly. If I try to set a submodule branch to an SHA-1 and I verify
>> that the SHA-1 points to a valid commit, how do I know that the commit
>> is in the same submodule that holds the reference?
>
> Good point. So will the new flag be "read_only" (no reference to
> submodule), or "submodule"? This flag will be passed in at ref-store
> init time and kept in files_ref_store.
I haven't checked carefully whether there are other operations that
don't work and/or don't make sense for submodules. If not, then
`read_only` would also be a fine name for the flag.
Michael
^ permalink raw reply
* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Duy Nguyen @ 2017-02-20 12:33 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <c4265faf-a04a-552b-fd72-1631a220f788@alum.mit.edu>
On Mon, Feb 20, 2017 at 7:30 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/20/2017 01:21 PM, Duy Nguyen wrote:
>> On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>>>> Since submodule or not is irrelevant to files-backend after the last
>>>> patch, remove the parameter from files_downcast() and kill
>>>> files_assert_main_repository().
>>>>
>>>> PS. This implies that all ref operations are allowed for submodules. But
>>>> we may need to look more closely to see if that's really true...
>>>
>>> I think you are jumping the gun here.
>>>
>>> Even after your changes, there is still a significant difference between
>>> the main repository and submodules: we have access to the object
>>> database for the main repository, but not for submodules.
>>>
>>> So, for example, the following don't work for submodules:
>>>
>>> * `parse_object()` is used to ensure that references are only pointed at
>>> valid objects, and that branches are only pointed at commit objects.
>>>
>>> * `peel_object()` is used to write the peeled version of references in
>>> `packed-refs`, and to peel references while they are being iterated
>>> over. Since this doesn't work, I think you can't write `packed-refs` in
>>> a submodule.
>>>
>>> These limitations, I think, imply that submodule references have to be
>>> treated as read-only.
>>
>> Behind the scene submodule does add_submodule_odb(), which basically
>> makes the submodule's odb an alternate of in-core odb. So odb access
>> works. I was puzzled how submodules could by pass odb access at the
>> beginning only to find that out. technical/api-ref-iteration.txt also
>> mentions that you need to add_submodule_odb(), so I think it's
>> deliberate (albeit hacky) design.
>
> That's interesting. I didn't know it before.
>
> But I still don't think that means that reference writing can work
> correctly. If I try to set a submodule branch to an SHA-1 and I verify
> that the SHA-1 points to a valid commit, how do I know that the commit
> is in the same submodule that holds the reference?
Good point. So will the new flag be "read_only" (no reference to
submodule), or "submodule"? This flag will be passed in at ref-store
init time and kept in files_ref_store.
--
Duy
^ permalink raw reply
* Re: [PATCH v4 06/15] files-backend: remove the use of git_path()
From: Duy Nguyen @ 2017-02-20 12:31 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <1d750672-441d-14ae-21e9-d7bdd47a50a4@alum.mit.edu>
On Mon, Feb 20, 2017 at 6:34 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/18/2017 02:32 PM, Nguyễn Thái Ngọc Duy wrote:
>> Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
>> deciding what goes where. The end goal is to pass $GIT_DIR only. A
>> refs "view" of a linked worktree is a logical ref store that combines
>> two files backends together.
>>
>> (*) Not entirely true since strbuf_git_path_submodule() still does path
>> translation underneath. But that's for another patch.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> refs/files-backend.c | 37 +++++++++++++++++++++++++++++++++----
>> 1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index b599ddf92..dbcaf9bda 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -924,6 +924,9 @@ struct files_ref_store {
>> */
>> const char *submodule;
>>
>> + struct strbuf gitdir;
>> + struct strbuf gitcommondir;
>
> Is there a reason for these to be `strbuf`s rather than `const char *`?
> (One reason would be if you planned to use the `len` field, but I don't
> think you do so.)
Nope. I just didn't think about char *. It may have to lose "const"
though because in submodule case we may need a new allocation.
>
>> @@ -937,15 +940,33 @@ static void files_path(struct files_ref_store *refs, struct strbuf *sb,
>> {
>> struct strbuf tmp = STRBUF_INIT;
>> va_list vap;
>> + const char *ref;
>>
>> va_start(vap, fmt);
>> strbuf_vaddf(&tmp, fmt, vap);
>> va_end(vap);
>> - if (refs->submodule)
>> + if (refs->submodule) {
>> strbuf_git_path_submodule(sb, refs->submodule,
>> "%s", tmp.buf);
>> - else
>> - strbuf_git_path(sb, "%s", tmp.buf);
>> + } else if (!strcmp(tmp.buf, "packed-refs") ||
>> + !strcmp(tmp.buf, "logs")) { /* non refname path */
>> + strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
>> + } else if (skip_prefix(tmp.buf, "logs/", &ref)) { /* reflog */
>> + if (is_per_worktree_ref(ref))
>> + strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
>> + else
>> + strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
>
> This code would also be simpler if there were separate functions for
> packed-refs, loose references, and reflogs.
And maybe keep the path to packed-refs, the base path up to "logs" in
struct files_ref_store too (they will be calculated at ref store
init)? That way the files_packed_refs_path() does no calculation.
files_reflog_path() and files_ref_path() will just do string
concatenation, no fancy addf.
--
Duy
^ permalink raw reply
* Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()
From: Michael Haggerty @ 2017-02-20 12:30 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <CACsJy8AZ27O-pxTqHOzYXRBuyv8dkxdGJ_5Z0u3eaxkNdnaEYA@mail.gmail.com>
On 02/20/2017 01:21 PM, Duy Nguyen wrote:
> On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>>> Since submodule or not is irrelevant to files-backend after the last
>>> patch, remove the parameter from files_downcast() and kill
>>> files_assert_main_repository().
>>>
>>> PS. This implies that all ref operations are allowed for submodules. But
>>> we may need to look more closely to see if that's really true...
>>
>> I think you are jumping the gun here.
>>
>> Even after your changes, there is still a significant difference between
>> the main repository and submodules: we have access to the object
>> database for the main repository, but not for submodules.
>>
>> So, for example, the following don't work for submodules:
>>
>> * `parse_object()` is used to ensure that references are only pointed at
>> valid objects, and that branches are only pointed at commit objects.
>>
>> * `peel_object()` is used to write the peeled version of references in
>> `packed-refs`, and to peel references while they are being iterated
>> over. Since this doesn't work, I think you can't write `packed-refs` in
>> a submodule.
>>
>> These limitations, I think, imply that submodule references have to be
>> treated as read-only.
>
> Behind the scene submodule does add_submodule_odb(), which basically
> makes the submodule's odb an alternate of in-core odb. So odb access
> works. I was puzzled how submodules could by pass odb access at the
> beginning only to find that out. technical/api-ref-iteration.txt also
> mentions that you need to add_submodule_odb(), so I think it's
> deliberate (albeit hacky) design.
That's interesting. I didn't know it before.
But I still don't think that means that reference writing can work
correctly. If I try to set a submodule branch to an SHA-1 and I verify
that the SHA-1 points to a valid commit, how do I know that the commit
is in the same submodule that holds the reference?
> [...]
Michael
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Jakub Narębski @ 2017-02-20 12:27 UTC (permalink / raw)
To: Oleg Taranenko, Jacob Keller
Cc: Alex Hoffman, Johannes Sixt, Christian Couder, Stephan Beyer, git
In-Reply-To: <CABEd3j8sgDd8DXW8+2Q7pjANPTr-Ws1Xs1ap875mkxFOfnenYw@mail.gmail.com>
W dniu 20.02.2017 o 08:38, Oleg Taranenko pisze:
>>>> Then you must adjust your definition of "good": All commits that do not have
>>>> the feature, yet, are "good": since they do not have the feature in the
>>>> first place, they cannot have the breakage that you found in the feature.
>>>>
>>>> That is exactly the situation in your original example! But you constructed
>>>> the condition of goodness in such a simplistic way (depending on the
>>>> presence of a string), that it was impossible to distinguish between "does
>>>> not have the feature at all" and "has the feature, but it is broken".
>>>
>>> Johannes, thank you for correctly identifying the error in my logic.
>>> Indeed I was using the term 'bad' also for the commit without the
>>> feature. In order to find the commit introducing the bug in my example
>>> a new state is needed, which would make 'git bisect' a bit more
>>> complicated than the user 'most of the time' probably needs. Or do you
>>> think, it would make sense to ask the user for this state (if e.g 'git
>>> bisect' would be started with a new parameter)?
>
>> If a commit doesn't have the feature, then it is by definition, not
>> containing a broken feature, and you can simply use the "good" state.
>> There is no need for a different state. If you can't test the commit
>> because it's broken in some other way, you can use "git bisect skip"
>> but that isn't what you want in this case.
>
> Commits missing feature == 'good' commit is a very confusing one.
Nowadays you can change the names for 'old' and 'new' with
`git bisect terms`. HTH.
> Looks like in real life it happens much often, then git developers can
> imagine. For multi-branch/multi-feature workflow it's pretty easy not
> to recognize whether it is missing or not developed yet, especially on
> retrospective view where cherry-picking/squashing/merging is being
> used. My experience shows most annoying bugs are generating after a
> heavy merge (evil merge) with conflicts resolutions, where developer
> is not involved in the knowing what happens on counterpart changes.
> Then feature can be disappeared after it was worked & tested in its
> own branches.
Good to know about this problem.
> @Alex, I'm pretty interesting in fixing this weird bisect behaviour as
> well, as far as I struggled on it last summer and continue struggling
> so far :) If you want we can join to your efforts on fixing.
Anyway, I don't think it is feasible to weaken the assumption that there
is only one transition from 'old' ('good') to 'new' ('bad'); this is
what allows O(log(N)) operations. See bisection method of root finding,
that is finding zeros of a continuous function.
Best,
--
Jakub Narębski
^ 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