Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
On Wed, Jul 29, 2015 at 11:30 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: A handful of huh? on the design. - The atom says if *exists* and explanation says has a value. How are they related? Does an atom whose value is an empty string has a value? Or is ifexists meant to be used only to ignore meaningless atom, e.g. %(*objectname) applied to a ref that refers to an object that is not an annotated tag? It's meant to ignore meaningless atom. atom's whose values are empty strings are ignored. That is a self-contradicting answer. If you ask for %(*objectname) on a commit, that request truly is meaningless, as a commit is not an annotated tag that points at another object whose objectname is being asked for. But if a commit has an empty log message (you should be able to create such an object with commit-tree), then %(subject) would be an empty string. The fact that the commit happens to have an empty string as its message is far from meaningless. Either you ignore an empty string, or you ignore meaningless one. Which does ifexists mean? I meant ignore atom values which are empty, sorry for the confusion. - That %s looks ugly. Are there cases where a user may want to say %(ifexists:[%i]) or something other than 's' after that per-cent? Couldn't think of a better replacer, any suggestions would be welcome :) See below. Its given as example, is that misleading? Othewise I wouldn't be asking. - What, if anything, is allowed to come between %(ifexists...) and the next atom like %(refname)? For example, are these valid constructs? . %(ifexists...)%(padright:20)%(refname) Doesn't work ... ... - This syntax does not seem to allow switching on an attribute to show or not to show another, e.g. if %(*objectname) makes sense, then show '%(padright:20)%(refname:short) %(*subject)' for it. Yes this doesn't do that, One way to do all of the above is to make it %(ifexists:atom:expansionString) That is, for example: %(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset)) would give you a string tag v1.0 with v1.0 painted in blue for refs/tags/v1.0 but nothing for refs/heads/master. Obviously expansionString part needs some escaping mechanism to allow it to include an unmatched ). I liked your other idea of if and endif better :) -- 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: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Couldn't think of a better replacer, any suggestions would be welcome :) See below. ... One way to do all of the above is ... Note that is just one way, not the only or not necessarily the best. It certainly is not the easiest, I think. %(if:atom)...%(endif) might be easier to implement. And I find it easier to read or write too. Nested parenthesis in a format string make them really tricky. That removes the need for escaping since the content of the if/endif is a format string like the others, it can use the same escaping rules (IIRC, %% to escape a %). THat's a really good point. Will work on 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
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); I think that would interact badly with verify_ref_format(). Usually, you have just one format string and call verify_ref_format() on it, not a different format string for each ref_array_item. That would probably be solvable. Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) -- 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: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
On Fri, Jul 31, 2015 at 11:46 PM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Couldn't think of a better replacer, any suggestions would be welcome :) See below. ... One way to do all of the above is ... Note that is just one way, not the only or not necessarily the best. It certainly is not the easiest, I think. %(if:atom)...%(endif) might be easier to implement. And I find it easier to read or write too. Nested parenthesis in a format string make them really tricky. That removes the need for escaping since the content of the if/endif is a format string like the others, it can use the same escaping rules (IIRC, %% to escape a %). THat's a really good point. Will work on this :) -- Regards, Karthik Nayak -- Not sure how much work it would take to extent other atoms to this behavior, such as %(padright) and %(align) and such, that way they could operate on literals and so forth. Maybe not worth it as part of this GSoC project, as it may be too complicated, but maybe something to mark as a TODO for future or something? The only issue being that it might mean we have to keep the old implementation too for backwards compatibility .. Maybe it's easier to implement that I think, or maybe it's far more challenging than I think 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 v7 03/11] ref-filter: add option to pad atoms to the right
On Thu, Jul 30, 2015 at 11:26 PM, Eric Sunshine sunsh...@sunshineco.com wrote: In an earlier review, Matthieu pointed out that this test failed to ensure that the 'padright' value did not leak into the next atom. In a subsequent version, you fixed the test to check that condition, but now you've somewhat lost it again, at least visually. That is, because whitespace is invisible and because 'padright' now also affects literal strings, someone reading this test can't tell if those trailing |'s in the expected output are padded or not. You could use a different format string to prove that the 'padright' value doesn't leak. For instance: %(padright:10)%(refname:short)|%(padright:5)|%(refname:short) This way, as long as the two |'s are side-by-side, then you've proved that the first one wasn't affected by the preceding 'padright:10'. You could also add back the %(refname:short) at the front of the pattern, as you currently have it, if you want to prove that padding is not enabled at the start of format. Thanks Eric. This seems like a better improvement over my test. Will implement 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
Re: [PATCH/RFC 0/2] bisect per-worktree
On 08/01/2015 07:12 AM, Junio C Hamano wrote: On Fri, Jul 31, 2015 at 8:59 PM, Michael Haggerty mhag...@alum.mit.edu wrote: It seems to me that adding a new top-level worktree-refs directory is pretty traumatic. Lots of people and tools will have made the assumption that all normal references live under refs/. ... It's all a bit frightening, frankly. I actually feel the prospect of pluggable ref backend more frightening, frankly ;-). These bisect refs are just like FETCH_HEAD and MERGE_HEAD, not about the primary purpose of the repository to grow the history of refs (branches), but about ephemeral pointers into the history used to help keep track of what is being done in the worktree upstairs. There is no need for these to be visible across worktrees. If we use the real refs that are grobal in the repository (as opposed to per-worktree ones), we would hit the backend databas with transactions to update these ephemeral things, which somehow makes me feel stupid. Hmm, ok, so you are thinking of a remote database with high latency. I was thinking more of something like LMDB, with latency comparable to filesystem storage. These worktree-specific references might be ephemeral, but they also imply reachability, which means that they need to be visible at least during object pruning. Moreover, if the references don't live in the same database with the rest of the references, then we have to deal with races due to updating references in different places without atomicity. The refs+object store is the most important thing for maintaining the integrity of a repo and avoiding races. To me it seems easier to do so if there is a single refs+objects store than if we have some references over here on the file system, some over there in a LMDB, etc. So my gut feeling is for the primary reference storage to be in a single reference namespace that (at least in principle) can be stored in a single ACID database. For each worktree, we could then create a different view of the references by splicing parts of the full reference namespace together. This could even be based on config settings so that we don't have to hardcode information like refs/bisect/* is worktree-specific deep in the references module. Suppose we could write [worktree.refs] map = refs/worktrees/*: map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/* which would mean (a) hide the references under refs/worktrees, and (b) make it look as if the references under refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect (where [worktree] is replaced with the current worktree's name). By making these settings configurable, we allow other projects to define their own worktree-specific reference namespaces too. The corresponding main repo might hide refs/worktrees/* but leave its refs/bisect namespace exposed in the usual place. git prune would see the whole namespace as it really is so that it can compute reachability correctly. 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: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Fri, Jul 31, 2015 at 11:48 PM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: What I was thinking of was something like this : struct strbuf format = STRBUF_INIT; char c = ' '; if (current) c = '*'; strbuf_addf(format, %c, c, other format options...); show_ref_array_item(item, format.buf, quote_style, 0); strbuf_release(format); I think that would interact badly with verify_ref_format(). Usually, you have just one format string and call verify_ref_format() on it, not a different format string for each ref_array_item. That would probably be solvable. Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) -- Wonder if there is some sort of more generic atom that would work? I can't think of anything obvious at all though... it may be worth having this even though it definitely seems less useful generically, as the reason above where --format should be at least as expressive as the builtin formatting. 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 2/2] notes: handle multiple worktrees
On Sat, Aug 1, 2015 at 12:11 AM, David Turner dtur...@twopensource.com wrote:
Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using
find_shared_symref and die if we find one. This prevents simultaneous
merges to the same notes branch from different worktrees.
Signed-off-by: David Turner dtur...@twopensource.com
---
builtin/notes.c | 5 +++
t/t3320-notes-merge-worktrees.sh | 72
2 files changed, 77 insertions(+)
create mode 100755 t/t3320-notes-merge-worktrees.sh
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc..e4dda79 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -19,6 +19,7 @@
#include string-list.h
#include notes-merge.h
#include notes-utils.h
+#include branch.h
static const char * const git_notes_usage[] = {
N_(git notes [--ref notes-ref] [list [object]]),
@@ -825,10 +826,14 @@ static int merge(int argc, const char **argv, const
char *prefix)
update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
0, UPDATE_REFS_DIE_ON_ERR);
else { /* Merge has unresolved conflicts */
+ char *existing;
/* Update .git/NOTES_MERGE_PARTIAL with partial merge result
*/
update_ref(msg.buf, NOTES_MERGE_PARTIAL, result_sha1, NULL,
0, UPDATE_REFS_DIE_ON_ERR);
/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
+ existing = find_shared_symref(NOTES_MERGE_REF,
default_notes_ref());
Please confirm my assumption here: existing originally comes from a
strbuf_detach(), so it's the caller's (i.e. our) responsibility to
free() it, but we don't care, as we just die()d anyway. Correct?
+ if (existing)
+ die(_(A notes merge on %s is already in-progress for
%s), default_notes_ref(), existing);
Not sure about your prepositions here. Would this maybe read better?:
A notes merge into %s is already in-progress at %s
if (create_symref(NOTES_MERGE_REF, default_notes_ref(),
NULL))
die(Failed to store link to current notes ref (%s),
default_notes_ref());
diff --git a/t/t3320-notes-merge-worktrees.sh
b/t/t3320-notes-merge-worktrees.sh
new file mode 100755
index 000..997621f
--- /dev/null
+++ b/t/t3320-notes-merge-worktrees.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+#
+# Copyright (c) 2015 Twitter, Inc
+#
+
+test_description='Test merging of notes trees in multiple worktrees'
+
+. ./test-lib.sh
+
+test_expect_success 'setup commit' '
+ test_commit tantrum
+'
+
+commit_tantrum=$(git rev-parse tantrum^{commit})
+
+test_expect_success 'setup notes ref (x)' '
+ git config core.notesRef refs/notes/x
+ git notes add -m x notes on tantrum tantrum
+'
+
+test_expect_success 'setup local branch (y)' '
+ git update-ref refs/notes/y refs/notes/x
+ git config core.notesRef refs/notes/y
+ git notes remove tantrum
+'
+
+test_expect_success 'setup remote branch (z)' '
+ git update-ref refs/notes/z refs/notes/x
+ git config core.notesRef refs/notes/z
+ git notes add -f -m conflicting notes on tantrum tantrum
+'
+
+test_expect_success 'modify notes ref ourselves (x)' '
+ git config core.notesRef refs/notes/x
+ git notes add -f -m more conflicting notes on tantrum tantrum
+'
+
+test_expect_success 'create some new worktrees' '
+ git worktree add -b newbranch worktree master
+ git worktree add -b newbranch2 worktree2 master
+'
+
+test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' '
+ git config core.notesRef refs/notes/y
+ test_must_fail git notes merge z
+ echo ref: refs/notes/y expect
+ test_cmp .git/NOTES_MERGE_REF expect
+'
+
+test_expect_success 'merge z into y while mid-merge in another workdir
fails' '
+ (
+ cd worktree
+ git config core.notesRef refs/notes/y
+ test_must_fail git notes merge z 2err
+ grep A notes merge on refs/notes/y is already in-progress
for err
+ )
+ test_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF
+'
+
+test_expect_success 'merge z into x while mid-merge on y succeeds' '
+ (
+ cd worktree2
+ git config core.notesRef refs/notes/x
+ test_must_fail git notes merge z 21 out
+ grep Automatic notes merge failed out
Missing ?
+ grep -v A notes merge on refs/notes/x is already in-progress
for out
+ )
+ echo ref: refs/notes/x expect
+ test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect
+'
+
+test_done
Otherwise, this looks good to me.
...Johan
--
2.0.4.315.gad8727a-twtrsrc
--
To unsubscribe from this list: send the line
Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms
On Sat, Aug 01, 2015 at 01:33:37AM +0200, Jan Viktorin wrote:
+ # Do not allow arbitrary strings.
+ my ($filtered_auth) = ;
+ foreach (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5) {
On my system, GSSAPI is also available, and it does indeed work, as I'm
not prompted for a password. (I have only PLAIN and GSSAPI available
server-side, and AUTH is required.)
It may be better to simply force the text to upper case, as that would
allow us not to have to change Git if Authen::SASL::Perl implements new
mechanisms.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
signature.asc
Description: Digital signature
Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
On Thu, Jul 30, 2015 at 9:47 PM, Junio C Hamano gits...@pobox.com wrote:
Jacob Keller jacob.kel...@gmail.com writes:
On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Eric Sunshine sunsh...@sunshineco.com writes:
Also, please explain here and in the commit message why this highly
specialized colorizer ('colornext'), is needed even though a more
general purpose one ('color') is already available.
It is needed in the current form to allow
%(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
and not the [].
But I now think that this would be more elegantly solved by Junio's
%(if) %(endif) idea:
%(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
(I added spaces for clarity)
I agree, this style seems a lot more elegant and expressive while much
easier to understand. Same for doing this to the alignment atoms and
such as it solves the same problem(s) there.
Do you mean something like these?
%(align:left,20) branch %(refname:short) %(end)
%(align:middle,20) branch %(refname:short) %(end)
%(align:right,20) branch %(refname:short) %(end)
to replace and enhance %(padright:20)?
I can't speak to how easy it is to implement tho.
Perhaps it would go like this:
* Instead of always emitting to the standard output, emit() and
print_value() will gain an option to append into a strbuf that is
passed as argument. Alternatively, appending to strbuf could be
made the only output channel for them; show_ref_array_item() can
prepare an empty strbuf, call them repeatedly to fill it, and
then print the resulting strbuf itself.
* Things like %(if) and %(align) would do the following:
(1) Push the currently active strbuf it got from the calling
show_ref_array_item() to the formatting state;
(2) Create a new strbuf and arrange so that further output would
be diverted to this new one; and
(3) Push the fact that the diverted output will be processed by
them (i.e. %(if), %(align), etc.) when the diversion finishes
to the formatting state.
* When %(end) is seen, the currently active strbuf (i.e. the new
one created in (2) above for diversion) holds the output made
since the previously seen %(if), %(align), etc. The formatting
state knows what processing needs to be performed on that from
(3) above.
- Pop the strbuf where the output of the entire %(if)...%(end)
construct needs to go from the formatting state;
- Have the processing popped from (3) above, e.g. %(if:atom) or
%(align:left,20), do whatever they need to do on the diverted
output, and emit their result.
Both %(if) and the hypotherical %(align) can use this same
diversion mechanism. And the above would properly nest,
e.g.
%(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)
This actually looks neat and good, I'll try implementing 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
Mail.
A phish attempt, banned phrase or sensitive information was detected in a message sent to you and the original message has been quarantined.Please you are to re-validate your email address by providing your Username(...) and Password(.) for verifications now. 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 v1] send-email: provide whitelist of SMTP AUTH mechanisms
Hello Eric,
thanks for comments. I've described the orignal problem before I tried
to fix it:
https://groups.google.com/forum/#!topic/git-users/PxtiVxAapUU
So, *this patch* was necessary to apply for me to send *this patch* to
the mailing list.
Later, I've tried git-send-email (without this patch) on two different
PCs with the same distro, same architecture, same git, same perl, same
perl libraries. The result was that on the first, it auto-selected
DIGEST-MD5 (didn't work) and on the second one, it selected PLAIN
(worked). I don't understand it.
More below...
On Sat, 1 Aug 2015 05:33:28 -0400
Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
vikto...@rehivetech.com wrote:
When sending an e-mail, the client and server must
agree on an authentication mechanism. Some servers
(due to misconfiguration or a bug) denies valid
s/denies/deny/
credentials for certain mechanisms. In this patch,
a new option --smtp-auth and configuration entry
smtpauth are introduced.
If smtp_auth is defined, it works as a whitelist
of allowed mechanisms for authentication. There
are four mechanisms supported: PLAIN, LOGIN,
CRAM-MD5, DIGEST-MD5. However, their availability
depends on the installed SASL library.
Signed-off-by: Jan Viktorin vikto...@rehivetech.com
---
git-send-email.perl | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
At the very least, you will also want to update the documentation
(Documentation/git-send-email.txt) and, if possible, add new tests
(t/t9001-send-email.sh).
I will update the documentation when it is clear, how the smtp-auth
works.
I have no idea, how to test the feature. I can see something like
fake.sendmail in the file. How does it work? I can image a test whether
user inserts valid values. What more?
More below.
diff --git a/git-send-email.perl b/git-send-email.perl
index ae9f869..b00ed9d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
return 1;
}
+ # Do not allow arbitrary strings.
Can you explain why this restriction is needed. What are the
consequences of not limiting the input to this approved list?
This is more a check of an arbitrary user input then a check
of an approved list. It should be also used to inform user
about invalid methods (however, I didn't implemented it yet).
+ my ($filtered_auth) = ;
Style: unnecessary parentheses
+ foreach (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5) {
This might read more nicely and be easier to maintain if written as:
foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {
+ if($smtp_auth $smtp_auth =~ /\b\Q$_\E\b/i) {
Style: space after 'if'
Also, why not lift the 'if ($smtp_auth)' check outside the loop since
its value never changes and there's no need to iterate over the list
if $smtp_auth is empty.
Sure. I just wanted to avoid another indentation level. I think, there
is no need for optimization at this place. I can rework it, no
problem...
+ $filtered_auth .= $_ . ;
Style question: Would this be more naturally expressed with
'filtered_auth' as an array onto which items are pushed, rather than
as a string? At the point of use, the string can be recreated via
join().
Not a big deal; just wondering.
I am not a Perl programmer. Yesterday, I've discovered for the first
time that Perl uses a dot for concatenation... I have no idea what
happens when passing an array to Authen::SASL-new(). Moreover, the
Perl arrays syntax rules scare me a bit ;).
+ }
+ }
+
+ die Invalid SMTP AUTH. if length $smtp_auth !length
$filtered_auth;
Style: drop capitalization: invalid...
Style: drop period at end
Agree.
Style: add \n at end in order to suppress printing of the
perl line number and input line number which aren't
very meaningful for a user error
Another hidden Perl suprise, I guess...
(Existing style in the script is not very consistent, but new code
probably should adhere the above suggestions.)
(Agree.)
Also, don't you want to warn the user about tokens that don't match
one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than
dropping them silently?
Yes, this would be great (as I've already mentioned). It's a question
whether to include the check for the mechanisms or whether to leave
the $smtp_auth variable as it is... Maybe just validate by a regex?
The naming rules are defiend here:
https://tools.ietf.org/html/rfc4422#page-8
So, this looks to me as a better way.
Note that, the current implementation does not force the user to use
only the listed mechanisms. If the $smtp_auth is empty, the original
behaviour is preserved...
# Workaround AUTH PLAIN/LOGIN interaction defect
# with Authen::SASL::Cyrus
eval {
@@
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller jacob.e.kel...@intel.com wrote:
Teach git-notes about a new configuration option notes.merge for
selecting the default notes merge strategy. Document the option in
config.txt and git-notes.txt
Add tests for the configuration option. Ensure that command line
--strategy option overrides the configured value. Ensure that -s can't
be passed with --commit or --abort. Ensure that --abort and --commit
can't be used together.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
Cc: Johan Herland jo...@herland.net
Cc: Michael Haggerty mhag...@alum.mit.edu
Cc: Eric Sunshine sunsh...@sunshineco.com
Thanks, this looks better than the previous round. A few comments below...
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..d8944f5aec60 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,13 +101,13 @@ merge::
any) into the current notes ref (called local).
+
If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
-the manual resolver is used. This resolver checks out the
-conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
-and instructs the user to manually resolve the conflicts there.
-When done, the user can either finalize the merge with
-'git notes merge --commit', or abort the merge with
-'git notes merge --abort'.
+conflicting notes (see the -s/--strategy option or notes.merge
+config option) is not given, the manual resolver is used.
+This resolver checks out the conflicting notes in a special
+worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user
+to manually resolve the conflicts there. When done, the user
+can either finalize the merge with 'git notes merge --commit',
+or abort the merge with 'git notes merge --abort'.
When you re-wrap the text like this, it's difficult to spot your one
little change in all the diff noise. For the sake of review, it's
often better to minimize the change, even if it leaves the text more
jagged than you'd like.
remove::
Remove the notes for given objects (defaults to HEAD). When
@@ -181,10 +181,10 @@ OPTIONS
-s strategy::
--strategy=strategy::
When merging notes, resolve notes conflicts using the given
- strategy. The following strategies are recognized: manual
- (default), ours, theirs, union and cat_sort_uniq.
- See the NOTES MERGE STRATEGIES section below for more
- information on each notes merge strategy.
+ strategy. Overrides notes.merge. The following strategies are
+ recognized: `manual`, `ours`, `theirs`, `union` and
+ `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
+ for more information on each notes merge strategy.
Ditto.
--commit::
Finalize an in-progress 'git notes merge'. Use this option
diff --git a/builtin/notes.c b/builtin/notes.c
index 88fdafb32bc0..728980bc79bf 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,6 +743,25 @@ static int merge_commit(struct notes_merge_options *o)
return ret;
}
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy
*strategy)
+{
+ if (!strcmp(arg, manual))
+ *strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(arg, ours))
+ *strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(arg, theirs))
+ *strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(arg, union))
+ *strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(arg, cat_sort_uniq))
+ *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else {
+ return 1;
In this codebase, it's customary to return 0 on success and -1 on error
+ }
Style: drop unnecessary braces
+
+ return 0;
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int git_notes_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, notes.merge)) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (parse_notes_strategy(value, merge_strategy))
+ return error(Unknown notes merge strategy: %s,
value);
For the non-error case, don't you want to 'return 0' here to indicate
that you handled the config variable rather than letting it fall
through to git_default_config() below?
+ }
+
+ return git_default_config(var, value, cb);
+}
+
int cmd_notes(int argc, const char **argv, const char *prefix)
{
int result;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller jacob.e.kel...@intel.com
wrote:
Teach git-notes about a new configuration option notes.merge for
selecting the default notes merge strategy. Document the option in
config.txt and git-notes.txt
Add tests for the configuration option. Ensure that command line
--strategy option overrides the configured value. Ensure that -s can't
be passed with --commit or --abort. Ensure that --abort and --commit
can't be used together.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
Cc: Johan Herland jo...@herland.net
Cc: Michael Haggerty mhag...@alum.mit.edu
Cc: Eric Sunshine sunsh...@sunshineco.com
Thanks, this looks better than the previous round. A few comments below...
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..d8944f5aec60 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,13 +101,13 @@ merge::
any) into the current notes ref (called local).
+
If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
-the manual resolver is used. This resolver checks out the
-conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
-and instructs the user to manually resolve the conflicts there.
-When done, the user can either finalize the merge with
-'git notes merge --commit', or abort the merge with
-'git notes merge --abort'.
+conflicting notes (see the -s/--strategy option or notes.merge
+config option) is not given, the manual resolver is used.
+This resolver checks out the conflicting notes in a special
+worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user
+to manually resolve the conflicts there. When done, the user
+can either finalize the merge with 'git notes merge --commit',
+or abort the merge with 'git notes merge --abort'.
When you re-wrap the text like this, it's difficult to spot your one
little change in all the diff noise. For the sake of review, it's
often better to minimize the change, even if it leaves the text more
jagged than you'd like.
Even if it leaves it incredibly jagged? That is one of the downsides
with diff of flowed text like this :(
remove::
Remove the notes for given objects (defaults to HEAD). When
@@ -181,10 +181,10 @@ OPTIONS
-s strategy::
--strategy=strategy::
When merging notes, resolve notes conflicts using the given
- strategy. The following strategies are recognized: manual
- (default), ours, theirs, union and cat_sort_uniq.
- See the NOTES MERGE STRATEGIES section below for more
- information on each notes merge strategy.
+ strategy. Overrides notes.merge. The following strategies are
+ recognized: `manual`, `ours`, `theirs`, `union` and
+ `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
+ for more information on each notes merge strategy.
Ditto.
--commit::
Finalize an in-progress 'git notes merge'. Use this option
diff --git a/builtin/notes.c b/builtin/notes.c
index 88fdafb32bc0..728980bc79bf 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,6 +743,25 @@ static int merge_commit(struct notes_merge_options *o)
return ret;
}
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy
*strategy)
+{
+ if (!strcmp(arg, manual))
+ *strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(arg, ours))
+ *strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(arg, theirs))
+ *strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(arg, union))
+ *strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(arg, cat_sort_uniq))
+ *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else {
+ return 1;
In this codebase, it's customary to return 0 on success and -1 on error
Fair enough.
+ }
Style: drop unnecessary braces
+
+ return 0;
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int git_notes_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, notes.merge)) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (parse_notes_strategy(value, merge_strategy))
+ return error(Unknown notes merge strategy: %s,
value);
For the non-error case, don't you want to 'return 0' here to indicate
that you handled the config variable rather than letting it fall
through to git_default_config() below?
Makes sense, I can fix that.
Re: [PATCH v2 0/2] notes: add notes.merge strategy option
On Sat, Aug 1, 2015 at 1:12 AM, Jacob Keller jacob.e.kel...@intel.com wrote: From: Jacob Keller jacob.kel...@gmail.com This series adds a default merge strategy option for git-notes, so that the user does not have to type -s every time. It is overridden by the -s option. I like this addition. A natural extension (i.e. future work, you needn't worry about it for now) would be to allow this configuration to be per notes ref. Different notes refs are used to hold different kinds of data, and where cat_sort_uniq may be a good strategy for one particular notes ref, it may be a poor default for other notes refs. So we should consider adding notes.notesref.merge options to allow more fine-grained control of which notes merge strategies apply to which notes refs. I also added some tests to ensure that the --abort --commit and -s options must be independent. Good. These could easily be split out into a separate commit, though, as they are independent of the notes.merge addition. In addition, I found a small documentation bug which is corrected in the first patch of the series. I Cc'd a couple more people in this version of the patch in order to hopefully get some more review. Thanks, appreciated (although AFAICS the cover letter was not CCed to me). This is based on pu incase there were any other conflicts, but I can easily rebase if necessary. Junio has the final word here, but I believe the preferred workflow is to base your patch series on master or next, as those do not jump around quite as much as pu does. Jacob Keller (2): notes: document cat_sort_uniq rewriteMode notes: add notes.merge option to select default strategy Both patches Acked-by: Johan Herland jo...@herland.net ...Johan Documentation/config.txt | 11 +-- Documentation/git-notes.txt | 33 + builtin/notes.c | 55 +-- notes-merge.h | 16 +- t/t3309-notes-merge-auto-resolve.sh | 29 ++ t/t3310-notes-merge-manual-resolve.sh | 12 6 files changed, 119 insertions(+), 37 deletions(-) -- 2.5.0.482.gfcd5645 -- 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, jo...@herland.net 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: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak karthik@gmail.com wrote: On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Good point! I just was wondering if we need another atom just to print a star. But your points certainly are valid. I'll implement this. Thanks :) This would remove the need of making the printing of the * to be needed by ref-filter. As this is only needed in branch.c If going on what you're saying we could have a %(starifcurrent) atom or something, but I don't see a general use for this. To have a really customizeable format, you would want to allow e.g. git branch --format '%(starifcurrent) %(objectname) %(refname)' if the user wants to get the sha1 before the refname, and still have the star in front. It's a bit frustrating to have a hardcoded format that the user can't reproduce with the --format option, since it means you can't easily make a small variation on it. Agreed. will have a starifcurrent atom :) Please don't. It's better to avoid these highly specialized solutions and instead seek general purpose ones. For instance, utilizing the %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch) which would allow a format like this: %(if:iscurrentbranch)*%(endif) Even more generic would be an %(ifeq:x:y) conditional and a %(currentbranch) atom: %(ifeq:refname:currentbranch)*%(endif) Those are just a couple ideas. Other variations are possible and likely preferable to the specialized %(starifcurrent). -- 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] completion: Add '--edit-todo' to rebase
Quoting Thomas Braun thomas.br...@virtuell-zuhause.de:
Am 31.07.2015 um 12:16 schrieb SZEDER Gábor:
Anyway, so this could be something like (modulo likely whitespace damage):
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 07c34ef913..fac01d6985 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1667,7 +1667,10 @@ _git_push ()
_git_rebase ()
{
local dir=$(__gitdir)
-if [ -d $dir/rebase-apply ] || [ -d $dir/rebase-merge ]; then
+if [ -f $dir/rebase-merge/interactive ]; then
+__gitcomp --continue --skip --abort --edit-todo
+return
+elif [ -d $dir/rebase-apply ] || [ -d $dir/rebase-merge ]; then
__gitcomp --continue --skip --abort
return
fi
This looks much better than my attempt. Thanks.
How is the protocol now? Do I reroll and add
Helped-By: John Keeping j...@keeping.me.uk
Completely-Overhauled-And-Properly-Implemented: SZEDER Gábor
sze...@ira.uka.de
Ugh :) I'm quite happy with Helped-by, if you do a proper reroll
after trying it out to see that it indeed does what it should.
Thanks,
Gábor
--
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 v1] send-email: provide whitelist of SMTP AUTH mechanisms
On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin vikto...@rehivetech.com wrote:
When sending an e-mail, the client and server must
agree on an authentication mechanism. Some servers
(due to misconfiguration or a bug) denies valid
s/denies/deny/
credentials for certain mechanisms. In this patch,
a new option --smtp-auth and configuration entry
smtpauth are introduced.
If smtp_auth is defined, it works as a whitelist
of allowed mechanisms for authentication. There
are four mechanisms supported: PLAIN, LOGIN,
CRAM-MD5, DIGEST-MD5. However, their availability
depends on the installed SASL library.
Signed-off-by: Jan Viktorin vikto...@rehivetech.com
---
git-send-email.perl | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
At the very least, you will also want to update the documentation
(Documentation/git-send-email.txt) and, if possible, add new tests
(t/t9001-send-email.sh).
More below.
diff --git a/git-send-email.perl b/git-send-email.perl
index ae9f869..b00ed9d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
return 1;
}
+ # Do not allow arbitrary strings.
Can you explain why this restriction is needed. What are the
consequences of not limiting the input to this approved list?
+ my ($filtered_auth) = ;
Style: unnecessary parentheses
+ foreach (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5) {
This might read more nicely and be easier to maintain if written as:
foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {
+ if($smtp_auth $smtp_auth =~ /\b\Q$_\E\b/i) {
Style: space after 'if'
Also, why not lift the 'if ($smtp_auth)' check outside the loop since
its value never changes and there's no need to iterate over the list
if $smtp_auth is empty.
+ $filtered_auth .= $_ . ;
Style question: Would this be more naturally expressed with
'filtered_auth' as an array onto which items are pushed, rather than
as a string? At the point of use, the string can be recreated via
join().
Not a big deal; just wondering.
+ }
+ }
+
+ die Invalid SMTP AUTH. if length $smtp_auth !length
$filtered_auth;
Style: drop capitalization: invalid...
Style: drop period at end
Style: add \n at end in order to suppress printing of the
perl line number and input line number which aren't
very meaningful for a user error
(Existing style in the script is not very consistent, but new code
probably should adhere the above suggestions.)
Also, don't you want to warn the user about tokens that don't match
one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than
dropping them silently?
# Workaround AUTH PLAIN/LOGIN interaction defect
# with Authen::SASL::Cyrus
eval {
@@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {
'password' = $smtp_authpass
}, sub {
my $cred = shift;
+
+ if($filtered_auth) {
Style: space after 'if'
+ my $sasl = Authen::SASL-new(
+ mechanism = $filtered_auth,
+ callback = {
+ user = $cred-{'username'},
+ pass = $cred-{'password'},
+ authname = $cred-{'username'},
+ }
+ );
+
+ return !!$smtp-auth($sasl);
+ }
+
return !!$smtp-auth($cred-{'username'},
$cred-{'password'});
});
--
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
