Re: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Hi, Quoting René Scharfe (2015-08-12 21:40:48) > Am 11.08.2015 um 12:40 schrieb Johannes Schauer: > > for repositories with more than 16k files and folders, git-archive will > > create > > zip files which store the wrong number of entries. That is, it stores the > > number of entries modulo 16k. This will break unpackers that do not include > > code to support this brokenness. > > The limit is rather 65535 entries, isn't it? The entries field has two > bytes, and they are used fully. seems to be 65535 indeed. I just forwarded the number Dieter Baron (libzip contributor) told me when they replied (off list) to my bug report against libzip: http://nih.at/listarchive/libzip-discuss/msg00554.html But reading https://en.wikipedia.org/wiki/Zip_(file_format)#ZIP64 the limit indeed seems to be 65535. > Which programs are affected? InfoZIP Zip 3.0 and 7-Zip 9.20 seem to handle an > archive with more entries just fine. The built-in functionality of Windows > 10 doesn't. In my case I discovered this because libzip http://nih.at/libzip does not implement reading an archive with more than 65535 entries without zip64. > Besides, 64K entries should be enough for anybody. ;-) :P > Seriously, though: What kind of repository has that many files and uses the > ZIP format to distribute snapshots? Just curious. I have not searched for any. In my case I was using git to keep track of the modifications our tools do to a directory of files to detect regressions. These tools are also able to read data from zip archives instead from a directory. I created the zip archive using git-archive because the files already were in git so that seemed most convenient to me. That's when I discovered the problem because our tools use libzip. The easy workaround was to use another packager instead of git-archive. We use the zip format because Windows has support for it. > > Instead, git-archive should use the zip64 extension to handle more than 16k > > files and folders correctly. > > That seems to be what InfoZIP does and Windows 10 handles it just fine. If > lower Windows versions and other popular extractors can unzip such archives > as well then this might indeed be the way to go. The wikipedia page above claims that windows versions starting with vista have support for zip64. It also lists some other software with support for it. Thanks! cheers, josch signature.asc Description: signature
Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names
SZEDER Gábor writes: >> >> s/becase/because/; > > OK. > ... >> I agree with Peff that "--names-only" has a subtle difference with >> an existing and well known subcommand option and it would be a bit >> irritating to remember which options is for which command. > > OK. > ... The topic is now in 'next'; I think I've locally fixed it up for these when I originally queued them a few days ago, so if there are any remaining issues, please throw incremental polishing patches. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitk: Alter the ordering for the "Tags and heads" view
On Tue, Jun 02, 2015 at 07:11:10AM -0400, Michael Rappazzo wrote:
> In the "Tags and heads" view, the list of refs is globally sorted.
> The list of local refs (heads) is separated by the remote refs. This
> change re-orders the view toi be: local refs, remote refs tracked by
> local refs, remote refs, tags, and then other refs
>
> Signed-off-by: Michael Rappazzo
Sorry it's taken me so long to get around to reviewing this. I have a
couple of comments:
> ---
> gitk-git/gitk | 48 ++--
> 1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 9a2daf3..431a6a1 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9879,35 +9879,71 @@ proc refill_reflist {} {
> global curview
>
> if {![info exists showrefstop] || ![winfo exists $showrefstop]} return
> -set refs {}
> +set localrefs {}
> +set remoterefs {}
> +set locally_tracked_remote_refs {}
> +set tagrefs {}
> +set otherrefs {}
> foreach n [array names headids] {
> - if {[string match $reflistfilter $n]} {
> + if {![string match "remotes/*" $n] && [string match $reflistfilter $n]}
> {
> + if {[commitinview $headids($n) $curview]} {
> + lappend localrefs [list $n H]
> + catch {set remote_name [exec git config --get branch.$n.remote]}
> + if {$remote_name ne ""} {
First off, if the git config command fails for any reason and returns
an error status, the set command won't get done and $remote_name will
either be undefined or will have whatever value it had before. If it
is undefined then the if statement is going to throw an error. I
don't think that is what you meant to happen. This same problem will
occur for other variables such as $remote_ref and $exists.
Secondly, I'm not very happy about doing all these external git
commands every time we run refill_reflist. Couldn't we cache which
remote each local branch is tracking? We would then throw away and
reload the cache in rereadrefs. Most executions of refill_reflist
would then not need to do any external git commands at all.
Paul.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names
Quoting Junio C Hamano :
SZEDER Gábor writes:
'git config' can only show values or name-value pairs, so if a shell
script needs the names of set config variables it has to run 'git config
--list' or '--get-regexp' and parse the output to separate config
variable names from their values. However, such a parsing can't cope
with multi-line values. Though 'git config' can produce null-terminated
output for newline-safe parsing, that's of no use in such a case, becase
s/becase/because/;
OK.
shells can't cope with null characters.
Even our own bash completion script suffers from these issues.
Help the completion script, and shell scripts in general, by introducing
the '--names-only' option to modify the output of '--list' and
'--get-regexp' to list only the names of config variables, so they don't
have to perform error-prone post processing to separate variable names
from their values anymore.
I agree with Peff that "--names-only" has a subtle difference with
an existing and well known subcommand option and it would be a bit
irritating to remember which options is for which command.
OK.
diff --git a/builtin/config.c b/builtin/config.c
index 7188405f7e..307980ab50 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -13,6 +13,7 @@ static char *key;
static regex_t *key_regexp;
static regex_t *regexp;
static int show_keys;
+static int omit_values;
static int use_key_regexp;
static int do_all;
static int do_not_match;
...
@@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) {
static int show_all_config(const char *key_, const char *value_, void *cb)
{
- if (value_)
+ if (!omit_values && value_)
H. As we have "show_keys",
if (show_values && value_)
would be a lot more intuitive, no?
Well, the name 'omit_values' was suggested by Peff after the first
round. I'm happy to rename it to whatever you agree upon :)
@@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf,
const char *key_, const char *value
strbuf_addstr(buf, key_);
must_print_delim = 1;
}
+ if (omit_values) {
+ strbuf_addch(buf, term);
+ return 0;
+ }
This hunk makes me wonder what the assignment to "must_print_delim"
is about. When the code is told to show only keys and not values,
it shouldn't even have to worry about key_delim, but that assignment
is done to control exactly that. It happens that you are lucky that
you can "return 0" early here so that the assignment does not have
any effect, but still conceptually the code structure is made ugly
by this patch.
How about restructuring the function like this? Perhaps even better
than a tri-state toggle would be.
(showing the result instead of the diff, because all the indentation
changes make the diff hard to read).
static int format_config(struct strbuf *buf, const char *key_, const
char *value_)
{
strbuf_init(buf, 0);
if (show_keys)
strbuf_addstr(buf, key_);
if (!omit_values) { // or show_values
int must_free_vptr = 0;
int must_add_delim = show_keys;
char value[256];
const char *vptr = value;
if (types == TYPE_INT)
sprintf(value, "%"PRId64,
git_config_int64(key_, value_ ? value_ : ""));
else if (types == TYPE_BOOL)
vptr = git_config_bool(key_, value_) ? "true"
: "false";
else if (types == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, &is_bool);
if (is_bool)
vptr = v ? "true" : "false";
else
sprintf(value, "%d", v);
} else if (types == TYPE_PATH) {
if (git_config_pathname(&vptr, key_, value_) < 0)
return -1;
must_free_vptr = 1;
} else if (value_) {
vptr = value_;
} else {
/* Just show the key name */
vptr = "";
must_add_delim = 0;
}
if (must_add_delim)
strbuf_addch(buf, key_delim);
strbuf_addstr(buf, vptr);
if (must_free_vptr)
free((char *)vptr);
}
strbuf_addch(buf, term);
return 0;
}
Isn't it more like the existing "show_keys" can be replaced/enhanced
with a single "show" tri-state toggle that chooses one among:
* show both keys and values (for --list)
* show only keys (for your new feature)
* show only value (for --get)
perhaps?
I see get_urlmatch() abuses show_keys variable in a strange way, and
it ma
Re: enhanced remote ref namespaces
On Wed, Aug 12, 2015 at 8:34 PM, Jacob Keller wrote:
> On Wed, Aug 12, 2015 at 9:10 AM, Junio C Hamano wrote:
>> Some design boundaries:
>>
>> - Moving the remote-tracking branch hierarchy from refs/remotes/$O/*
>>to refs/remotes/$O/heads/* would not fly, because it will break
>>existing repositories. Do not even waste time on pursuing
>>refs/remotes/$O/{heads,tags,notes...}/*
>
> even if we maintained new git's abililty to work with this, ie: only
> external-to-git scripts would break and only for new clones? Maybe we
> don't want to go this route, but it seems like the way that the
> original proposal was headed.
I don't think it's worth trying to go that route. Even though it's
theoretically possible to solve it, the complexity added by e.g.
multiple git versions operating on the same repository (maybe even
simultaneously?) suggests that it's much simpler to just go with a new
clean hierarchy that simply Works for new Git versions, and the older
versions don't have to deal with at all. IMHO, maybe even going as far
as incrementing core.repositoryFormatVersion, so that older Git
versions will refuse to work with new-style repos...
[...]
>> If somebody got confused, notice that in the above description, I
>> said refs/remotes/ and refs/remote/. The former must stay. The
>> name of the latter is open to bikeshedding. Some may prefer a name
>> that is more distinct (refs/tracking/ or something, perhaps?). I
>> happen to prefer a name that is similar, but this preference is very
>> weak and I can persuaded to go either way.
>
> I don't like it being so similar that we now have typos between
> remotes and remote.. ie: remotes/ works for heads, but
> "remotes//tags" does not... that sounds like it would get
> confusing.
I like refs/tracking. At some point I was planning to use refs/peers,
but that's merely another color for the bikeshed...
> Symlinking the old location seems reasonable to me, as it would leave
> all the same data in the locations expected by the old tools, while
> keeping all actual storage in the new location.
>
> In this way, we would no longer need configuration settings. It
> honestly doesn't matter to me which direction we symlink either.
>
> As for the other complex issue is what to do about
> "refs/tracking//tags
>
> The big discussion on the original thread is about how tags would
> work. I'm personally ok with *ignoring* tags and leaving it the way it
> is for now, and just doing this as a solid place to stick
> notes/replace/etc.
That is probably the best plan for now: Solve most of the problem, and
punt on the controversial parts.
> Or, we could go the route of continuing to stick tags into "refs/tags"
> at the same time as also sticking them into
> refs/tracking//tags
I don't like the copying approach; makes it harder to deduce which tags
are (truly) local, and which came from a remote.
> Or.. we could go the full route of fixing up lookup of tags such that
> we put tags in refs/tracking//tags and we have it lookup tags
> there via something like:
>
> 1) local tags preferred
>
> 2) any remote tag as long as all remote tags point to the same commit
> object (how we select which to use is not very relevant here... we
> could actually go with as long as the tag object is the same so two
> remotes with annotated tags pointing to the same object but different
> tag id would be ambiguous as well)
>
> 3) warn the user must disambiguate the tag via the remote name.
As was probably obvious from the old threads, this is where I'd like
to go. Eventually.
> We could also teach fetch to warn about remote tags which are
> ambiguous (ie: two remotes with same named tag objects pointing to
> different things)
Agreed. This would actually be a good improvement Right Now, without
changing any ref layout at all: If get fetch would otherwise copy a
remote tag into refs/tags/*, but doesn't because there is already a
_different_ tag in its place, then warn loudly to the user, so that
user has a chance to discover the tag difference.
> How goes this sound? I think it makes sense... I don't know how to do
> all this without breaking some backwards compatibility though... I
> think we can maintain expectations for the general user but I feel
> that any change here will break *someones* scripts.
As I said above: Punt on tags for now, and you might be able to not
break anyone's scripts (and if you do, it's probably a poorly written
script). Provided that you leave a symlink to/from refs/remotes/$O in
place, you're AFAICS only adding functionality, not (visibly) changing
existing behavior.
BTW, thanks for resurrecting this topic!
...Johan
--
Johan Herland,
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 3:41 PM, Junio C Hamano wrote: > Jacob Keller writes: > >> I spoke to soon. We have an "init_notes_check" function which shows >> that it does refuse to merge outside of refs/notes/* It prevents all >> notes operations outside of refs/notes > > OK. Then it is OK to limit notes..mergestrategy so that > refers to what comes after refs/notes/, because we will not allow > merging to happen outside the hierarchy. > > If you are planning to break that promise, however, must be > always spelled fully (i.e. with refs/notes/ prefix for those inside > the hierarchy) to avoid ambiguity. Otherwise it will be hard to > interpret a configuration that does something like this (note that > these could come from multiple places, e.g. $HOME/.gitconfig and > $GIT_DIR/config): > Agreed. Today, we do not allow any notes operations at all that function outside of refs/notes/* I suggest we enforce that all configs for merge strategy must be the unqualified notation, and not allow the variance of refs/notes/* and such that DWIM does on the command line. > [notes "commits"] > mergestrategy = concatenate > [notes "notes/commits"] > mergestrategy = cat_sort_uniq > [notes "refs/notes/commits"] > mergestrategy = overwrite > > The three entries in the above example obviously are all meant to > refer to the same refs/notes/commits notes tree, and the usual "last > one wins" rule should apply. But with the recent git_config_get_*() > interface, you cannot tell which one among them was given the last, > overriding the previous entries. Ya, I'd like to avoid this if possible. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Aug 2015, #02; Wed, 12)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The second batch of topics have graduated to 'master'. Most notably, the rewritten "git am" is in. Also "worktree add" is getting improved. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ad/bisect-cleanup (2015-08-03) 6 commits (merged to 'next' on 2015-08-03 at 13b9314) + bisect: don't mix option parsing and non-trivial code + bisect: simplify the addition of new bisect terms + bisect: replace hardcoded "bad|good" by variables + Documentation/bisect: revise overall content + Documentation/bisect: move getting help section to the end + bisect: correction of typo (this branch is used by ad/bisect-terms.) Originally merged to 'next' on 2015-07-09 Code and documentation clean-up to "git bisect". * dt/reflog-tests (2015-07-28) 2 commits (merged to 'next' on 2015-08-03 at 9d2fa1a) + tests: remove some direct access to .git/logs + t/t7509: remove unnecessary manipulation of reflog Tests that assume how reflogs are represented on the filesystem too much have been corrected. * dt/unpack-trees-cache-tree-revalidate (2015-07-28) 1 commit (merged to 'next' on 2015-08-03 at 5b0d620) + unpack-trees: populate cache-tree on successful merge The code to perform multi-tree merges has been taught to repopulate the cache-tree upon a successful merge into the index, so that subsequent "diff-index --cached" (hence "status") and "write-tree" (hence "commit") will go faster. The same logic in "git checkout" may now be removed, but that is a separate issue. * es/worktree-add (2015-07-20) 5 commits (merged to 'next' on 2015-08-03 at 9771a44) + config: rename "gc.pruneWorktreesExpire" to "gc.worktreePruneExpire" + Documentation/git-worktree: wordsmith worktree-related manpages + Documentation/config: fix stale "git prune --worktree" reference + Documentation/git-worktree: fix incorrect reference to file "locked" + Documentation/git-worktree: consistently use term "linked working tree" (this branch is used by dt/notes-multiple and es/worktree-add-cleanup.) Originally merged to 'next' on 2015-07-20 Remove remaining cruft from "git checkout --to", which transitioned to "git worktree add". * es/worktree-add-cleanup (2015-08-05) 25 commits (merged to 'next' on 2015-08-12 at 9168b42) + Documentation/git-worktree: fix duplicated 'from' + Documentation/config: mention "now" and "never" for 'expire' settings + Documentation/git-worktree: fix broken 'linkgit' invocation + checkout: drop intimate knowledge of newly created worktree + worktree: populate via "git reset --hard" rather than "git checkout" + worktree: avoid resolving HEAD unnecessarily + worktree: make setup of new HEAD distinct from worktree population + worktree: detect branch-name/detached and error conditions locally + worktree: add_worktree: construct worktree-population command locally + worktree: elucidate environment variables intended for child processes + worktree: make branch creation distinct from worktree population + worktree: add: suppress auto-vivication with --detach and no + worktree: make --detach mutually exclusive with -b/-B + worktree: introduce options container + worktree: simplify new branch (-b/-B) option checking + worktree: improve worktree setup message + branch: publish die_if_checked_out() + checkout: teach check_linked_checkout() about symbolic link HEAD + checkout: check_linked_checkout: simplify symref parsing + checkout: check_linked_checkout: improve "already checked out" aesthetic + checkout: generalize die_if_checked_out() branch name argument + checkout: die_if_checked_out: simplify strbuf management + checkout: improve die_if_checked_out() robustness + checkout: name check_linked_checkouts() more meaningfully + checkout: avoid resolving HEAD unnecessarily (this branch is used by dt/notes-multiple; uses es/worktree-add.) Originally merged to 'next' on 2015-07-29 The "new-worktree-mode" hack in "checkout" that was added in nd/multiple-work-trees topic has been removed by updating the implementation of new "worktree add". * pt/am-builtin (2015-08-04) 46 commits (merged to 'next' on 2015-08-12 at 10d0c56) + git-am: add am.threeWay config variable + builtin-am: remove redirection to git-am.sh + builtin-am: check for valid committer ident + builtin-am: implement legacy -b/--binary option + builtin-am: implement -i/--interactive + builtin-am: support and auto-detect mercurial patches + builtin-am: support and auto-detect StGit series files + builtin-am: support and auto-detect StGit patches + builtin-am: rerere support + builtin-am: invoke post-applypatch hook + builtin-am: invoke pre-applypatch hook
Re: [PATCH v3 2/4] path: optimize common dir checking
David Turner writes: > Instead of a linear search over common_list to check whether > a path is common, use a trie. The trie search operates on > path prefixes, and handles excludes. > > Signed-off-by: David Turner > --- > > Probably overkill, but maybe we could later use it for making exclude > or sparse-checkout matching faster (or maybe we have to go all the way > to McNaughton-Yamada for that to be truly worthwhile). This is why I love this list. A mere mention of "something better than linear list" immediately is answered by a trie and a mention of McNaughton-Yamada ;-). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Donation
Am Catherine, a dying widow, am donating my trust fund money to any God fearing individual willing to embrasse a life changing encounter. Kindly contact me on catherinemelco...@gmail.com if you are priviledged to read this mail for details. Cath --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
Jacob Keller writes: > I spoke to soon. We have an "init_notes_check" function which shows > that it does refuse to merge outside of refs/notes/* It prevents all > notes operations outside of refs/notes OK. Then it is OK to limit notes..mergestrategy so that refers to what comes after refs/notes/, because we will not allow merging to happen outside the hierarchy. If you are planning to break that promise, however, must be always spelled fully (i.e. with refs/notes/ prefix for those inside the hierarchy) to avoid ambiguity. Otherwise it will be hard to interpret a configuration that does something like this (note that these could come from multiple places, e.g. $HOME/.gitconfig and $GIT_DIR/config): [notes "commits"] mergestrategy = concatenate [notes "notes/commits"] mergestrategy = cat_sort_uniq [notes "refs/notes/commits"] mergestrategy = overwrite The three entries in the above example obviously are all meant to refer to the same refs/notes/commits notes tree, and the usual "last one wins" rule should apply. But with the recent git_config_get_*() interface, you cannot tell which one among them was given the last, overriding the previous entries. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 2:57 PM, Jacob Keller wrote: > On Wed, Aug 12, 2015 at 2:46 PM, Johan Herland wrote: >> If we don't already refuse to merge into a ref outside refs/notes, then >> I would consider that a bug to be fixed, and not some corner use case that >> we must preserve for all future. >> >> After all, we do already have a test in t3308 named 'fail to merge into >> various non-notes refs', where one of the non-notes ref being tested are: >> >> test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x >> > > This test is checking if the ref pointed at by refs/heads/master *is* > a note. But you could create a ref outside of refs/notes which is a > note but which isn't inside refs/notes > > I did just find that we expand remote-ref using expand_notes_ref, and > it does *not* currently let us reference refs outside of refs/notes.. > so we can merge IN to a ref not inside refs/notes (using the > environment variable) but we can't merge FROM > refs/tracking/origin/notes/y for example, which means currently all > notes we merge from have to be located into refs/notes/* > > There are some weird issues here. > > Regards, > Jake I spoke to soon. We have an "init_notes_check" function which shows that it does refuse to merge outside of refs/notes/* It prevents all notes operations outside of refs/notes Since this is the case, I would prefer to modify the DWIM to be as I suggested, and use this DWIM for the notes. We will need to modify the DWIM so that it doesn't change refs/* even if this will fail later, as we use expand_notes_ref for the remote_ref of a merge, and we probably want to allow notes refs to be located somewhere outside of notes such as refs/tracking//notes or something in the future. So we can make our config option take only unqualified values. Thoughts? Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 11:43 PM, Jacob Keller wrote:
> On Wed, Aug 12, 2015 at 12:16 PM, Jacob Keller wrote:
>> Oh interesting. I did a test. If you provide a fully qualified ref not
>> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y
>> rather than refs/foo/y
>>
>> I need to do some more digging on this to determine the exact thing going
>> on...
>>
>> Regards,
>> Jake
>
> I did some more digging. If you pass a notes ref to "--refs" option,
You're referring to the --ref option to 'git notes'?
> that requires all notes to be bound to refs/notes/* and does not allow
> passing of arbitrary refs. However, you can set the environment
> variable GIT_NOTES_REF or core.notesRef to a fully qualified
> reference.
>
> That seems very arbitrary that --ref works by expanding notes and the
> environment variable and configuration option do not... [1]
I believe the intention here was to provide the DWIM-ing at the most
end-user-facing interface (leaving the two other interfaces as possible
"loopholes" for e.g. scripts that "known what they're doing" and don't
want the DWIM-ery to get in their way). Looking back at it now, that
approach was probably misguided.
> I think this inconsistency is very weird, because *most* people will
> not use refs/notes/* etc. This makes it so that --refs forces you to
> use refs/notes/* or it will prefix it for you... ie: you can use
> notes/x, refs/notes/x, x, but if you use refs/tags/x it will DWIM into
> refs/notes/refs/tags/x
>
> I think this is very confusing that --refs doesn't behave the same as
> other sections... either we should enforce this on all refs or we
> should fix the DWIM-ery to be consistent.
>
> that is, we should fix DWIM-ery to be:
>
> (1) if it starts with refs/* leave it alone
>
> (2) if it starts with notes/*, prefix it with refs/
>
> (3) otherwise prefix it with refs/notes/
>
> But that way, refs/some-other-notes/ will work fine instead of
> becoming something else.
Yes, that is probably a better way to do the DWIM-ery.
However, we then need to provide an additional layer of safety/checks
that prevent notes operations from manipulating non-notes refs. First:
- As mentioned elsewhere in the thread, git notes merge should certainly
refuse to merge into anything not under refs/notes/*.
- Preferably, all notes _manipulation_ should be limited to only operating
under refs/notes/* (although I haven't fully thought through all of the
ramifications of that).
- Notes _querying_ (as opposed to manipulation) should be allowed both
within and outside refs/notes/*
I think this should cover the use cases where you fetch notes from a remote
and put them in e.g. refs/remote-notes/* (or refs/remotes/origin/notes/*).
After all, you should not manipulate those notes directly (just as you
don't manipulate your remote-tracking branches directly), but you should
definitely be able to query them, and merge them into a _local_ notes ref
(living under refs/notes/*).
Does that make sense?
> We should also fix reads of environment variable etc such taht we
> enforce these values always are fully qualified and begin with refs.
> Otherwise, use of --refs and the environment variable don't allow the
> same formats.
Agreed.
...Johan
> Regards,
> Jake
>
> [1] 8ef313e1ec3b ("builtin/notes.c: Split notes ref DWIMmery into a
> separate function")
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Johan Herland,
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cleanup submodule_config a bit.
On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller wrote:
> On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine
> wrote:
>> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:
>>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
>>> - return submodule;
>>> + return NULL;
>>
>> There are a couple other places which return 'submodule' when it is
>> known to be NULL. One could rightly expect that they deserve the same
>> treatment, otherwise, the code becomes more confusing since it's not
>> obvious why 'return NULL' is used some places but not others.
>
> They were slightly less obvious to me, fixed now as well!
>
>>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
>>> submodule_cache *cache,
>>> switch (lookup_type) {
>>> case lookup_name:
>>> - submodule = cache_lookup_name(cache, sha1, key);
>>> - break;
>>> + return cache_lookup_name(cache, sha1, key);
>>> case lookup_path:
>>> - submodule = cache_lookup_path(cache, sha1, key);
>>> - break;
>>> + return cache_lookup_path(cache, sha1, key);
>>> + default:
>>> + return NULL;
>>> }
>>> -
>>> - return submodule;
>>
>> Earlier in the function, there's effectively a clone of this logic to
>> which you could apply the same transformation. Changing it here, while
>> ignoring the clone, makes the code more confusing (or at least
>> inconsistent) rather than less.
>
> Not quite. Note the `if (submodule)` in the earlier version, so in case
> cache_lookup_{name, path} return NULL, we keep going. The change I
> propose is at the end of the function and we definitely return no matter
> if it is NULL or not.
Okay, cache_lookup_{name, path}() can indeed return NULL, so the same
transformation won't work. Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] bisect: make bisection refs per-worktree
Using the new refs/worktree/ refs, make bisection per-worktree.
Signed-off-by: David Turner
---
Documentation/git-bisect.txt | 4 ++--
Documentation/rev-list-options.txt | 14 +++---
bisect.c | 2 +-
builtin/rev-parse.c| 6 --
git-bisect.sh | 14 +++---
revision.c | 2 +-
t/t6030-bisect-porcelain.sh| 20 ++--
7 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index e97f2de..f0c52d1 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -82,7 +82,7 @@ to ask for the next commit that needs testing.
Eventually there will be no more revisions left to inspect, and the
command will print out a description of the first bad commit. The
-reference `refs/bisect/bad` will be left pointing at that commit.
+reference `refs/worktree/bisect/bad` will be left pointing at that commit.
Bisect reset
@@ -373,7 +373,7 @@ on a single line.
$ git bisect start HEAD [ ... ]
--no-checkout
$ git bisect run sh -c '
- GOOD=$(git for-each-ref "--format=%(objectname)" refs/bisect/good-*) &&
+ GOOD=$(git for-each-ref "--format=%(objectname)"
refs/worktree/bisect/good-*) &&
git rev-list --objects BISECT_HEAD --not $GOOD >tmp.$$ &&
git pack-objects --stdout >/dev/null >"$GIT_DIR/BISECT_LOG"
test -n "$nolog" || echo "git bisect $state $rev"
>>"$GIT_DIR/BISECT_LOG"
}
@@ -282,8 +282,8 @@ bisect_state() {
bisect_next_check() {
missing_good= missing_bad=
- git show-ref -q --verify refs/bisect/$TERM_BAD || missing_bad=t
- test -n "$(git for-each-ref "refs/bisect/$TERM_GOOD-*")" ||
missing_good=t
+ git show-ref -q --verify refs/worktree/bisect/$TERM_BAD || missing_bad=t
+ test -n "$(git for-each-ref "refs/worktree/bisect/$TERM_GOOD-*")" ||
missing_good=t
case "$missing_good,$missing_bad,$1" in
,,*)
@@ -341,15 +341,15 @@ bisect_next() {
# Check if we should exit because bisection is finished
if test $res -eq 10
then
- bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
+ bad_rev=$(git show-ref --hash --verify
refs/worktree/bisect/$TERM_BAD)
bad_commit=$(git show-branch $bad_rev)
echo "# first $TERM_BAD commit: $bad_commit"
>>"$GIT_DIR/BISECT_LOG"
exit 0
elif test $res -eq 2
then
echo "# only skipped commits left to test"
>>"$GIT_DIR/BISECT_LOG"
- good_revs=$(git for-each-ref --format="%(objectname)"
"refs/bisect/$TERM_GOOD-*")
- for skipped in $(git rev-list refs/bisect/$TERM_BAD --not
$good_revs)
+ good_revs=$(git for-each-ref --format="%(objectname)"
"refs/worktree/bisect/$TERM_GOOD-*")
+ for skipped in $(git rev-list refs/worktree/bisect/$TERM_BAD
--not $good_revs)
do
skipped_commit=$(git show-branch $skipped)
echo "# possible first $TERM_BAD commit:
$skipped_commit" >>"$GIT_DIR/BISECT_LOG"
@@ -412,7 +412,7 @@ Try 'git bisect reset '.")"
bisect_clean_state() {
# There may be some refs packed during bisection.
- git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
+ git for-each-ref --format='%(refname) %(objectname)'
refs/worktree/bisect/\* |
while read ref hash
do
git update-ref -d $ref $hash || exit
diff --git a/revision.c b/revision.c
index b6b2cf7..974a11f 100644
--- a/revision.c
+++ b/revision.c
@@ -2084,7 +2084,7 @@ extern void read_bisect_terms(const char **bad, const
char **good);
static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void
*cb_data, const char *term) {
struct strbuf bisect_refs = STRBUF_INIT;
int status;
- strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
+ strbuf_addf(&bisect_refs, "refs/worktree/bisect/%s", term);
status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn,
cb_data);
strbuf_release(&bisect_refs);
return status;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 9e2c203..bfd5463 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -68,7 +68,7 @@ test_expect_success 'bisect fails if given any junk instead
of revs' '
git bisect reset &&
test_must_fail git bisect start foo $HASH1 -- &&
test_must_fail git bisect start $HASH4 $HASH1 bar -- &&
- test -z "$(git for-each-ref "refs/bisect/*")" &&
+ test -z "$(git for-each-ref "refs/worktree/bisect/*")" &&
test -z "$(ls .git/BISECT_* 2>/dev/null)" &&
git bisect start &&
test_must_fail git bisect good foo $HASH1 &&
@@ -77,7 +77,7 @@ test_expect_success 'bisect fails if given any jun
[PATCH v3 3/4] refs: make refs/worktree/* per-worktree
We need a place to stick refs for bisects in progress that is not
shared between worktrees. So we use the refs/worktree/ hierarchy.
The is_per_worktree_ref function and associated docs learn that
refs/worktree/ is per-worktree, as does the git_path code in path.c
The ref-packing functions learn that per-worktree refs should not be
packed (since packed-refs is common rather than per-worktree).
Signed-off-by: David Turner
---
Documentation/glossary-content.txt | 5 +++--
path.c | 1 +
refs.c | 32 +++-
t/t0060-path-utils.sh | 1 +
t/t1400-update-ref.sh | 18 ++
t/t3210-pack-refs.sh | 7 +++
6 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index 8c6478b..5c707e6 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -413,8 +413,9 @@ exclude;;
[[def_per_worktree_ref]]per-worktree ref::
Refs that are per-<>, rather than
- global. This is presently only <>, but might
- later include other unusual refs.
+ global. This is presently only <> and any refs
+ that start with `refs/worktree/`, but might later include other
+ unusual refs.
[[def_pseudoref]]pseudoref::
Pseudorefs are a class of files under `$GIT_DIR` which behave
diff --git a/path.c b/path.c
index 21a4ce7..c53f732 100644
--- a/path.c
+++ b/path.c
@@ -110,6 +110,7 @@ struct common_dir common_list[] = {
{ "lost-found", 0, 1, 0 },
{ "objects", 0, 1, 0 },
{ "refs", 0, 1, 0 },
+ { "refs/worktree", 0, 1, 1 },
{ "remotes", 0, 1, 0 },
{ "worktrees", 0, 1, 0 },
{ "rr-cache", 0, 1, 0 },
diff --git a/refs.c b/refs.c
index e6fc3fe..30331bc 100644
--- a/refs.c
+++ b/refs.c
@@ -298,6 +298,11 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int search_ref_dir(struct ref_dir *dir, const char *refname, size_t
len);
+static struct ref_entry *create_dir_entry(struct ref_cache *ref_cache,
+ const char *dirname, size_t len,
+ int incomplete);
+static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
@@ -306,6 +311,24 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)
dir = &entry->u.subdir;
if (entry->flag & REF_INCOMPLETE) {
read_loose_refs(entry->name, dir);
+
+ /*
+* Manually add refs/worktree, which, being
+* per-worktree, might not appear in the directory
+* listing for refs/ in the main repo.
+*/
+ if (!strcmp(entry->name, "refs/")) {
+ int pos = search_ref_dir(dir, "refs/worktree/", 14);
+ if (pos < 0) {
+ struct ref_entry *child_entry;
+ child_entry = create_dir_entry(dir->ref_cache,
+ "refs/worktree/",
+ 14, 1);
+ add_entry_to_dir(dir, child_entry);
+ read_loose_refs("refs/worktree",
+ &child_entry->u.subdir);
+ }
+ }
entry->flag &= ~REF_INCOMPLETE;
}
return dir;
@@ -2643,6 +2666,8 @@ struct pack_refs_cb_data {
struct ref_to_prune *ref_to_prune;
};
+static int is_per_worktree_ref(const char *refname);
+
/*
* An each_ref_entry_fn that is run over loose references only. If
* the loose reference can be packed, add an entry in the packed ref
@@ -2656,6 +2681,10 @@ static int pack_if_possible_fn(struct ref_entry *entry,
void *cb_data)
struct ref_entry *packed_entry;
int is_tag_ref = starts_with(entry->name, "refs/tags/");
+ /* Do not pack per-worktree refs: */
+ if (is_per_worktree_ref(entry->name))
+ return 0;
+
/* ALWAYS pack tags */
if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref)
return 0;
@@ -2850,7 +2879,8 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
static int is_per_worktree_ref(const char *refname)
{
- return !strcmp(refname, "HEAD");
+ return !strcmp(refname, "HEAD") ||
+ starts_with(refname, "refs/worktree/");
}
static int is_pseudoref_syntax(const char *refname)
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 93605f4..28e6dff 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -275,6 +275,7 @@ test_git_path GIT_COMMON_DIR=bar remotes/bar
[PATCH v3 2/4] path: optimize common dir checking
Instead of a linear search over common_list to check whether
a path is common, use a trie. The trie search operates on
path prefixes, and handles excludes.
Signed-off-by: David Turner
---
Probably overkill, but maybe we could later use it for making exclude
or sparse-checkout matching faster (or maybe we have to go all the way
to McNaughton-Yamada for that to be truly worthwhile).
---
path.c | 215 -
1 file changed, 201 insertions(+), 14 deletions(-)
diff --git a/path.c b/path.c
index 236f797..21a4ce7 100644
--- a/path.c
+++ b/path.c
@@ -121,25 +121,212 @@ struct common_dir common_list[] = {
{ NULL, 0, 0, 0 }
};
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+/*
+ * A compressed trie. A trie node consists of zero or more characters that
+ * are common to all elements with this prefix, optionally followed by some
+ * children. If value is not NULL, the trie node is a terminal node.
+ *
+ * For example, consider the following set of strings:
+ * abc
+ * def
+ * definite
+ * definition
+ *
+ * The trie would look look like:
+ * root: len = 0, value = (something), children a and d non-NULL.
+ *a: len = 2, contents = bc
+ *d: len = 2, contents = ef, children i non-NULL, value = (something)
+ * i: len = 3, contents = nit, children e and i non-NULL, value = NULL
+ * e: len = 0, children all NULL, value = (something)
+ * i: len = 2, contents = on, children all NULL, value = (something)
+ */
+struct trie {
+ struct trie *children[256];
+ int len;
+ char *contents;
+ void *value;
+};
+
+static struct trie *make_trie_node(const char *key, void *value)
{
- char *base = buf->buf + git_dir_len;
- const struct common_dir *p;
+ struct trie *new_node = xcalloc(1, sizeof(*new_node));
+ new_node->len = strlen(key);
+ if (new_node->len) {
+ new_node->contents = xmalloc(new_node->len);
+ memcpy(new_node->contents, key, new_node->len);
+ }
+ new_node->value = value;
+ return new_node;
+}
- if (is_dir_file(base, "logs", "HEAD") ||
- is_dir_file(base, "info", "sparse-checkout"))
- return; /* keep this in $GIT_DIR */
- for (p = common_list; p->dirname; p++) {
- const char *path = p->dirname;
- if (p->is_dir && dir_prefix(base, path)) {
- replace_dir(buf, git_dir_len, get_git_common_dir());
- return;
+/*
+ * Add a key/value pair to a trie. The key is assumed to be \0-terminated.
+ * If there was an existing value for this key, return it.
+ */
+static void *add_to_trie(struct trie *root, const char *key, void *value)
+{
+ struct trie *child;
+ void *old;
+ int i;
+
+ if (!*key) {
+ /* we have reached the end of the key */
+ old = root->value;
+ root->value = value;
+ return old;
+ }
+
+ for (i = 0; i < root->len; ++i) {
+ if (root->contents[i] == key[i])
+ continue;
+
+ /*
+* Split this node: child will contain this node's
+* existing children.
+*/
+ child = malloc(sizeof(*child));
+ memcpy(child->children, root->children, sizeof(root->children));
+
+ child->len = root->len - i - 1;
+ if (child->len) {
+ child->contents = strndup(root->contents + i + 1,
+ child->len);
}
- if (!p->is_dir && !strcmp(base, path)) {
- replace_dir(buf, git_dir_len, get_git_common_dir());
- return;
+ child->value = root->value;
+ root->value = NULL;
+ root->len = i;
+
+ memset(root->children, 0, sizeof(root->children));
+ root->children[(unsigned char)root->contents[i]] = child;
+
+ /* This is the newly-added child. */
+ root->children[(unsigned char)key[i]] =
+ make_trie_node(key + i + 1, value);
+ return NULL;
+ }
+
+ /* We have matched the entire compressed section */
+ if (key[i]) {
+ child = root->children[(unsigned char)key[root->len]];
+ if (child) {
+ return add_to_trie(child, key + root->len + 1, value);
+ } else {
+ child = make_trie_node(key + root->len + 1, value);
+ root->children[(unsigned char)key[root->len]] = child;
+ return NULL;
}
}
+
+ old = root->value;
+ root->value = value;
+ return old;
+}
+
+typedef int (*match_fn)(const char *unmatched, void *data, void *baton);
+
+/*
+ * Search a tri
[PATCH v3 1/4] refs: clean up common_list
Instead of common_list having formatting like ! and /, use a struct to
hold common_list data in a structured form.
We don't use 'exclude' yet; instead, we keep the old codepath that
handles info/sparse-checkout and logs/HEAD. Later, we will use exclude.
Signed-off-by: David Turner
---
Junio was worried about the performance of common_list and the weird
string parsing bits of update_common_dir, so this version of the patch
series begins by cleaning and optimizing those bits.
Additionally, I incorporated Junio's suggestion to use
is_per_worktree_ref, and his formatting suggestions.
There is now a hack so that git for-each-ref works on per-worktree
refs.
I also added git-bisect.sh, which I had overzealously reverted during
my proofreading step last time.
---
path.c | 58 +-
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/path.c b/path.c
index 10f4cbf..236f797 100644
--- a/path.c
+++ b/path.c
@@ -91,35 +91,51 @@ static void replace_dir(struct strbuf *buf, int len, const
char *newdir)
buf->buf[newlen] = '/';
}
-static const char *common_list[] = {
- "/branches", "/hooks", "/info", "!/logs", "/lost-found",
- "/objects", "/refs", "/remotes", "/worktrees", "/rr-cache", "/svn",
- "config", "!gc.pid", "packed-refs", "shallow",
- NULL
+struct common_dir {
+ const char *dirname;
+ /* Not considered garbage for report_linked_checkout_garbage */
+ unsigned ignore_garbage:1;
+ unsigned is_dir:1;
+ /* Not common even though its parent is */
+ unsigned exclude:1;
+};
+
+struct common_dir common_list[] = {
+ { "branches", 0, 1, 0 },
+ { "hooks", 0, 1, 0 },
+ { "info", 0, 1, 0 },
+ { "info/sparse-checkout", 0, 0, 1 },
+ { "logs", 1, 1, 0 },
+ { "logs/HEAD", 1, 1, 1 },
+ { "lost-found", 0, 1, 0 },
+ { "objects", 0, 1, 0 },
+ { "refs", 0, 1, 0 },
+ { "remotes", 0, 1, 0 },
+ { "worktrees", 0, 1, 0 },
+ { "rr-cache", 0, 1, 0 },
+ { "svn", 0, 1, 0 },
+ { "config", 0, 0, 0 },
+ { "gc.pid", 1, 0, 0 },
+ { "packed-refs", 0, 0, 0 },
+ { "shallow", 0, 0, 0 },
+ { NULL, 0, 0, 0 }
};
static void update_common_dir(struct strbuf *buf, int git_dir_len)
{
char *base = buf->buf + git_dir_len;
- const char **p;
+ const struct common_dir *p;
if (is_dir_file(base, "logs", "HEAD") ||
is_dir_file(base, "info", "sparse-checkout"))
return; /* keep this in $GIT_DIR */
- for (p = common_list; *p; p++) {
- const char *path = *p;
- int is_dir = 0;
- if (*path == '!')
- path++;
- if (*path == '/') {
- path++;
- is_dir = 1;
- }
- if (is_dir && dir_prefix(base, path)) {
+ for (p = common_list; p->dirname; p++) {
+ const char *path = p->dirname;
+ if (p->is_dir && dir_prefix(base, path)) {
replace_dir(buf, git_dir_len, get_git_common_dir());
return;
}
- if (!is_dir && !strcmp(base, path)) {
+ if (!p->is_dir && !strcmp(base, path)) {
replace_dir(buf, git_dir_len, get_git_common_dir());
return;
}
@@ -129,16 +145,16 @@ static void update_common_dir(struct strbuf *buf, int
git_dir_len)
void report_linked_checkout_garbage(void)
{
struct strbuf sb = STRBUF_INIT;
- const char **p;
+ const struct common_dir *p;
int len;
if (!git_common_dir_env)
return;
strbuf_addf(&sb, "%s/", get_git_dir());
len = sb.len;
- for (p = common_list; *p; p++) {
- const char *path = *p;
- if (*path == '!')
+ for (p = common_list; p->dirname; p++) {
+ const char *path = p->dirname;
+ if (p->ignore_garbage)
continue;
strbuf_setlen(&sb, len);
strbuf_addstr(&sb, path);
--
2.0.4.315.gad8727a-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 2:46 PM, Johan Herland wrote: > If we don't already refuse to merge into a ref outside refs/notes, then > I would consider that a bug to be fixed, and not some corner use case that > we must preserve for all future. > > After all, we do already have a test in t3308 named 'fail to merge into > various non-notes refs', where one of the non-notes ref being tested are: > > test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x > This test is checking if the ref pointed at by refs/heads/master *is* a note. But you could create a ref outside of refs/notes which is a note but which isn't inside refs/notes I did just find that we expand remote-ref using expand_notes_ref, and it does *not* currently let us reference refs outside of refs/notes.. so we can merge IN to a ref not inside refs/notes (using the environment variable) but we can't merge FROM refs/tracking/origin/notes/y for example, which means currently all notes we merge from have to be located into refs/notes/* There are some weird issues here. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] run-command: Improve readability of struct child_process
Reordering the struct member env to be next to env_array
helps understanding the struct better.
This also adds comments to indicate that arg{s,v} and (env, env_array)
are used for the same purpose and only one must be used. Although
these comments are in the Documentation, I still think they are
a good idea in the code here as well.
Signed-off-by: Stefan Beller
---
run-command.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/run-command.h b/run-command.h
index 1103805..e67395d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -8,8 +8,9 @@
#include "argv-array.h"
struct child_process {
- const char **argv;
+ const char **argv; /* Use only one of arg{v,s} */
struct argv_array args;
+ const char *const *env; /* Use only one of (env, env_array) */
struct argv_array env_array;
pid_t pid;
/*
@@ -34,7 +35,6 @@ struct child_process {
int out;
int err;
const char *dir;
- const char *const *env;
unsigned no_stdin:1;
unsigned no_stdout:1;
unsigned no_stderr:1;
@@ -45,7 +45,7 @@ struct child_process {
unsigned clean_on_exit:1;
};
-#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
+#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, NULL, ARGV_ARRAY_INIT }
void child_process_init(struct child_process *);
int start_command(struct child_process *);
--
2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 4:26 AM, Junio C Hamano wrote: > Johan Herland writes: >> I know that we don't yet have a "proper" place to put remote notes refs, >> but the in notes..merge _must_ be a "local" notes ref (you even >> use the notation in the documentation below). Thus, I believe >> we can presume that the local notes ref must live under refs/notes/*, >> and hence drop the "refs/notes/" prefix from the config key (just like >> branch..* can presume that lives under refs/heads/*). > > I am OK going in that direction, as long as we promise that "notes > merge" will forever refuse to work on --notes=$ref where $ref does > not begin with refs/notes/. If we don't already refuse to merge into a ref outside refs/notes, then I would consider that a bug to be fixed, and not some corner use case that we must preserve for all future. After all, we do already have a test in t3308 named 'fail to merge into various non-notes refs', where one of the non-notes ref being tested are: test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x >> Except that this patch in its current form will occupy the .merge config >> key... >> >> Can you rename to notes..mergestrategy instead? > > This is an excellent suggestion. > >> Or even better, take inspiration from branch..mergeoptions, > > Please don't. > > That is one of the design mistakes that was copied from another > design mistake (remotes.*.tagopt). I'd want to see us not to repeat > these design mistakes. > > These configuration variables were made to take free-form text value > that is split according to shell rules, primarily because it was > expedient to implement. Read its value into a $variable and put it > at the end of the command line to let the shell split it. "tagopt" > was done a bit more carefully in that it made to react only with a > fixed string "--no-tags", so it was hard to abuse, but "mergeoptions" > allowed you to override something that you wouldn't want to (e.g. it > even allowed you to feed '--message=foo'). > > Once you start from such a broken design, it would be hard to later > make it saner, even if you wanted to. You have to retroactively > forbid something that "worked" (with some definition of "working"), > or you have to split, parse and then reject something that does not > make sense yourself, reimplementing dequote/split rule used in the > shell---which is especially problematic when you no longer write in > shell scripts. > > So a single string value that names one of the supported strategy > stored in notes..mergestrategy is an excellent choice. An > arbitrary string in notes..mergeoptions is to be avoided. Understood. And agreed. ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 12:16 PM, Jacob Keller wrote:
> Oh interesting. I did a test. If you provide a fully qualified ref not
> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y
> rather than refs/foo/y
>
> I need to do some more digging on this to determine the exact thing going
> on...
>
> Regards,
> Jake
I did some more digging. If you pass a notes ref to "--refs" option,
that requires all notes to be bound to refs/notes/* and does not allow
passing of arbitrary refs. However, you can set the environment
variable GIT_NOTES_REF or core.notesRef to a fully qualified
reference.
That seems very arbitrary that --ref works by expanding notes and the
environment variable and configuration option do not... [1]
I think this inconsistency is very weird, because *most* people will
not use refs/notes/* etc. This makes it so that --refs forces you to
use refs/notes/* or it will prefix it for you... ie: you can use
notes/x, refs/notes/x, x, but if you use refs/tags/x it will DWIM into
refs/notes/refs/tags/x
I think this is very confusing that --refs doesn't behave the same as
other sections... either we should enforce this on all refs or we
should fix the DWIM-ery to be consistent.
that is, we should fix DWIM-ery to be:
(1) if it starts with refs/* leave it alone
(2) if it starts with notes/*, prefix it with refs/
(3) otherwise prefix it with refs/notes/
But that way, refs/some-other-notes/ will work fine instead of
becoming something else.
We should also fix reads of environment variable etc such taht we
enforce these values always are fully qualified and begin with refs.
Otherwise, use of --refs and the environment variable don't allow the
same formats.
Regards,
Jake
[1] 8ef313e1ec3b ("builtin/notes.c: Split notes ref DWIMmery into a
separate function")
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cleanup submodule_config a bit.
On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine wrote:
> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:
>> In the first hunk, `submodule` is NULL all the time, so we can make it
>> clearer
>> by directly returning NULL.
>>
>> In the second hunk, we can directly return the lookup values as it also makes
>> the coder clearer.
>>
>> Signed-off-by: Stefan Beller
>> ---
>> submodule-config.c | 12 +---
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/submodule-config.c b/submodule-config.c
>> index 199692b..08e93cc 100644
>> --- a/submodule-config.c
>> +++ b/submodule-config.c
>> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct
>> submodule_cache *cache,
>> }
>>
>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
>> - return submodule;
>> + return NULL;
>
> There are a couple other places which return 'submodule' when it is
> known to be NULL. One could rightly expect that they deserve the same
> treatment, otherwise, the code becomes more confusing since it's not
> obvious why 'return NULL' is used some places but not others.
They were slightly less obvious to me, fixed now as well!
>
>> switch (lookup_type) {
>> case lookup_name:
>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
>> submodule_cache *cache,
>>
>> switch (lookup_type) {
>> case lookup_name:
>> - submodule = cache_lookup_name(cache, sha1, key);
>> - break;
>> + return cache_lookup_name(cache, sha1, key);
>> case lookup_path:
>> - submodule = cache_lookup_path(cache, sha1, key);
>> - break;
>> + return cache_lookup_path(cache, sha1, key);
>> + default:
>> + return NULL;
>> }
>> -
>> - return submodule;
>
> Earlier in the function, there's effectively a clone of this logic to
> which you could apply the same transformation. Changing it here, while
> ignoring the clone, makes the code more confusing (or at least
> inconsistent) rather than less.
Not quite. Note the `if (submodule)` in the earlier version, so in case
cache_lookup_{name, path} return NULL, we keep going. The change I
propose is at the end of the function and we definitely return no matter
if it is NULL or not.
>
>> }
>>
>> static const struct submodule *config_from_path(struct submodule_cache
>> *cache,
>> --
>> 2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cleanup submodule_config a bit.
On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:
> In the first hunk, `submodule` is NULL all the time, so we can make it clearer
> by directly returning NULL.
>
> In the second hunk, we can directly return the lookup values as it also makes
> the coder clearer.
>
> Signed-off-by: Stefan Beller
> ---
> submodule-config.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 199692b..08e93cc 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct
> submodule_cache *cache,
> }
>
> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
> - return submodule;
> + return NULL;
There are a couple other places which return 'submodule' when it is
known to be NULL. One could rightly expect that they deserve the same
treatment, otherwise, the code becomes more confusing since it's not
obvious why 'return NULL' is used some places but not others.
> switch (lookup_type) {
> case lookup_name:
> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
> submodule_cache *cache,
>
> switch (lookup_type) {
> case lookup_name:
> - submodule = cache_lookup_name(cache, sha1, key);
> - break;
> + return cache_lookup_name(cache, sha1, key);
> case lookup_path:
> - submodule = cache_lookup_path(cache, sha1, key);
> - break;
> + return cache_lookup_path(cache, sha1, key);
> + default:
> + return NULL;
> }
> -
> - return submodule;
Earlier in the function, there's effectively a clone of this logic to
which you could apply the same transformation. Changing it here, while
ignoring the clone, makes the code more confusing (or at least
inconsistent) rather than less.
> }
>
> static const struct submodule *config_from_path(struct submodule_cache
> *cache,
> --
> 2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
Karthik Nayak writes: > On Wed, Aug 12, 2015 at 10:43 PM, Junio C Hamano wrote: > ... >> %(objectname:abbrev=8). To specify two modification magics, each of >> which takes a number, the user would say e.g. >> >> %(objectname:abbrev=8,magic=4) >> ... >> And that would be following %(align:8). Both 'left' (implied >> default) and '8' are instructing 'align' what to do. > > Will follow this. :) I think the most generic way to think about this is to consider that the most fully spelled form of align would be this: %(align:width=12,position=left) And another rule you would have is that the user is allowed to omit "=" when it is obvious from its value. For "align", 'left' can only possibly be the value for 'position' and similarly '12' for 'width'. That is why the "objectname" example says "abbrev=8", and not "abbrev,8", because from the value of "8" without the attribute name, you cannot tell if the user meant abbrev=8 or magic=8. That '"=" can be omitted' rule makes both of these valid constructs: %(align:12,left) %(align:left,12) Moreover, if you make "left aligned" the default behaviour when position is not specified, you can make %(align:12) the shortest way to spell the same thing. Note that I said "if you make"; I do not offhand know if all the internal callers of this mechanism your updates to "branch -l" and "tag -l" would want left aligned output (if so, that is one argument to favor making left-aligned the implicit default; if not, it may be better to require the position always specified). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
On Wed, Aug 12, 2015 at 10:43 PM, Junio C Hamano wrote: > Karthik Nayak writes: > >> On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano wrote: >> >>> Minor nits on the design. %(align:[,]) would let >>> us write %(align:16)...%(end) and use the "default position", which >>> may be beneficial if one kind of alignment is prevalent (I guess all >>> the internal users left-align?) %(align:,) forces >>> users to spell both out all the time. >>> >> >> Isn't that better? I mean It sets a format which the others eventually >> can follow >> %(atom:suboption,value). >> For example: %(objectname:abbrev,size) > > No, I do not think it is better. First of all, the similarity you > are perceiving does not exist. For 'objectname:abbrev', the 'size' > is a property of the 'abbrev'. For 'align:left', the 'width' is not > a property of the position. It is a property given to 'align' > (i.e. you have this many display columns to work with). > > Also, if there are ways other than 'abbrev' that '8' could affect > the way how %(objectname) is modified, then it should be spelled as > %(objectname:abbrev=8). To specify two modification magics, each of > which takes a number, the user would say e.g. > > %(objectname:abbrev=8,magic=4) > > The syntax %(objectname:abbrev,size) would not allow you to extend > it nicely---you would end up with %(objectname:abbrev,8,magic,4) or > something silly like that. > > Seeing your %(objectname:abbrev,size), I'd imagine that you are > assuming that you will never allow any magic other than 'abbrev' > that takes a number to %(objectname). And under that assumption > %(objectname:8) would be a short-hand for %(objectname:8,abbrev). > I never thought about this, thanks for explaining. > And that would be following %(align:8). Both 'left' (implied > default) and '8' are instructing 'align' what to do. > Will follow this. :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
partial stash, reversed-merge, and file modifications
I'm pretty happy about Git in general, but for two situations where I've found workarounds, which both have the same problem, which is that they "touch" files unnecessarily: * First case: merge into a dirty tree. I often want to "git pull" into a tree that's dirty. I know many people find this to be heresy, but for various reasons, I have a few trees that are pretty much always dirty and where I want to pull anyway without ever wanting to commit those changes. The simplest solution I found is: git stash; git merge --ff-only; git stash apply; git stash drop Problem with it: this will needlessly "touch" all the files which are locally modified but aren't affected by the merge. So a subsequent "make" can easily end up taking a lot more time than needed. A simple solution to this problem would be to only stash those files which conflict: git stash save --only-some-files $(git merge 2>&1 | sed -ne 's/^ //p') git merge --ff-only; git stash apply; git stash drop but of course the "--only-some-files" option to "stash save" doesn't exist. And writing an equivalent script is pretty painful. * Second case: merge with reversed parents The order of parents in a merge is sometimes important. Say you're in your branch "newfeature" and you want to install it into "master", you could do it this way: git merge master; <..make; check; push..> but that gives you a history where the first parent is your feature branch and all the changes made to master in the mean time look like secondary changes. This is probably OK seen from "newfeature" but if you push this to "master", it will look odd on "master". So instead, you'll want to do what I call a "reversed merge": git checkout master; git merge newfeature; <..make; check; push..> Now the history tree is right. Good. But beside it being sightly more cumbersome, the main problem with it is that, again, this will needlessly "touch" all those files that are modified by "newfeature" but not by "master" (compared to the ancestor). So again, the subsequent "make" can take a lot more time than needed. I'd love to either hear about ways to avoid/reduce this problem with current Git, or else to see some new features added to Git to reduce/solve those problems. Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: enhanced remote ref namespaces
Jacob Keller writes: > That still hasn't really resolved the question of how to deal with > tags, but it does solve the question of how to deal with replace and > notes refs. I do not think it would be a good change to add a [remote "foo"] fetch = refs/tags/*:refs/tracking/foo/tags/* as people do expect to see tags from upstream to appear in their local tag namespace when cloning from the central meeting point of their project (e.g. Linus's kernel repository). I'm willing to believe those who argued that they have both private tags and public tags in their repository, but I think that would be something better supported by adding "git tag --local foo" that creates a local tag in e.g. refs/local-tags/foo, and by teaching DWIMmery to also look in there when the user says "foo". Alternatively, they can use their local branch namespace, which is already private to them. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Am 11.08.2015 um 12:40 schrieb Johannes Schauer: Hi, for repositories with more than 16k files and folders, git-archive will create zip files which store the wrong number of entries. That is, it stores the number of entries modulo 16k. This will break unpackers that do not include code to support this brokenness. The limit is rather 65535 entries, isn't it? The entries field has two bytes, and they are used fully. Which programs are affected? InfoZIP Zip 3.0 and 7-Zip 9.20 seem to handle an archive with more entries just fine. The built-in functionality of Windows 10 doesn't. Besides, 64K entries should be enough for anybody. ;-) Seriously, though: What kind of repository has that many files and uses the ZIP format to distribute snapshots? Just curious. Instead, git-archive should use the zip64 extension to handle more than 16k files and folders correctly. That seems to be what InfoZIP does and Windows 10 handles it just fine. If lower Windows versions and other popular extractors can unzip such archives as well then this might indeed be the way to go. Thanks, René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: proper remote ref namespaces
Marc Branchaud writes: > Not a lot. Existing DWIMery already handles ambiguous branches, by > preferring a local branch name over any remote ones. The only teaching > that's really needed is ... You need to remember that there are five useful things you can do to mutable things. - Creation can be covered by teaching "clone" to put a new-style refspec but the same change needs to also go to "remote add". - Reading is done by the DWIMery in ref_rev_parse_rules[] (your point above). - Updating is automatic, as "fetch" does not have any funny built-in intuit and blindly follows configured fetch refspec. - Deletion by "branch -d -r" and "remote remove" needs to be careful about designing how the case where both old and new hierarchies exist should be handled (my gut feeling is "delete both", but there may be funny corner cases). - Enumeration by "branch -l -r" probably shares the same issue as deletion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] submodule config lookup API
Stefan Beller writes: > However just as I was convinced of my review and sent out the email, I started > working with it. And I found nits which I'd ask you to squash into the round > or > put on top. Good ;-). I'd prefer a full reroll, as it has been quite a while since v5 was originally posted, once you get to a reasonable state where you are reasonably confident that you won't find more of such nits. Hopefully more people might have eyes on the list to review and comment this round. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Wed, Aug 12, 2015 at 12:09 PM, Junio C Hamano wrote: > Jacob Keller writes: > >> On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano wrote: >>> Johan Herland writes: >>> I know that we don't yet have a "proper" place to put remote notes refs, but the in notes..merge _must_ be a "local" notes ref (you even use the notation in the documentation below). Thus, I believe we can presume that the local notes ref must live under refs/notes/*, and hence drop the "refs/notes/" prefix from the config key (just like branch..* can presume that lives under refs/heads/*). >>> >>> I am OK going in that direction, as long as we promise that "notes >>> merge" will forever refuse to work on --notes=$ref where $ref does >>> not begin with refs/notes/. >> >> It appears that notes merge already allows operating on refs not in >> "refs/notes" > > If that is the case, then the only sane choice left to us is to > accept only fully qualified refname, e.g. refs/notes/commit, in > notes..mergestrategy, without shortening, I would think. Oh interesting. I did a test. If you provide a fully qualified ref not inside refs/notes, then it assumes you meant refs/notes/refs/foo/y rather than refs/foo/y I need to do some more digging on this to determine the exact thing going on... Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] cleanup submodule_config a bit.
In the first hunk, `submodule` is NULL all the time, so we can make it clearer
by directly returning NULL.
In the second hunk, we can directly return the lookup values as it also makes
the coder clearer.
Signed-off-by: Stefan Beller
---
submodule-config.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 199692b..08e93cc 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -387,7 +387,7 @@ static const struct submodule *config_from(struct
submodule_cache *cache,
}
if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
- return submodule;
+ return NULL;
switch (lookup_type) {
case lookup_name:
@@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
submodule_cache *cache,
switch (lookup_type) {
case lookup_name:
- submodule = cache_lookup_name(cache, sha1, key);
- break;
+ return cache_lookup_name(cache, sha1, key);
case lookup_path:
- submodule = cache_lookup_path(cache, sha1, key);
- break;
+ return cache_lookup_path(cache, sha1, key);
+ default:
+ return NULL;
}
-
- return submodule;
}
static const struct submodule *config_from_path(struct submodule_cache *cache,
--
2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] submodule config lookup API
On Wed, Aug 12, 2015 at 10:53 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Mon, Jun 15, 2015 at 2:48 PM, Junio C Hamano wrote: >>> Thanks. Will replace and wait for comments from others. >> >> I have reviewed the patches carefully and they look good to me. > > OK, I recall there were a few iterations with review comments before > this round. Is it your impression that they have been addressed > adequately? I payed only little attention to the previous rounds and the review comments thereof, because round 5 was up for quite a long time and nobody commented so far. However just as I was convinced of my review and sent out the email, I started working with it. And I found nits which I'd ask you to squash into the round or put on top. > > Do you prefer it to be rebased to a more recent 'master' before you > build your work on top of it (I think the topic currently builds on > top of v2.5.0-rc0~56)? Looking at the reviews for the "RFC parallel fetch for submodules" I'd like to use `argv_array_pushv` which was introduced via 85b343245b (2015-06-14, argv-array: implement argv_array_pushv()) and is already in origin/next, but not origin/master. But as I first work on the submodule--helper (both the small helper functions as well as the whole "update" thereafter), I think this dependency is not a pressing issue yet. > > Thanks. Stefan Beller (2): Fixup hv/documentation cleanup submodule_config a bit. Documentation/technical/api-submodule-config.txt | 3 +-- submodule-config.c | 12 +--- 2 files changed, 6 insertions(+), 9 deletions(-) -- 2.5.0.234.gefc8a62 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Fixup hv/documentation
If you want to look up by name, use `submodule_from_name` instead. Signed-off-by: Stefan Beller --- Documentation/technical/api-submodule-config.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/technical/api-submodule-config.txt b/Documentation/technical/api-submodule-config.txt index 2ea520a..941fa17 100644 --- a/Documentation/technical/api-submodule-config.txt +++ b/Documentation/technical/api-submodule-config.txt @@ -49,8 +49,7 @@ Functions `const struct submodule *submodule_from_path(const unsigned char *commit_sha1, const char *path)`:: - Lookup values for one submodule by its commit_sha1 and path or - name. + Lookup values for one submodule by its commit_sha1 and path. `const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`:: -- 2.5.0.234.gefc8a62 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
Jacob Keller writes: > On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano wrote: >> Johan Herland writes: >> >>> I know that we don't yet have a "proper" place to put remote notes refs, >>> but the in notes..merge _must_ be a "local" notes ref (you even >>> use the notation in the documentation below). Thus, I believe >>> we can presume that the local notes ref must live under refs/notes/*, >>> and hence drop the "refs/notes/" prefix from the config key (just like >>> branch..* can presume that lives under refs/heads/*). >> >> I am OK going in that direction, as long as we promise that "notes >> merge" will forever refuse to work on --notes=$ref where $ref does >> not begin with refs/notes/. > > It appears that notes merge already allows operating on refs not in > "refs/notes" If that is the case, then the only sane choice left to us is to accept only fully qualified refname, e.g. refs/notes/commit, in notes..mergestrategy, without shortening, I would think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano wrote: > Johan Herland writes: > >> I know that we don't yet have a "proper" place to put remote notes refs, >> but the in notes..merge _must_ be a "local" notes ref (you even >> use the notation in the documentation below). Thus, I believe >> we can presume that the local notes ref must live under refs/notes/*, >> and hence drop the "refs/notes/" prefix from the config key (just like >> branch..* can presume that lives under refs/heads/*). > > I am OK going in that direction, as long as we promise that "notes > merge" will forever refuse to work on --notes=$ref where $ref does > not begin with refs/notes/. > It appears that notes merge already allows operating on refs not in "refs/notes" the way you select what you are merging into is via either --ref or the environment variable.I chose to use the full refs/notes/* format as it was the only one that also supported all layouts. So, should we enforce that default_notes_ref always requires refs inside refs/notes? that doesn't seem a bad idea here. If we only disallow it for merge I think that would be very confusing. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: enhanced remote ref namespaces
On Wed, Aug 12, 2015 at 11:54 AM, Junio C Hamano wrote:
> Jacob Keller writes:
>
>>> Just thinking aloud, perhaps we can introduce a brand new top level
>>> hierarchy refs/remote/$O/{heads,tags,notes,...}, and give backward
>>> compatibility by making a moral equivalent of a symbolic link from
>>> refs/remote/$O/heads to refs/remotes/$O/. The true remote-tracknig
>>> refs continue to live in refs/remotes/$O/* and old tools that do not
>>> know the new layout would not be hurt. New tools that want to
>>> ignore and look only at refs/remote/$O/* can do so through the moral
>>> equivalent of a symbolic link. "remote remove" needs to be taught
>>> about this single exception (i.e. "the symbolic link"), but the
>>> places it needs to touch is limited only to two places and will not
>>> grow.
>>
>> I think this proposal makes the sense.. I'd go with something like:
>>
>> refs/tracking//(heads|tags|notes|replace|etc)/*
>>
>> with a symlink from or into
>>
>> refs/tracking//heads -> refs/remotes/
>
> I actually do not think we even need the "symlink" thing.
>
> We can just say refs/remotes/$O has been and forever will be where
> the remote-tracking branches will live. With or without the symlink
> for backward compatibility, the updated Git will need to be aware of
> the two places to deal with older and newer repositories anyway.
>
> "everything is now under refs/tracking/$O, not spread over many
> unbounded number of places like refs/remotes-$foo/$O" may appear to
> be cleaner but two is already not too bad and will greatly reduce
> the transition pain.
Ok, so just have branches be in refs/remotes/$O and all new things
be in refs/tracking/$O/category
that sounds reasonable enough to me.
That still hasn't really resolved the question of how to deal with
tags, but it does solve the question of how to deal with replace and
notes refs.
Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: enhanced remote ref namespaces
Jacob Keller writes:
>> Just thinking aloud, perhaps we can introduce a brand new top level
>> hierarchy refs/remote/$O/{heads,tags,notes,...}, and give backward
>> compatibility by making a moral equivalent of a symbolic link from
>> refs/remote/$O/heads to refs/remotes/$O/. The true remote-tracknig
>> refs continue to live in refs/remotes/$O/* and old tools that do not
>> know the new layout would not be hurt. New tools that want to
>> ignore and look only at refs/remote/$O/* can do so through the moral
>> equivalent of a symbolic link. "remote remove" needs to be taught
>> about this single exception (i.e. "the symbolic link"), but the
>> places it needs to touch is limited only to two places and will not
>> grow.
>
> I think this proposal makes the sense.. I'd go with something like:
>
> refs/tracking//(heads|tags|notes|replace|etc)/*
>
> with a symlink from or into
>
> refs/tracking//heads -> refs/remotes/
I actually do not think we even need the "symlink" thing.
We can just say refs/remotes/$O has been and forever will be where
the remote-tracking branches will live. With or without the symlink
for backward compatibility, the updated Git will need to be aware of
the two places to deal with older and newer repositories anyway.
"everything is now under refs/tracking/$O, not spread over many
unbounded number of places like refs/remotes-$foo/$O" may appear to
be cleaner but two is already not too bad and will greatly reduce
the transition pain.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: enhanced remote ref namespaces
On Wed, Aug 12, 2015 at 9:10 AM, Junio C Hamano wrote:
> Jacob Keller writes:
>
>> Recently there was some discussion about git-notes and how we do not
>> fetch notes from remotes by default. The big problem with doing so is
>> because refs/remotes/* hierarchy is only setup for branches (heads),
>> so we don't have any clean location to put them.
>
> I wouldn't call this topic "proper" namespaces, though. What we
> have is widely used and it shouldn't be broken. Call it "enhanced",
> perhaps.
>
Ok.
> Some design boundaries:
>
> - Moving the remote-tracking branch hierarchy from refs/remotes/$O/*
>to refs/remotes/$O/heads/* would not fly, because it will break
>existing repositories. Do not even waste time on pursuing
>refs/remotes/$O/{heads,tags,notes...}/*
>
even if we maintained new git's abililty to work with this, ie: only
external-to-git scripts would break and only for new clones? Maybe we
don't want to go this route, but it seems like the way that the
original proposal was headed.
> - Extending the current refs/remotes/$O/* (for branches) and doing
>refs/remote-tags/$O/* (for tags) may work, would not break
>existing repositories, and could to be extended further to cover
>refs/remote-notes/$O and friends. It however may not look pretty
>(weak objection) and more importantly, it would make it harder to
>"cull" things that came from a single remote.
>
> Just thinking aloud, perhaps we can introduce a brand new top level
> hierarchy refs/remote/$O/{heads,tags,notes,...}, and give backward
> compatibility by making a moral equivalent of a symbolic link from
> refs/remote/$O/heads to refs/remotes/$O/. The true remote-tracknig
> refs continue to live in refs/remotes/$O/* and old tools that do not
> know the new layout would not be hurt. New tools that want to
> ignore and look only at refs/remote/$O/* can do so through the moral
> equivalent of a symbolic link. "remote remove" needs to be taught
> about this single exception (i.e. "the symbolic link"), but the
> places it needs to touch is limited only to two places and will not
> grow.
>
I think this proposal makes the sense.. I'd go with something like:
refs/tracking//(heads|tags|notes|replace|etc)/*
with a symlink from or into
refs/tracking//heads -> refs/remotes/
I prefer tracking vs remote because "remote" and "remotes" are too
similar for my taste. tracking I think gets the idea across pretty
straight forward. Using a symlink personally I'd symlink the
refs/tracking into refs/remotes rather than make refs/remotes the real
storage.
> If somebody got confused, notice that in the above description, I
> said refs/remotes/ and refs/remote/. The former must stay. The
> name of the latter is open to bikeshedding. Some may prefer a name
> that is more distinct (refs/tracking/ or something, perhaps?). I
> happen to prefer a name that is similar, but this preference is very
> weak and I can persuaded to go either way.
I don't like it being so similar that we now have typos between
remotes and remote.. ie: remotes/ works for heads, but
"remotes//tags" does not... that sounds like it would get
confusing.
Symlinking the old location seems reasonable to me, as it would leave
all the same data in the locations expected by the old tools, while
keeping all actual storage in the new location.
In this way, we would no longer need configuration settings. It
honestly doesn't matter to me which direction we symlink either.
As for the other complex issue is what to do about "refs/tracking//tags
The big discussion on the original thread is about how tags would
work. I'm personally ok with *ignoring* tags and leaving it the way it
is for now, and just doing this as a solid place to stick
notes/replace/etc.
Or, we could go the route of continuing to stick tags into "refs/tags"
at the same time as also sticking them into
refs/tracking//tags
Or.. we could go the full route of fixing up lookup of tags such that
we put tags in refs/tracking//tags and we have it lookup tags
there via something like:
1) local tags preferred
2) any remote tag as long as all remote tags point to the same commit
object (how we select which to use is not very relevant here... we
could actually go with as long as the tag object is the same so two
remotes with annotated tags pointing to the same object but different
tag id would be ambiguous as well)
3) warn the user must disambiguate the tag via the remote name.
We could also teach fetch to warn about remote tags which are
ambiguous (ie: two remotes with same named tag objects pointing to
different things)
How goes this sound? I think it makes sense... I don't know how to do
all this without breaking some backwards compatibility though... I
think we can maintain expectations for the general user but I feel
that any change here will break *someones* scripts.
Regards,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.k
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund: On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: FWIW Git for Windows has this patch (that I wanted to contribute in due time, what with being busy with all those tickets) to solve the problem mentioned in your patch in a different way: https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. I, too, think that it is a wrong decision to pessimize git for the sake of a single test case. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] revisions --stdin: accept CRLF line terminators
Am 12.08.2015 um 00:14 schrieb Junio C Hamano: Now, I am wondering if it makes sense to do these two things: * Teach revision.c::read_revisions_from_stdin() to use strbuf_getline() instead of strbuf_getwholeline(). * Teach strbuf_getline() to remove CR at the end when stripping the LF at the end, only if "term" parameter is set to LF. Doing so would solve 1. and 2., but we obviously need to audit all the other uses of strbuf_getline() to see if they can benefit (or if some of them may be broken because they _always_ need LF terminated lines, i.e. CRLF terminated input is illegal to them). I can see what I can do with these. Don't hold your breath, though. As to 3., I think it is OK. The code structure of 4. is too ugly and needs to be revamped to go one line at a time first before even thinking about how to proceed, I would think. Regarding update-ref --stdin (your 4.), I notice that the input format is very strict, so the solution is to allow an optional CR before the LF. I alread have a patch, but it skips all trailing space, which is probably too lenient. (I only needed the patch once for a debug sesssion, but there is no obvious breakage without the patch.) -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] http: add support for specifying the SSL version
On Wed, Aug 12, 2015 at 04:24:51PM +0200, Elia Pinto wrote: > Teach git about a new option, "http.sslVersion", which permits one to > specify the SSL version to use when negotiating SSL connections. The > setting can be overridden by the GIT_SSL_VERSION environment > variable. > > Signed-off-by: Elia Pinto > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 315f271..76a4f2b 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1595,6 +1595,27 @@ http.saveCookies:: > +http.sslVersion:: > + The SSL version to use when negotiating an SSL connection, if you > + want to force the default. The available and default version depend on > + whether libcurl was built against NSS or OpenSSL and the particular > configuration > + of the crypto library in use. Internally this sets the > 'CURLOPT_SSL_VERSION' > + option; see the libcurl documentation for more details on the format > + of this option and for the ssl version supported. Actually the possible > values > + of this option are: > + > + - sslv2 > + - sslv3 > + - tlsv1 > + - tlsv1.0 > + - tlsv1.1 > + - tlsv1.2 > ++ > +Can be overridden by the 'GIT_SSL_VERSION' environment variable. > +To force git to use libcurl's default ssl version and ignore any > +explicit http.sslversion option, set 'GIT_SSL_VERSION' to the > +empty string. Unfortunately, this won't format properly in Asciidoc; the final paragraph will be indented as part of the itemized list supported SSL versions. Here's a squash-in to fix it: 8< Subject: [PATCH] fixup! http: add support for specifying the SSL version --- Documentation/config.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 76a4f2b..b23b01a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1610,6 +1610,7 @@ http.sslVersion:: - tlsv1.0 - tlsv1.1 - tlsv1.2 + + Can be overridden by the 'GIT_SSL_VERSION' environment variable. To force git to use libcurl's default ssl version and ignore any -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH jk/prune-mtime] prune: close directory earlier during loose-object directory traversal
On Wed, Aug 12, 2015 at 07:43:01PM +0200, Johannes Sixt wrote: > 27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16) > introduced a new function for_each_loose_file_in_objdir() with a helper > for_each_file_in_obj_subdir(). The latter calls callbacks for each file > found during a directory traversal and finally also a callback for the > directory itself. > > git-prune uses the function to clean up the object directory. In > particular, in the directory callback it calls rmdir(). On Windows XP, > this rmdir call fails, because the directory is still open while the > callback is called. Close the directory before calling the callback. Makes sense, and the patch looks good to me. Sorry for breaking things on Windows. Acked-by: Jeff King -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] submodule config lookup API
Stefan Beller writes: > On Mon, Jun 15, 2015 at 2:48 PM, Junio C Hamano wrote: >> Thanks. Will replace and wait for comments from others. > > I have reviewed the patches carefully and they look good to me. OK, I recall there were a few iterations with review comments before this round. Is it your impression that they have been addressed adequately? Do you prefer it to be rebased to a more recent 'master' before you build your work on top of it (I think the topic currently builds on top of v2.5.0-rc0~56)? Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH jk/prune-mtime] prune: close directory earlier during loose-object directory traversal
27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16) introduced a new function for_each_loose_file_in_objdir() with a helper for_each_file_in_obj_subdir(). The latter calls callbacks for each file found during a directory traversal and finally also a callback for the directory itself. git-prune uses the function to clean up the object directory. In particular, in the directory callback it calls rmdir(). On Windows XP, this rmdir call fails, because the directory is still open while the callback is called. Close the directory before calling the callback. Signed-off-by: Johannes Sixt --- My Windows 8.1 machine does not require this fix for some unkonwn reason. But we still cater for Windows XP users, where this change is a real improvement. sha1_file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 4d1b26f..5cecc68 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -3473,12 +3473,12 @@ static int for_each_file_in_obj_subdir(int subdir_nr, break; } } + closedir(dir); + strbuf_setlen(path, baselen); - if (!r && subdir_cb) r = subdir_cb(subdir_nr, path->buf, data); - closedir(dir); return r; } -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
Karthik Nayak writes: > On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano wrote: > >> Minor nits on the design. %(align:[,]) would let >> us write %(align:16)...%(end) and use the "default position", which >> may be beneficial if one kind of alignment is prevalent (I guess all >> the internal users left-align?) %(align:,) forces >> users to spell both out all the time. >> > > Isn't that better? I mean It sets a format which the others eventually > can follow > %(atom:suboption,value). > For example: %(objectname:abbrev,size) No, I do not think it is better. First of all, the similarity you are perceiving does not exist. For 'objectname:abbrev', the 'size' is a property of the 'abbrev'. For 'align:left', the 'width' is not a property of the position. It is a property given to 'align' (i.e. you have this many display columns to work with). Also, if there are ways other than 'abbrev' that '8' could affect the way how %(objectname) is modified, then it should be spelled as %(objectname:abbrev=8). To specify two modification magics, each of which takes a number, the user would say e.g. %(objectname:abbrev=8,magic=4) The syntax %(objectname:abbrev,size) would not allow you to extend it nicely---you would end up with %(objectname:abbrev,8,magic,4) or something silly like that. Seeing your %(objectname:abbrev,size), I'd imagine that you are assuming that you will never allow any magic other than 'abbrev' that takes a number to %(objectname). And under that assumption %(objectname:8) would be a short-hand for %(objectname:8,abbrev). And that would be following %(align:8). Both 'left' (implied default) and '8' are instructing 'align' what to do. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2' 12/16] diff: use tempfile module
Also add some code comments explaining how the fields in "struct
diff_tempfile" are used.
Signed-off-by: Michael Haggerty
---
This is a replacement for tempfile patch v2 12/16 that includes some
extra code comments. It is also available from my GitHub repo [1] on
branch "tempfile".
[1] https://github.com/mhagger/git
diff.c | 46 +++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/diff.c b/diff.c
index 7500c55..528d25c 100644
--- a/diff.c
+++ b/diff.c
@@ -2,6 +2,7 @@
* Copyright (C) 2005 Junio C Hamano
*/
#include "cache.h"
+#include "tempfile.h"
#include "quote.h"
#include "diff.h"
#include "diffcore.h"
@@ -308,11 +309,26 @@ static const char *external_diff(void)
return external_diff_cmd;
}
+/*
+ * Keep track of files used for diffing. Sometimes such an entry
+ * refers to a temporary file, sometimes to an existing file, and
+ * sometimes to "/dev/null".
+ */
static struct diff_tempfile {
- const char *name; /* filename external diff should read from */
+ /*
+* filename external diff should read from, or NULL if this
+* entry is currently not in use:
+*/
+ const char *name;
+
char hex[41];
char mode[10];
- char tmp_path[PATH_MAX];
+
+ /*
+* If this diff_tempfile instance refers to a temporary file,
+* this tempfile object is used to manage its lifetime.
+*/
+ struct tempfile tempfile;
} diff_temp[2];
typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
@@ -564,25 +580,16 @@ static struct diff_tempfile *claim_diff_tempfile(void) {
die("BUG: diff is failing to clean up its tempfiles");
}
-static int remove_tempfile_installed;
-
static void remove_tempfile(void)
{
int i;
for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
- if (diff_temp[i].name == diff_temp[i].tmp_path)
- unlink_or_warn(diff_temp[i].name);
+ if (is_tempfile_active(&diff_temp[i].tempfile))
+ delete_tempfile(&diff_temp[i].tempfile);
diff_temp[i].name = NULL;
}
}
-static void remove_tempfile_on_signal(int signo)
-{
- remove_tempfile();
- sigchain_pop(signo);
- raise(signo);
-}
-
static void print_line_count(FILE *file, int count)
{
switch (count) {
@@ -2817,8 +2824,7 @@ static void prep_temp_blob(const char *path, struct
diff_tempfile *temp,
strbuf_addstr(&template, "XX_");
strbuf_addstr(&template, base);
- fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,
- strlen(base) + 1);
+ fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1);
if (fd < 0)
die_errno("unable to create temp-file");
if (convert_to_working_tree(path,
@@ -2828,8 +2834,8 @@ static void prep_temp_blob(const char *path, struct
diff_tempfile *temp,
}
if (write_in_full(fd, blob, size) != size)
die_errno("unable to write temp-file");
- close(fd);
- temp->name = temp->tmp_path;
+ close_tempfile(&temp->tempfile);
+ temp->name = get_tempfile_path(&temp->tempfile);
strcpy(temp->hex, sha1_to_hex(sha1));
temp->hex[40] = 0;
sprintf(temp->mode, "%06o", mode);
@@ -2854,12 +2860,6 @@ static struct diff_tempfile *prepare_temp_file(const
char *name,
return temp;
}
- if (!remove_tempfile_installed) {
- atexit(remove_tempfile);
- sigchain_push_common(remove_tempfile_on_signal);
- remove_tempfile_installed = 1;
- }
-
if (!S_ISGITLINK(one->mode) &&
(!one->sha1_valid ||
reuse_worktree_file(name, one->sha1, 1))) {
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
On Wed, Aug 12, 2015 at 10:10 PM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano wrote:
>>> Karthik Nayak writes:
>>>
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned
int width,
+const char *s)
+{
+ int display_len = utf8_strnwidth(s, strlen(s), 0);
+ int utf8_compenstation = strlen(s) - display_len;
>>>
>>> compensation, perhaps? But notice you are running two strlen and
>>> then also a call to utf8-strnwidth here already, and then
>>>
>>
>> compensation it is.
>> probably have a single strlen() call and set the value to another variable.
>
> That is not what I meant. If you want to keep the early return of
> "doing nothing for an empty string" (which you should *NOT*, by the
> way), declare these variables upfront, do the early return and then
> call utf8_strnwidth() to compute the value you are going to use,
> only when you know you are going to use them. That is what I meant.
>
Oh okay, after reading your mail now that makes sense.
+ if (!strlen(s))
+ return;
>>>
>>> you return here without doing anything.
>>>
>>> Worse yet, this logic looks very strange. If you give it a width of
>>> 8 because you want to align like-item on each record at that column,
>>> a record with 1-display-column item will be shown in 8-display-column
>>> with 7 SP padding (either at the beginning, or at the end, or on
>>> both ends to center). If it happens to be an empty string, the entire
>>> 8-display-column disappears.
>>>
>>> Is that really what you meant to do? The logic does not make much
>>> sense to me, even though the code may implement that logic correctly
>>> and clearly.
>>
>> Not sure what you meant.
>> But this does not act on empty strings and that was intentional.
>
> If it is truly intentional, then the design is wrong. You are
> writing a public function that can be called by other people.
>
> Which one makes more sense? Remember that you are going to have
> many callers over time in the course of project in the future.
>
> (A) The caller is forced to always check the string that he is
> merely passing on, i.e.
>
> struct strbuf buf = STRBUF_INIT;
> struct strbuf out = STRBUF_INIT;
>
> fill_some_placeholder(&buf, fmt, args);
> if (buf.len)
> strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);
> else
> strbuf_addchars(&out, ' ', 8);
>
> only because the callee strbuf_utf8_align() refuses to do the
> trivial work.
>
> (B) The caller does not have to care, i.e.
>
> struct strbuf buf = STRBUF_INIT;
> struct strbuf out = STRBUF_INIT;
>
> fill_some_placeholder(&buf, fmt, args);
> strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);
>
Yea, good point here, thanks for putting it out :)
>> It
>> does not make
>> sense to have an alignment on an empty string, where the caller could
>> themselves
>> just do a `strbuf_addchars(&buf, " ", width)`.
>
> It simply does not make sense to force the caller to even care.
> What they want is a series of lines, whose columns are aligned.
>
My ignorance, sorry!
+ if (display_len >= width) {
+ strbuf_addstr(buf, s);
+ return;
+ }
>>>
>>> Mental note: this function values the information more than
>>> presentation; it refuses to truncate (to lose information) and
>>> instead accepts jaggy output. OK.
>>
>> With regards to its usage in ref-filter, this is needed, don't you think?
>
> That is exactly why I said "OK".
>
> I was primarily trying to lead other reviewers by example,
> demonstrating that a review will become more credible if it shows
> that the reviewer actually read the patch by explaining the thinking
> behind what the code does in his own words. I see too many "FWIW,
> reviewed by me" without indicating if it is "from just a cursory
> read it looks nice" or "I fully read and understood it and I agree
> with the design choices it makes", which does not help me very much
> when queuing a patch.
>
>
Ah! okay! was just wondering if you were looking for an explanation :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
On Wed, Aug 12, 2015 at 9:59 PM, Junio C Hamano wrote: > Karthik Nayak writes: > + format_quote_value(atomv, quote_style, &output); >>> >>> If the one to add a literal string (with %hex escaping) is called "append_", >>> then this should be called append_quoted_atom() or something, no? >> >> Although it does append like "append_non_atom" this is more of formatting >> based >> on the quote given hence the name. > > Appending formatted values is still appending, no? Of course :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/16] diff: use tempfile module
Michael Haggerty writes: > No, prepare_temp_file() sometimes sets diff_tempfile::name to > "/dev/null", and sometimes to point at its argument `name`. That explains everything. Thanks. It's been a while since I wrote this part of the system ;-). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
Karthik Nayak writes:
> On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano wrote:
>> Karthik Nayak writes:
>>
>>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned
>>> int width,
>>> +const char *s)
>>> +{
>>> + int display_len = utf8_strnwidth(s, strlen(s), 0);
>>> + int utf8_compenstation = strlen(s) - display_len;
>>
>> compensation, perhaps? But notice you are running two strlen and
>> then also a call to utf8-strnwidth here already, and then
>>
>
> compensation it is.
> probably have a single strlen() call and set the value to another variable.
That is not what I meant. If you want to keep the early return of
"doing nothing for an empty string" (which you should *NOT*, by the
way), declare these variables upfront, do the early return and then
call utf8_strnwidth() to compute the value you are going to use,
only when you know you are going to use them. That is what I meant.
>>> + if (!strlen(s))
>>> + return;
>>
>> you return here without doing anything.
>>
>> Worse yet, this logic looks very strange. If you give it a width of
>> 8 because you want to align like-item on each record at that column,
>> a record with 1-display-column item will be shown in 8-display-column
>> with 7 SP padding (either at the beginning, or at the end, or on
>> both ends to center). If it happens to be an empty string, the entire
>> 8-display-column disappears.
>>
>> Is that really what you meant to do? The logic does not make much
>> sense to me, even though the code may implement that logic correctly
>> and clearly.
>
> Not sure what you meant.
> But this does not act on empty strings and that was intentional.
If it is truly intentional, then the design is wrong. You are
writing a public function that can be called by other people.
Which one makes more sense? Remember that you are going to have
many callers over time in the course of project in the future.
(A) The caller is forced to always check the string that he is
merely passing on, i.e.
struct strbuf buf = STRBUF_INIT;
struct strbuf out = STRBUF_INIT;
fill_some_placeholder(&buf, fmt, args);
if (buf.len)
strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);
else
strbuf_addchars(&out, ' ', 8);
only because the callee strbuf_utf8_align() refuses to do the
trivial work.
(B) The caller does not have to care, i.e.
struct strbuf buf = STRBUF_INIT;
struct strbuf out = STRBUF_INIT;
fill_some_placeholder(&buf, fmt, args);
strbuf_utf8_align(&out, ALIGN_LEFT, 8, buf.buf);
> It
> does not make
> sense to have an alignment on an empty string, where the caller could
> themselves
> just do a `strbuf_addchars(&buf, " ", width)`.
It simply does not make sense to force the caller to even care.
What they want is a series of lines, whose columns are aligned.
>>> + if (display_len >= width) {
>>> + strbuf_addstr(buf, s);
>>> + return;
>>> + }
>>
>> Mental note: this function values the information more than
>> presentation; it refuses to truncate (to lose information) and
>> instead accepts jaggy output. OK.
>
> With regards to its usage in ref-filter, this is needed, don't you think?
That is exactly why I said "OK".
I was primarily trying to lead other reviewers by example,
demonstrating that a review will become more credible if it shows
that the reviewer actually read the patch by explaining the thinking
behind what the code does in his own words. I see too many "FWIW,
reviewed by me" without indicating if it is "from just a cursory
read it looks nice" or "I fully read and understood it and I agree
with the design choices it makes", which does not help me very much
when queuing a patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
Karthik Nayak writes: >>> + format_quote_value(atomv, quote_style, &output); >> >> If the one to add a literal string (with %hex escaping) is called "append_", >> then this should be called append_quoted_atom() or something, no? > > Although it does append like "append_non_atom" this is more of formatting > based > on the quote given hence the name. Appending formatted values is still appending, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nd/dwim-wildcards-as-pathspecs] t2019: skip test requiring '*' in a file name non Windows
Johannes Schindelin writes: > Hi, > > On 2015-08-11 22:38, Johannes Sixt wrote: > >> diff --git a/t/t2019-checkout-ambiguous-ref.sh >> b/t/t2019-checkout-ambiguous-ref.sh >> index 8396320..199b22d 100755 >> --- a/t/t2019-checkout-ambiguous-ref.sh >> +++ b/t/t2019-checkout-ambiguous-ref.sh >> @@ -69,7 +69,7 @@ test_expect_success 'wildcard ambiguation, paths win' ' >> ) >> ' >> >> -test_expect_success 'wildcard ambiguation, refs lose' ' >> +test_expect_success !MINGW 'wildcard ambiguation, refs lose' ' >> git init ambi2 && >> ( >> cd ambi2 && > > FWIW I planned to submit a patch including this fix: > > https://github.com/git-for-windows/git/commit/4694320330e1b4d9178e13e215ce60a1cc8e0b1c > > (The idea of the `fixup! ` was to make this change part of a larger > change during the next merging rebase of Git for Windows.) Thanks. Is that an Ack? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> struct atom_value{
>
> Obviously not a problem with this step, but you need a SP before the
> open brace.
>
Will add.
>> @@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)
>> else
>> v->s = " ";
>> continue;
>> + } else if (skip_prefix(name, "align:", &valp)) {
>> + struct align *align = xmalloc(sizeof(struct align));
>> +
>> + if (skip_prefix(valp, "left,", &valp))
>> + align->position = ALIGN_LEFT;
>> + else if (skip_prefix(valp, "right,", &valp))
>> + align->position = ALIGN_RIGHT;
>> + else if (skip_prefix(valp, "middle,", &valp))
>> + align->position = ALIGN_MIDDLE;
>> + else
>> + die(_("improper format entered align:%s"),
>> valp);
>> + if (strtoul_ui(valp, 10, &align->width))
>> + die(_("positive width expected align:%s"),
>> valp);
>
> Minor nits on the design. %(align:[,]) would let
> us write %(align:16)...%(end) and use the "default position", which
> may be beneficial if one kind of alignment is prevalent (I guess all
> the internal users left-align?) %(align:,) forces
> users to spell both out all the time.
>
Isn't that better? I mean It sets a format which the others eventually
can follow
%(atom:suboption,value).
For example: %(objectname:abbrev,size)
Changing this would cause inconsistency according to me.
>> @@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting,
>> struct ref_array *array)
>>
>> struct ref_formatting_state {
>> struct strbuf output;
>> + struct align *align;
>> int quote_style;
>> + unsigned int end : 1;
>> };
>
> Mental note: it is not clear why you need 'end' field in the state.
> Perhaps it is an indication that the division of labor is poorly
> designed between the helper that updates the formatting state and
> the other helper that reflects the formatting state to the final
> string.
>
This goes with what you've said below!
>> @@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const
>> char *ep, struct ref_formattin
>>
>> static void set_formatting_state(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> {
>> - /* Based on the atomv values, the formatting state is set */
>> + if (atomv->align) {
>> + state->align = atomv->align;
>> + atomv->align = NULL;
>> + }
>> + if (atomv->end)
>> + state->end = 1;
>> +}
>> +
>> +static int align_ref_strbuf(struct ref_formatting_state *state, struct
>> strbuf *final)
>> +{
>> + if (state->align && state->end) {
>
> ... and I think that is what I see. If this function knows that we
> are processing %(end), i.e. perform-state-formatting is called for
> each atom and receives atomv, there wouldn't have to be a code like
> this.
>
Agreed, your suggestion below eradicates this
>> + struct align *align = state->align;
>> + strbuf_utf8_align(final, align->position, align->width,
>> state->output.buf);
>> + strbuf_reset(&state->output);
>> + state->align = NULL;
>> + return 1;
>> + } else if (state->align)
>> + return 1;
>> + return 0;
>> }
>>
>> static void perform_state_formatting(struct ref_formatting_state *state,
>> struct strbuf *final)
>> {
>> - /* More formatting options to be eventually added */
>> + if (align_ref_strbuf(state, final))
>> + return;
>
> At the design level, I have a strong suspicion that it is a wrong
> way to go. It piles more "if (this state bit was left by the
> previous atom) then do this" on this function and will make an
> unmanageable mess.
Hmm, yeah makes sense.
>
> You have a dictionary of all possible atoms somewhere. Why not hook
> a pointer to the "handler" function (or two) to each element in it,
> instead of duplicating "this one is special" information down to
> individual atom instantiations (i.e. atomv) as atomv.modifier_atom
> bit, an dstructure the caller more like this?
>
> get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
> if (atomv->pre_handler)
> atomv->pre_handler(atomv, &state);
> format_quote_value(atomv, &state);
> if (atomv->post_handler)
> atomv->post_handler(atomv, &state);
>
> Actually, each atom could just have a single handler; an atom like
> %(refname:short) whose sole effect is to append atomv->s to the
> state buffer can point a function to do so in its handler.
>
> On the other hand, align atom's handler would push a new state on
> the stack, marking that it is the one to handle diverted output.
>
>
Re: proper remote ref namespaces
Jacob Keller writes:
> Recently there was some discussion about git-notes and how we do not
> fetch notes from remotes by default. The big problem with doing so is
> because refs/remotes/* hierarchy is only setup for branches (heads),
> so we don't have any clean location to put them.
I wouldn't call this topic "proper" namespaces, though. What we
have is widely used and it shouldn't be broken. Call it "enhanced",
perhaps.
Some design boundaries:
- Moving the remote-tracking branch hierarchy from refs/remotes/$O/*
to refs/remotes/$O/heads/* would not fly, because it will break
existing repositories. Do not even waste time on pursuing
refs/remotes/$O/{heads,tags,notes...}/*
- Extending the current refs/remotes/$O/* (for branches) and doing
refs/remote-tags/$O/* (for tags) may work, would not break
existing repositories, and could to be extended further to cover
refs/remote-notes/$O and friends. It however may not look pretty
(weak objection) and more importantly, it would make it harder to
"cull" things that came from a single remote.
Just thinking aloud, perhaps we can introduce a brand new top level
hierarchy refs/remote/$O/{heads,tags,notes,...}, and give backward
compatibility by making a moral equivalent of a symbolic link from
refs/remote/$O/heads to refs/remotes/$O/. The true remote-tracknig
refs continue to live in refs/remotes/$O/* and old tools that do not
know the new layout would not be hurt. New tools that want to
ignore and look only at refs/remote/$O/* can do so through the moral
equivalent of a symbolic link. "remote remove" needs to be taught
about this single exception (i.e. "the symbolic link"), but the
places it needs to touch is limited only to two places and will not
grow.
If somebody got confused, notice that in the above description, I
said refs/remotes/ and refs/remote/. The former must stay. The
name of the latter is open to bikeshedding. Some may prefer a name
that is more distinct (refs/tracking/ or something, perhaps?). I
happen to prefer a name that is similar, but this preference is very
weak and I can persuaded to go either way.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: pack negotiation algorithm between 2 share-nothing repos
Duy Nguyen writes: > I know this is a corner case, but because it has a valid use case, > maybe we should do something about it. Immediate reaction is to add an > option to send no "have"s. But maybe you guys have better ideas. This and similar corner cases were discussed in very early days of Git. One interesting idea floated back then but was not pursued was to dig and send have's sparsely and then back up. Instead of digging and sending _all_ commits in a contiguous history, after sending the tip, you skip the commits from the history before sending the next one, and progressively make the skipping larger (e.g. Fibonacci, or exponential). You need to remember what you sent and for each of what you sent its topologically-oldest descendant you sent earlier that you heard the other side does not have. Then, when you get an Ack, you know a stretch of history between a commit that is known to be common (i.e. the one you heard an Ack just now) and its descendant that is known only to you (i.e. the topologically-oldest one you remember that you did send and they didn't say is common). At that point, you and the other end can bisect that range. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: proper remote ref namespaces
On 15-08-12 02:43 AM, Jacob Keller wrote: > Hello, > > Recently there was some discussion about git-notes and how we do not fetch > notes from remotes by default. The big problem with doing so is because > refs/remotes/* hierarchy is only setup for branches (heads), so we don't > have any clean location to put them. > > Around the time of git 1.8.0, Johan Herland made a proposal for remotes to > put all their refs in refs/remtoes/*, by moving heads into > refs/remotes/heads/* [1] Thanks for resurrecting this discussion. I feel this is a fundamental inconsistency in git's core design. Not a fatal flaw by any means, but something that keeps git from reaching its full potential. > In addition, his proposal was to include remote tags into > refs/remotes//tags and also refs/remotes//replace and > notes similarly. > > During this discussion there was many people who liked the idea, and > others who rejected it. The main rejection reason was two fold: > > (a) tags are "global" per project, so their namespace should be treated > global as it is now. > > The proposal's counter to this is that tags aren't guaranteed to be > global, because today two remotes you fetch might have tags that are the > same name with different pointers. This is currently hidden, and git > silently picks the tag it fetched first. > > (b) script compatibility, as changing the ref layout such that new git > can't work with old repository would be bad > > the counter to this, is that we make git smart enough to recognize old > remote format, and continue to work with it. Scripts which depend on this > layout will break, but that may not be such a huge concern. > > Personally, I think this proposal at least for heads, notes, replace, and > other remote refs we'd like to pull is very useful. I don't rightly know > the answer for tags. The linked discussion below covers several pages of > back and forth between a few people about which method is best. > > I like the idea of simplifying tags and branches and notes and others to > all fetch the same way. local stuff is in refs/heads or refs/notes and > remote stuff is (by default) in refs/remotes//tags etc > > But it does bring up some discussion as today we "auto follow" tags into > refs/tags, and it can get weird for tags since now "ambiguous" tags must > mean if there are tags of same name which point to different refs, The tags would be disambiguated by their remote name, e.g. "origin/tags/v5.6" vs. "hacker/tags/v5.6". The change would actually simplify tag auto-following. > and we'd need to teach a bunch of logic to the ref lookup code. Not a lot. Existing DWIMery already handles ambiguous branches, by preferring a local branch name over any remote ones. The only teaching that's really needed is to resolve shorthand like "origin/v5.6" to "refs/remotes/origin/tags/v5.6" (i.e. to look for anything matching "refs/remotes/origin/*/v5.6") but that doesn't seem very difficult. There is the question of how the user can even know if there's ambiguity. Aside from paying attention to "git fetch" output, I think we could extend "git tag" (and "git branch") in the case where the user specifies an existing tag (or branch). Right now both commands fail because the name already exists. All we need to do is extend that error message a bit, e.g.: > git tag v5.6 fatal: tag 'next' already exists as v5.6 -> a3a0e5d67d554a685abd897bc3ce4ffa4e74c812 origin/v5.6 remoteX/v5.6 -> 504346bbf9b02387b51f232f4db9c1860789b135 hacker/v5.6 -> fb4aa3533f81700789b3fb119e527410678e8d8d Here we see that our local v5.6, and hacker's v5.6, are unique, but origin and remoteX both have the same v5.6. Well, same-ish. I think those SHA IDs should be the things the tags point at, not the tag objects' IDs. This keeps things consistent between lightweight and annotated/signed tags. It does mean though that origin/v5.6 and remoteX/v5.6 might be different tag objects that happen to point at the same thing. I'm not sure the distinction is all that germane, and it's something that the user could disambiguate with "git tag -v" or "git show". > I am looking at ways to help git-notes be easier to use, so that we by > default fetch notes, and enable easier merge, since we'd have default > locations to merge from and to. This would make the sharing of notes a lot > easier, which is one of their primary sticking points.. you can't really > share them without *everyone* working to do it the same way you do. By > making a default policy, sharing becomes natural, and users can easily add > *public* notes to commits for things such as bug ids and other things > which are not discovered until after the commit is created. > > In addition, the easy ability to share replaces might also be helpful, > though IMHO not as valuable as git-notes. > > I think that the only logical refs layout is > "refs/remotes//(heads|tags|notes|replace)" > > and adding "refs/remote-notes" an
Re: [PATCH v2] http: add support for specifying the SSL version
Elia Pinto writes:
> diff --git a/http.c b/http.c
> index e9c6fdd..1504005 100644
> --- a/http.c
> +++ b/http.c
> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> static const char *ssl_cert;
> static const char *ssl_cipherlist;
> +static const char *ssl_version;
> +static long sslversion = CURL_SSLVERSION_DEFAULT;
I think you shouldn't have to initialize this variable.
See below.
> init_curl_http_auth(result);
>
> +
An unnecessary double blank?
> + if (getenv("GIT_SSL_VERSION"))
> + ssl_version = getenv("GIT_SSL_VERSION");
> + if (ssl_version != NULL && *ssl_version) {
> + if (!strcmp(ssl_version,"tlsv1")) {
> + sslversion = CURL_SSLVERSION_TLSv1;
> + } else if (!strcmp(ssl_version,"sslv2")) {
> + sslversion = CURL_SSLVERSION_SSLv2;
> + } else if (!strcmp(ssl_version,"sslv3")) {
> + sslversion = CURL_SSLVERSION_SSLv3;
> +#if LIBCURL_VERSION_NUM >= 0x072200
> + } else if (!strcmp(ssl_version,"tlsv1.0")) {
> + sslversion = CURL_SSLVERSION_TLSv1_0;
> + } else if (!strcmp(ssl_version,"tlsv1.1")) {
> + sslversion = CURL_SSLVERSION_TLSv1_1;
> + } else if (!strcmp(ssl_version,"tlsv1.2")) {
> + sslversion = CURL_SSLVERSION_TLSv1_2;
> +#endif
> + } else {
> + warning("unsupported ssl version %s : using default",
> + ssl_version);
> + }
> +}
> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
> + sslversion);
A few problems:
- Why do we even have to call this when sslversion is not given by
the end user, either from the environment or from the config?
Wouldn't we use the default version if we do not make this call?
- It is true that 0x072200 is described as introducing these three
in [*1*] but the structure is maintenance burden waiting to
happen. If you #if/#endif based on defined(CURL_SSLVERSION_$v),
it will be obvious to other people how to add their future
favourite versions in their patches. Also it may be read better
if you separated the logic and the code by using a table like
this:
static struct {
const char *name;
long ssl_version;
} sslversions[] = {
{ "tlsv1", CURL_SSLVERSION_TLSv1 },
{ "sslv2", CURL_SSLVERSION_SSLv2 },
...
#ifdef CURL_SSLVERSION_TLSv1_0
{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
#endif
...,
{ NULL }
};
> if (getenv("GIT_SSL_CIPHER_LIST"))
> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -
This blank removal is understandable (i.e. group related things
together).
> if (ssl_cipherlist != NULL && *ssl_cipherlist)
> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> ssl_cipherlist);
> -
This is not.
> if (ssl_cert != NULL)
> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> if (has_cert_password())
[References]
*1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/16] Introduce a tempfile module
On 08/11/2015 10:21 PM, Junio C Hamano wrote: > Thanks for a pleasant read. All looked reasonable. Thanks for your review! Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/16] diff: use tempfile module
On 08/11/2015 10:03 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Signed-off-by: Michael Haggerty
>> ---
>> diff.c | 29 +++--
>> 1 file changed, 7 insertions(+), 22 deletions(-)
>
> Nice code reduction.
>
>> diff --git a/diff.c b/diff.c
>> index 7500c55..dc95247 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2,6 +2,7 @@
>> * Copyright (C) 2005 Junio C Hamano
>> */
>> #include "cache.h"
>> +#include "tempfile.h"
>> #include "quote.h"
>> #include "diff.h"
>> #include "diffcore.h"
>> @@ -312,7 +313,7 @@ static struct diff_tempfile {
>> const char *name; /* filename external diff should read from */
>> char hex[41];
>> char mode[10];
>> -char tmp_path[PATH_MAX];
>> +struct tempfile tempfile;
>> } diff_temp[2];
>>
>> typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
>> @@ -564,25 +565,16 @@ static struct diff_tempfile *claim_diff_tempfile(void)
>> {
>> die("BUG: diff is failing to clean up its tempfiles");
>> }
>>
>> -static int remove_tempfile_installed;
>> -
>> static void remove_tempfile(void)
>> {
>> int i;
>> for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
>> -if (diff_temp[i].name == diff_temp[i].tmp_path)
>> -unlink_or_warn(diff_temp[i].name);
>> +if (is_tempfile_active(&diff_temp[i].tempfile))
>> +delete_tempfile(&diff_temp[i].tempfile);
>
> I suspect that this indicates that there is something iffy in the
> conversion. The original invariant, that is consistently used
> between claim_diff_tempfile() and remove_tempfile(), is that .name
> field points at .tmp_path for a slot in diff_temp[] that holds a
> temporary that is in use. Otherwise, .name is NULL and it can be
> claimed for your own use.
No, prepare_temp_file() sometimes sets diff_tempfile::name to
"/dev/null", and sometimes to point at its argument `name`. In either of
these cases .tmp_path can hold anything, and the file is *not* cleaned
up even though the diff_temp entry is considered by
claim_diff_tempfile() to be in use.
If I'm not mistaken, the old invariant was:
* Iff diff_tempfile::name is NULL, the entry is not in use.
* Iff diff_tempfile::name == diff_tempfile::tmp_path, then the entry is
in use and refers to a temporary file that needs to be cleaned up.
* Otherwise, the entry is in use but the corresponding file should *not*
be cleaned up.
The new invariant is:
* Iff diff_tempfile::name is NULL, the entry is not in use. In these
cases, is_tempfile_active() is always false.
* Iff is_tempfile_active(diff_tempfile::tempfile), then it refers to a
file that needs to get cleaned up. In these cases name points at the
tempfile object's filename.
* If neither of the above is true, then the entry is in use but the
corresponding file should not be cleaned up.
> Here the updated code uses a different and new invariant: .tempfile
> satisfies is_tempfile_active() for a slot in use. But the check in
> claim_diff_tempfile() still relies on the original invariant.
That is not true. The is_tempfile_active() check is only used in
remove_tempfile() when deciding whether to clean up the file. The check
in claim_diff_tempfile() wants to know whether the entry is in use, so
it uses the other check.
> The updated code may happen to always have an active tempfile in
> tempfile and always set NULL when it clears .name, but that would
> mean (1) future changes may easily violate one of invariants (we
> used to have only one, now we have two that have to be sync) by
> mistake, and (2) we are keeping track of two closely linked things
> as two invariants.
>
> As the value that used to be in the .name field can now be obtained
> by calling get_tempfile_path() on the .tempfile field, perhaps we
> should drop .name (and its associated invariant) at the same time?
This is also incorrect. See my first paragraph above.
I will change this patch to document the invariants.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Git stash behavior
Hello all, I am using git stashes to ensure that my source builds and tests correctly. My general work flow is this: Before committing I create a stash and clean everything: git stash save -q --keep-index --include-untracked Then I perform some tests (mvn compile test), after that I restore everything: git stash pop -q I am using this from a pre-commit hook so I really need this to work reliably. The problem is that I think that it really doesn't. I created a small gist to show the problem here: https://gist.github.com/x2b/3cc3d8aa8979561de4b5 There are actually multiple problems here: 1. If an untracked file already exists then git refuses to pop the stash. This is certainly the desired behavior in most cases. However, I would appreciate a "--force" option to override it. 2. As you can see the content of the "untracked" file in the gist is the same in the stash and the working directory. Is it really necessary to abort the operation in this case?? 3. The most severe problem is that after unsuccessfully trying to pop the stash the "first_untracked" file is restored while the "untracked" file is not. The stash is *partially* applied to the working directory. It seems like git restores some files before giving up after encountering the first file which can't be restored. I think this behavior is not generally what is expected. Git should either fail and leave the working directory as-is or succeed and change the directory's content. Since there is no "--force" option (see 1.) it is necessary to remove the already restored untracked files by hand before attempting to pop the stash once more (this is really inconvenient to me). While these are not technically bugs I would appreciate it if you could address these issues all the same. x2b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] http: add support for specifying the SSL version
Teach git about a new option, "http.sslVersion", which permits one to
specify the SSL version to use when negotiating SSL connections. The
setting can be overridden by the GIT_SSL_VERSION environment
variable.
Signed-off-by: Elia Pinto
---
This is the second version. I moved out of the else clause from the #ifdef.
Documentation/config.txt | 21 +
contrib/completion/git-completion.bash | 1 +
http.c | 31 +--
3 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 315f271..76a4f2b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1595,6 +1595,27 @@ http.saveCookies::
If set, store cookies received during requests to the file specified by
http.cookieFile. Has no effect if http.cookieFile is unset.
+http.sslVersion::
+ The SSL version to use when negotiating an SSL connection, if you
+ want to force the default. The available and default version depend on
+ whether libcurl was built against NSS or OpenSSL and the particular
configuration
+ of the crypto library in use. Internally this sets the
'CURLOPT_SSL_VERSION'
+ option; see the libcurl documentation for more details on the format
+ of this option and for the ssl version supported. Actually the possible
values
+ of this option are:
+
+ - sslv2
+ - sslv3
+ - tlsv1
+ - tlsv1.0
+ - tlsv1.1
+ - tlsv1.2
++
+Can be overridden by the 'GIT_SSL_VERSION' environment variable.
+To force git to use libcurl's default ssl version and ignore any
+explicit http.sslversion option, set 'GIT_SSL_VERSION' to the
+empty string.
+
http.sslCipherList::
A list of SSL ciphers to use when negotiating an SSL connection.
The available ciphers depend on whether libcurl was built against
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index c97c648..6e9359c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2118,6 +2118,7 @@ _git_config ()
http.postBuffer
http.proxy
http.sslCipherList
+ http.sslVersion
http.sslCAInfo
http.sslCAPath
http.sslCert
diff --git a/http.c b/http.c
index e9c6fdd..1504005 100644
--- a/http.c
+++ b/http.c
@@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
static int curl_ssl_try;
static const char *ssl_cert;
static const char *ssl_cipherlist;
+static const char *ssl_version;
+static long sslversion = CURL_SSLVERSION_DEFAULT;
#if LIBCURL_VERSION_NUM >= 0x070903
static const char *ssl_key;
#endif
@@ -190,6 +192,8 @@ static int http_options(const char *var, const char *value,
void *cb)
}
if (!strcmp("http.sslcipherlist", var))
return git_config_string(&ssl_cipherlist, var, value);
+ if (!strcmp("http.sslversion", var))
+ return git_config_string(&ssl_version, var, value);
if (!strcmp("http.sslcert", var))
return git_config_string(&ssl_cert, var, value);
#if LIBCURL_VERSION_NUM >= 0x070903
@@ -364,13 +368,36 @@ static CURL *get_curl_handle(void)
if (http_proactive_auth)
init_curl_http_auth(result);
+
+ if (getenv("GIT_SSL_VERSION"))
+ ssl_version = getenv("GIT_SSL_VERSION");
+ if (ssl_version != NULL && *ssl_version) {
+ if (!strcmp(ssl_version,"tlsv1")) {
+ sslversion = CURL_SSLVERSION_TLSv1;
+ } else if (!strcmp(ssl_version,"sslv2")) {
+ sslversion = CURL_SSLVERSION_SSLv2;
+ } else if (!strcmp(ssl_version,"sslv3")) {
+ sslversion = CURL_SSLVERSION_SSLv3;
+#if LIBCURL_VERSION_NUM >= 0x072200
+ } else if (!strcmp(ssl_version,"tlsv1.0")) {
+ sslversion = CURL_SSLVERSION_TLSv1_0;
+ } else if (!strcmp(ssl_version,"tlsv1.1")) {
+ sslversion = CURL_SSLVERSION_TLSv1_1;
+ } else if (!strcmp(ssl_version,"tlsv1.2")) {
+ sslversion = CURL_SSLVERSION_TLSv1_2;
+#endif
+ } else {
+ warning("unsupported ssl version %s : using default",
+ ssl_version);
+ }
+}
+ curl_easy_setopt(result, CURLOPT_SSLVERSION,
+ sslversion);
if (getenv("GIT_SSL_CIPHER_LIST"))
ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
-
if (ssl_cipherlist != NULL && *ssl_cipherlist)
curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
ssl_cipherlist);
-
if (ssl_cert != NULL)
curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
if (has_cert_password())
--
Re: [PATCH] http: add support for specifying the SSL version
2015-08-12 15:33 GMT+02:00 Remi Galan Alfonso
:
> Hello, Elia
>
> Elia Pinto writes:
>> +if (ssl_version != NULL && *ssl_version) {
>> +if (!strcmp(ssl_version,"tlsv1")) {
>> +sslversion = CURL_SSLVERSION_TLSv1;
>> +} else if (!strcmp(ssl_version,"sslv2")) {
>> +sslversion = CURL_SSLVERSION_SSLv2;
>> +} else if (!strcmp(ssl_version,"sslv3")) {
>> +sslversion = CURL_SSLVERSION_SSLv3;
>> +#if LIBCURL_VERSION_NUM >= 0x072200
>> +} else if (!strcmp(ssl_version,"tlsv1.0")) {
>> +sslversion = CURL_SSLVERSION_TLSv1_0;
>> +} else if (!strcmp(ssl_version,"tlsv1.1")) {
>> +sslversion = CURL_SSLVERSION_TLSv1_1;
>> +} else if (!strcmp(ssl_version,"tlsv1.2")) {
>> +sslversion = CURL_SSLVERSION_TLSv1_2;
>> +} else {
>> +warning("unsupported ssl version %s : using
>> default",
>> +ssl_version);
>> +#endif
>> +}
>> +}
>
> I'm curious about what would happen should 'sslVersion' be set to
> 'tlsv1.0' (or a value that doesn't belong to the possible values) and
> 'LIBCURL_VERSION_NUM' be inferior to '0x072200'.
> Since we wouldn't go through any 'if' and would also skip the 'else'
> case, wouldn't we have a silent 'unsupported ssl version: using
> default' ?
You are right. I will resend. Thank you very much
>
> So I think that the 'else' case should be outside of the '#if [...]
> #endif'.
>
> Thanks,
> Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
On Tue, Aug 11, 2015 at 11:52 PM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned
>> int width,
>> +const char *s)
>> +{
>> + int display_len = utf8_strnwidth(s, strlen(s), 0);
>> + int utf8_compenstation = strlen(s) - display_len;
>
> compensation, perhaps? But notice you are running two strlen and
> then also a call to utf8-strnwidth here already, and then
>
compensation it is.
probably have a single strlen() call and set the value to another variable.
>> + if (!strlen(s))
>> + return;
>
> you return here without doing anything.
>
> Worse yet, this logic looks very strange. If you give it a width of
> 8 because you want to align like-item on each record at that column,
> a record with 1-display-column item will be shown in 8-display-column
> with 7 SP padding (either at the beginning, or at the end, or on
> both ends to center). If it happens to be an empty string, the entire
> 8-display-column disappears.
>
> Is that really what you meant to do? The logic does not make much
> sense to me, even though the code may implement that logic correctly
> and clearly.
>
Not sure what you meant.
But this does not act on empty strings and that was intentional. It
does not make
sense to have an alignment on an empty string, where the caller could themselves
just do a `strbuf_addchars(&buf, " ", width)`.
>> + if (display_len >= width) {
>> + strbuf_addstr(buf, s);
>> + return;
>> + }
>
> Mental note: this function values the information more than
> presentation; it refuses to truncate (to lose information) and
> instead accepts jaggy output. OK.
>
With regards to its usage in ref-filter, this is needed, don't you think?
>> + if (position == ALIGN_LEFT)
>> + strbuf_addf(buf, "%-*s", width + utf8_compenstation, s);
>> + else if (position == ALIGN_MIDDLE) {
>> + int left = (width - display_len)/2;
>> + strbuf_addf(buf, "%*s%-*s", left, "", width - left +
>> utf8_compenstation, s);
>> + } else if (position == ALIGN_RIGHT)
>> + strbuf_addf(buf, "%*s", width + utf8_compenstation, s);
>> +}
Thanks!
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http: add support for specifying the SSL version
Hello, Elia
Elia Pinto writes:
> +if (ssl_version != NULL && *ssl_version) {
> +if (!strcmp(ssl_version,"tlsv1")) {
> +sslversion = CURL_SSLVERSION_TLSv1;
> +} else if (!strcmp(ssl_version,"sslv2")) {
> +sslversion = CURL_SSLVERSION_SSLv2;
> +} else if (!strcmp(ssl_version,"sslv3")) {
> +sslversion = CURL_SSLVERSION_SSLv3;
> +#if LIBCURL_VERSION_NUM >= 0x072200
> +} else if (!strcmp(ssl_version,"tlsv1.0")) {
> +sslversion = CURL_SSLVERSION_TLSv1_0;
> +} else if (!strcmp(ssl_version,"tlsv1.1")) {
> +sslversion = CURL_SSLVERSION_TLSv1_1;
> +} else if (!strcmp(ssl_version,"tlsv1.2")) {
> +sslversion = CURL_SSLVERSION_TLSv1_2;
> +} else {
> +warning("unsupported ssl version %s : using default",
> +ssl_version);
> +#endif
> +}
> +}
I'm curious about what would happen should 'sslVersion' be set to
'tlsv1.0' (or a value that doesn't belong to the possible values) and
'LIBCURL_VERSION_NUM' be inferior to '0x072200'.
Since we wouldn't go through any 'if' and would also skip the 'else'
case, wouldn't we have a silent 'unsupported ssl version: using
default' ?
So I think that the 'else' case should be outside of the '#if [...]
#endif'.
Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 03/13] ref-filter: introduce ref_formatting_state
On Tue, Aug 11, 2015 at 11:43 PM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>> &atomv);
>> - format_quote_value(atomv, quote_style, &output);
>> + set_formatting_state(atomv, &state);
>> + format_quote_value(atomv, &state);
>> + perform_state_formatting(&state, &final_buf);
>> }
>> if (*cp) {
>> sp = cp + strlen(cp);
>> - append_non_atom(cp, sp, &output);
>> + append_non_atom(cp, sp, &state);
>> + perform_state_formatting(&state, &final_buf);
>> }
>
> With the two helpers being very sketchy at this stage, it is very
> hard to judge if they make sense. At the conceptual level, I can
> see that set-formatting-state is to allow an atom to affect the
> state before the value of the atom is emitted into the buffer.
> I cannot tell what perform-state-formatting is meant to do from
> these call sites.
True, set formatting state is to ensure that the state is manipulated for
a given atom.
perform_state_formatting() is meant to act on the state set by the atom,
It performs formatting based on the state values, here is just copies the
strbuf set within the state to the final_buf.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
On Tue, Aug 11, 2015 at 11:30 PM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> @@ -1283,9 +1279,11 @@ void show_ref_array_item(struct ref_array_item *info,
>> const char *format, int qu
>> if (color_parse("reset", color) < 0)
>> die("BUG: couldn't parse 'reset' as a color");
>> resetv.s = color;
>> - print_value(&resetv, quote_style);
>> + format_quote_value(&resetv, quote_style, &output);
>
> Mental note: I _think_ the logic to scan the string and set
> need_color_reset_at_eol that happens at the beginning can be removed
> once the code fully utilizes formatting-state information. A
> coloring atom would leave a bit in the formatting state to say that
> the line color has been changed to something other than reset, and
> then this "at the end of line" code can decide if that is the case
> and add a "reset" thing here (i.e. the code inside the "if
> (need_color_reset_at_eol)" block shown here does not need to change,
> but the "if" condition would).
>
>
This could be done, when we implement color as a state :) Thanks for the note
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
On Tue, Aug 11, 2015 at 11:26 PM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> -static void print_value(struct atom_value *v, int quote_style)
>> +static void format_quote_value(struct atom_value *v, int quote_style,
>> struct strbuf *output)
>> {
>
> Hmph...
>
>> -static void emit(const char *cp, const char *ep)
>> +static void append_non_atom(const char *cp, const char *ep, struct strbuf
>> *output)
>
> I was tempted to suggest s/non_atom/literal/, but this would do for now...
>
Since I'll be doing a re-roll I could do that.
>> @@ -1262,19 +1257,20 @@ static void emit(const char *cp, const char *ep)
>> void show_ref_array_item(struct ref_array_item *info, const char *format,
>> int quote_style)
>> {
>> const char *cp, *sp, *ep;
>> + struct strbuf output = STRBUF_INIT;
>>
>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>> struct atom_value *atomv;
>>
>> ep = strchr(sp, ')');
>> if (cp < sp)
>> - emit(cp, sp);
>> + append_non_atom(cp, sp, &output);
>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>> &atomv);
>> - print_value(atomv, quote_style);
>> + format_quote_value(atomv, quote_style, &output);
>
> If the one to add a literal string (with %hex escaping) is called "append_",
> then this should be called append_quoted_atom() or something, no?
Although it does append like "append_non_atom" this is more of formatting based
on the quote given hence the name.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] http: add support for specifying the SSL version
Teach git about a new option, "http.sslVersion", which permits one to
specify the SSL version to use when negotiating SSL connections. The
setting can be overridden by the GIT_SSL_VERSION environment
variable.
Signed-off-by: Elia Pinto
---
Documentation/config.txt | 21 +
contrib/completion/git-completion.bash | 1 +
http.c | 31 +--
3 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 315f271..76a4f2b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1595,6 +1595,27 @@ http.saveCookies::
If set, store cookies received during requests to the file specified by
http.cookieFile. Has no effect if http.cookieFile is unset.
+http.sslVersion::
+ The SSL version to use when negotiating an SSL connection, if you
+ want to force the default. The available and default version depend on
+ whether libcurl was built against NSS or OpenSSL and the particular
configuration
+ of the crypto library in use. Internally this sets the
'CURLOPT_SSL_VERSION'
+ option; see the libcurl documentation for more details on the format
+ of this option and for the ssl version supported. Actually the possible
values
+ of this option are:
+
+ - sslv2
+ - sslv3
+ - tlsv1
+ - tlsv1.0
+ - tlsv1.1
+ - tlsv1.2
++
+Can be overridden by the 'GIT_SSL_VERSION' environment variable.
+To force git to use libcurl's default ssl version and ignore any
+explicit http.sslversion option, set 'GIT_SSL_VERSION' to the
+empty string.
+
http.sslCipherList::
A list of SSL ciphers to use when negotiating an SSL connection.
The available ciphers depend on whether libcurl was built against
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index c97c648..6e9359c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2118,6 +2118,7 @@ _git_config ()
http.postBuffer
http.proxy
http.sslCipherList
+ http.sslVersion
http.sslCAInfo
http.sslCAPath
http.sslCert
diff --git a/http.c b/http.c
index e9c6fdd..a7401b1 100644
--- a/http.c
+++ b/http.c
@@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
static int curl_ssl_try;
static const char *ssl_cert;
static const char *ssl_cipherlist;
+static const char *ssl_version;
+static long sslversion = CURL_SSLVERSION_DEFAULT;
#if LIBCURL_VERSION_NUM >= 0x070903
static const char *ssl_key;
#endif
@@ -190,6 +192,8 @@ static int http_options(const char *var, const char *value,
void *cb)
}
if (!strcmp("http.sslcipherlist", var))
return git_config_string(&ssl_cipherlist, var, value);
+ if (!strcmp("http.sslversion", var))
+ return git_config_string(&ssl_version, var, value);
if (!strcmp("http.sslcert", var))
return git_config_string(&ssl_cert, var, value);
#if LIBCURL_VERSION_NUM >= 0x070903
@@ -364,13 +368,36 @@ static CURL *get_curl_handle(void)
if (http_proactive_auth)
init_curl_http_auth(result);
+
+ if (getenv("GIT_SSL_VERSION"))
+ ssl_version = getenv("GIT_SSL_VERSION");
+ if (ssl_version != NULL && *ssl_version) {
+ if (!strcmp(ssl_version,"tlsv1")) {
+ sslversion = CURL_SSLVERSION_TLSv1;
+ } else if (!strcmp(ssl_version,"sslv2")) {
+ sslversion = CURL_SSLVERSION_SSLv2;
+ } else if (!strcmp(ssl_version,"sslv3")) {
+ sslversion = CURL_SSLVERSION_SSLv3;
+#if LIBCURL_VERSION_NUM >= 0x072200
+ } else if (!strcmp(ssl_version,"tlsv1.0")) {
+ sslversion = CURL_SSLVERSION_TLSv1_0;
+ } else if (!strcmp(ssl_version,"tlsv1.1")) {
+ sslversion = CURL_SSLVERSION_TLSv1_1;
+ } else if (!strcmp(ssl_version,"tlsv1.2")) {
+ sslversion = CURL_SSLVERSION_TLSv1_2;
+} else {
+ warning("unsupported ssl version %s : using default",
+ ssl_version);
+#endif
+ }
+}
+ curl_easy_setopt(result, CURLOPT_SSLVERSION,
+ sslversion);
if (getenv("GIT_SSL_CIPHER_LIST"))
ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
-
if (ssl_cipherlist != NULL && *ssl_cipherlist)
curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
ssl_cipherlist);
-
if (ssl_cert != NULL)
curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
if (has_cert_password())
--
2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubs
Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: > Hi Johannes, > > On 2015-08-11 22:51, Johannes Sixt wrote: >> Invoking plink requires special treatment, and we have support and even >> test cases for the commands 'plink' and 'tortoiseplink'. We also support >> .exe variants for these two and there is a test for 'plink.exe'. >> >> On Windows, however, where support for plink.exe would be relevant, the >> test case fails because it is not possible to execute a file with a .exe >> extension that is actually not a binary executable---it is a shell >> script in our test. We have to disable the test case on Windows. > > Oh how would I wish you were working on Git for Windows even *just* a bit > *with* me. At least I would wish for a more specific description of the > development environment, because it sure as hell is not anything anybody can > download and install as easily as Git for Windows' SDK. > > FWIW Git for Windows has this patch (that I wanted to contribute in due time, > what with being busy with all those tickets) to solve the problem mentioned > in your patch in a different way: > > https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
pack negotiation algorithm between 2 share-nothing repos
This is a corner case that has a real use case: git clone linux-2.6.git cd linux-2.6 git remote add history git-history.git git fetch history # graft graft graft Because history.gi and linux-2.6.git have nothing in common, the server side keeps asking for more "have"s and the client keeps sending in "git fetch history". Negotiation phase takes longer than my patience so I abort it, hack git to send no "have"s and retry. I know this is a corner case, but because it has a valid use case, maybe we should do something about it. Immediate reaction is to add an option to send no "have"s. But maybe you guys have better ideas. PS. I know i'm behind my git inbox. Looking into it soon. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH nd/dwim-wildcards-as-pathspecs] t2019: skip test requiring '*' in a file name non Windows
Hi, On 2015-08-11 22:38, Johannes Sixt wrote: > diff --git a/t/t2019-checkout-ambiguous-ref.sh > b/t/t2019-checkout-ambiguous-ref.sh > index 8396320..199b22d 100755 > --- a/t/t2019-checkout-ambiguous-ref.sh > +++ b/t/t2019-checkout-ambiguous-ref.sh > @@ -69,7 +69,7 @@ test_expect_success 'wildcard ambiguation, paths win' ' > ) > ' > > -test_expect_success 'wildcard ambiguation, refs lose' ' > +test_expect_success !MINGW 'wildcard ambiguation, refs lose' ' > git init ambi2 && > ( > cd ambi2 && FWIW I planned to submit a patch including this fix: https://github.com/git-for-windows/git/commit/4694320330e1b4d9178e13e215ce60a1cc8e0b1c (The idea of the `fixup! ` was to make this change part of a larger change during the next merging rebase of Git for Windows.) Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Hi Johannes, On 2015-08-11 22:51, Johannes Sixt wrote: > Invoking plink requires special treatment, and we have support and even > test cases for the commands 'plink' and 'tortoiseplink'. We also support > .exe variants for these two and there is a test for 'plink.exe'. > > On Windows, however, where support for plink.exe would be relevant, the > test case fails because it is not possible to execute a file with a .exe > extension that is actually not a binary executable---it is a shell > script in our test. We have to disable the test case on Windows. Oh how would I wish you were working on Git for Windows even *just* a bit *with* me. At least I would wish for a more specific description of the development environment, because it sure as hell is not anything anybody can download and install as easily as Git for Windows' SDK. FWIW Git for Windows has this patch (that I wanted to contribute in due time, what with being busy with all those tickets) to solve the problem mentioned in your patch in a different way: https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Please read this as my vote not to remove the test cases. Thanks, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
