* Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Duy Nguyen @ 2017-02-15 0:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, git@vger.kernel.org, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <xmqq8tp8aawb.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 1:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> Direct call sites are just middle men though, e.g.
>> for_each_ref_submodule(). I'll attempt to clean this up at some point
>> in future (probably at the same time I attempt to kill *_submodule ref
>> api). But I think for now I'll just put a TODO or FIXME comment here.
>
> So we'd have get_*_ref_store() for various cases and then will have
> a unifying for_each_ref_in_refstore() that takes a ref-store, which
> supersedes all the ad-hoc iterators like for_each_ref_submodule(),
> for_each_namespaced_ref(), etc?
>
> That's a very sensible longer-term goal, methinks.
Ah I forgot about ref namespace. I'll need to try something out in that area.
> I am wondering what we should do to for_each_{tag,branch,...}_ref().
> Would that become
>
> ref_store = get_ref_main_store();
> tag_ref_store = filter_ref_store(ref_store, "refs/tags/");
> for_each_ref_in_refstore(tag_ref_store, ...);
>
> or do you plan to have some other pattern?
Long term, I think that's what Mike wants. Though the filter_ref_store
might return an (opaque) iterator instead of a ref store and
for_each_refstore() becomes a thin wrapper around the iterator
interface.
--
Duy
^ permalink raw reply
* Re: [PATCH v2] clean: use warning_errno() when appropriate
From: Duy Nguyen @ 2017-02-15 0:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King
In-Reply-To: <xmqqh93wabev.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> All these warning() calls are preceded by a system call. Report the
>> actual error to help the user understand why we fail to remove
>> something.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> v2 dances with errno
>
> Thanks.
>
>>
>> builtin/clean.c | 19 ++++++++++++++-----
>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d6bc3aaae..3569736f6 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>> struct strbuf quoted = STRBUF_INIT;
>> struct dirent *e;
>> int res = 0, ret = 0, gone = 1, original_len = path->len, len;
>> + int saved_errno;
>> struct string_list dels = STRING_LIST_INIT_DUP;
>>
>> *dir_gone = 1;
>> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>> if (!dir) {
>> /* an empty dir could be removed even if it is unreadble */
>> res = dry_run ? 0 : rmdir(path->buf);
>> + saved_errno = errno;
>> if (res) {
>> quote_path_relative(path->buf, prefix, "ed);
>
> I think this part should be more like
>
> res = ... : rmdir(...);
> if (res) {
> int saved_errno = errno;
> ... do other things that can touch errno ...
> errno = saved_errno;
> ... now we know what the original error was ...
>
> The reason to store the errno in saved_errno here is not because we
> want to help code after "if (res) {...}", but the patch sent as-is
> gives that impression and is confusing to the readers.
>
> Perhaps all hunks of this patch share the same issue? I could
> locally amend, of course, but I'd like to double check before doing
> so myself---perhaps you did it this way for a good reason that I am
> missing?
One thing I like about putting saved_errno right next to the related
syscall is, the syscall is visible from the diff (previously some are
out of context). This is really minor though. I briefly thought of
introducing rmdir_errno() and friends that return -errno on error, so
we could do
res = ... : rmdir_errno(..);
if (res) {
errno = -res;
warning_errno(...);
}
But that's more work and the errno = -res is not particularly pleasing
to read. I'm fine with moving saved_errno in the error handling
blocks.
--
Duy
^ permalink raw reply
* Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Junio C Hamano @ 2017-02-15 1:16 UTC (permalink / raw)
To: Duy Nguyen
Cc: Stefan Beller, git@vger.kernel.org, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <CACsJy8AfwGK_Y8vH-mF4NXWts_4_CPZamO0L_rWD-1WR3=36Yg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Long term, I think that's what Mike wants. Though the filter_ref_store
> might return an (opaque) iterator instead of a ref store and
> for_each_refstore() becomes a thin wrapper around the iterator
> interface.
Ah, yes, an iterator would be a lot more suitable abstraction there.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] clean: use warning_errno() when appropriate
From: Junio C Hamano @ 2017-02-15 1:28 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Jeff King
In-Reply-To: <CACsJy8BXSAUr2knrkOfO0gXYAwQoJpL2hCXy44Q37H4GE_-yVA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> The reason to store the errno in saved_errno here is not because we
>> want to help code after "if (res) {...}", but the patch sent as-is
>> gives that impression and is confusing to the readers.
>>
>> Perhaps all hunks of this patch share the same issue? I could
>> locally amend, of course, but I'd like to double check before doing
>> so myself---perhaps you did it this way for a good reason that I am
>> missing?
>
> One thing I like about putting saved_errno right next to the related
> syscall is, the syscall is visible from the diff (previously some are
> out of context). This is really minor though.
I agree that this is minor.
I care more about looking at errno ONLY after we saw our call to a
system library function indicated an error, and I wanted to avoid
doing:
res = dry_run ? 0 : rmdir(path);
saved_errno = errno;
if (res) {
... do something else ...
errno = saved_errno;
call_something_that_uses_errno();
When our call to rmdir() did not fail, or when we didn't even call
rmdir() at all, what is in errno has nothing to do with what we are
doing, and making a copy of it makes the code doubly confusing.
Rather, I'd prefer to see:
res = dry_run ? 0 : rmdir(path);
if (res) {
int saved_errno = errno;
... do something else ...
errno = saved_errno;
call_something_that_uses_errno();
which makes it clear what is going on when reading the resulting
code.
For now, I'll queue a separate SQUASH??? and wait for others to
comment.
Thanks.
^ permalink raw reply
* Re: [PATCH] completion: restore removed line continuating backslash
From: SZEDER Gábor @ 2017-02-15 1:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
In-Reply-To: <xmqqfujhddl2.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 9:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
>> commands, 2017-02-03) rewrapped a couple of long lines, and while
>> doing so it inadvertently removed a '\' from the end of a line, thus
>> breaking completion for 'git config remote.name.push <TAB>'.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>
>> I wanted this to be a fixup!, but then noticed that this series is
>> already in next, hence the proper commit message.
>
> We get "--format=... : command not found"?
Yeah, that's the one.
> Thanks for a set of sharp eyes.
Heh. Sharp?! It took over five minutes to notice after I first got
that error...
Furthermore, that '\' was already missing in v1, almost a year ago :)
>> I see the last What's cooking marks this series as "will merge to
>> master". That doesn't mean that it will be in v2.12, does it?
>
> I actually was hoping that these can go in. Others that can (or
> should) wait are marked as "Will cook in 'next'".
>
> If you feel uncomfortable and want these to cook longer, please tell
> me so.
Well, it was mainly my surprise that a 20+ patch series arriving so
late that it gets queued on top of -rc0 would still make it into the
release. However, I have been using this series with only minor
modifications for the better part of a year now, so it's unlikely that
there will be any big issues with it. Maybe something small, like
this missing '\', but we will deal with it when it arises.
Gábor
^ permalink raw reply
* Re: [PATCH v2] clean: use warning_errno() when appropriate
From: Jeff King @ 2017-02-15 1:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <xmqqfujg5jjv.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote:
> I care more about looking at errno ONLY after we saw our call to a
> system library function indicated an error, and I wanted to avoid
> doing:
>
> res = dry_run ? 0 : rmdir(path);
> saved_errno = errno;
> if (res) {
> ... do something else ...
> errno = saved_errno;
> call_something_that_uses_errno();
>
> When our call to rmdir() did not fail, or when we didn't even call
> rmdir() at all, what is in errno has nothing to do with what we are
> doing, and making a copy of it makes the code doubly confusing.
>
> Rather, I'd prefer to see:
>
> res = dry_run ? 0 : rmdir(path);
> if (res) {
> int saved_errno = errno;
> ... do something else ...
> errno = saved_errno;
> call_something_that_uses_errno();
>
> which makes it clear what is going on when reading the resulting
> code.
>
> For now, I'll queue a separate SQUASH??? and wait for others to
> comment.
I don't have a strong feeling either way, but I think your second
example there is probably preferable. The reason to save errno is
because of the "something else" that may affect it, and it puts the
saving close to that.
Duy's version above keeps the saved_errno close to the syscall that
caused it, which is nicer for making sure we're saving the right thing,
and didn't get fooled by:
res = rmdir(path);
... some other stuff ...
if (res) {
int saved_errno = errno;
... something else ...
errno = saved_errno;
That's wrong if "some other stuff" touches errno. But I think
"saved_errno" is not the right pattern there. It is "stuff away the
result _and_ errno for this thing so we can use it later".
IOW, I'd expect it to be more like:
rmdir_result = rmdir(path);
rmdir_errno = errno;
... some other stuff ...
if (rmdir_result)
show_error(rmdir_errno);
And that leads to the "gee, why don't we just encode error values as
negative integers" pattern. Which I agree is nicer, but I'm not sure I
want to get into wrapping every syscall to give it a better interface.
-Peff
^ permalink raw reply
* Re: [PATCH 01/14] lib-submodule-update.sh: reorder create_lib_submodule_repo
From: brian m. carlson @ 2017-02-15 1:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jrnieder, gitster
In-Reply-To: <20170215003423.20245-2-sbeller@google.com>
[-- Attachment #1: Type: text/plain, Size: 613 bytes --]
On Tue, Feb 14, 2017 at 04:34:10PM -0800, Stefan Beller wrote:
> create_lib_submodule_repo () {
> git init submodule_update_repo &&
> (
> @@ -49,10 +54,11 @@ create_lib_submodule_repo () {
> git config submodule.sub1.ignore all &&
> git add .gitmodules &&
> git commit -m "Add sub1" &&
> - git checkout -b remove_sub1 &&
> +
> + git checkout -b remove_sub1 add_sub1&&
You're missing a space before the "&&".
--
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: Feature request: flagging “volatile” branches for integration/development
From: Junio C Hamano @ 2017-02-15 1:55 UTC (permalink / raw)
To: Herbert, Marc; +Cc: git, Josh Triplett
In-Reply-To: <6c7cb3da-714b-87ed-5885-220a433c646f@intel.com>
"Herbert, Marc" <marc.herbert@intel.com> writes:
>> The hard part may be policy (e.g. what if the user does not want a branch
>> to be treated volatile by various commands even if it receives such
>> flag from a git server).
>
> There would be instant, human-readable value in such a new "volatile"
> flag. Machine use and policies can be discussed later. These will be
> easier to prototype, experiment and refine once the flag exists.
We tend to avoid adding random cruft to the system whose semantics
is not well defined, so that we can avoid having to support an ill
defined feature forever.
> ... Now I bet this on the other hand must have been
> discussed (and rejected?) before, any pointer?
I suspect that people may have expressed vague wish from time to
time, but I do not think we saw a proposal that outlines the design
at a concrete enough level to let us rejecting in the past ;-)
Let me list some things that needs to be designed that comes to my
mind offhand:
* How would a user mark a ref as "volatile"? I am assuming that
anybody do so in her own local repository, but how does one mark
a ref as "volatile" at a hosting site, and who is allowed to do
so (one possibile design is "new option to 'git push' will do so,
and anybody who can push to the ref is allowed to", and I am fine
with that design, but you have to spell that out in a proposal)?
* How would a user learn if a ref is "volatile"? Does "ls-remote"
show that information?
* Does volatile-ness of a ref at the remote site propagate to your
remote-tracking ref that corresponds to it? What does it mean
that refs/remotes/origin/pu is marked as volatile in your local
repository? You cannot "checkout -b" based on it? Does "branch"
based on it need to be forbidden as well?
* Can a local ref be "volatile"? What does it mean (the same
subquestions as above)?
* If your local branch myA is set to build on a remote-tracking
branch A and push back to branch A at the remote, i.e.
$ git checkout -t -b myA refs/remotes/origin/A
$ ... work work work ...
$ git push
is set to result in their branch A updated with what you built in
myA, and if the branch A at the remote is marked as "volatile",
does it make your "myA" also "volatile"? How is the volatile-ness
inherited? From their A to your remotes/origin/A and then to
your myA? Any other useful rule that defines the propagation?
* Do we only care about "volatile"? If we are extending the system
to allow setting and propagating this new bit per ref (I am
blindly assuming that you do not have a strong reason to limit
this to branches), we may as well just design this extension to
allow the projects to assign arbitrary set of bits to refs. Some
projects may want to have different degree of volatile-ness and
have "volatile" refs, "unstable" refs and "iffy" refs, for
example.
Side note: even if we were to go with "any random label can be
assigned and the meaning for the labels can be defined by the
project convention" approach, it does not necessarily mean we are
adding a random cruft whose semantics is ill-defined. "Git does
not do anything special to a ref based on what labels it has--it
just remembers what labels the user told it to attach to the ref,
and shows what labels the ref has when asked" can be very well
defined semantics.
^ permalink raw reply
* Re: [PATCH 09/14] update submodules: add submodule_go_from_to
From: brian m. carlson @ 2017-02-15 2:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jrnieder, gitster
In-Reply-To: <20170215003423.20245-10-sbeller@google.com>
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
On Tue, Feb 14, 2017 at 04:34:18PM -0800, Stefan Beller wrote:
> + prepare_submodule_repo_env_no_git_dir(&cp.env_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
> + argv_array_pushl(&cp.args, "read-tree", NULL);
> +
> + if (!dry_run)
> + argv_array_push(&cp.args, "-u");
> + else
> + argv_array_push(&cp.args, "-n");
I might write this as
if (dry_run)
argv_array_push(&cp.args, "-n");
else
argv_array_push(&cp.args, "-u");
In other words, avoiding the negation when you have an else branch. I
can also see an argument for keeping the condition identical to the
other branches, though.
--
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 14/14] builtin/checkout: add --recurse-submodules switch
From: brian m. carlson @ 2017-02-15 2:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jrnieder, gitster
In-Reply-To: <20170215003423.20245-15-sbeller@google.com>
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
On Tue, Feb 14, 2017 at 04:34:23PM -0800, Stefan Beller wrote:
> +--[no-]recurse-submodules::
> + Using --recurse-submodules will update the content of all initialized
> + submodules according to the commit recorded in the superproject. If
> + local modifications in a submodule would be overwritten the checkout
> + will fail until `-f` is used. If nothing (or --no-recurse-submodules)
I would say "unless" instead of "until". "Until" implies an ongoing
or repetitive action being interrupted, which isn't the case here.
--
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: [RFCv3 PATCH 00/14] Checkout aware of Submodules!
From: brian m. carlson @ 2017-02-15 2:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, jrnieder, gitster
In-Reply-To: <20170215003423.20245-1-sbeller@google.com>
[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]
On Tue, Feb 14, 2017 at 04:34:09PM -0800, Stefan Beller wrote:
> Integrate updating the submodules into git checkout, with the same
> safety promises that git-checkout has, i.e. not throw away data unless
> asked to. This is done by first checking if the submodule is at the same
> sha1 as it is recorded in the superproject. If there are changes we stop
> proceeding the checkout just like it is when checking out a file that
> has local changes.
>
> The integration happens in the code that is also used in other commands
> such that it will be easier in the future to make other commands aware
> of submodule.
>
> This also solves d/f conflicts in case you replace a file/directory
> with a submodule or vice versa.
>
> The patches are still a bit rough, but the overall series seems
> promising enough to me that I want to put it out here.
>
> Any review, specifically on the design level welcome!
Overall, I'm very pleased with this. I don't really have any
design-level comments at this point, only small nits. I'm considering
building and testing it out at work, where this would be extremely
useful. I'm therefore hoping to see how it works under real-world
conditions shortly.
--
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] completion: restore removed line continuating backslash
From: Junio C Hamano @ 2017-02-15 2:41 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list
In-Reply-To: <CAM0VKjmU57saSfyRuoWfC+UZFNypH1Wp9X33VgzPq9fatD=qtg@mail.gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> If you feel uncomfortable and want these to cook longer, please tell
>> me so.
>
> Well, it was mainly my surprise that a 20+ patch series arriving so
> late that it gets queued on top of -rc0 would still make it into the
> release.
It all depends on what area the changes are about ;-)
Not meaning to make light of your contribution, but if the upcoming
version of Git shipped with a slightly broken completion, and the
breakage is severe enough, the only thing we need to do is to do a
maintenance release that reverts a single script. Distro packagers
may do that for us. While waiting, the user can choose not to load
the completion and can keep using Git just fine. A broken
"mergetool" would be similar in that a workaround to inspect "git
diff" is available. If we break "pull" or "commit" on the other
hand, the necessary workaround would be a lot more involved.
Some changes in low-impact areas can wait without reducing the
end-user value of the new release, but if the risk of regression is
small, I favour merging them, rather than postponing them for one
cycle, if only to reduce the number of patches that I need to hold
onto. Patches to clarify existing documentation fall into this
category.
My perception of "risk of regression" obviously is affected by the
size of the series, and 20+ is certainly on the larger side. But
other things also come into the picture. Patches from an author who
knows the area the patches touch very well and with track record of
not causing embarrassing regressions, would make me feel safer than
patches from others, for example.
^ permalink raw reply
* Re: [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: SZEDER Gábor @ 2017-02-15 3:11 UTC (permalink / raw)
To: Cornelius Weig; +Cc: Git mailing list, Richard Wagner, j6t
In-Reply-To: <20170214212404.31469-2-cornelius.weig@tngtech.com>
On Tue, Feb 14, 2017 at 10:24 PM, <cornelius.weig@tngtech.com> wrote:
> From: Cornelius Weig <cornelius.weig@tngtech.com>
>
> Git-checkout completes words starting with '--' as options and other
> words as refs. Even after specifying a ref, further words not starting
> with '--' are completed as refs, which is invalid for git-checkout.
Refs completion is never attempted for words after the disambiguating
double-dash.
Even when refs completion is attempted, if it is unsuccessful, i.e.
there is no ref that matches the current word to be completed, then
Bash falls back to standard filename completion. No refs match
'./<TAB>'.
Furthermore, Bash performs filename completion on Alt-/ independently
from any completion function.
Granted, none of these will limit to only modified files... But that
might be a good thing, see below.
> This commit ensures that after specifying a ref, further non-option
> words are completed as paths. Four cases are considered:
>
> - If the word contains a ':', do not treat it as reference and use
> regular revlist completion.
> - If no ref is found on the command line, complete non-options as refs
> as before.
> - If the ref is HEAD or @, complete only with modified files because
> checking out unmodified files is a noop.
Here you use "modified" in the 'ls-files --modified' sense, but that
doesn't include modifications already staged in the index, see below.
> This case also applies if no ref is given, but '--' is present.
> - If a ref other than HEAD or @ is found, offer only valid paths from
> that revision.
>
> Note that one corner-case is not covered by the current implementation:
> if a refname contains a ':' and is followed by '--' the completion would
> not recognize the valid refname.
I'm not sure what you mean here. Refnames can't contain ':'.
> Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
> ---
> contrib/completion/git-completion.bash | 39 +++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 4ab119d..df46f62 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1068,7 +1068,7 @@ _git_bundle ()
>
> _git_checkout ()
> {
> - __git_has_doubledash && return
> + local i c=2 ref="" seen_double_dash=""
>
> case "$cur" in
> --conflict=*)
> @@ -1081,13 +1081,36 @@ _git_checkout ()
> "
> ;;
> *)
> - # check if --track, --no-track, or --no-guess was specified
> - # if so, disable DWIM mode
> - local flags="--track --no-track --no-guess" track=1
> - if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
> - track=''
> - fi
> - __gitcomp_nl "$(__git_refs '' $track)"
> + while [ $c -lt $cword ]; do
> + i="${words[c]}"
> + case "$i" in
> + --) seen_double_dash=1 ;;
> + -*|?*:*) ;;
> + *) ref="$i"; break ;;
I haven't tried it, but this would trigger on e.g. 'git checkout -b
new-feature <TAB>', wouldn't it?
> + esac
> + ((c++))
> + done
> +
> + case "$ref,$seen_double_dash,$cur" in
> + ,,*:*)
> + __git_complete_revlist_file
> + ;;
> + ,,*)
> + # check for --track, --no-track, or --no-guess
> + # if so, disable DWIM mode
> + local flags="--track --no-track --no-guess" track=1
> + if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
> + track=''
> + fi
> + __gitcomp_nl "$(__git_refs '' $track)"
> + ;;
> + ,1,*|@,*|HEAD,*)
> + __git_complete_index_file "--modified"
What about
$ echo "unintentional change" >>tracked-file && git add -u
$ git rm important-file
$ git checkout HEAD <TAB>
? It seems it will offer neither 'tracked-file' nor 'important-file',
but I think it should offer both.
We have __git_complete_index_file() for a while now, but only use it
for commands that accept only --options and filenames, e.g. 'add',
'clean', 'rm'. This would be the first case when we would use it for
a command that accept both refs and filenames. Perhaps similar corner
cases and the easy ways to trigger filename completion are why no one
thought it's worth it.
> + ;;
> + *)
> + __git_complete_tree_file "$ref" "$cur"
Well, here you could go all-in, and say that this should complete only
files that are different from the version in $ref, because checking
out files that are still the same is a noop :)
> + ;;
> + esac
> ;;
> esac
> }
> --
> 2.10.2
>
^ permalink raw reply
* difflame improvements
From: Edmundo Carmona Antoranz @ 2017-02-15 5:19 UTC (permalink / raw)
To: Git List; +Cc: Jeff King
Hi!
I've been working on detecting revisions where a "real" deletion was
made and I think I advanced a lot in that front. I still have to work
on many scenarios (renamed files, for example... also performance) but
at least I'm using a few runs against git-scm history and the results
are "promising":
23:05 $ git blame -s --reverse -L 25,40 HEAD~20..HEAD -- versioncmp.c
066fb0494e 25) static int initialized;
066fb0494e 26)
066fb0494e 27) /*
8ec68d1ae2 28) * p1 and p2 point to the first different character in
two strings. If
8ec68d1ae2 29) * either p1 or p2 starts with a prerelease suffix, it
will be forced
8ec68d1ae2 30) * to be on top.
8ec68d1ae2 31) *
8ec68d1ae2 32) * If both p1 and p2 start with (different) suffix, the order is
8ec68d1ae2 33) * determined by config file.
066fb0494e 34) *
8ec68d1ae2 35) * Note that we don't have to deal with the situation
when both p1 and
8ec68d1ae2 36) * p2 start with the same suffix because the common
part is already
8ec68d1ae2 37) * consumed by the caller.
066fb0494e 38) *
066fb0494e 39) * Return non-zero if *diff contains the return value
for versioncmp()
066fb0494e 40) */
Lines 28-33:
23:05 $ git show --summary 8ec68d1ae2
commit 8ec68d1ae2863823b74d67c5e92297e38bbf97bc
Merge: e801be066 c48886779
Author: Junio C Hamano <>
Date: Mon Jan 23 15:59:21 2017 -0800
Merge branch 'vn/diff-ihc-config'
"git diff" learned diff.interHunkContext configuration variable
that gives the default value for its --inter-hunk-context option.
* vn/diff-ihc-config:
diff: add interhunk context config option
And this is not telling me the _real_ revision where the lines were
_deleted_ so it's not very helpful, as Peff has already mentioned.
Running difflame:
23:06 $ time ~/proyectos/git/difflame/difflame.py -bp=-s -w HEAD~20
HEAD -- versioncmp.c
diff --git a/versioncmp.c b/versioncmp.c
index 80bfd109f..9f81dc106 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -24,42 +24,83 @@
.
.
.
+b17846432d 33) static void find_better_matching_suffix(const char
*tagname, const char *suffix,
+b17846432d 34) int
suffix_len, int start, int conf_pos,
+b17846432d 35) struct
suffix_match *match)
+b17846432d 36) {
b17846432d 37) /*
c026557a3 versioncmp: generalize version sort suffix reordering
-c026557a3 (SZEDER 28) * p1 and p2 point to the first different
character in two strings. If
-c026557a3 (SZEDER 29) * either p1 or p2 starts with a prerelease
suffix, it will be forced
-c026557a3 (SZEDER 30) * to be on top.
-c026557a3 (SZEDER 31) *
-c026557a3 (SZEDER 32) * If both p1 and p2 start with (different)
suffix, the order is
-c026557a3 (SZEDER 33) * determined by config file.
b17846432 versioncmp: factor out helper for suffix matching
+b17846432d 38) * A better match either starts earlier or
starts at the same offset
+b17846432d 39) * but is longer.
+b17846432d 40) */
+b17846432d 41) int end = match->len < suffix_len ?
match->start : match->start-1;
.
.
.
Same range of (deleted) lines:
23:10 $ git --show --name-status c026557a3
commit c026557a37361b7019acca28f240a19f546739e9
Author: SZEDER Gábor <>
Date: Thu Dec 8 15:24:01 2016 +0100
versioncmp: generalize version sort suffix reordering
The 'versionsort.prereleaseSuffix' configuration variable, as its name
suggests, is supposed to only deal with tagnames with prerelease
.
.
.
Signed-off-by: SZEDER Gábor <>
Signed-off-by: Junio C Hamano <>
M Documentation/config.txt
M Documentation/git-tag.txt
M t/t7004-tag.sh
M versioncmp.c
This is the revision where the deletion happened.
That's it for the time being.
^ permalink raw reply related
* Re: [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: Cornelius Weig @ 2017-02-15 10:46 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list, Richard Wagner, j6t
In-Reply-To: <CAM0VKjkqdto83Qo8PVbxt-2r8prQguNbAtNELxj5AmJEgugC_Q@mail.gmail.com>
Although I'm not convinced that completion of modified files is unnecessary, I'm at least persuaded that not all users would welcome such a change. Given the hint from Gabor that Alt-/ forces filesystem completion, there is even no big win in stopping to offer further refnames after one has already been given.
If you think that this would be desirable, find a revised version below. Otherwise drop it.
On 02/15/2017 04:11 AM, SZEDER Gábor wrote:
> On Tue, Feb 14, 2017 at 10:24 PM, <cornelius.weig@tngtech.com> wrote:
>> From: Cornelius Weig <cornelius.weig@tngtech.com>
>> Note that one corner-case is not covered by the current implementation:
>> if a refname contains a ':' and is followed by '--' the completion would
>> not recognize the valid refname.
>
> I'm not sure what you mean here. Refnames can't contain ':'.
Yes, my bad. I was confusing it with the case where filename and ref name are identical.
>> + while [ $c -lt $cword ]; do
>> + i="${words[c]}"
>> + case "$i" in
>> + --) seen_double_dash=1 ;;
>> + -*|?*:*) ;;
>> + *) ref="$i"; break ;;
>
> I haven't tried it, but this would trigger on e.g. 'git checkout -b
> new-feature <TAB>', wouldn't it?
Correct, good eyes.
> What about
>
> $ echo "unintentional change" >>tracked-file && git add -u
> $ git rm important-file
> $ git checkout HEAD <TAB>
>
> ? It seems it will offer neither 'tracked-file' nor 'important-file',
> but I think it should offer both.
Ideally yes. The latter of the two would also not work with Alt/.
-------------------------------------------------------------------
From d0e0c9af8a30dec479c393ae7fe75205c9b3b229 Mon Sep 17 00:00:00 2001
From: Cornelius Weig <cornelius.weig@tngtech.com>
Date: Tue, 14 Feb 2017 21:01:45 +0100
Subject: [PATCH] completion: checkout: complete paths when ref given
Git-checkout completes words starting with '--' as options and other
words as refs. Even after specifying a ref, further words not starting
with '--' are completed as refs, which is invalid for git-checkout.
With this commit completion suppresses refname suggestion after finding
what looks like a refname. Words before a '--' not starting with a '-'
and containing no ':' are considered to be refnames.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c774d..42e6463b67 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1059,7 +1059,16 @@ _git_bundle ()
_git_checkout ()
{
- __git_has_doubledash && return
+ local c=2 seen_ref=""
+ while [ $c -lt $cword ]; do
+ case "${words[c]}" in
+ --) return ;;
+ -b|-B|--orphan|--branch) ((c++)) ;;
+ -*|*:*) ;;
+ *) seen_ref="y" ;;
+ esac
+ ((c++))
+ done
case "$cur" in
--conflict=*)
@@ -1072,13 +1081,16 @@ _git_checkout ()
"
;;
*)
- # check if --track, --no-track, or --no-guess was specified
- # if so, disable DWIM mode
- local flags="--track --no-track --no-guess" track=1
- if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
- track=''
+ if [ -z "$seen_ref" ]
+ then
+ # check for --track, --no-track, or --no-guess
+ # if so, disable DWIM mode
+ local flags="--track --no-track --no-guess" track=1
+ if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
+ track=''
+ fi
+ __gitcomp_nl "$(__git_refs '' $track)"
fi
- __gitcomp_nl "$(__git_refs '' $track)"
;;
esac
}
--
2.11.1
^ permalink raw reply related
* [BUG] submodule config does not apply to upper case submodules?
From: Lars Schneider @ 2017-02-15 11:17 UTC (permalink / raw)
To: git; +Cc: sbeller
It looks like as if submodule configs ("submodule.*") for submodules
with upper case names are ignored. The test cases shows that skipping
a submodule during a recursive clone seems not to work.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
I observed the bug on Windows, macOS, and Linux and at least on
v2.11.0 and v2.11.1:
https://travis-ci.org/larsxschneider/git/builds/201828672
Right now I have no time to fix it but I might be able to look into it
next week (if no one else tackles it before that).
Cheers,
Lars
Notes:
Base Commit: 3b9e3c2ced (v2.11.1)
Diff on Web: https://github.com/larsxschneider/git/commit/a122feaf9f
Checkout: git fetch https://github.com/larsxschneider/git submodule/uppercase-bug-v1 && git checkout a122feaf9f
t/t7400-submodule-basic.sh | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8e40..83b5c0d1e0 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1116,5 +1116,39 @@ test_expect_success 'submodule helper list is not confused by common prefixes' '
test_cmp expect actual
'
+test_expect_success 'submodule config does not apply to upper case submodules' '
+ test_when_finished "rm -rf super lowersub clone-success clone-failure" &&
+ mkdir lowersub &&
+ (
+ cd lowersub &&
+ git init &&
+ >t &&
+ git add t &&
+ git commit -m "initial commit lowersub"
+ ) &&
+ mkdir UPPERSUB &&
+ (
+ cd UPPERSUB &&
+ git init &&
+ >t &&
+ git add t &&
+ git commit -m "initial commit UPPERSUB"
+ ) &&
+ mkdir super &&
+ (
+ cd super &&
+ git init &&
+ >t &&
+ git add t &&
+ git commit -m "initial commit super" &&
+ git submodule add ../lowersub &&
+ git submodule add ../UPPERSUB &&
+ git commit -m "add submodules"
+ ) &&
+ git -c submodule.lowersub.update=none clone --recursive super clone-success 2>&1 |
+ grep "Skipping submodule" &&
+ git -c submodule.UPPERSUB.update=none clone --recursive super clone-failure 2>&1 |
+ grep "Skipping submodule"
+'
test_done
base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436
--
2.11.0
^ permalink raw reply related
* [PATCH v1] t7400: cleanup "submodule add clone shallow submodule" test
From: Lars Schneider @ 2017-02-15 11:33 UTC (permalink / raw)
To: git; +Cc: gitster, sbeller
In-Reply-To: <20170215111704.78320-1-larsxschneider@gmail.com>
The test creates a "super" directory that is not removed after the
test finished. This directory is not used in any subsequent tests and
should therefore be removed.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
I just noticed that my bug report test does not run properly without this
patch: http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/
@Junio: I think this patch should be applied regardless of the bug.
Thank,
Lars
Notes:
Base Commit: 3b9e3c2ced (v2.11.1)
Diff on Web: https://github.com/larsxschneider/git/commit/1699a6894e
Checkout: git fetch https://github.com/larsxschneider/git submodule/cleanup-test-v1 && git checkout 1699a6894e
t/t7400-submodule-basic.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8e40..08df483280 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1078,6 +1078,7 @@ test_expect_success 'submodule with UTF-8 name' '
'
test_expect_success 'submodule add clone shallow submodule' '
+ test_when_finished "rm -rf super" &&
mkdir super &&
pwd=$(pwd) &&
(
base-commit: 3b9e3c2cede15057af3ff8076c45ad5f33829436
--
2.11.0
^ permalink raw reply related
* Re: missing handling of "No newline at end of file" in git am
From: Olaf Hering @ 2017-02-15 11:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20170214215103.7d5e5f4c@probook.ubnt.lan>
[-- Attachment #1: Type: text/plain, Size: 290 bytes --]
On Tue, Feb 14, Olaf Hering wrote:
> How would I debug it?
One line is supposed to be longer than 998 chars, but something along
the way truncated it and corrupted the patch. No idea why the error
today is different from the error yesterday.
'git pull' has to be used in this case.
Olaf
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Clarification about "Better tooling for reviews", was Re: Google Doc about the Contributors' Summit
From: Johannes Schindelin @ 2017-02-15 12:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4m01if1z.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Technically, it is not a write-up, and I never meant it to be that. I
> > intended this document to help me remember what had been discussed,
> > and I doubt it is useful at all to anybody who has not been there.
> >
> > I abused the Git mailing list to share that link, what I really should
> > have done is to use an URL shortener and jot the result down on the
> > whiteboard.
> >
> > Very sorry for that,
>
> Heh, no need to apologize.
Well, you clearly misunderstood the purpose of the document, and that was
my fault, as I had not made that clear.
> I saw your <alpine.DEB.2.20.1702071248430.3496@virtualbox> that was
> sent to the list long after the event, which obviously no longer
> meant for collaborative note taking and thought that you are
> inviting others to read the result of that note taking, and that is
> why I commented on that.
Of course you are free to read it, and to guess from the sparse notes
around what the discussions revolved. I do not think that you'll get much
out of the notes, though.
> I've hopefully touched some "ask Junio what he thinks of this" items and
> the whole thing was not wasted ;-)
I am afraid that it was quite clear to everybody in the room "what you
think of this".
And I do not think that your clarifications how you review code had any
direct relation with the discussions in particular about better tooling
for the review process.
To open the discussion of that particular part of the Contributors'
Summit, I mentioned a couple of pain points, and the remainder of the
discussion really revolved around those, constructively so, I want to add:
- we actively keep potential contributors out by insisting that email
communication is the most open and inclusive, when right off the bat,
without any notice to anybody, our mailing list rejects mails sent both
by the most popular desktop mail client as well as by the most popular
web mail client.
- developers who really, really want to contribute may switch email
clients or try to configure their existing email clients to skip the
HTML part of their emails, only to be met with the reply "your patch is
white-space corrupted" which cannot fail to sound harsh.
- the few contributors not deterred by this problem, and persistent enough
to try until they manage to get through, often drop the ball after being
met with suggestions that would ideally be addressed by automation so
that humans can focus on problems only humans can solve (every time I
read "this line is too long" in a review I want to cry: how is this
worth the time of contributor or reviewer? There are *tools* for that).
- discussions often veer into completely tangential topics so that the
actual review of the patches is drowned out (and subsequently
forgotten).
- for any given patch series, it requires a good amount of manual digging
to figure out what its current state is: what reviewers' comments are
still unaddressed? Is the patch series in pu/next/master yet? Is the
*correct* iteration of the patch series in pu/next/master? How does the
version in pu/next/master differ from what the contributor has in their
local repository? etc
- the closest thing to "this is the state of that patch series" is the
What's Cooking email that neither CC:s the original contributors nor
does it bear any reliable link to the original patch mail, let alone the
original commit(s) in the contributor's repository.
I really do not think that any of your descriptions of your workflow and
of your review priorities could possibly be expected to fix any of these.
I have an additional pain point, of course, in that your priorities in
patch review (let's admit that it is not a code review, but a patch review
instead, and as such limited in other ways than just the lack of focus on
avoiding potential bugs) are unfortunate in my opinion. But that is not
your problem.
It is clear to me that these pain points only affect potential
contributors (and some of them only frequent contributors who are as
uncomfortable with the sheer amount of tedious "where is that mail that
contained that patch? Oh, and what was the latest reply to this one? Okay,
and in which worktree do I have those changes again?" type of things that
really should not by *my* burden, given that we are trying to develop an
application that helps relieve developers of tedious burden by automating
recurring tasks).
That is why I did not call session "Let's criticize Junio for something
that does not even bother him", but instead "Better tooling for reviews".
The only way forward, as I see it, is for other like-minded contributors
(one of the GitMerge talks even dedicated a good chunk of time to the
description of the pitfalls of the Git mailing list, and home-grown tools
to work around them, so I am definitely not alone) to come together and
try to consolidate and develop the tools to work around the problems
incurred from the choice of using the Git mailing list as the only vehicle
for code contributions [*1*].
When (or if?) those tools become polished and convenient enough to make a
difference, who knows? Maybe even you or Peff find them useful enough to
switch. After all, even The Linus switched from tarballs and patches to
BitKeeper (and I read that it took only a substantial amount of time and
effort and money on Larry McVoy's part[*2*]).
But nothing of that comes as news to you. You knew all that about my
opinions and my pains. I actually did not expect there to be any benefit
in the "Better tools for review" session for you, specifically, and it
seems I was correct in that assumption.
Ciao,
Johannes
Footnote *1*: git-svn, gitk and git-gui Pull Requests and subsequent
merges do not count, as it is clear that they are not *really* part of the
Git source code, and therefore not *really* reviewed on the Git mailing
list.
Footnote *2*: Some posts I read about it are unfortuntely gone, but here
is an archived one that still works:
https://web.archive.org/web/20120414183625/http://kerneltrap.org/node/222
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-15 12:32 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <ef8549ea-7222-fdd0-739d-855ad428e39c@kdbg.org>
Hi Hannes,
On Tue, 14 Feb 2017, Johannes Sixt wrote:
> Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > When removing the hack for isatty(), we actually removed more than
> > > > just an isatty() hack: we removed the hack where internal data
> > > > structures of the MSVC runtime are modified in order to redirect
> > > > stdout/stderr.
> > > >
> > > > Instead of using that hack (that does not work with newer versions
> > > > of the runtime, anyway), we replaced it by reopening the
> > > > respective file descriptors.
> > > >
> > > > What we forgot was to mark stderr as unbuffered again.
>
> I do not see how the earlier patch turned stderr from unbuffered to
> buffered, as it did not add or remove any setvbuf() call. Can you
> explain?
Certainly. I hoped that the commit message would do the job, but then, I
am under time pressure these days, so it was probably a bit terse.
The hack we used to make isatty() work used to change data structures
directly that are *internal* to the MSVC runtime. That is, instead of
*really* redirecting stdout/stderr, we simply changed the target of the
existing stdout/stderr and thereby could fool MSVC into believing that it
is *still* writing to the terminal (because the bit is set) and that it is
*not* a pipe (because we forcibly unset that bit).
Needless to say, this meddling with internal data structures is not only
prone to break with future updates of the MSVC runtime, it is also
inappropriate because the implementation may rely on certain side effects
(or not so side effects) that may very well cause crashes or data loss.
Imagine, for example, that the internal data structure were variable-size,
based on the HANDLE type. That is totally legitimate for an internal data
structure. And if we meddle with the HANDLE, we can easily cause data
corruption.
As GCC is basically tied to using an older version of the MSVC runtime
(unlike GLIBC, multiple versions of the MSVC runtime can coexist happily,
making it relatively easy to maintain backwards-compatibility, but that
concept is foreign to GCC), this used to not be a problem.
However, with our recent push to allow developing, building, debugging and
performance profiling in Visual Studio, that limitation no longer holds
true: if you develop with Visual Studio 2015, you will link to a newer
MSVC runtime, and that runtime changed those internal data structures
rather dramatically.
That means that we had to come up with a non-hacky way to redirect
stdout/stderr (e.g. to parse ESC color sequences and apply them to the
Win32 Console as appropriate) and still have isatty() report what we want
it to report. That is, if we redirect stdout/stderr to a pipe that parses
the color sequences for use in the Win32 Console, we want isatty() to
report that we are writing to a Terminal Typewriter (a charming
anachronism, isn't it?).
My colleague Jeff Hostetler worked on this and figured out that the only
way to do this properly is to wrap isatty() and override it via a #define,
and simply remember when stdout/stderr referred to a terminal before
redirecting, say, to that pipe.
As my famously broken patch to override isatty() (so that /dev/null would
not be treated as a terminal) *already* overrode isatty() with a custom
wrapper, and as it was broken and needed fixing, I decided to reconcile
the two approaches into what is now the version in `master`.
So instead of "bending" the target HANDLE of the existing stdout/stderr
(which would *naturally* have kept the buffered/unbuffered nature as-is),
we now redirect with correct API calls. And the patch I provided at the
bottom of this mail thread reinstates the unbuffered nature of stderr now
that it gets reopened.
Hopefully that makes it clear why the setvbuf() call is required now, but
was previously unnecessary?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-15 12:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <xmqq37fga9rn.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 14 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> OK. Should this go directly to 'master', as the isatty thing is
> >> already in?
> >
> > From my point of view, it is not crucial. The next Git for Windows
> > version will have it, of course, and Hannes is always running with his
> > set of patches, he can easily cherry-pick this one, too.
>
> What hat were you wearing when you said the above "From my point of
> view"?
The hat of a person who sees how patches are reviewed before they enter
pu/next/master/maint of git.git.
If that review had anything to do with Windows, and refused to accept
patches unless they work correctly on Windows, I would agree that it is a
wise idea to fast-track important fixes for Windows.
But that is not the case. Quite often `pu`, sometimes `next`, and rarely
even `master` are regularly broken on Windows.
The only branch that is tested very stringently on Windows, and into which
nothing is allowed that breaks on Windows, is git-for-windows/git's
`master` branch.
BTW this is not just an opinion, this is just an account of the current
state.
Once you accept that this is reality, you will understand why I *dared* to
say that a criticial Windows-specific fix needs to be fast-tracked to
git-for-windows/git's `master`, but not into git.git's `master` branch.
FWIW I wish it were different, that git.git's `master` reflected more
closely what the current Git for Windows version has. If you are
attentive, you will have noticed that I continuously work toward that
goal. I frequently "upstream patches" from Git for Windows[*1*], even if
it seems that the influx of new patches is higher than the rate of patches
that make it into git.git's `master`. And even if I am often asked to
change these patches so much that it is virtually guaranteed that they
regress (hence my recently increasing reluctance to accept each and every
reviewer's suggestions as "must implement").
Ciao,
Johannes
Footnote *1*: Yes, I even "upstream patches" from developers other than
myself, trying to shield contributors from having to send their patches as
mails and to cope with the reviewers' suggestions that may, or may not,
make sense. This makes my life harder, but I believe that the alternative
would be *not* to have those patches in git.git *at all*.
^ permalink raw reply
* Re: [PATCH] completion: restore removed line continuating backslash
From: SZEDER Gábor @ 2017-02-15 13:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
In-Reply-To: <xmqqtw7w41m5.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 3:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> If you feel uncomfortable and want these to cook longer, please tell
>>> me so.
>>
>> Well, it was mainly my surprise that a 20+ patch series arriving so
>> late that it gets queued on top of -rc0 would still make it into the
>> release.
>
> It all depends on what area the changes are about ;-)
Sure. I actually wanted to end that email with something like "and
it's in contrib anyway :)", but by the time I got there I forgot about
it.
^ permalink raw reply
* Re: git-p4.py caching
From: Lars Schneider @ 2017-02-15 13:10 UTC (permalink / raw)
To: Martin-Louis Bright; +Cc: Git List, Luke Diamand, George Vanburgh
In-Reply-To: <CAG2PGsqs2dQM0SJ25ocHJ+Nex_m2NC5yQQuROxcF6SG2e93wCQ@mail.gmail.com>
> On 14 Feb 2017, at 19:16, Martin-Louis Bright <mlbright@gmail.com> wrote:
[CC'ing Luke and George]
> hi!
>
> I am using git-p4.py to migrate a lot of medium and large Perforce
> depots into git. I almost exclusively go one way: from Perforce to
> git. I also frequently re-clone/re-migrate as the Perforce migration
> client spec is refined.
>
> For this, I have added rudimentary caching in git-p4.py so that
> Perforce requests are not repeated over the network.
Martin implemented an on disk cache and "requests not repeated" applies
mostly for re-migrations. Re-migrations are multiple test migrations ("git-p4 clone")
with minor client spec changes until we find the right client spec for the
production migration.
> I have it working and tested on a ~150MB repo and the migration time was halved.
>
> Is this something that would be of interest to the larger community?
I like the idea!
Disclaimer: I work together with Martin on the P4 -> Git migrations :-)
Cheers,
Lars
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Johannes Schindelin @ 2017-02-15 14:07 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git-for-windows, git
In-Reply-To: <CAP8UFD1+AgBVqSh=wHteM3uKO+55ZqqD4cHzBUfN0KTPXyvutQ@mail.gmail.com>
Hi Christian,
On Wed, 15 Feb 2017, Christian Couder wrote:
> On Tue, Feb 14, 2017 at 10:08 PM, Junio C Hamano <gitster@pobox.com>
> wrote:
>
> > Probably a less resource intensive approach is to find the tips of the
> > topics not in 'next' but in 'pu' and test them. That would give you
> > which topic(s) are problematic, which is a better starting point than
> > "Oh, 'pu' does not build". After identifying which branch is
> > problematic, bisection of individual topic would be of more manageable
> > size.
>
> It is still probably more resource intensive than it couls be.
Indeed.
Ciao,
Dscho
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Johannes Schindelin @ 2017-02-15 14:22 UTC (permalink / raw)
To: Philip Oakley; +Cc: Christian Couder, Junio C Hamano, git-for-windows, git
In-Reply-To: <E2C1B7A8FBF94C8CB1C9C5754D882800@PhilipOakley>
Hi Philip,
On Tue, 14 Feb 2017, Philip Oakley wrote:
> From: "Christian Couder" <christian.couder@gmail.com>
> > On Tue, Feb 14, 2017 at 10:08 PM, Junio C Hamano <gitster@pobox.com>
> > wrote:
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > >
> > > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > > > >
> > > > > > That is why I taught the Git for Windows CI job that tests the
> > > > > > four upstream Git integration branches to *also* bisect test
> > > > > > breakages and then upload comments to the identified commit on
> > > > > > GitHub
> > > > >
> > > > > Good. I do not think it is useful to try 'pu' as an aggregate
> > > > > and expect it to always build and work [*1*], but your "bisect
> > > > > and pinpoint" approach makes it useful to identify individual
> > > > > topic that brings in a breakage.
> > > >
> > > > Sadly the many different merge bases[*1*] between `next` and `pu`
> > > > (which are the obvious good/bad points for bisecting
> > > > automatically) bring my build agents to its knees. I may have to
> > > > disable the bisecting feature as a consequence.
> >
> > Yeah, this is a bug in the bisect algorithm. Fixing it is in the GSoC
> > 2017 Ideas.
>
> There are also a few ideas at the SO answers:
> http://stackoverflow.com/a/5652323/717355
Thanks for that link!
However, my main aim was not to get distracted into yet another corner of
Git that needs to be fixed (I am on enough of those projects already).
I was merely surprised (and not in a good way) that a plenty ordinary
bisect between `next` and `pu` all of a sudden tested a *one year old*
commit whether it was good or not.
And I doubt that the strategy to mark all second parents of all merge
commits in pu..next as "good" would work well, as the merge bases *still*
would have to be tested.
I guess what I have to resort to is this: if I know that `next` tests
fine, and that `pu` fails, I shall mark all merge bases as "good". I am
sure this has its own set of pitfalls, undoubtedly costing me more time on
that front...
But at least my cursory analysis of this idea seems to make sense: I use
`next` essentially as a catch-all to verify that the breakage has entered
`pu`, but not yet `next`. This reasoning makes sense, given that we know
the waterfall topology of pu/next/master/maint: patches enter from left to
right, i.e. anything that entered `pu` may later enter `next`, but not
vice versa.
Ciao,
Dscho
^ 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