Re: [PATCH v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva wrote:
> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.
>
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const
> char *prefix)
> - if (!next_all)
> + if (!cmdmode)
> usage_with_options(git_bisect_helper_usage, options);
>
> - /* next-all */
> - return bisect_next_all(prefix, no_checkout);
> + switch (cmdmode) {
> + case NEXT_ALL:
> + return bisect_next_all(prefix, no_checkout);
> + default:
> + die("BUG: unknown subcommand '%d'", cmdmode);
> + }
> + return 0;
What happens if you remove this useless 'return 0'? Does the (or some)
compiler incorrectly complain about it falling off the end of the
function without returning a value?
> }
--
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 v6 3/3] bisect--helper: `write_terms` shell function in C
On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva wrote:
> Reimplement the `write_terms` shell function in C and add a `write-terms`
> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
> remove the subcommand `--check-term-format` as it can now be called from
> inside the function write_terms() C implementation.
>
> Using `--write-terms` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other method.
>
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -56,18 +56,41 @@ static int check_term_format(const char *term, const char
> *orig_term)
> +int write_terms(const char *bad, const char *good)
s/^/static/
> +{
> + struct strbuf content = STRBUF_INIT;
> + FILE *fp;
> + int res;
> +
> + if (!strcmp(bad, good))
> + return error(_("please use two different terms"));
> +
> + if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> + return -1;
> +
> + strbuf_addf(&content, "%s\n%s\n", bad, good);
What's the point of the strbuf when you could more easily emit this
same content directly to the file via:
fprintf(fp, "%s\n%s\n", bad, good);
> + fp = fopen(".git/BISECT_TERMS", "w");
Hardcoding ".git/" is wrong for a variety of reasons: It won't be
correct with linked worktrees, or when GIT_DIR is pointing elsewhere,
or when ".git" is a symbolic link, etc. Check out the get_git_dir(),
get_git_common_dir(), etc. functions in cache.h instead.
> + if (!fp){
Style: space before '{'
> + strbuf_release(&content);
> + die_errno(_("could not open the file to read terms"));
"read terms"? I thought this was writing.
Is dying here correct? I thought we established previously that you
should be reporting failure via normal -1 return value rather than
dying. Indeed, you're doing so below when strbuf_write() fails.
> + }
> + res = strbuf_write(&content, fp);
> + fclose(fp);
> + strbuf_release(&content);
> + return (res < 0) ? -1 : 0;
> +}
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
Style: insert blank line between functions
--
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 1/2] bisect--helper: `bisect_log` shell function in C
On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva wrote:
> bisect--helper: `bisect_log` shell function in C
Do you need to insert "rewrite" or "reimplement" in the subject?
> Reimplement the `bisect_log` shell function in C and add a
> `--bisect-log` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
>
> Using `--bisect-log` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other method.
>
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
> +int bisect_log(void)
s/^/static/
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)
As mentioned in my review of the "write-terms" rewrite, hardcoding
".git/" here is wrong for a variety of reasons. See get_git_dir(),
get_git_common_dir(), etc. in cache.h instead.
> + return error(_("We are not bisecting."));
> +
> + printf("%s", buf.buf);
> + strbuf_release(&buf);
> +
> + return 0;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> @@ -109,6 +127,8 @@ int cmd_bisect__helper(int argc, const char **argv, const
> char *prefix)
> if (argc != 2)
> die(_("--write-terms requires two arguments"));
> return write_terms(argv[0], argv[1]);
> + case BISECT_LOG:
Shouldn't you be die()ing here with an appropriate error message if
argc is not 0?
> + return bisect_log();
> default:
> die("BUG: unknown subcommand '%d'", cmdmode);
> }
--
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] bisect--helper: `bisect_voc` shell function in C
Pranit Bauva writes:
> +int bisect_voc(const char *term)
> +{
> + if (!strcmp(term, "bad"))
> + printf("bad|new\n");
> + if (!strcmp(term, "good"))
> + printf("good|old\n");
If you meant to use this as a helper command, then the implementation is
right, but you're not doing that.
If you write the function because one day you'll be calling it from C,
then:
1) First, I'd wait for this "one day" to happen. In general, write code
when you need it, don't write it ahead of time. Currently, you have
dead and untested code (I know, *you* have tested it, but it's still
"untested" as far as git.git is concerned). Dead code may bother
people reading the code (one would not understand why it's there),
and untested code means it may break later without anyone noticing.
2) Second, you'd need to return the string, not print it. You'll
typically use it like this:
printf(_("You need to give me at least one %s and one %s"),
bisect_voc(BISECT_BAD), bisect_voc(BISECT_GOOD));
which gives one more argument for 1): once you have a use-case, you
can design the API properly, and not blindly guess that you're going
to need printf. Actually, writting these 2 example lines, I also
noticed that the parameters could/should be an enum type rather than
a string, it makes the code both more efficient and clearer.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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
Ensimag students contribution
Hello everyone, We are a team of 4 students from Ensimag (french computer science engineering school) tutored by Matthieu Moy, doing a full-time Git project, planning to work on few features during the next month. We would like to start with microproject first, "Add more builtin patterns for userdiff" and "Make upload-pack and receive-pack use the parse-options api" as a warm up. Then, we would like to work on features from the SmallProjectsIdeas Page on the wiki. We don't know yet what we can implement though, we will choose after contributing on our first project, to have a better understanding on the project as a whole. If you have any suggestion for us, please feel free to warn us. Best regards, Beutin François, Duclot William Queru Antoine, Rabourg simon -- 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: git push --quiet option does not seem to work
PS H:\test-ps\myrepo> "hi" >> whatever PS H:\test-ps\myrepo> git add . PS H:\test-ps\myrepo> git commit -m 'boo' [test1 3cde450] boo Committer: Your name and email address were configured automatically based on your username and hostname. Please check that they are accurate. You can suppress this message by setting them explicitly. Run the following command and follow the instructions in your editor to edit your configuration file: git config --global --edit After doing this, you may fix the identity used for this commit with: git commit --amend --reset-author 1 file changed, 0 insertions(+), 0 deletions(-) PS H:\test-ps\myrepo> git push --quiet git : remote: At line:1 char:1 + git push --quiet + ~~ + CategoryInfo : NotSpecified: (remote: :String) [], RemoteException + FullyQualifiedErrorId : NativeCommandError remote: Analyzing objects... (3/3) (119 ms) remote: Storing packfile... done (113 ms) remote: Storing index... done (29 ms) -- 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 v6 3/3] bisect--helper: `write_terms` shell function in C
Hi,
On Mon, 16 May 2016, Eric Sunshine wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva wrote:
>
> > + fp = fopen(".git/BISECT_TERMS", "w");
>
> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
> etc. functions in cache.h instead.
Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
better: follow the example of bisect.c and use
`GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.
> > + strbuf_release(&content);
> > + die_errno(_("could not open the file to read terms"));
>
> "read terms"? I thought this was writing.
>
> Is dying here correct? I thought we established previously that you
> should be reporting failure via normal -1 return value rather than
> dying. Indeed, you're doing so below when strbuf_write() fails.
The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
in *.c. So technically, it would be okay here, too. However, I think that
this code should be written with libification in mind, so I would also
prefer it to error() and return, to give the caller a chance to do other
things after an error occurred.
Ciao,
Dscho
--
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: git push --quiet option does not seem to work
Hi Chris, could you please quote just the parts of the mail you are replying to, and *not* top-post? It would be appreciated. On Mon, 16 May 2016, Chris B wrote: > PS H:\test-ps\myrepo> git push --quiet > git : remote: > At line:1 char:1 > + git push --quiet > + ~~ > + CategoryInfo : NotSpecified: (remote: :String) [], > RemoteException > + FullyQualifiedErrorId : NativeCommandError > > remote: Analyzing objects... (3/3) (119 ms) > remote: Storing packfile... done (113 ms) > remote: Storing index... done (29 ms) So it actually works, eh? Could you please verify that this is a PowerShell-only problem by performing a similar push in Git CMD and in Git Bash? 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
mail-patch-series.sh, was Re: [PATCH v7 0/3] Add support for sending additional HTTP headers (part 2)
Hi Lars, On Tue, 10 May 2016, Lars Schneider wrote: > > On 09 May 2016, at 08:18, Johannes Schindelin > > wrote: > > > > [...] > > > > Published-As: > > https://github.com/dscho/git/releases/tag/extra-http-headers-v7 > > Interdiff vs v6: > > Published-As and Interdiff are really neat. I assume you have some kind > of script to create all this? If yes, are you willing to share it? Thanks for prodding me. I had planned to clean up my script and make it public for a long time already, as I think that submitting code to the Git project is quite a little bit too tedious, even for old timers like me, in particular when compared to the ease of opening Pull Requests a la GitHub. So now I cleaned up my script a bit and published it here: https://github.com/dscho/mail-patch-series Ironically, I welcome Pull Requests for this script ;-) Ciao, Dscho -- 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: git push --quiet option does not seem to work
On Mon, May 16, 2016 at 03:28:30PM +0200, Johannes Schindelin wrote: > > PS H:\test-ps\myrepo> git push --quiet > > git : remote: > > At line:1 char:1 > > + git push --quiet > > + ~~ > > + CategoryInfo : NotSpecified: (remote: :String) [], > > RemoteException > > + FullyQualifiedErrorId : NativeCommandError > > > > remote: Analyzing objects... (3/3) (119 ms) > > remote: Storing packfile... done (113 ms) > > remote: Storing index... done (29 ms) > > So it actually works, eh? > > Could you please verify that this is a PowerShell-only problem by > performing a similar push in Git CMD and in Git Bash? I don't know much about PowerShell, but presumably it is responsible for the first few lines. But there is something else going on, too, which is those "remote:" lines. Those are being relayed from the server by the git client. I don't think it would be correct for the client to suppress them, even with "--quiet", because the client side has no idea if they are progress junk or critical error messages. The client _does_ pass along the "quiet" flag to the server via the git protocol, so it should be the server's responsibility to drop progress output when it is present. The server side here is clearly not stock git, from the content of those progress messages (some googling shows it looks like whatever visualstudio.com is running, but I don't know what that is). So either the server implementation doesn't support the "quiet" protocol extension, or it is ignoring it. It might be worth filing a bug with them. -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: git push --quiet option does not seem to work
Once I included the whole email in my reply, but otherwise I deleted it all. Anyway, it is not a Powershell thing. I tested on another repo on GitHub and it worked as expected. So I guess indeed the problem lies with Microsoft's implementation. -- 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: git push --quiet option does not seem to work
Hi Chris, On Mon, 16 May 2016, Chris B wrote: > Once I included the whole email in my reply, but otherwise I deleted it > all. Both are bad practice. If you are considerate with the reader's time, this consideration is typically reprocicated. So it is a good idea to save the reader time by giving them the precise context they need. > Anyway, it is not a Powershell thing. I tested on another repo on > GitHub and it worked as expected. So I guess indeed the problem lies > with Microsoft's implementation. This is *really* unclear. What "Microsoft's implementation"??? Do you refer to VSTS, or do you refer to Git for Windows, or PowerShell? Please. To make it really simple for everybody involved, try to repeat as closely as possible the same push from PowerShell, Git CMD and Git Bash. We want to compare oranges to oranges. FWIW You can repeat the same push by force-pushing the previous state between re-runs. E.g. git push myremote HEAD^:my-branch git push myremote my-branch 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: git push --quiet option does not seem to work
On Mon, May 16, 2016 at 05:04:34PM +0200, Johannes Schindelin wrote: > > Anyway, it is not a Powershell thing. I tested on another repo on > > GitHub and it worked as expected. So I guess indeed the problem lies > > with Microsoft's implementation. > > This is *really* unclear. > > What "Microsoft's implementation"??? Do you refer to VSTS, or do you refer > to Git for Windows, or PowerShell? > > Please. To make it really simple for everybody involved, try to repeat as > closely as possible the same push from PowerShell, Git CMD and Git Bash. > We want to compare oranges to oranges. I don't think there is much to debug there. According to http://superuser.com/questions/213848/using-powershell-call-native-command-line-app-and-capture-stderr/462362#462362 and other sources, it looks like PowerShell is very picky about calling programs which produce any output on stderr, and may in an error object. So I think both Powershell and Git are working as advertised, it's just that their behaviors are incompatible. The "bug" is that the server is asking the client to write non-error output to stderr, even though the client should have asked the server to be quiet (though it would not hurt to check that it is doing so by looking at the output of GIT_TRACE_PACKET). -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 v6 3/3] bisect--helper: `write_terms` shell function in C
On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
wrote:
> On Mon, 16 May 2016, Eric Sunshine wrote:
>> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva wrote:
>> > + fp = fopen(".git/BISECT_TERMS", "w");
>>
>> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
>> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git"
>> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
>> etc. functions in cache.h instead.
>
> Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> better: follow the example of bisect.c and use
> `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.
Thanks for pointing this out. My review time is severely limited these
days so I didn't go the distance of re-studying and re-digesting which
function was the correct one to use.
>> > + strbuf_release(&content);
>> > + die_errno(_("could not open the file to read terms"));
>>
>> Is dying here correct? I thought we established previously that you
>> should be reporting failure via normal -1 return value rather than
>> dying. Indeed, you're doing so below when strbuf_write() fails.
>
> The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> in *.c. So technically, it would be okay here, too. However, I think that
> this code should be written with libification in mind, so I would also
> prefer it to error() and return, to give the caller a chance to do other
> things after an error occurred.
Agreed. Specific to the "established previously" I wrote above, I was
referring to [1] from just a few days ago which explained why 'return
-1' was preferable to die() in a similar case that had an odd mix of
'return -1' and die() in the same function.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556
--
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] Ignore dirty submodule states during stash
Hi Stefan,
On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:
> On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy wrote:
> > Do not save states of submodules as stash should ignore it.
>
> Can you explain why this is a good idea?
> (It is not obvious to me either way.)
Actually, submodules are already ignored by stash, but not fully (it was
introduced in commit 6848d58).
Current behavior is counter-intuitive, for example (if one has a project with a
submodule):
$ cd sub1
$ edit .. commit .. edit .. commit. Alternatively, just checkout some other
commit
$ cd .. # back to main project
$ git stash save
No local changes to save
$ # so, stash declares there are no changes
$ edit main.cpp
$ # For example, I need to update my working tree to latest master
$ git stash save # save local changes of 'main.cpp'...
$ git pull --recurse-submodules && git submodule update --recursive # update
to latest
$ git stash pop # I expect to get stashed changes for 'main.cpp', but...
warning: Failed to merge submodule sub1 (commits don't follow merge-base)
Auto-merging sub1
CONFLICT (submodule): Merge conflict in sub1
So, this is the issue. Instead of getting my local changes, I got a conflict
(and stash is not
poped out). The root cause is the 'stash' command does not know how to deal
with submodules,
but currently it tries to save the state of submodules, and even tries to
re-apply the state
(and it fails of course). The proposed solution fixes this behaviour.
All internal tests work fine with the change.
>
> Do we need a test/documentation updates for this?
I don't think so, 'stash' have never claimed submodule support.
>
> >
> > Signed-off-by: Vasily Titskiy
> > ---
> > git-stash.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/git-stash.sh b/git-stash.sh
> > index c7c65e2..b500c44 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -116,7 +116,7 @@ create_stash () {
> > git read-tree --index-output="$TMPindex" -m $i_tree
> > &&
> > GIT_INDEX_FILE="$TMPindex" &&
> > export GIT_INDEX_FILE &&
> > - git diff --name-only -z HEAD -- >"$TMP-stagenames"
> > &&
> > + git diff --name-only --ignore-submodules -z HEAD --
> > >"$TMP-stagenames" &&
> > git update-index -z --add --remove --stdin
> > <"$TMP-stagenames" &&
> > git write-tree &&
> > rm -f "$TMPindex"
> > --
> > 2.1.4
> >
> > --
> > 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
--
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 1/1] convert: ce_compare_data() checks for a sha1 of a path
From: Torsten Bögershausen
To compare a file in working tree with the index, convert_to_git() is used,
the the result is hashed and the hash value compared with ce->sha1.
Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
are converted or not: When a CRLF had been in the index before, CRLF in
the working tree are not converted.
While in a merge, a file name in the working tree has different blobs
in the index with different hash values.
Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
the would_convert_crlf_at_commit() looks at the appropriate blob.
Signed-off-by: Torsten Bögershausen
---
cache.h | 1 +
convert.c| 35 +--
convert.h| 23 +++
read-cache.c | 4 +++-
sha1_file.c | 17 +
5 files changed, 57 insertions(+), 23 deletions(-)
diff --git a/cache.h b/cache.h
index 160f8e3..5b25462 100644
--- a/cache.h
+++ b/cache.h
@@ -604,6 +604,7 @@ extern int ie_modified(const struct index_state *, const
struct cache_entry *, s
#define HASH_WRITE_OBJECT 1
#define HASH_FORMAT_CHECK 2
+#define HASH_CE_HAS_SHA1 4
extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum
object_type type, const char *path, unsigned flags);
extern int index_path(unsigned char *sha1, const char *path, struct stat *st,
unsigned flags);
diff --git a/convert.c b/convert.c
index f524b8d..dc86899 100644
--- a/convert.c
+++ b/convert.c
@@ -217,21 +217,25 @@ static void check_safe_crlf(const char *path, enum
crlf_action crlf_action,
}
}
-static int has_cr_in_index(const char *path)
+static int has_cr_in_index(const char *path, const unsigned char *sha1)
{
unsigned long sz;
void *data;
- int has_cr;
-
- data = read_blob_data_from_cache(path, &sz);
- if (!data)
- return 0;
- has_cr = memchr(data, '\r', sz) != NULL;
+ int has_cr = 0;
+ enum object_type type = OBJ_BLOB;
+ if (sha1)
+ data = read_sha1_file(sha1, &type, &sz);
+ else
+ data = read_blob_data_from_cache(path, &sz);
+
+ if (data)
+ has_cr = memchr(data, '\r', sz) != NULL;
free(data);
return has_cr;
}
-static int crlf_to_git(const char *path, const char *src, size_t len,
+static int crlf_to_git(const char *path, const unsigned char *sha1,
+ const char *src, size_t len,
struct strbuf *buf,
enum crlf_action crlf_action, enum safe_crlf checksafe)
{
@@ -260,7 +264,7 @@ static int crlf_to_git(const char *path, const char *src,
size_t len,
* If the file in the index has any CR in it, do not
convert.
* This is the new safer autocrlf handling.
*/
- if (has_cr_in_index(path))
+ if (has_cr_in_index(path, sha1))
return 0;
}
}
@@ -852,8 +856,9 @@ const char *get_convert_attr_ascii(const char *path)
return "";
}
-int convert_to_git(const char *path, const char *src, size_t len,
- struct strbuf *dst, enum safe_crlf checksafe)
+int convert_to_git_ce_sha1(const char *path, const unsigned char *sha1,
+ const char *src, size_t len,
+ struct strbuf *dst, enum safe_crlf checksafe)
{
int ret = 0;
const char *filter = NULL;
@@ -874,7 +879,7 @@ int convert_to_git(const char *path, const char *src,
size_t len,
src = dst->buf;
len = dst->len;
}
- ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
+ ret |= crlf_to_git(path, sha1, src, len, dst, ca.crlf_action,
checksafe);
if (ret && dst) {
src = dst->buf;
len = dst->len;
@@ -882,7 +887,9 @@ int convert_to_git(const char *path, const char *src,
size_t len,
return ret | ident_to_git(path, src, len, dst, ca.ident);
}
-void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
+void convert_to_git_filter_fd(const char *path,
+ const unsigned char *sha1,
+ int fd, struct strbuf *dst,
enum safe_crlf checksafe)
{
struct conv_attrs ca;
@@ -894,7 +901,7 @@ void convert_to_git_filter_fd(const char *path, int fd,
struct strbuf *dst,
if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
die("%s: clean filter '%s' failed", path, ca.drv->name);
- crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
+ crlf_to_git(path, sha1, dst->buf, dst->len, dst, ca.crlf_action,
checksafe);
ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
}
diff --git a/convert.h b/convert.h
index ccf436b..1634d37 100644
--- a/convert.h
+++ b/convert.h
@@
[PATCH v3 0/1] CRLF-Handling: bug fix around ce_compare_data()
From: Torsten Bögershausen Changes since v2: - Only 1 patch, the t6038 needs to go as a separate "series" has_cr_in_index() uses either data = read_sha1_file(sha1, &type, &sz); or data = read_blob_data_from_cache(path, &sz); (I like v2 better, since there is a single code to get a sha object into memory, which later will be replaced by a streaming object) But in any case, here is v3 Torsten Bögershausen (1): convert: ce_compare_data() checks for a sha1 of a path cache.h| 1 + convert.c | 35 +-- convert.h | 23 +++ read-cache.c | 4 +++- sha1_file.c| 17 + -- 2.0.0.rc1.6318.g0c2c796 -- 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] commit.c: use strchrnul() to scan for one line
Johannes Schindelin writes:
> On Sun, 15 May 2016, Junio C Hamano wrote:
>
>> diff --git a/commit.c b/commit.c
>> index 3f4f371..1f9ee8a 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const
>> char **subject)
>> p++;
>> if (*p) {
>> p += 2;
>> -for (eol = p; *eol && *eol != '\n'; eol++)
>> -; /* do nothing */
>> +eol = strchrnul(p, '\n');
>> } else
>> eol = p;
>
> ACK. This was my fault, when I introduced the code in 9509af68 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01). To be fair,
> strchrnul() was introduced only later, in 659c69c (Add strchrnul(),
> 2007-11-09).
Oh, there is no fault.
I was reading through attr.c to see how bad a work to revamp
attribute lookup would look like, found a hand-rolled strchrnul()
that predates the widespread use of the function, and looked for
similar patterns while I was updating it. As there were many false
positives (i.e. "Yes, this loop is looking for the end of line, but
it does something else to the characters on the line while doing so,
so it cannot become strchrnul()") that I needed eyeballing in order
to reject and avoid incorrect conversion, it is very much appreciated
that you double-checked this one that I spotted.
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 v2 56/94] apply: move 'struct apply_state' to apply.h
Eric Sunshine writes: > On Wed, May 11, 2016 at 9:17 AM, Christian Couder > wrote: >> To libify `git apply` functionality we must make 'struct apply_state' >> usable outside "builtin/apply.c". > > Why is this patch plopped right in the middle of a bunch of other > patches which are making functions return -1 rather than die()ing? > Seems out of place. Two possible places that would make more sense are (1) when it is introduced very early in the series, or (2) when it absorbed all the file-scope-static global states in the middle of the series. I think either is fine. That would be a good place to end the first batch of the topic. Then the second batch would be "turn die() into error status that is propagated upwards". -- 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
[no subject]
Recently I have run into an issue with the git-completion.tcsh script
where an error would occur upon sourcing the file on login. The error
that would occur was:
__git_tcsh_completion_version: Subscript out of range.
In taking a look at the script, I noticed that when the script parses
the tcsh version that I am running it does not put the output into an
array. Thus, when it attempts to access subscript 2 of
__git_tcsh_completion_version it throws the above error. To correct
this, I changed
set __git_tcsh_completion_version = ` \echo ${tcsh} | \sed 's/\./ /g'`
to
set __git_tcsh_completion_version = `( \echo ${tcsh} | \sed 's/\./ /g' )`
and the script runs correctly. I could not find others having issues
with this in the forums so I am not sure if this is unique to my
system or not. Thought I would pass it along.
For reference, I am running tcsh 6.17.00 on RHEL 6.8
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 v3 0/1] CRLF-Handling: bug fix around ce_compare_data()
tbo...@web.de writes: > From: Torsten Bögershausen > > Changes since v2: > > - Only 1 patch, the t6038 needs to go as a separate "series" > > has_cr_in_index() uses either > data = read_sha1_file(sha1, &type, &sz); > or > data = read_blob_data_from_cache(path, &sz); > > (I like v2 better, since there is a single code to get a sha object > into memory, which later will be replaced by a streaming object) Wait a minute, please. I only asked the reason why you did it that way and mentioned that the end result seemed equivalent. "The end result seems equivalent" does not automatically mean "therefore you must avoid changing the code." If you still prefer the original code, and your preference is backed by a solid reasoning, don't change the code to a less preferrable version. You may have to explain what you wrote in () above clearly in an updated log message to save other readers from asking the same question as I asked, though. 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] attr.c: simplify macroexpand_one()
On Sun, May 15, 2016 at 9:41 PM, Junio C Hamano wrote: > On Sun, May 15, 2016 at 8:31 PM, Eric Sunshine > wrote: >> On Sun, May 15, 2016 at 6:57 PM, Junio C Hamano wrote: >>> The double-loop wants to do an early return immediately when one >>> matching macro is found. Eliminate the extra variable 'a' used for >>> that purpose and rewrite the "assign found itme to 'a' to make it >> >> What's "itme"? > > item. The code looks good. Thanks, 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: [PATCH] Ignore dirty submodule states during stash
On Mon, May 16, 2016 at 8:46 AM, Vasily Titskiy wrote:
> Hi Stefan,
>
> On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:
>> On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy wrote:
>> > Do not save states of submodules as stash should ignore it.
>>
>> Can you explain why this is a good idea?
>> (It is not obvious to me either way.)
> Actually, submodules are already ignored by stash, but not fully (it was
> introduced in commit 6848d58).
> Current behavior is counter-intuitive, for example (if one has a project with
> a submodule):
> $ cd sub1
> $ edit .. commit .. edit .. commit. Alternatively, just checkout some other
> commit
> $ cd .. # back to main project
> $ git stash save
>No local changes to save
> $ # so, stash declares there are no changes
> $ edit main.cpp
> $ # For example, I need to update my working tree to latest master
> $ git stash save # save local changes of 'main.cpp'...
> $ git pull --recurse-submodules && git submodule update --recursive # update
> to latest
> $ git stash pop # I expect to get stashed changes for 'main.cpp', but...
>warning: Failed to merge submodule sub1 (commits don't follow merge-base)
>Auto-merging sub1
>CONFLICT (submodule): Merge conflict in sub1
>
> So, this is the issue. Instead of getting my local changes, I got a conflict
> (and stash is not
> poped out). The root cause is the 'stash' command does not know how to deal
> with submodules,
> but currently it tries to save the state of submodules, and even tries to
> re-apply the state
> (and it fails of course). The proposed solution fixes this behaviour.
>
> All internal tests work fine with the change.
I think you could take the example as above and make it into a test?
Showing that this change actually fixes a bug.
Looking for a good place, I would have expected t/t3906-stash-submodule.sh
would be a good place to put your code, but I am not sure how to
properly integrate that test there.
Maybe we can put the test in t3903 instead?
>
>
>>
>> Do we need a test/documentation updates for this?
> I don't think so, 'stash' have never claimed submodule support.
But it also never explicitly claimed it doesn't support it.
Maybe we want to squash in something like
(with better wording):
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..b2649eb 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -41,6 +41,8 @@ the usual reflog syntax (e.g. `stash@{0}` is the most recently
created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
is also possible).
+Stashing ignores submodule operations completely.
+
OPTIONS
---
Thanks,
Stefan
>
>>
>> >
>> > Signed-off-by: Vasily Titskiy
>> > ---
>> > git-stash.sh | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index c7c65e2..b500c44 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -116,7 +116,7 @@ create_stash () {
>> > git read-tree --index-output="$TMPindex" -m
>> > $i_tree &&
>> > GIT_INDEX_FILE="$TMPindex" &&
>> > export GIT_INDEX_FILE &&
>> > - git diff --name-only -z HEAD -- >"$TMP-stagenames"
>> > &&
>> > + git diff --name-only --ignore-submodules -z HEAD
>> > -- >"$TMP-stagenames" &&
>> > git update-index -z --add --remove --stdin
>> > <"$TMP-stagenames" &&
>> > git write-tree &&
>> > rm -f "$TMPindex"
>> > --
>> > 2.1.4
>> >
>> > --
>> > 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
--
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] Ignore dirty submodule states during stash
Hi Vasily, On Mon, 16 May 2016, Vasily Titskiy wrote: > On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote: > > On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy wrote: > > > Do not save states of submodules as stash should ignore it. > > > > Can you explain why this is a good idea? > > (It is not obvious to me either way.) > Actually, submodules are already ignored by stash, but not fully (it was > introduced in commit 6848d58). > Current behavior is counter-intuitive, for example (if one has a project > with a submodule): > [...] > > So, this is the issue. Instead of getting my local changes, I got a > conflict (and stash is not poped out). The root cause is the 'stash' > command does not know how to deal with submodules, but currently it > tries to save the state of submodules, and even tries to re-apply the > state (and it fails of course). The proposed solution fixes this > behaviour. Please make it a habit to put such information into the commit message. I have to admit that I was as puzzled as Stefan by the proposed patch. Such problems, and tedious back-and-forth discussion, can be easily avoided by providing additional background in the commit message. We even ask in our Documentation/SubmittingPatches explicitly to do that: > The body should provide a meaningful commit message, which: > > . explains the problem the change tries to solve, iow, what is wrong > with the current code without the change. > > . justifies the way the change solves the problem, iow, why the > result with the change is better. > > . alternate solutions considered but discarded, if any. BTW this is not something we ask to make contributors' lives harder. In fact it helps everybody, including the contributors themselves. Just imagine what kind of kick-ass commit message you would want to read in six months when you stumble over your very own commit and want to know what the heck it was about. 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 v6 3/3] bisect--helper: `write_terms` shell function in C
Hi Eric,
On Mon, 16 May 2016, Eric Sunshine wrote:
> On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin
> wrote:
> > On Mon, 16 May 2016, Eric Sunshine wrote:
> >> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva
> >> wrote:
> >> > + fp = fopen(".git/BISECT_TERMS", "w");
> >>
> >> Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct
> >> with linked worktrees, or when GIT_DIR is pointing elsewhere, or when
> >> ".git"
> >> is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(),
> >> etc. functions in cache.h instead.
> >
> > Maybe in this case, `git_path("BISECT_TERMS")` would be a good idea. Or even
> > better: follow the example of bisect.c and use
> > `GIT_PATH_FUNC(bisect_terms_path, "BISECT_TERMS")`.
>
> Thanks for pointing this out. My review time is severely limited these
> days so I didn't go the distance of re-studying and re-digesting which
> function was the correct one to use.
I am constantly amazed how much you manage to review, and how helpful your
comments are. I am glad if I could contribute a little.
> >> > + strbuf_release(&content);
> >> > + die_errno(_("could not open the file to read terms"));
> >>
> >> Is dying here correct? I thought we established previously that you
> >> should be reporting failure via normal -1 return value rather than
> >> dying. Indeed, you're doing so below when strbuf_write() fails.
> >
> > The rule of thumb seems to be that die()ing is okay in builtin/*.c, but not
> > in *.c. So technically, it would be okay here, too. However, I think that
> > this code should be written with libification in mind, so I would also
> > prefer it to error() and return, to give the caller a chance to do other
> > things after an error occurred.
>
> Agreed. Specific to the "established previously" I wrote above, I was
> referring to [1] from just a few days ago which explained why 'return
> -1' was preferable to die() in a similar case that had an odd mix of
> 'return -1' and die() in the same function.
>
> [1]:
> http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556
Yes, this is an excellent link. Maybe we would want something like this,
too?
-- snipsnap --
(From: Eric Sunshine )
CodingGuidelines: mention that die() is for fatal errors only
Let's formalize the rule when to use die() and when to use error().
diff --git a/Documentation/CodingGuidelines
b/Documentation/CodingGuidelines
index 0ddd368..5d2d65f 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -333,6 +333,10 @@ For C programs:
- When you come up with an API, document it.
+ - Use die() only to signal an exceptional conditions which should
+ abort the program. All other error conditions should instead
+ return e.g. using "return error(...)".
+
- The first #include in C files, except in platform specific compat/
implementations, must be either "git-compat-util.h", "cache.h" or
"builtin.h". You do not have to include more than one of these.
--
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 v6 3/3] bisect--helper: `write_terms` shell function in C
On Mon, May 16, 2016 at 12:45 PM, Johannes Schindelin wrote: > On Mon, 16 May 2016, Eric Sunshine wrote: >> On Mon, May 16, 2016 at 9:16 AM, Johannes Schindelin >> wrote: >> > On Mon, 16 May 2016, Eric Sunshine wrote: >> Agreed. Specific to the "established previously" I wrote above, I was >> referring to [1] from just a few days ago which explained why 'return >> -1' was preferable to die() in a similar case that had an odd mix of >> 'return -1' and die() in the same function. >> >> [1]: >> http://thread.gmane.org/gmane.comp.version-control.git/289476/focus=293556 > > Yes, this is an excellent link. Maybe we would want something like this, > too? > > -- snipsnap -- > diff --git a/Documentation/CodingGuidelines > @@ -333,6 +333,10 @@ For C programs: > + - Use die() only to signal an exceptional conditions which should > + abort the program. All other error conditions should instead > + return e.g. using "return error(...)". Hmm, the bit you said earlier about "library" code vs. end-user code would be important, too, I'd think. Also, I'd expect that this sort of thing is implicitly understood by most programmers, and as this is only the first time (in recent memory) that it has needed explanation on the mailing list, I'm not sure that it deserves explicit mention in CodingGuidelines (but perhaps I'm not a good judge of these things). -- 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/2] bisect--helper: rewrite of check-term-format()
On Mon, May 16, 2016 at 2:35 AM, Eric Sunshine wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano wrote:
Pranit Bauva writes:
> I completely missed your point and you want me to go the Eric Sunshine's
> way?
I am neutral.
When I read your response to Eric's "top down" suggestion, I didn't
quite get much more than "I started going this way, and I do not
want to change to the other direction.", which was what I had the
most trouble with. If your justification for the approach to start
from building a tiny bottom layer that will need to be dismantled
soon and repeat that (which sounds somewhat wasteful) were more
convincing, I may have felt differently.
>>>
>>> Sorry if it seemed that "I have done quite some work and I don't want
>>> to scrape it off and redo everything". This isn't a case for me. I
>>> think of this as just a small part in the process of learning and my
>>> efforts would be completely wasted as I can still reuse the methods I
>>
>> efforts would **not** be completely wasted
>>
>>> wrote. This is still open for a "philosophical" discussion. I am
>>> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
> 2015-06-14), one of the (numerous) things Paul Tan did which impressed
> me was to formally measure test suite coverage of the commands he was
> converting to C, and then improve coverage where it was lacking. That
> approach increases confidence in the conversion far more than fallible
> human reviews do.
>
> Setting aside the top-down vs. bottom-up discussion, as a reviewer
> (and user) I'd be far more interested in seeing you spend a good
> initial chunk of your project emulating Paul's approach to measuring
> and improving test coverage (though I don't know how your GSoC mentors
> feel about that).
If we could test each `git bisect-helper` subcommand that has been
converted to C separately, then I think it would be quite easy and a
good idea. And maybe Paul's approach is really great and easy to use,
but I haven't followed his GSoC, so I cannot tell.
As we take the approach of testing the whole thing, which is made of a
number of different source files (shell code in git-bisect.sh, C code
in `git bisect-helper` subcommands and in bisect.{c,h}), I am worried
that Pranit would spend too much time initially not only to measure
test coverage but especially to find ways to improve it.
The good thing with converting code incrementally to C is that one
does not need to understand everything first. While if one wants to
increase test coverage initially, it might be needed to understand
many things.
What I started asking though is to take advantage of bugs that are
found, and were not covered by the test suite, to improve test
coverage.
Also starting to convert functions right away can make the student
feel productive early and in turn make everyone else happy to see some
progress.
--
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 5/5] pathspec: record labels
On Sat, May 14, 2016 at 12:23 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> Labels were originally designed to manage large amount of
>> submodules, the discussion steered this in a more general
>> direction, such that other files can be labeled as well.
>
> s/other files/any path/.
>
>> Labels are meant to describe arbitrary set of files, which
>> is not described via the tree layout.
>
> Let's avoid "are meant to", which is merely to give you an excuse to
> say "it was meant to ... but the implementation did not quite achieve
> that goal".
>
> The "label" attribute can be attached to paths, and the pathspec
> mechanism is extended via the new ":(label=X)pattern/to/match"
I wasn't sure about whether we want to have '=' or ':' as the separator
of "label" and its values. The prefix which is passed around uses
a colon instead of equal sign, so I thought consistency would be neat
there. But as that is only internal, we can be inconsistent to a new
public feature, so '=' seems better for the labeling system.
> syntax to filter paths so that it requires paths to not just
> match the given pattern but also have the specified labels
> attached for them to be chosen.
>
> or something?
>
>> +Attaching labels to path
>> +
>> +
>> +`label`
>> +^^
>> +By the value of this attribute you can specify a list of arbitrary strings
>> +to be attached to a path as labels. These labels can be used for
>> +easier paths matching using pathspecs (linkgit:gitglossary[1]).
>> +It is recommended to use only alphanumeric characters, dashes and
>> +underscores for the labels.
>
> Make this recomendation stronger to requirement. It is always
> possible to loosen it later, but once we allow random things even
> with a warning, it gets impossible to take them back. Users will say
> "Even though we got a warning, it used to correctly filter; now Git
> is broken and my setup does not work."
I'll change the text here with s/recommended/required/ and error out
for that.
But still we do not enforce it early enough. Some crazy upstream may
add some labels which do not follow the requirements and then
tell downstream to run a command like `git foo "(label=!@#$)".
and then downstream curses at upstream as well as Git.
>
> s/habe/have/
done
>
>> + if (ATTR_TRUE(check.value))
>> + ret = 1; /* has all the labels */
>
> So this is "pretend that I have all the labels under the sun".
>
>> + else if (ATTR_FALSE(check.value))
>> + ret = 0; /* has no labels */
>
> I do not see a value in this. What difference does it have compared
> to having a "label=" that explicitly says "I do not have one"? What
> is your answer to an end-user question "When should I say 'label='
> and when should I say '!label'?"
>
> Just forbid it for now; we may find a better use for it later.
I don't think we want to die in that code as it is in the data already.
If we were to die in that case, you cannot query any path with
labels any more if asking for any kind of label.
>
>> + else if (ATTR_UNSET(check.value))
>> + /*
>> + * If no label was specified this matches, otherwise
>> + * there is a missing label.
>> + */
>> + ret = (required_labels->nr > 0) ? 0 : 1;
>
> Hmm, I can see that this is making things more complicated to
> explain and understand, but I cannot see what the expected use case
> is.
>
> Unless there is any compelling use case, I'd say we should forbid it
> for now.
We need to allow the UNSET case, as otherwise you'd need to label
any path if using labels? In the next iteration I'll go with
if (ATTR_TRUE(check.value))
ret = 1; /* has all the labels */
else if (ATTR_FALSE(check.value))
die(_("Label attributes must not be unset"));
else if (ATTR_UNSET(check.value))
ret = 0; /* has no labels */
else {
...
>
>> + else {
>> + struct string_list_item *si;
>> + struct string_list attr_labels = STRING_LIST_INIT_DUP;
>> + string_list_split(&attr_labels, check.value, ',', -1);
>> + string_list_sort(&attr_labels);
>
> Filter out a non-compliant label values here, so that they are
> ignored from day one. That way you would not have to deal with "I
> know I got the warning, but it used to work and you broke it" later.
So you're saying we should not die(...) but just ignore those labels?
>
>> + ret = 1;
>> + for_each_string_list_item(si, required_labels) {
>> + if (!string_list_has_string(&attr_labels, si->string))
>> {
>> + ret = 0;
>> + break;
>> + }
>> + }
>> + string_list_clear(&attr_labels, 0);
>> + }
>> +
>> + return ret;
>> +}
>
>> +static void validate_label_name(const char *label)
>> +{
>> +
Re: [PATCH v2 52/94] builtin/apply: read_patch_file() return -1 instead of die()ing
On Mon, May 16, 2016 at 3:56 AM, Eric Sunshine wrote:
> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
> wrote:
>> To libify `git apply` functionality we have to signal errors to the
>> caller instead of die()ing. Let's do that by using error() instead
>> of die()ing in read_patch_file().
>>
>> Signed-off-by: Christian Couder
>> ---
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> @@ -445,10 +445,10 @@ static void say_patch_name(FILE *output, const char
>> *fmt, struct patch *patch)
>> -static void read_patch_file(struct strbuf *sb, int fd)
>> +static int read_patch_file(struct strbuf *sb, int fd)
>> {
>> if (strbuf_read(sb, fd, 0) < 0)
>> - die_errno("git apply: failed to read");
>> + return error("git apply: failed to read: %s",
>> strerror(errno));
>
> When Duy's nd/error-errno series, which is in 'next', gets promoted to
> 'master', then this could become:
>
> return error_errno(...);
Yeah, sure.
--
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 0/4] pathspec labels [WAS: submodule groups]
On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen wrote: > Instead of putting everything in under the same attribute name > "label", make one attribute per label? Would this work? > > *.[ch] c-group code-group > > And the pathspec magic name can be simply "attr", any git attribute > can be used with it, e.g. :(attr:c-group) So you want to be able to query something like: git ls-files ":(crlf=auto)" as well? -- 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 5/5] pathspec: record labels
Stefan Beller writes:
>> Let's avoid "are meant to", which is merely to give you an excuse to
>> say "it was meant to ... but the implementation did not quite achieve
>> that goal".
>>
>> The "label" attribute can be attached to paths, and the pathspec
>> mechanism is extended via the new ":(label=X)pattern/to/match"
>
> I wasn't sure about whether we want to have '=' or ':' as the separator
> of "label" and its values. ...
Oh, sorry, the above wasn't a suggestion to change : to = at all. I
was merely being sloppy in the details that is irrelevant to my main
point (which is, it is better spend the bits you spent for "meant
to" instead for saying clearly what it does).
> ... But as that is only internal, we can be inconsistent to a new
> public feature, so '=' seems better for the labeling system.
I can buy that, too. Good that my sloppy wording helped you think
about it further ;-).
> But still we do not enforce it early enough. Some crazy upstream may
> add some labels which do not follow the requirements and then
> tell downstream to run a command like `git foo "(label=!@#$)".
> and then downstream curses at upstream as well as Git.
That is why it is "warn and ignore", not "die".
>>> + if (ATTR_TRUE(check.value))
>>> + ret = 1; /* has all the labels */
>>
>> So this is "pretend that I have all the labels under the sun".
>>
>>> + else if (ATTR_FALSE(check.value))
>>> + ret = 0; /* has no labels */
>>
>> I do not see a value in this. What difference does it have compared
>> to having a "label=" that explicitly says "I do not have one"? What
>> is your answer to an end-user question "When should I say 'label='
>> and when should I say '!label'?"
>>
>> Just forbid it for now; we may find a better use for it later.
>
> I don't think we want to die in that code as it is in the data already.
Is it? I think this is coming from the command line pathspec magic,
not from .gitattributes file recorded in the tree.
> We need to allow the UNSET case, as otherwise you'd need to label
> any path if using labels?
You do need UNSET (roughly, "no label is mentioned for the path").
If I want to say "Everything under Documentation/ and also things
with label=doc", I'd say
git cmd "Documentation/" ":(label=doc)"
and no path in Documentation/ need any label, i.e. for them, the
labels are unset. They will not be caught with ":(label=doc)"
because they are not set, but that is OK.
FALSE is different from UNSET. It needs an explicit mention, i.e.
*.doc doc
false.doc -doc
What's the difference between saying "-doc" and not saying anything?
If you really want to explicitly say "doc attribute is unset for
this path (perhaps because you may have other patterns that set the
doc elsewhere), you would say "!doc", and you already have code for
that.
> if (ATTR_TRUE(check.value))
> ret = 1; /* has all the labels */
> else if (ATTR_FALSE(check.value))
> die(_("Label attributes must not be unset"));
The message is wrong. You are dying because the user explicitly set
it to false, not because the user made it unset.
> else if (ATTR_UNSET(check.value))
> ret = 0; /* has no labels */
>>> + struct string_list_item *si;
>>> + struct string_list attr_labels = STRING_LIST_INIT_DUP;
>>> + string_list_split(&attr_labels, check.value, ',', -1);
>>> + string_list_sort(&attr_labels);
>>
>> Filter out a non-compliant label values here, so that they are
>> ignored from day one. That way you would not have to deal with "I
>> know I got the warning, but it used to work and you broke it" later.
>
> So you're saying we should not die(...) but just ignore those labels?
Do not die() but warn-and-ignore when you see funnies in
.gitattributes; do die() if you see funnies in pathspec magic.
>> I am NOT suggesting to make this enhancement in the prototype to
>> allow us experiment with submodule selection use case, but this is
>> an obvious place to allow
>>
>> :(label=A B):(label=C D)
>>
>> to mean ((A & B) | (C & D)) by making item->labels an array of set
>> of labels.
>>
>> And no, I do not think arbitrary boolean expression is too complex
s/do not/do/
>> to understand to the end-users, especially if we have to stay within
>> the pathspec magic syntax. And my gut feeling is that it is not
>> worth it to support anything more complex than "OR of these ANDed
>> ones".
>
> That makes sense.
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: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]
Stefan Beller writes: > On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen wrote: >> Instead of putting everything in under the same attribute name >> "label", make one attribute per label? Would this work? >> >> *.[ch] c-group code-group >> >> And the pathspec magic name can be simply "attr", any git attribute >> can be used with it, e.g. :(attr:c-group) > > So you want to be able to query something like: > > git ls-files ":(crlf=auto)" > > as well? It would be more like git ls-files ":(attr:crlf=auto)" It certainly sounds tempting, even though I do not want to keep what this initial chunk of the series needs to do to the minimum. -- 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 0/6] modernize t1500
On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano wrote: > Junio C Hamano writes: >> Junio C Hamano writes: >> Subject: [PATCH 7/6] t1500: finish preparation upfront >> >> The earlier tests do not attempt to modify the contents of .git (by >> creating objects or refs, for example), which means that even if >> some earlier tests before "cp -R" fail, the tests in repo.git will >> run in an environment that we can expect them to succeed in. >> >> Make it clear that tests in repo.git will be independent from the >> results of earlier tests done in .git by moving its initialization >> "cp -R .git repo.git" much higher in the test sequence. >> >> Signed-off-by: Junio C Hamano >> --- > > I think the same logic applies to other preparatory things like > creation of sub/dir in the working tree etc. Hmm, so are you suggesting a single 'setup' test at the start of script which does the 'cp -R' and creates those other directories needed by later tests? -- 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 5/5] pathspec: record labels
On Mon, May 16, 2016 at 10:38 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>>> Let's avoid "are meant to", which is merely to give you an excuse to
>>> say "it was meant to ... but the implementation did not quite achieve
>>> that goal".
>>>
>>> The "label" attribute can be attached to paths, and the pathspec
>>> mechanism is extended via the new ":(label=X)pattern/to/match"
>>
>> I wasn't sure about whether we want to have '=' or ':' as the separator
>> of "label" and its values. ...
>
> Oh, sorry, the above wasn't a suggestion to change : to = at all. I
> was merely being sloppy in the details that is irrelevant to my main
> point (which is, it is better spend the bits you spent for "meant
> to" instead for saying clearly what it does).
>
>> ... But as that is only internal, we can be inconsistent to a new
>> public feature, so '=' seems better for the labeling system.
>
> I can buy that, too. Good that my sloppy wording helped you think
> about it further ;-).
>
>> But still we do not enforce it early enough. Some crazy upstream may
>> add some labels which do not follow the requirements and then
>> tell downstream to run a command like `git foo "(label=!@#$)".
>> and then downstream curses at upstream as well as Git.
>
> That is why it is "warn and ignore", not "die".
So "warn and ignore" for data from .gitattributes and die for
commandline arguments? That makes sense.
>
+ if (ATTR_TRUE(check.value))
+ ret = 1; /* has all the labels */
>>>
>>> So this is "pretend that I have all the labels under the sun".
>>>
+ else if (ATTR_FALSE(check.value))
+ ret = 0; /* has no labels */
>>>
>>> I do not see a value in this. What difference does it have compared
>>> to having a "label=" that explicitly says "I do not have one"? What
>>> is your answer to an end-user question "When should I say 'label='
>>> and when should I say '!label'?"
>>>
>>> Just forbid it for now; we may find a better use for it later.
>>
>> I don't think we want to die in that code as it is in the data already.
>
> Is it? I think this is coming from the command line pathspec magic,
> not from .gitattributes file recorded in the tree.
The 'check.value' is coming from the .gitattributes.
The 'required_labels' string list comes from the command line.
("The command line requires these labels")
>
>> We need to allow the UNSET case, as otherwise you'd need to label
>> any path if using labels?
>
> You do need UNSET (roughly, "no label is mentioned for the path").
>
> If I want to say "Everything under Documentation/ and also things
> with label=doc", I'd say
>
> git cmd "Documentation/" ":(label=doc)"
>
> and no path in Documentation/ need any label, i.e. for them, the
> labels are unset. They will not be caught with ":(label=doc)"
> because they are not set, but that is OK.
right. that is what the end user should be able to do. That also means,
we need to handle files with UNSET labels (all under Documentation/)
>
> FALSE is different from UNSET. It needs an explicit mention, i.e.
>
> *.doc doc
> false.doc -doc
and this is in the .gitattribues, so warn-and-ignore and by ignore
we mean treat it as UNSET?
>
> What's the difference between saying "-doc" and not saying anything?
> If you really want to explicitly say "doc attribute is unset for
> this path (perhaps because you may have other patterns that set the
> doc elsewhere), you would say "!doc", and you already have code for
> that.
>
>> if (ATTR_TRUE(check.value))
>> ret = 1; /* has all the labels */
>> else if (ATTR_FALSE(check.value))
>> die(_("Label attributes must not be unset"));
>
> The message is wrong. You are dying because the user explicitly set
> it to false, not because the user made it unset.
Ok, so here is the warn-and-ignore code:
if (ATTR_TRUE(check.value))
ret = 1; /* has all the labels */
else if (ATTR_FALSE(check.value)) {
warning(_("Path '%s': Label must not be false. Treat
as if no label was set"), path);
ret = 0;
else if (ATTR_UNSET(check.value))
ret = 0; /* has no labels */
else {
Thanks,
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: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]
On Mon, May 16, 2016 at 10:39 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Sun, May 15, 2016 at 3:06 AM, Duy Nguyen wrote: >>> Instead of putting everything in under the same attribute name >>> "label", make one attribute per label? Would this work? >>> >>> *.[ch] c-group code-group >>> >>> And the pathspec magic name can be simply "attr", any git attribute >>> can be used with it, e.g. :(attr:c-group) >> >> So you want to be able to query something like: >> >> git ls-files ":(crlf=auto)" >> >> as well? > > It would be more like > > git ls-files ":(attr:crlf=auto)" > > It certainly sounds tempting, even though I do not want to keep what > this initial chunk of the series needs to do to the minimum. This is another case for using ':' instead of '='. So I think ':' is better for this future enhancement. Also this future enhancement may ask for git ls-files ":(attr:label=foo)" or git ls-files ":(attr:-label)" so the user can circumvent the warn and ignore strategy. :( -- 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 5/5] pathspec: record labels
> I am NOT suggesting to make this enhancement in the prototype to > allow us experiment with submodule selection use case, but this is > an obvious place to allow > > :(label=A B):(label=C D) > > to mean ((A & B) | (C & D)) by making item->labels an array of set > of labels. This is what already works with the series. Or rather: ":(label=A B)" ":(label=C D)" works as you would expect for (A&B) | (C&D). So I am a bit hesitant to replace the string list by an array of stringlists or such, as the future enhancement can also do that? The enhancement may bring in more expressions into the label string, so it may even parse that string into a tree for Lexicographical order instead of just using an array of lists. > > And no, I do not think arbitrary boolean expression is too complex > to understand to the end-users, especially if we have to stay within > the pathspec magic syntax. And my gut feeling is that it is not > worth it to support anything more complex than "OR of these ANDed > ones". > >> + string_list_split(item->labels, sb.buf, ' ', -1); >> + string_list_remove_empty_items(item->labels, 0); >> + strbuf_release(&sb); >> + continue; > > The data structure to record the "required labels" is shared > knowledge between this function and the has_all_labels() and nobody > else knows it is done with string_list, so I think this is a good > balance between expediency and future optimization possibilities (I > am anticipating that linear search of string list would be found as > performance bottleneck). > -- 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 20/20] untracked-cache: config option
On Sun, 2016-05-15 at 16:43 +0700, Duy Nguyen wrote:
> On Fri, May 13, 2016 at 3:20 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > Add a config option to populate the untracked cache.
> >
> > For installations that have centrally-managed configuration, it's
> > easier to set a config once than to run update-index on every
> > repository.
>
> This sounds like the job for core.untrackedCache. It populates after
> reading the index though (in post_read_index_from) not writing, but I
> think it accomplishes the same thing.
OK, I'll drop this one.
> > Signed-off-by: David Turner
> > ---
> > Documentation/config.txt | 4
> > read-cache.c | 7 ++-
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 385ea66..c7b76ef 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1848,6 +1848,10 @@ imap::
> > The configuration variables in the 'imap' section are
> > described
> > in linkgit:git-imap-send[1].
> >
> > +index.adduntrackedcache::
> > + Automatically populate the untracked cache whenever the
> > index
> > + is written.
> > +
> > index.addwatchmanextension::
> > Automatically add the watchman extension to the index
> > whenever
> > it is written.
> > diff --git a/read-cache.c b/read-cache.c
> > index 22c64d0..4a1cccf 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -2472,7 +2472,7 @@ static int do_write_index(struct index_state
> > *istate, int newfd,
> > int entries = istate->cache_nr;
> > struct stat st;
> > struct strbuf previous_name_buf = STRBUF_INIT,
> > *previous_name;
> > - int watchman = 0;
> > + int watchman = 0, untracked = 0;
> > uint64_t start = getnanotime();
> >
> > for (i = removed = extended = 0; i < entries; i++) {
> > @@ -2502,6 +2502,11 @@ static int do_write_index(struct index_state
> > *istate, int newfd,
> > !the_index.last_update)
> > the_index.last_update = xstrdup("");
> >
> > + if (!git_config_get_bool("index.adduntrackedcache",
> > &untracked) &&
> > + untracked &&
> > + !istate->untracked)
> > + add_untracked_cache(&the_index);
> > +
> > hdr_version = istate->version;
> >
> > hdr.hdr_signature = htonl(CACHE_SIGNATURE);
> > --
> > 2.4.2.767.g62658d5-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
[PATCHv4 3/5] pathspec: move long magic parsing out of prefix_pathspec
`prefix_pathspec` is quite a lengthy function and we plan on adding more.
Split it up for better readability. As we want to add code into the
inner loop of the long magic parsing, we also benefit from lower
indentation.
Signed-off-by: Stefan Beller
---
pathspec.c | 84 +++---
1 file changed, 47 insertions(+), 37 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..eba37c2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
+static void eat_long_magic(struct pathspec_item *item, const char *elt,
+ unsigned *magic, int *pathspec_prefix,
+ const char **copyfrom_, const char **long_magic_end)
+{
+ int i;
+ const char *copyfrom = *copyfrom_;
+ /* longhand */
+ const char *nextat;
+ for (copyfrom = elt + 2;
+*copyfrom && *copyfrom != ')';
+copyfrom = nextat) {
+ size_t len = strcspn(copyfrom, ",)");
+ if (copyfrom[len] == ',')
+ nextat = copyfrom + len + 1;
+ else
+ /* handle ')' and '\0' */
+ nextat = copyfrom + len;
+ if (!len)
+ continue;
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (strlen(pathspec_magic[i].name) == len &&
+ !strncmp(pathspec_magic[i].name, copyfrom, len)) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ if (starts_with(copyfrom, "prefix:")) {
+ char *endptr;
+ *pathspec_prefix = strtol(copyfrom + 7,
+ &endptr, 10);
+ if (endptr - copyfrom != len)
+ die(_("invalid parameter for pathspec
magic 'prefix'"));
+ /* "i" would be wrong, but it does not matter */
+ break;
+ }
+ }
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Invalid pathspec magic '%.*s' in '%s'"),
+ (int) len, copyfrom, elt);
+ }
+ if (*copyfrom != ')')
+ die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
+ *long_magic_end = copyfrom;
+ copyfrom++;
+ *copyfrom_ = copyfrom;
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
- /* longhand */
- const char *nextat;
- for (copyfrom = elt + 2;
-*copyfrom && *copyfrom != ')';
-copyfrom = nextat) {
- size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == ',')
- nextat = copyfrom + len + 1;
- else
- /* handle ')' and '\0' */
- nextat = copyfrom + len;
- if (!len)
- continue;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
- if (strlen(pathspec_magic[i].name) == len &&
- !strncmp(pathspec_magic[i].name, copyfrom,
len)) {
- magic |= pathspec_magic[i].bit;
- break;
- }
- if (starts_with(copyfrom, "prefix:")) {
- char *endptr;
- pathspec_prefix = strtol(copyfrom + 7,
-&endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for
pathspec magic 'prefix'"));
- /* "i" would be wrong, but it does not
matter */
- break;
- }
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Invalid pathspec magic '%.*s' in '%s'"),
- (int) len, copyfrom, elt);
- }
- if (*copyfrom != ')')
- die(_("Missing ')' at the end of pathspec
[PATCHv4 0/5] pathspec labels
Thanks Junio for the review!
Thanks Duy for suggestions to think about in the not-submodule case :)
* when invalid labels are found
-> in the .gitattributes "warn and ignore"
-> in command line args die(..)
* treat labels set to false as unset.
* fixes in documentation/ reworded the commit message
Thanks,
Stefan
diff to current origin/sb/pathspec-label (v2 of the series, so a lot):
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index af2c682..b846848 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -906,6 +906,18 @@ If this attribute is not set or has an invalid value, the
value of the
(See linkgit:git-config[1]).
+Attaching labels to path
+
+
+`label`
+^^^
+By the value of this attribute you can specify a list of arbitrary strings
+to be attached to a path as labels. These labels can be used for
+easier paths matching using pathspecs (linkgit:gitglossary[1]).
+It is required to use only alphanumeric characters, dashes and
+underscores for the labels.
+
+
USING MACRO ATTRIBUTES
--
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index a1fc9e0..e264a58 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -362,11 +362,6 @@ glob;;
For example, "Documentation/{asterisk}.html" matches
"Documentation/git.html" but not "Documentation/ppc/ppc.html"
or "tools/perf/Documentation/perf.html".
-
-label:;;
- Labels can be assigned to pathspecs in the .gitattributes file.
- By specifying a list of labels the pattern will match only
- files which have all of the listed labels.
+
Two consecutive asterisks ("`**`") in patterns matched against
full pathname may have special meaning:
@@ -389,6 +384,10 @@ full pathname may have special meaning:
+
Glob magic is incompatible with literal magic.
+label=;;
+ Additionally to matching the pathspec, the path must have a 'label'
+ attribute having set all labels listed here.
+
exclude;;
After a path matches any non-exclude pathspec, it will be run
through all exclude pathspec (magic signature: `!`). If it
diff --git a/attr.h b/attr.h
index f6fc7c3..8b08d33 100644
--- a/attr.h
+++ b/attr.h
@@ -18,7 +18,6 @@ extern const char git_attr__false[];
#define ATTR_TRUE(v) ((v) == git_attr__true)
#define ATTR_FALSE(v) ((v) == git_attr__false)
#define ATTR_UNSET(v) ((v) == NULL)
-#define ATTR_CUSTOM(v) (!(ATTR_UNSET(v) || ATTR_FALSE(v) || ATTR_TRUE(v)))
/*
* Send one or more git_attr_check to git_check_attr(), and
diff --git a/dir.c b/dir.c
index 51d5965..fd39f92 100644
--- a/dir.c
+++ b/dir.c
@@ -209,25 +209,56 @@ int within_depth(const char *name, int namelen,
return 1;
}
-void load_labels(const char *name, int namelen, struct string_list *list)
+static int has_all_labels(const char *name, int namelen,
+ const struct string_list *required_labels)
{
static struct git_attr *attr;
+
struct git_attr_check check;
- char *path = xmemdupz(name, namelen);
+ char *path;
+ int ret;
if (!attr)
attr = git_attr("label");
check.attr = attr;
+ path = xmemdupz(name, namelen);
if (git_check_attr(path, 1, &check))
- die("git_check_attr died");
-
- if (ATTR_CUSTOM(check.value)) {
- string_list_split(list, check.value, ',', -1);
- string_list_sort(list);
+ die("internal bug: git_check_attr died.");
+
+ if (ATTR_TRUE(check.value))
+ ret = 1; /* has all the labels */
+ else if (ATTR_FALSE(check.value)) {
+ warning(_("Path '%s': Label must not be false. Treat as if no
label was set."), path);
+ ret = 0;
+ } else if (ATTR_UNSET(check.value))
+ ret = 0; /* has no labels */
+ else {
+ struct string_list_item *si;
+ struct string_list attr_labels = STRING_LIST_INIT_DUP;
+ string_list_split(&attr_labels, check.value, ',', -1);
+ for_each_string_list_item(si, &attr_labels) {
+ if (validate_label_name(si->string)) {
+ warning(_("Ignoring label '%s'"), si->string);
+ free(si->string);
+ si->string = "";
+ }
+ }
+ string_list_remove_empty_items(&attr_labels, 0);
+ string_list_sort(&attr_labels);
+ ret = 1;
+ for_each_string_list_item(si, required_labels) {
+ if (!string_list_has_string(&attr_labels, si->string)) {
+ ret = 0;
+ break;
+ }
+ }
+ string_list_clear(&attr_labels, 0);
}
[PATCHv4 2/5] Documentation: correct typo in example for querying attributes
The introductory text of the example has a typo in the specification which makes it harder to follow that example. Signed-off-by: Stefan Beller --- Documentation/technical/api-gitattributes.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt index 2602668..36fc9af 100644 --- a/Documentation/technical/api-gitattributes.txt +++ b/Documentation/technical/api-gitattributes.txt @@ -61,7 +61,7 @@ Querying Specific Attributes Example --- -To see how attributes "crlf" and "indent" are set for different paths. +To see how attributes "crlf" and "ident" are set for different paths. . Prepare an array of `struct git_attr_check` with two elements (because we are checking two attributes). Initialize their `attr` member with -- 2.8.2.401.gb49ffe6.dirty -- 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] difftool: initialize variables for readability
The code always goes into one of the two conditional blocks but make it
clear that not doing so is an error condition by setting $ok to 0.
Signed-off-by: David Aguilar
---
git-difftool.perl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 488d14b..8cf0040 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -273,7 +273,7 @@ EOF
# temporary file to both the left and right directories to show the
# change in the recorded SHA1 for the submodule.
for my $path (keys %submodule) {
- my $ok;
+ my $ok = 0;
if (defined($submodule{$path}{left})) {
$ok = write_to_file("$ldir/$path",
"Subproject commit $submodule{$path}{left}");
@@ -289,7 +289,7 @@ EOF
# shows only the link itself, not the contents of the link target.
# This loop replicates that behavior.
for my $path (keys %symlink) {
- my $ok;
+ my $ok = 0;
if (defined($symlink{$path}{left})) {
$ok = write_to_file("$ldir/$path",
$symlink{$path}{left});
--
2.8.2.404.gdfc6a507
--
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
[PATCHv4 1/5] Documentation: fix a typo
Signed-off-by: Stefan Beller --- Documentation/gitattributes.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index e3b1de8..af2c682 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -86,7 +86,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead. Attributes for all users on a system should be placed in the `$(prefix)/etc/gitattributes` file. -Sometimes you would need to override an setting of an attribute +Sometimes you would need to override a setting of an attribute for a path to `Unspecified` state. This can be done by listing the name of the attribute prefixed with an exclamation point `!`. -- 2.8.2.401.gb49ffe6.dirty -- 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
[PATCHv4 4/5] pathspec: move prefix check out of the inner loop
The prefix check is not related the check of pathspec magic; also there
is no code that is relevant after we'd break the loop on a match for
"prefix:". So move the check before the loop and shortcircuit the outer
loop.
Signed-off-by: Stefan Beller
---
pathspec.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index eba37c2..4dff252 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item,
const char *elt,
nextat = copyfrom + len;
if (!len)
continue;
+
+ if (starts_with(copyfrom, "prefix:")) {
+ char *endptr;
+ *pathspec_prefix = strtol(copyfrom + 7,
+ &endptr, 10);
+ if (endptr - copyfrom != len)
+ die(_("invalid parameter for pathspec magic
'prefix'"));
+ continue;
+ }
+
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom, len)) {
*magic |= pathspec_magic[i].bit;
break;
}
- if (starts_with(copyfrom, "prefix:")) {
- char *endptr;
- *pathspec_prefix = strtol(copyfrom + 7,
- &endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for pathspec
magic 'prefix'"));
- /* "i" would be wrong, but it does not matter */
- break;
- }
}
if (ARRAY_SIZE(pathspec_magic) <= i)
die(_("Invalid pathspec magic '%.*s' in '%s'"),
--
2.8.2.401.gb49ffe6.dirty
--
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
[PATCHv4 5/5] pathspec: record labels
The "label" attribute can be attached to paths, and the pathspec
mechanism is extended via the new ":(label=X)pattern/to/match"
syntax to filter paths so that it requires paths to not just
match the given pattern but also have the specified labels
attached for them to be chosen.
Labels are meant to describe arbitrary set of files, which
is not described via the tree layout.
Signed-off-by: Stefan Beller
---
Documentation/gitattributes.txt| 12 +++
Documentation/glossary-content.txt | 4 +
dir.c | 56 +
pathspec.c | 65 ++-
pathspec.h | 2 +
t/t6134-pathspec-with-labels.sh| 158 +
6 files changed, 293 insertions(+), 4 deletions(-)
create mode 100755 t/t6134-pathspec-with-labels.sh
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index af2c682..b846848 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -906,6 +906,18 @@ If this attribute is not set or has an invalid value, the
value of the
(See linkgit:git-config[1]).
+Attaching labels to path
+
+
+`label`
+^^^
+By the value of this attribute you can specify a list of arbitrary strings
+to be attached to a path as labels. These labels can be used for
+easier paths matching using pathspecs (linkgit:gitglossary[1]).
+It is required to use only alphanumeric characters, dashes and
+underscores for the labels.
+
+
USING MACRO ATTRIBUTES
--
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index 8ad29e6..e264a58 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -384,6 +384,10 @@ full pathname may have special meaning:
+
Glob magic is incompatible with literal magic.
+label=;;
+ Additionally to matching the pathspec, the path must have a 'label'
+ attribute having set all labels listed here.
+
exclude;;
After a path matches any non-exclude pathspec, it will be run
through all exclude pathspec (magic signature: `!`). If it
diff --git a/dir.c b/dir.c
index 656f272..fd39f92 100644
--- a/dir.c
+++ b/dir.c
@@ -9,6 +9,7 @@
*/
#include "cache.h"
#include "dir.h"
+#include "attr.h"
#include "refs.h"
#include "wildmatch.h"
#include "pathspec.h"
@@ -208,6 +209,58 @@ int within_depth(const char *name, int namelen,
return 1;
}
+static int has_all_labels(const char *name, int namelen,
+ const struct string_list *required_labels)
+{
+ static struct git_attr *attr;
+
+ struct git_attr_check check;
+ char *path;
+ int ret;
+
+ if (!attr)
+ attr = git_attr("label");
+ check.attr = attr;
+
+ path = xmemdupz(name, namelen);
+ if (git_check_attr(path, 1, &check))
+ die("internal bug: git_check_attr died.");
+
+ if (ATTR_TRUE(check.value))
+ ret = 1; /* has all the labels */
+ else if (ATTR_FALSE(check.value)) {
+ warning(_("Path '%s': Label must not be false. Treat as if no
label was set."), path);
+ ret = 0;
+ } else if (ATTR_UNSET(check.value))
+ ret = 0; /* has no labels */
+ else {
+ struct string_list_item *si;
+ struct string_list attr_labels = STRING_LIST_INIT_DUP;
+ string_list_split(&attr_labels, check.value, ',', -1);
+ for_each_string_list_item(si, &attr_labels) {
+ if (validate_label_name(si->string)) {
+ warning(_("Ignoring label '%s'"), si->string);
+ free(si->string);
+ si->string = "";
+ }
+ }
+ string_list_remove_empty_items(&attr_labels, 0);
+ string_list_sort(&attr_labels);
+ ret = 1;
+ for_each_string_list_item(si, required_labels) {
+ if (!string_list_has_string(&attr_labels, si->string)) {
+ ret = 0;
+ break;
+ }
+ }
+ string_list_clear(&attr_labels, 0);
+ }
+
+ free(path);
+
+ return ret;
+}
+
#define DO_MATCH_EXCLUDE 1
#define DO_MATCH_DIRECTORY 2
@@ -263,6 +316,9 @@ static int match_pathspec_item(const struct pathspec_item
*item, int prefix,
strncmp(item->match, name - prefix, item->prefix))
return 0;
+ if (item->labels && !has_all_labels(name, namelen, item->labels))
+ return 0;
+
/* If the match was just the prefix, we matched */
if (!*match)
return MATCHED_RECURSIVELY;
diff --git a/pathspec.c b/pathspec.c
index 4dff252..2f053fb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,12 +88,42 @@
[PATCH 2/2] difftool: handle unmerged files in dir-diff mode
When files are unmerged they can show up as both unmerged and
modified in the output of `git diff --raw`. This causes
difftool's dir-diff to create filesystem entries for the same
path twice, which fails when it encounters a duplicate path.
Ensure that each worktree path is only processed once.
Add a test to demonstrate the breakage.
Reported-by: Jan Smets
Signed-off-by: David Aguilar
---
git-difftool.perl | 5 +
t/t7800-difftool.sh | 23 +++
2 files changed, 28 insertions(+)
diff --git a/git-difftool.perl b/git-difftool.perl
index 8cf0040..ebd13ba 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -138,6 +138,7 @@ sub setup_dir_diff
my %submodule;
my %symlink;
my @working_tree = ();
+ my %working_tree_dups = ();
my @rawdiff = split('\0', $diffrtn);
my $i = 0;
@@ -188,6 +189,10 @@ EOF
}
if ($rmode ne $null_mode) {
+ # Avoid duplicate working_tree entries
+ if ($working_tree_dups{$dst_path}++) {
+ next;
+ }
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
$dst_path, $rsha1);
if ($use) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ff7a9e9..7ce4cd7 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -419,6 +419,29 @@ run_dir_diff_test 'difftool --dir-diff when worktree file
is missing' '
grep file2 output
'
+run_dir_diff_test 'difftool --dir-diff with unmerged files' '
+ test_when_finished git reset --hard &&
+ test_config difftool.echo.cmd "echo ok" &&
+ git checkout -B conflict-a &&
+ git checkout -B conflict-b &&
+ git checkout conflict-a &&
+ echo a >>file &&
+ git add file &&
+ git commit -m conflict-a &&
+ git checkout conflict-b &&
+ echo b >>file &&
+ git add file &&
+ git commit -m conflict-b &&
+ git checkout master &&
+ git merge conflict-a &&
+ test_must_fail git merge conflict-b &&
+ cat >expect <<-EOF &&
+ ok
+ EOF
+ git difftool --dir-diff $symlinks -t echo >actual &&
+ test_cmp expect actual
+'
+
write_script .git/CHECK_SYMLINKS <<\EOF
for f in file file2 sub/sub
do
--
2.8.2.404.gdfc6a507
--
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 54/94] builtin/apply: make parse_chunk() return a negative integer on error
On Mon, May 16, 2016 at 5:04 AM, Eric Sunshine wrote:
> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
> wrote:
>> To libify `git apply` functionality we have to signal errors to the
>> caller instead of die()ing or exit()ing.
>>
>> To do that in a compatible manner with the rest of the error handling
>> in builtin/apply.c, find_header() should return -1 instead of calling
>> die() or exit().
>
> Why is this talking about making find_header() return -1? Didn't that
> happen in the previous patch?
>
>> As parse_chunk() is called only by apply_patch() which already
>> returns -1 when an error happened, let's make apply_patch() return -1
>> when parse_chunk() returns -1.
>>
>> If find_header() returns -2 because no patch header has been found, it
>> is ok for parse_chunk() to also return -2. If find_header() returns -1
>> because an error happened, it is ok for parse_chunk() to do the same.
>>
>> Helped-by: Eric Sunshine
>> Signed-off-by: Christian Couder
>> ---
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> @@ -2176,8 +2176,9 @@ static int parse_chunk(struct apply_state *state, char
>> *buffer, unsigned long si
>> * empty to us here.
>> */
>> if ((state->apply || state->check) &&
>> - (!patch->is_binary && !metadata_changes(patch)))
>> - die(_("patch with only garbage at line %d"),
>> state->linenr);
>> + (!patch->is_binary && !metadata_changes(patch))) {
>> + return error(_("patch with only garbage at line
>> %d"), state->linenr);
>> + }
>
> Unnecessary braces.
Ok, will remove.
--
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 5/5] pathspec: record labels
Stefan Beller writes: >> I am NOT suggesting to make this enhancement in the prototype to >> allow us experiment with submodule selection use case, but this is >> an obvious place to allow >> >> :(label=A B):(label=C D) >> >> to mean ((A & B) | (C & D)) by making item->labels an array of set >> of labels. > > This is what already works with the series. Or rather: > > ":(label=A B)" ":(label=C D)" > > works as you would expect for (A&B) | (C&D). That is "duplicationg path". I was envisioning a single ":(label=A B):(label=C D)tediously/long/path/because/java/" a shorter and sweeter way to say your two pathspec variant, i.e. ":(label=A B)tediously/long/path/because/java/" \ ":(label=C D)tediously/long/path/because/java/" -- 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 5/5] pathspec: record labels
Stefan Beller writes:
> So "warn and ignore" for data from .gitattributes and die for
> commandline arguments? That makes sense.
Yes.
On the "command line" front, because we may want to give different
meanings to these two entries in the future:
:(label=-doc)Documentation/
:(label=!doc)Documentation/
we should diagnose -doc (FALSE) as an error, not treating it as the
same as !doc (UNSET). And we should warn and ignore -doc (FALSE) in
.gitattributes. Yes, ignoring it would be more or less equivalent
to treating it as UNSET, but because we may use -doc (FALSE) for a
better purpose later, we should still warn.
> Ok, so here is the warn-and-ignore code:
>
>
> if (ATTR_TRUE(check.value))
> ret = 1; /* has all the labels */
> else if (ATTR_FALSE(check.value)) {
> warning(_("Path '%s': Label must not be false. Treat
> as if no label was set"), path);
> ret = 0;
s/Treat as if .../The -label may be used differently in future
versions of Git, so do not use it/;
But if we are going in the direction of :(attr:crlf=auto), all this
discussion is moot, isn't it? I haven't formed a firm opinion on
this, but it sure does sound tempting, doesn't it?
--
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 0/6] modernize t1500
Eric Sunshine writes: > On Tue, May 10, 2016 at 2:26 PM, Junio C Hamano wrote: >> Junio C Hamano writes: >>> Junio C Hamano writes: >>> Subject: [PATCH 7/6] t1500: finish preparation upfront >>> >>> The earlier tests do not attempt to modify the contents of .git (by >>> creating objects or refs, for example), which means that even if >>> some earlier tests before "cp -R" fail, the tests in repo.git will >>> run in an environment that we can expect them to succeed in. >>> >>> Make it clear that tests in repo.git will be independent from the >>> results of earlier tests done in .git by moving its initialization >>> "cp -R .git repo.git" much higher in the test sequence. >>> >>> Signed-off-by: Junio C Hamano >>> --- >> >> I think the same logic applies to other preparatory things like >> creation of sub/dir in the working tree etc. > > Hmm, so are you suggesting a single 'setup' test at the start of > script which does the 'cp -R' and creates those other directories > needed by later tests? Exactly. -- 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 5/5] pathspec: record labels
On Mon, May 16, 2016 at 11:52 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> So "warn and ignore" for data from .gitattributes and die for
>> commandline arguments? That makes sense.
>
> Yes.
>
> On the "command line" front, because we may want to give different
> meanings to these two entries in the future:
>
> :(label=-doc)Documentation/
> :(label=!doc)Documentation/
Yes but both of these already errors out with:
Label -doc': Label names must start with an
alphabetic character and use only alphanumeric
characters, dashes and underscores.
fatal: Labels in the wrong syntax are prohibited.
The command line arguments are not parsed via the attr system.
Only data from .gitattributes are parsed via the attr system.
I can add tests for those to fail though.
>
> we should diagnose -doc (FALSE) as an error, not treating it as the
> same as !doc (UNSET). And we should warn and ignore -doc (FALSE) in
> .gitattributes. Yes, ignoring it would be more or less equivalent
> to treating it as UNSET, but because we may use -doc (FALSE) for a
> better purpose later, we should still warn.
>
>> Ok, so here is the warn-and-ignore code:
>>
>>
>> if (ATTR_TRUE(check.value))
>> ret = 1; /* has all the labels */
>> else if (ATTR_FALSE(check.value)) {
>> warning(_("Path '%s': Label must not be false. Treat
>> as if no label was set"), path);
>> ret = 0;
>
> s/Treat as if .../The -label may be used differently in future
> versions of Git, so do not use it/;
"... but for now Git treats it as if it is not set at all" is a valuable
information to the user, still?
That code path is only used for data coming from .gitattributes,
so we can bike shed the best warning message.
>
> But if we are going in the direction of :(attr:crlf=auto), all this
> discussion is moot, isn't it? I haven't formed a firm opinion on
> this, but it sure does sound tempting, doesn't it?
>
That direction sounds scary as people may use any sort of labels, so
we can no longer add labels later on sanely.
:(attr:random-attr=foo)
should also fall into the "warn and ignore" space. We only want to allow
:(attr:known-attr)
:(attr:label=..)
instead of label= we may want to allow a little bit more, such as
abbreviated 'l='
or 'group=', but not any sort of labeling.
--
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 5/5] pathspec: record labels
Stefan Beller writes: > "... but for now Git treats it as if it is not set at all" is a valuable > information to the user, still? Not at all. "What you are using is wrong and there is no guarantee what behaviour you would get" is the message we need to convey. -- 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 5/5] pathspec: record labels
On Mon, May 16, 2016 at 12:08 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> "... but for now Git treats it as if it is not set at all" is a valuable >> information to the user, still? > > Not at all. "What you are using is wrong and there is no guarantee > what behaviour you would get" is the message we need to convey. "Path '%s': Ignoring label set to false; This may behave differently in future versions of Git." Maybe even with s/may/will/ I think that conveys both the message about the future as well as "what happened just now in that command" -- 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
Best Practices with code/build fixes post-merge?
Sometimes, I merge 2 branches that have deviated quite a bit. A worst-case example would be some API change. The topic branch (long-lived) may start using the old API. However, once I merge the topic back to master, that API no longer exists. As such, every place that introduces a usage of the old API will fail to build (but won't necessarily cause a conflict during a merge). Concerning best practices, which of the following is better? 1. Make the fixes (which may be vast), smoke test, get a general feel that everything is working on master again. Amend the changes to the previous merge commit. 2. Make the fixes as in step 1, but instead of amending to the merge commit, create a new descendant commit representing the changes. Concerns I see with either choice: 1. Pros: Changes are atomic. Cons: merge commits are typically very difficult to view, especially from logs. For example, `git log --name-status` does nothing for merge commits, so it makes it less-intuitive to get a good overview of changes. 2. Opposite of #1: Changes are not atomic (con), but it makes it very clear that these changes were made "on top of" the merge (pro) Is there a best practice here? What do each of you do as a personal preference or policy? Thanks in advance. -- 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 5/5] pathspec: record labels
Stefan Beller writes: > "Path '%s': Ignoring label set to false; This may behave > differently in future versions of Git." I agree that mentioning "ignoring" is good enough. I think our recent messages begin with lowercase, though. -- 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: Best Practices with code/build fixes post-merge?
Robert Dailey writes: > Sometimes, I merge 2 branches that have deviated quite a bit. A > worst-case example would be some API change. The topic branch > (long-lived) may start using the old API. However, once I merge the > topic back to master, that API no longer exists. As such, every place > that introduces a usage of the old API will fail to build (but won't > necessarily cause a conflict during a merge). > ... > Is there a best practice here? What do each of you do as a personal > preference or policy? Thanks in advance. I think the best practice is to make sure your "some API change" is done in a way competent people do. Namely, have a thin wrapper to emulate the old API so that such a long-lived topic that adds new caller of old API would still be syntactically and semantically correct after getting textually merged. Your developers however are not perfect (nobody is), so the next best is to arrange that a build fails (which seems to be what you have), so that you can notice at the merge time. If the adjustment of an application that uses the old API to use the new one is simple enough, amending the merge would be my preference, as a merge commit should be "a single atomic change" as any other commit. But at the same time, you do not have to be too worried about such a "mismerge". A merge commit is a change that can introduce a bug as any other commit. After all, we are dealing with human endeavours. So "I did this merge as a single atomic change, merging the work done in the topic while adjusting the way the topic did certain things using old API to use new API, to the best of my ability, but I may have introduced a bug" is perfectly fine. You do a commit to ensure it is correct to the best of your ability, and because everybody knows you are not perfect (nobody is), nobody expects your commit is always correct. If you find a bug later, you just fix up with a follow-up commit. A merge commit is no special. My personal preference, if adjustment from old API to new API is more involved, is to have a new topic branch that is forked from the trunk and merge the topic with new call sites of old API into it. I'd fix as much breakage in that merge commit, and then fix things up as necessary in follow up commits on that new topic branch as necessary. After that, merging the result to the trunk. -- 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 02/12] attr.c: use strchrnul() to scan for one line
Signed-off-by: Junio C Hamano
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index eec5d7d..45aec1b 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char
*path, int macro_ok)
for (sp = buf; *sp; ) {
char *ep;
int more;
- for (ep = sp; *ep && *ep != '\n'; ep++)
- ;
+
+ ep = strchrnul(sp, '\n');
more = (*ep == '\n');
*ep = '\0';
handle_attr_line(res, sp, path, ++lineno, macro_ok);
--
2.8.2-748-gfb85f76
--
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 00/12] revamping git_check_attr() API
A common pattern to check N attributes for many paths is to (1) prepare an array A of N git_attr_check_elem items; (2) call git_attr() to intern the N attribute names and fill A; (3) repeatedly call git_check_attrs() for path with N and A; A look-up for these N attributes for a single path P scans the entire attr_stack, starting from the .git/info/attributes file and then .gitattributes file in the directory the path P is in, going upwards to find .gitattributes file found in parent directories. An earlier commit 06a604e6 (attr: avoid heavy work when we know the specified attr is not defined, 2014-12-28) tried to optimize out this scanning for one trivial special case: when the attribute being sought is known not to exist, we do not have to scan for it. While this may be a cheap and effective heuristic, it would not work well when N is (much) more than 1. What we would want is a more customized way to skip irrelevant entries in the attribute stack, and the definition of irrelevance is tied to the set of attributes passed to git_check_attrs() call, i.e. the set of attributes being sought. The data necessary for this optimization needs to live alongside the set of attributes, but a simple array of git_attr_check_elem simply does not have any place for that. Introduce "struct git_attr_check" that contains N, the number of attributes being sought, and A, the array that holds N git_attr_check_elem items, and a function git_check_attr() that takes a path P and this structure as its parameters. This structure can later be extended to hold extra data necessary for optimization. Side Note: While in this series, I am not interested in specifying the exact optimization, it would help readers to give what is envisioned. Even when the caller is interested in only a few attributes (say, "diff", "text" and "group-doc"), we'd walk the attribute entries in various .gitattributes files, perform the pattern match and collect attributes for the path. An earlier commit 06a604e6 (attr: avoid heavy work when we know the specified attr is not defined, 2014-12-28) tried to optimize out this scanning for one trivial special case: where an asked-for attribute does not appear anywhere in these files, we know we do not have to scan at all. We can do much better. We can scan these attribute entries that came from .gitattributes files once and mark the ones that affect one or more of the attributes we know the caller is interested in. Then we only go through the marked entries. E.g. if the caller is interested in "diff", "text" and "group-doc" attributes, we can mark entries that talks about any of "diff", "text", "group-doc" and "binary" attributes. The last one is because it is an attribute macro that expands to affect "diff" and "text" (these attribute are unset by having "binary"). The way git_attr_check() API is structured expects that the caller expresses the set of attributes it is interested in upfront, and reuses to ask the same question about many paths, and it is already optimized for the case where it does so in in-order recursive descent of a tree hierarchy by having an attr_stack that reads, pre-parses and caches contents of the .gitattributes files found in each directory hierarchy. The cached optimization data that sits in "struct git_attr_check" would be effective for this expected access pattern. The optimization data cannot be file-scope static to attr.c, even though that might be easier to implement, in anticipation of having more than one "struct git_attr_check" working alternatingly, e.g. having more than one pathspec with ":(attr=X)" pathspec magic, i.e. git cmd ":(attr=X)dir/" ":(attr=Y)dir/ectory/" Each pathspec element is expected to keep its own "struct git_attr_check" in the custom data to support pathspec magic to match with attributes, and the optimization data kept there would survive repeated calls to git_check_attr() for the same path with different set of attributes. The patches in the earliest part of the series have been sent to the list already; there is no substantial change (I think I made a typofix in the commit log message found by Eric). commit.c: use strchrnul() to scan for one line attr.c: use strchrnul() to scan for one line attr.c: update a stale comment on "struct match_attr" attr.c: explain the lack of attr-name syntax check in parse_attr() attr.c: complete a sentence in a comment attr.c: mark where #if DEBUG ends more clearly attr.c: simplify macroexpand_one() Step 8 is to make sure I can catch all the existing callers and update the API incrementally. attr: rename function and struct related to checking attributes Step 9 is the most important one. By updating the API to allow the callers hold piece of information more than just a simple array of pair, it paves the way to add data
[PATCH v2 01/12] commit.c: use strchrnul() to scan for one line
Signed-off-by: Junio C Hamano
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 3f4f371..1f9ee8a 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const
char **subject)
p++;
if (*p) {
p += 2;
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
} else
eol = p;
--
2.8.2-748-gfb85f76
--
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 03/12] attr.c: update a stale comment on "struct match_attr"
When 82dce998 (attr: more matching optimizations from .gitignore,
2012-10-15) changed a pointer to a string "*pattern" into an
embedded "struct pattern" in struct match_attr, it forgot to update
the comment that describes the structure.
Signed-off-by: Junio C Hamano
---
attr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 45aec1b..4ae7801 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
* If is_macro is true, then u.attr is a pointer to the git_attr being
* defined.
*
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies. (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
*
* In either case, num_attr is the number of attributes affected by
* this rule, and state is an array listing them. The attributes are
--
2.8.2-748-gfb85f76
--
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 05/12] attr.c: complete a sentence in a comment
Signed-off-by: Junio C Hamano
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 05db667..a7f2c3f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line,
const char *src,
* directory (again, reading the file from top to bottom) down to the
* current directory, and then scan the list backwards to find the first match.
* This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
*/
static struct attr_stack {
--
2.8.2-748-gfb85f76
--
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 04/12] attr.c: explain the lack of attr-name syntax check in parse_attr()
Signed-off-by: Junio C Hamano
---
attr.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 4ae7801..05db667 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno,
const char *cp,
return NULL;
}
} else {
+ /*
+* As this function is always called twice, once with
+* e == NULL in the first pass and then e != NULL in
+* the second pass, no need for invalid_attr_name()
+* check here.
+*/
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
--
2.8.2-748-gfb85f76
--
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 09/12] attr: (re)introduce git_check_attr() and struct git_attr_check
A common pattern to check N attributes for many paths is to
(1) prepare an array A of N git_attr_check_elem items;
(2) call git_attr() to intern the N attribute names and fill A;
(3) repeatedly call git_check_attrs() for path with N and A;
A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.
An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it. While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.
What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought. The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.
Introduce "struct git_attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
git_attr_check_elem items, and a function git_check_attr() that
takes a path P and this structure as its parameters. This structure
can later be extended to hold extra data necessary for optimization.
Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.
As an illustration of this new API, convert archive.c that asks for
export-subst and export-ignore attributes for each paths.
Signed-off-by: Junio C Hamano
---
archive.c | 24 ++--
attr.c| 34 ++
attr.h| 9 +
3 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/archive.c b/archive.c
index 0f6acc5..7779af1 100644
--- a/archive.c
+++ b/archive.c
@@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
}
-static void setup_archive_check(struct git_attr_check_elem *check)
-{
- static struct git_attr *attr_export_ignore;
- static struct git_attr *attr_export_subst;
-
- if (!attr_export_ignore) {
- attr_export_ignore = git_attr("export-ignore");
- attr_export_subst = git_attr("export-subst");
- }
- check[0].attr = attr_export_ignore;
- check[1].attr = attr_export_subst;
-}
-
struct directory {
struct directory *up;
struct object_id oid;
@@ -123,7 +110,7 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check_elem check[2];
+ static struct git_attr_check *check;
const char *path_without_prefix;
int err;
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- setup_archive_check(check);
- if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
- if (ATTR_TRUE(check[0].value))
+ if (!check)
+ check = git_attr_check_initl("export-ignore", "export-subst",
NULL);
+ if (!git_check_attr(path_without_prefix, check)) {
+ if (ATTR_TRUE(check->check[0].value))
return 0;
- args->convert = ATTR_TRUE(check[1].value);
+ args->convert = ATTR_TRUE(check->check[1].value);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index 8aa346c..285fc58 100644
--- a/attr.c
+++ b/attr.c
@@ -825,3 +825,37 @@ void git_attr_set_direction(enum git_attr_direction new,
struct index_state *ist
drop_attr_stack();
use_index = istate;
}
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+ return git_check_attrs(path, check->check_nr, check->check);
+}
+
+struct git_attr_check *git_attr_check_initl(const char *one, ...)
+{
+ struct git_attr_check *check;
+ int cnt;
+ va_list params;
+ const char *param;
+
+ va_start(params, one);
+ for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+ ;
+ va_end(params);
+ check = xcalloc(1,
+ sizeof(*check) + cnt * sizeof(*(check->check)));
+ check->check_nr = cnt;
+ check->check = (struct git_attr_check_elem *)(check + 1)
[PATCH v2 07/12] attr.c: simplify macroexpand_one()
The double-loop wants to do an early return immediately when one
matching macro is found. Eliminate the extra variable 'a' used for
that purpose and rewrite the "assign the found item to 'a' to make
it non-NULL and force the loop(s) to terminate" with a direct return
from there.
Signed-off-by: Junio C Hamano
---
attr.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 95416d3..7bfeef3 100644
--- a/attr.c
+++ b/attr.c
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int
basename_offset,
static int macroexpand_one(int nr, int rem)
{
struct attr_stack *stk;
- struct match_attr *a = NULL;
int i;
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
}
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
return rem;
}
--
2.8.2-748-gfb85f76
--
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 08/12] attr: rename function and struct related to checking attributes
The traditional API to check attributes is to prepare an N-element
array of "struct git_attr_check" and pass N and the array to the
function "git_check_attr()" as arguments.
In preparation to revamp the API to pass a single structure, in
which these N elements are held, rename the type used for these
individual array elements to "struct git_attr_check_elem" and rename
the function to "git_check_attrs()".
Signed-off-by: Junio C Hamano
---
archive.c | 6 +++---
attr.c | 12 ++--
attr.h | 8
builtin/check-attr.c | 19 ++-
builtin/pack-objects.c | 6 +++---
convert.c | 12 ++--
ll-merge.c | 10 +-
userdiff.c | 4 ++--
ws.c | 6 +++---
9 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/archive.c b/archive.c
index 5d735ae..0f6acc5 100644
--- a/archive.c
+++ b/archive.c
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
}
-static void setup_archive_check(struct git_attr_check *check)
+static void setup_archive_check(struct git_attr_check_elem *check)
{
static struct git_attr *attr_export_ignore;
static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check check[2];
+ struct git_attr_check_elem check[2];
const char *path_without_prefix;
int err;
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
path_without_prefix = path.buf + args->baselen;
setup_archive_check(check);
- if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+ if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
if (ATTR_TRUE(check[0].value))
return 0;
args->convert = ATTR_TRUE(check[1].value);
diff --git a/attr.c b/attr.c
index 7bfeef3..8aa346c 100644
--- a/attr.c
+++ b/attr.c
@@ -40,7 +40,7 @@ struct git_attr {
static int attr_nr;
static int cannot_trust_maybe_real;
-static struct git_attr_check *check_all_attr;
+static struct git_attr_check_elem *check_all_attr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
char *git_attr_name(struct git_attr *attr)
@@ -661,7 +661,7 @@ static int macroexpand_one(int attr_nr, int rem);
static int fill_one(const char *what, struct match_attr *a, int rem)
{
- struct git_attr_check *check = check_all_attr;
+ struct git_attr_check_elem *check = check_all_attr;
int i;
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -726,7 +726,7 @@ static int macroexpand_one(int nr, int rem)
* collected. Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, int num,
- struct git_attr_check *check)
+ struct git_attr_check_elem *check)
{
struct attr_stack *stk;
@@ -754,7 +754,7 @@ static void collect_some_attrs(const char *path, int num,
rem = 0;
for (i = 0; i < num; i++) {
if (!check[i].attr->maybe_real) {
- struct git_attr_check *c;
+ struct git_attr_check_elem *c;
c = check_all_attr + check[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
@@ -769,7 +769,7 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-int git_check_attr(const char *path, int num, struct git_attr_check *check)
+int git_check_attrs(const char *path, int num, struct git_attr_check_elem
*check)
{
int i;
@@ -785,7 +785,7 @@ int git_check_attr(const char *path, int num, struct
git_attr_check *check)
return 0;
}
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem
**check)
{
int i, count, j;
diff --git a/attr.h b/attr.h
index 8b08d33..cab82ec 100644
--- a/attr.h
+++ b/attr.h
@@ -20,11 +20,11 @@ extern const char git_attr__false[];
#define ATTR_UNSET(v) ((v) == NULL)
/*
- * Send one or more git_attr_check to git_check_attr(), and
+ * Send one or more git_attr_check to git_check_attrs(), and
* each 'value' member tells what its value is.
* Unset one is returned as NULL.
*/
-struct git_attr_check {
+struct git_attr_check_elem {
struct git_attr *attr;
const char *value;
};
@@ -36,7 +36,7 @@ struct git_attr_check {
*/
char *git_attr_name(struct git_attr *);
-int git_che
[PATCH v2 06/12] attr.c: mark where #if DEBUG ends more clearly
Signed-off-by: Junio C Hamano
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index a7f2c3f..95416d3 100644
--- a/attr.c
+++ b/attr.c
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match,
struct git_attr *attr
#define debug_push(a) do { ; } while (0)
#define debug_pop(a) do { ; } while (0)
#define debug_set(a,b,c,d) do { ; } while (0)
-#endif
+#endif /* DEBUG_ATTR */
static void drop_attr_stack(void)
{
--
2.8.2-748-gfb85f76
--
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 10/12] attr: convert git_all_attrs() to use "struct git_attr_check"
This updates the other two ways the attribute check is done via an
array of "struct git_attr_check_elem" elements. These two niches
appear only in "git check-attr".
* The caller does not know offhand what attributes it wants to ask
about and cannot use git_attr_check_initl() to prepare the
git_attr_check structure.
* The caller may not know what attributes it wants to ask at all,
and instead wants to learn everything that the given path has.
Such a caller can call git_attr_check_alloc() to allocate an empty
git_attr_check, and then call git_attr_check_append() to add
attribute names one by one. A new attribute can be appended until
git_attr_check structure is "finalized", which happens when it is
used to ask for attributes for any path by calling git_check_attr()
or git_all_attrs(). A git_attr_check structure that is initialized
by git_attr_check_initl() is already finalized when it is returned.
I am not at all happy with the way git_all_attrs() API turned out to
be, but it is only to support one niche caller ("check-attr --all"),
so I'll stop here for now.
Signed-off-by: Junio C Hamano
---
attr.c | 70 +---
attr.h | 16 +++-
builtin/check-attr.c | 54 ++--
3 files changed, 84 insertions(+), 56 deletions(-)
diff --git a/attr.c b/attr.c
index 285fc58..9c187bc 100644
--- a/attr.c
+++ b/attr.c
@@ -785,32 +785,21 @@ int git_check_attrs(const char *path, int num, struct
git_attr_check_elem *check
return 0;
}
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem
**check)
+void git_all_attrs(const char *path, struct git_attr_check *check)
{
- int i, count, j;
+ int i;
+ git_attr_check_clear(check);
collect_some_attrs(path, 0, NULL);
- /* Count the number of attributes that are set. */
- count = 0;
- for (i = 0; i < attr_nr; i++) {
- const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
- ++count;
- }
- *num = count;
- ALLOC_ARRAY(*check, count);
- j = 0;
for (i = 0; i < attr_nr; i++) {
+ const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
- if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
- (*check)[j].attr = check_all_attr[i].attr;
- (*check)[j].value = value;
- ++j;
- }
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ git_attr_check_append(check, name);
+ check->check[check->check_nr - 1].value = value;
}
-
- return 0;
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state
*istate)
@@ -828,6 +817,7 @@ void git_attr_set_direction(enum git_attr_direction new,
struct index_state *ist
int git_check_attr(const char *path, struct git_attr_check *check)
{
+ check->finalized = 1;
return git_check_attrs(path, check->check_nr, check->check);
}
@@ -845,17 +835,57 @@ struct git_attr_check *git_attr_check_initl(const char
*one, ...)
check = xcalloc(1,
sizeof(*check) + cnt * sizeof(*(check->check)));
check->check_nr = cnt;
+ check->finalized = 1;
check->check = (struct git_attr_check_elem *)(check + 1);
check->check[0].attr = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
+ struct git_attr *attr;
param = va_arg(params, const char *);
if (!param)
die("BUG: counted %d != ended at %d",
check->check_nr, cnt);
- check->check[cnt].attr = git_attr(param);
+ attr = git_attr(param);
+ if (!attr)
+ die("BUG: %s: not a valid attribute name", param);
+ check->check[cnt].attr = attr;
}
va_end(params);
return check;
}
+
+struct git_attr_check *git_attr_check_alloc(void)
+{
+ return xcalloc(1, sizeof(struct git_attr_check));
+}
+
+void git_attr_check_append(struct git_attr_check *check, const char *name)
+{
+ struct git_attr *attr;
+
+ if (check->finalized)
+ die("BUG: append after git_attr_check structure is finalized");
+
+ attr = git_attr(name);
+ if (!attr)
+ die("%s: not a valid attribute name", name);
+ ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
+ check->check[check->check_nr++].attr = attr;
+}
+
+void git_attr_check_clear(struct git_attr_check *check)
+{
+ if ((void *)(check->check) == (void *)(check + 1))
+ die("BUG: clearing a statically initialized
[PATCH v2 12/12] attr: retire git_check_attrs() API
Since nobody uses the old API, make it file-scope static, and update
the documentation to describe the new API.
Signed-off-by: Junio C Hamano
---
Documentation/technical/api-gitattributes.txt | 62 +++
attr.c| 3 +-
attr.h| 2 -
3 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/Documentation/technical/api-gitattributes.txt
b/Documentation/technical/api-gitattributes.txt
index 2602668..6f13f94 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
of no interest to the calling programs. The name of the
attribute can be retrieved by calling `git_attr_name()`.
+`struct git_attr_check_elem`::
+
+ This structure represents one attribute and its value.
+
`struct git_attr_check`::
- This structure represents a set of attributes to check in a call
- to `git_check_attr()` function, and receives the results.
+ This structure represents a collection of `git_attr_check_elem`.
+ It is passed to `git_check_attr()` function, specifying the
+ attributes to check, and receives their values.
Attribute Values
@@ -48,49 +53,47 @@ value of the attribute for the path.
Querying Specific Attributes
-* Prepare an array of `struct git_attr_check` to define the list of
- attributes you would want to check. To populate this array, you would
- need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct git_attr_check` using git_attr_check_initl()
+ function, enumerating the names of attributes whose values you are
+ interested in, terminated with a NULL pointer.
* Call `git_check_attr()` to check the attributes for the path.
-* Inspect `git_attr_check` structure to see how each of the attribute in
- the array is defined for the path.
+* Inspect `git_attr_check` structure to see how each of the
+ attribute in the array is defined for the path.
Example
---
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
-. Prepare an array of `struct git_attr_check` with two elements (because
- we are checking two attributes). Initialize their `attr` member with
- pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct git_attr_check` with two elements (because
+ we are checking two attributes):
-static struct git_attr_check check[2];
+static struct git_attr_check *check;
static void setup_check(void)
{
- if (check[0].attr)
+ if (check)
return; /* already done */
- check[0].attr = git_attr("crlf");
- check[1].attr = git_attr("ident");
+ check = git_attr_check_initl("crlf", "ident", NULL);
}
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct git_attr_check`:
const char *path;
setup_check();
- git_check_attr(path, ARRAY_SIZE(check), check);
+ git_check_attr(path, check);
-. Act on `.value` member of the result, left in `check[]`:
+. Act on `.value` member of the result, left in `check->check[]`:
- const char *value = check[0].value;
+ const char *value = check->check[0].value;
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -115,14 +118,17 @@ Querying All Attributes
To get the values of all attributes associated with a file:
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
- structures.
+* Prepare an empty `git_attr_check` structure by calling
+ `git_attr_check_alloc()`.
+
+* Call `git_all_attrs()`, which populates the `git_attr_check`
+ with the attributes attached to the path.
-* Iterate over the `git_attr_check` array to examine the attribute
- names and values. The name of the attribute described by a
- `git_attr_check` object can be retrieved via
- `git_attr_name(check[i].attr)`. (Please note that no items will be
- returned for unset attributes, so `ATTR_UNSET()` will return false
- for all returned `git_array_check` objects.)
+* Iterate over the `git_attr_check.check[]` array to examine
+ the attribute names and values. The name of the attribute
+ described by a `git_attr_check.check[]` object can be retrieved via
+ `git_attr_name(check->check[i].attr)`. (Please note that no items
+ will be returned for unset attributes, so `ATTR_UNSET()` will return
+ false for all returned `git_array_check` objects.)
-* Free the `git_array_check` array.
+* Free the `git_array_check` by calling `git_attr_check_free()`.
diff --git a/attr.c b/attr.c
index 9c187bc..c3295a9 100644
--- a/attr.c
+++ b/attr.c
@@ -769,7 +769,8 @@ stat
[PATCH v2 11/12] attr: convert git_check_attrs() callers to use the new API
The remaining callers are all simple "I have N attributes I am
interested in. I'll ask about them with various paths one by one".
After this step, no caller to git_check_attrs() remains. After
removing it, we can extend "struct git_attr_check" struct with data
that can be used in optimizing the query for the specific N
attributes it contains.
Signed-off-by: Junio C Hamano
---
builtin/pack-objects.c | 19 +--
convert.c | 18 +++---
ll-merge.c | 33 ++---
userdiff.c | 19 ---
ws.c | 19 ++-
5 files changed, 40 insertions(+), 68 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 167c301..c6c2a69 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -886,24 +886,15 @@ static void write_pack_file(void)
written, nr_result);
}
-static void setup_delta_attr_check(struct git_attr_check_elem *check)
-{
- static struct git_attr *attr_delta;
-
- if (!attr_delta)
- attr_delta = git_attr("delta");
-
- check[0].attr = attr_delta;
-}
-
static int no_try_delta(const char *path)
{
- struct git_attr_check_elem check[1];
+ static struct git_attr_check *check;
- setup_delta_attr_check(check);
- if (git_check_attrs(path, ARRAY_SIZE(check), check))
+ if (!check)
+ check = git_attr_check_initl("delta", NULL);
+ if (git_check_attr(path, check))
return 0;
- if (ATTR_FALSE(check->value))
+ if (ATTR_FALSE(check->check[0].value))
return 1;
return 0;
}
diff --git a/convert.c b/convert.c
index 058da86..510d1b9 100644
--- a/convert.c
+++ b/convert.c
@@ -760,24 +760,20 @@ struct conv_attrs {
int ident;
};
-static const char *conv_attr_name[] = {
- "crlf", "ident", "filter", "eol", "text",
-};
-#define NUM_CONV_ATTRS ARRAY_SIZE(conv_attr_name)
-
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
- int i;
- static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS];
+ static struct git_attr_check *check;
- if (!ccheck[0].attr) {
- for (i = 0; i < NUM_CONV_ATTRS; i++)
- ccheck[i].attr = git_attr(conv_attr_name[i]);
+ if (!check) {
+ check = git_attr_check_initl("crlf", "ident",
+"filter", "eol", "text",
+NULL);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
- if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attr(path, check)) {
+ struct git_attr_check_elem *ccheck = check->check;
ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_UNDEFINED)
ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index 772b14e..d4380c4 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -334,15 +334,6 @@ static const struct ll_merge_driver
*find_ll_merge_driver(const char *merge_attr
return &ll_merge_drv[LL_TEXT_MERGE];
}
-static int git_path_check_merge(const char *path, struct git_attr_check_elem
check[2])
-{
- if (!check[0].attr) {
- check[0].attr = git_attr("merge");
- check[1].attr = git_attr("conflict-marker-size");
- }
- return git_check_attrs(path, 2, check);
-}
-
static void normalize_file(mmfile_t *mm, const char *path)
{
struct strbuf strbuf = STRBUF_INIT;
@@ -360,7 +351,7 @@ int ll_merge(mmbuffer_t *result_buf,
mmfile_t *theirs, const char *their_label,
const struct ll_merge_options *opts)
{
- static struct git_attr_check_elem check[2];
+ static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -374,10 +365,14 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(ours, path);
normalize_file(theirs, path);
}
- if (!git_path_check_merge(path, check)) {
- ll_driver_name = check[0].value;
- if (check[1].value) {
- marker_size = atoi(check[1].value);
+
+ if (!check)
+ check = git_attr_check_initl("merge", "conflict-marker-size",
NULL);
+
+ if (!git_check_attr(path, check)) {
+ ll_driver_name = check->check[0].value;
+ if (check->check[1].value) {
+ marker_size = atoi(check->check[1].value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
}
@@ -392,13 +387,13 @@ int ll_merge
Re: [RFC PATCH 0/4] pathspec labels [WAS: submodule groups]
Stefan Beller writes: > This is another case for using ':' instead of '='. > So I think ':' is better for this future enhancement. > > Also this future enhancement may ask for > > git ls-files ":(attr:label=foo)" > > or > > git ls-files ":(attr:-label)" > > so the user can circumvent the warn and ignore strategy. :( That is exactly what we want, I would say, if we want to accept "arbitrary set of attributes with their states". The "warn and ignore" comes only from "with '(:label=X Y Z)', we inspect attributes X, Y and Z and insist them to be set to true; it is a configuration error to set the label to anything but a string", and accepting "arbitrary set of attributes with their states" makes it moot (as I said earlier in $gmane/294776). So are we leaning towards ":(attr:)", where is one of var=value var -var !var now? It makes the pathspec magic parser a bit more complex (and I am not sure if we have to worry too much about quoting "value" part to allow arbitrary sequence of bytes), though. -- 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: autostash is lost after aborted rebase
The bug still persists when you abort the rebase by using :cq in Vim (exit with an error code). See also http://stackoverflow.com/q/37252108/244297 -- View this message in context: http://git.661346.n2.nabble.com/bug-autostash-is-lost-after-aborted-rebase-tp7611141p7656556.html Sent from the git mailing list archive at Nabble.com. -- 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 0/4] pathspec labels [WAS: submodule groups]
On Mon, May 16, 2016 at 2:18 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> This is another case for using ':' instead of '='. >> So I think ':' is better for this future enhancement. >> >> Also this future enhancement may ask for >> >> git ls-files ":(attr:label=foo)" >> >> or >> >> git ls-files ":(attr:-label)" >> >> so the user can circumvent the warn and ignore strategy. :( > > That is exactly what we want, I would say, if we want to accept > "arbitrary set of attributes with their states". > > The "warn and ignore" comes only from "with '(:label=X Y Z)', we > inspect attributes X, Y and Z and insist them to be set to true; it > is a configuration error to set the label to anything but a string", > and accepting "arbitrary set of attributes with their states" makes > it moot (as I said earlier in $gmane/294776). > > So are we leaning towards ":(attr:)", where is one of > > var=value > var > -var > !var > > now? It makes the pathspec magic parser a bit more complex (and I > am not sure if we have to worry too much about quoting "value" part > to allow arbitrary sequence of bytes), though. And we want to have both "label=A B C" and attr:label=A B C" or *just* the attr query? We should not allow the user to add arbitrary attributes (i.e. labels). Instead each of the "attr for labeling purposes" needs to follow a clear scheme that allows us to add attributes later on that are outside of that scheme. In a very first implementation we could require the attribute to start with "label=". ;) -- 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 0/4] pathspec labels [WAS: submodule groups]
Stefan Beller writes: > And we want to have both "label=A B C" and attr:label=A B C" or *just* the > attr query? I think the choice at this point is between supporting just "label=A B C" or supporting just "attr:eol=crlf text=auto !diff". I think "attr:label=A" is merely a degenerated case of the latter. > We should not allow the user to add arbitrary attributes (i.e. labels). Hmph, why not? > Instead each of the "attr for labeling purposes" needs to follow a clear > scheme > that allows us to add attributes later on that are outside of that scheme. That was my initial reaction when I saw Duy's "attr:crlf=auto" (by the way, there is no such setting; crlf should be one of TRUE, UNSET or set to string "input") idea. But I do not think of a good argument to justify that arbitrary attributes are not allowed. -- 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] difftool: handle unmerged files in dir-diff mode
Thanks, will queue. -- 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 0/4] pathspec labels [WAS: submodule groups]
On Mon, May 16, 2016 at 2:50 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> And we want to have both "label=A B C" and attr:label=A B C" or *just* the >> attr query? > > I think the choice at this point is between supporting just "label=A > B C" or supporting just "attr:eol=crlf text=auto !diff". > > I think "attr:label=A" is merely a degenerated case of the latter. > >> We should not allow the user to add arbitrary attributes (i.e. labels). Because we cannot extend our attributes any further from that? Consider a user starts using attr for their labeling purposes. Later (in 2 years) we decide that is an attribute we want to use to mark files as foo-ish. so we add meaning to that attribute (just like eol.crlf does an arbitrary thing, foo would do another arbitrary thing). Then the user picks up the new version of Git and expects to still be a "I use it for labeling purposes only" thing. They would not expect to all files labeled as to start behaving ? > Hmph, why not? We need a namespace for which * we can guarantee that it is for labeling purposes only (even in the future) * is obvious to the user to be a labeling name space Starting with "label" offers both? > >> Instead each of the "attr for labeling purposes" needs to follow a clear >> scheme >> that allows us to add attributes later on that are outside of that scheme. > > That was my initial reaction when I saw Duy's "attr:crlf=auto" (by > the way, there is no such setting; crlf should be one of TRUE, UNSET > or set to string "input") idea. But I do not think of a good > argument to justify that arbitrary attributes are not allowed. I agree that querying for attr:eol or attr:diff is a good idea. I do however want to point out that all labels for "labeling purposes" MUST be a clear from the beginning, otherwise we'll have the maintenance problem down the road? -- 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 0/4] pathspec labels [WAS: submodule groups]
Stefan Beller writes: >> Hmph, why not? > > We need a namespace for which > * we can guarantee that it is for labeling purposes only (even in the future) > * is obvious to the user to be a labeling name space > > Starting with "label" offers both? Ah, of course. I thought that you were trying to limit ":(attr:)" magic only to attributes that begin with "label-", which is where my "why not?" comes from. -- 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 0/4] pathspec labels [WAS: submodule groups]
On Mon, May 16, 2016 at 3:02 PM, Junio C Hamano wrote: > Stefan Beller writes: > >>> Hmph, why not? >> >> We need a namespace for which >> * we can guarantee that it is for labeling purposes only (even in the future) >> * is obvious to the user to be a labeling name space >> >> Starting with "label" offers both? > > Ah, of course. I thought that you were trying to limit ":(attr:)" > magic only to attributes that begin with "label-", which is where my > "why not?" comes from. And going by the logic you presented before, we would need to error out for the given pathspec ":()" if * either the string is not well known (e.g. diif, eol ) * or is outside of the labeling namespace. So we don't want to see users complaining about "bug attr:foo worked as a label, now it is a feature; you broke my code" We would need to ignore data from .gitattributes as it may be crafted from a newer version of Git, but the command line argument still needs to die for unknown arguments? So asking for :(foo) would yield a fatal: attr 'foo' is not known to Git, nor is it in the labeling name space I guess what I am asking is if there is a nice way to query "do we know this attribute?" 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: [PATCH v6 00/17] Port branch.c to use ref-filter's printing options
Karthik Nayak writes: > This is part of unification of the commands 'git tag -l, git branch -l > and git for-each-ref'. This ports over branch.c to use ref-filter's > printing options. > > Initially posted here: $(gmane/279226). It was decided that this series > would follow up after refactoring ref-filter parsing mechanism, which > is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c). 9606218b? > Changes in this version: > > 1. Rebased on top of f307218 (t6302: simplify non-gpg cases). Huh? $ git checkout f307218 $ git am 0001-ref-filter-implement-if.txt Applying: ref-filter: implement %(if), %(then), and %(else) atoms error: patch failed: Documentation/git-for-each-ref.txt:181 error: Documentation/git-for-each-ref.txt: patch does not apply Patch failed at 0001 ref-filter: implement %(if), %(then), and %(else) atoms The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". -- 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 0/4] pathspec labels [WAS: submodule groups]
Stefan Beller writes: >> Ah, of course. I thought that you were trying to limit ":(attr:)" >> magic only to attributes that begin with "label-", which is where my >> "why not?" comes from. > > And going by the logic you presented before, we would > need to error out for the given pathspec ":()" if > > * either the string is not well known (e.g. diif, eol ) > * or is outside of the labeling namespace. I do not think that follows _my_ line of thought. What is "well known"? Doesn't that change over time? If we are to do the "attribute match", there is no useful enforcement point. An arbitrary version of Git cannot differentiate a random cruft users will write in their .gitattributes from a legitimate attribute that will be introduced in the future, so both "data in .gitattributes" and "pathspec magic that referes to attribute" cannot be limited by us. So if we are going to take the arbitrary ":(attr:)" route, "label-" prefix would be limitation on the "core Git" side and does not limit what end-user does. We will promise that we won't use names that begin with the prefix ourselves and leave them up to the projects. If the end user uses an attribute "foo", which does not begin with "label-", the end user is risking to be broken by future versions of Git. If you want to compile an authoritative list of attributes used by core Git and maintain it forever, you are welcome to add warning, but I wouldn't bother if I were doing this series, at least at the beginning. -- 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 2/9] connect: only match the host with core.gitProxy
Mike Hommey writes: > Currently, core.gitProxy doesn't actually match purely on domain names > as documented: it also matches ports. > ... > This per-port behavior seems like an oversight rather than a deliberate > choice, so, make git://kernel.org:port/path call the gitProxy script in Hmph. The fact that hostandport, not just host after stripping possible ":port" part, is passed to the function smells like a deliberate design to allow people to use different proxy for different port, so I am not sure everybody agrees with your "seems like an oversight". Don't existing users depend on the behaviour? Isn't the change robbing Peter to pay 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: [PATCH v5 2/9] connect: only match the host with core.gitProxy
On Mon, May 16, 2016 at 03:30:01PM -0700, Junio C Hamano wrote: > Mike Hommey writes: > > > Currently, core.gitProxy doesn't actually match purely on domain names > > as documented: it also matches ports. > > ... > > This per-port behavior seems like an oversight rather than a deliberate > > choice, so, make git://kernel.org:port/path call the gitProxy script in > > Hmph. The fact that hostandport, not just host after stripping > possible ":port" part, is passed to the function smells like a > deliberate design to allow people to use different proxy for > different port, so I am not sure everybody agrees with your "seems > like an oversight". > > Don't existing users depend on the behaviour? Isn't the change > robbing Peter to pay Paul? The gitProxy script gets the port passed. Why would you need different scripts for different ports if the port is passed as an argument? Also, if it's deliberate, it's widely undocumented. Mike -- 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 4/9] connect: make parse_connect_url() return separated host and port
Mike Hommey writes:
> + get_host_and_port(&host, &port);
> +
> + if (*host && !port) {
> + /* The host might contain a user:password string, ignore it
> + * when searching for the port again */
> + char *end_user = strrchr(host, '@');
> + port = get_port(end_user ? end_user : host);
Scanning from the right because host part would never have '@', but
there could be an invalid URL with an unquoted '@' in userinfo part?
Then this makes sense.
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 91a69fc..9acba2b 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -553,7 +553,7 @@ check_prot_path () {
> Diag: protocol=$2
> Diag: path=$3
> EOF
> - git fetch-pack --diag-url "$1" | grep -v hostandport= >actual &&
> + git fetch-pack --diag-url "$1" | grep -v host= | grep -v port= >actual
> &&
A single process:
... | grep -v -e '^host=' -e '^port='
perhaps?
> @@ -562,22 +562,17 @@ check_prot_host_port_path () {
> case "$2" in
> *ssh*)
> pp=ssh
> - uah=userandhost
> - ehost=$(echo $3 | tr -d "[]")
> - diagport="Diag: port=$4"
> ;;
> *)
> - pp=$p
> - uah=hostandport
> - ehost=$(echo $3$4 | sed -e "s/22$/:22/" -e "s/NONE//")
> - diagport=""
> + pp=$2
> ;;
> esac
> + ehost=$(echo $3 | tr -d "[]")
> cat >exp <<-EOF &&
> Diag: url=$1
> Diag: protocol=$pp
> - Diag: $uah=$ehost
> - $diagport
> + Diag: userandhost=$ehost
> + Diag: port=$4
> Diag: path=$5
> EOF
This makes the diag output simpler and allows the caller to expect
the same set of variables, which is good.
--
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 8/9] connect: actively reject git:// urls with a user part
Mike Hommey writes:
> Currently, urls of the for git://user@host don't work because user@host
> is not resolving at the DNS level, but we shouldn't be relying on it
> being an invalid host name, and actively reject it for containing a
> username in the first place.
Makes sense. Connecting to host by stripping user@ would probably
give us a better behaviour, but this is a good first step even if we
are aiming for that endgame state.
>
> Signed-off-by: Mike Hommey
> ---
> connect.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index df15ff3..fdd40b0 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -716,6 +716,9 @@ struct child_process *git_connect(int fd[2], const char
> *url,
>*/
> struct strbuf target_host = STRBUF_INIT;
> char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> + if (user)
> + die("user@host is not allowed in git:// urls");
> +
> if (override_vhost)
> strbuf_addstr(&target_host, override_vhost);
> else {
--
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 4/9] connect: make parse_connect_url() return separated host and port
On Mon, May 16, 2016 at 03:39:08PM -0700, Junio C Hamano wrote:
> Mike Hommey writes:
>
> > + get_host_and_port(&host, &port);
> > +
> > + if (*host && !port) {
> > + /* The host might contain a user:password string, ignore it
> > +* when searching for the port again */
> > + char *end_user = strrchr(host, '@');
> > + port = get_port(end_user ? end_user : host);
>
> Scanning from the right because host part would never have '@', but
> there could be an invalid URL with an unquoted '@' in userinfo part?
> Then this makes sense.
Indeed, I forgot to update the comment after the discussion on this
list.
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 91a69fc..9acba2b 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -553,7 +553,7 @@ check_prot_path () {
> > Diag: protocol=$2
> > Diag: path=$3
> > EOF
> > - git fetch-pack --diag-url "$1" | grep -v hostandport= >actual &&
> > + git fetch-pack --diag-url "$1" | grep -v host= | grep -v port= >actual
> > &&
>
> A single process:
>
> ... | grep -v -e '^host=' -e '^port='
heh, I'm actually replacing it with a single egrep in a subsequent
patch, following feedback from previous round, but missed that there was
this intermediate step with multiple greps still. Do you want an update
anyways, or does only the final result really matter? I guess I can
update it in any case, since I'll have to update the patch for the
comment above anyways.
Mike
--
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 2/9] connect: only match the host with core.gitProxy
Mike Hommey writes: > The gitProxy script gets the port passed. Why would you need different > scripts for different ports if the port is passed as an argument? Also, > if it's deliberate, it's widely undocumented. Fair enough. A user who has been working around thsi "oversight", would have relied on her proxy configured with 'script for myhost.xz:9111' to be called for git://myhost.xz:9111/path, right? We'd need to somehow let her know that her configuration will be broken, but as long as we can find a way to do so to ease the transition, I think the updated "if you want to use a different behaviour depending on port, use the port parameter" is a lot more sensible behaviour than what we had traditionally. -- 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 01/12] commit.c: use strchrnul() to scan for one line
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano
> ---
> commit.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 3f4f371..1f9ee8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const
> char **subject)
> p++;
> if (*p) {
> p += 2;
> - for (eol = p; *eol && *eol != '\n'; eol++)
> - ; /* do nothing */
> + eol = strchrnul(p, '\n');
Nit:
You could include the +2 into the arg of strchrnul,
such that you can drop the braces.
> } else
> eol = p;
>
> --
> 2.8.2-748-gfb85f76
>
> --
> 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
--
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 02/12] attr.c: use strchrnul() to scan for one line
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano
> ---
> attr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index eec5d7d..45aec1b 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char
> *path, int macro_ok)
> for (sp = buf; *sp; ) {
> char *ep;
> int more;
> - for (ep = sp; *ep && *ep != '\n'; ep++)
> - ;
> +
> + ep = strchrnul(sp, '\n');
(Even lesser nit as in patch 1)
You could directly assign ep and more when declaring them.
> more = (*ep == '\n');
> *ep = '\0';
> handle_attr_line(res, sp, path, ++lineno, macro_ok);
> --
> 2.8.2-748-gfb85f76
>
> --
> 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
--
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 03/12] attr.c: update a stale comment on "struct match_attr"
On Mon, May 16, 2016 at 2:05 PM, Junio C Hamano wrote:
> When 82dce998 (attr: more matching optimizations from .gitignore,
> 2012-10-15) changed a pointer to a string "*pattern" into an
> embedded "struct pattern" in struct match_attr, it forgot to update
> the comment that describes the structure.
>
> Signed-off-by: Junio C Hamano
> ---
> attr.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 45aec1b..4ae7801 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -131,9 +131,8 @@ struct pattern {
> * If is_macro is true, then u.attr is a pointer to the git_attr being
> * defined.
> *
> - * If is_macro is false, then u.pattern points at the filename pattern
> - * to which the rule applies. (The memory pointed to is part of the
> - * memory block allocated for the match_attr instance.)
> + * If is_macro is false, then u.pat is the filename pattern to which the
> + * rule applies.
and we don't need to talk about memory ownership here as that
is clear for 'struct pattern' documented elsewhere?
> *
> * In either case, num_attr is the number of attributes affected by
> * this rule, and state is an array listing them. The attributes are
> --
> 2.8.2-748-gfb85f76
>
> --
> 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
--
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 03/12] attr.c: update a stale comment on "struct match_attr"
On Mon, May 16, 2016 at 4:34 PM, Stefan Beller wrote: >> - * If is_macro is false, then u.pattern points at the filename pattern >> - * to which the rule applies. (The memory pointed to is part of the >> - * memory block allocated for the match_attr instance.) >> + * If is_macro is false, then u.pat is the filename pattern to which the >> + * rule applies. > > and we don't need to talk about memory ownership here as that > is clear for 'struct pattern' documented elsewhere? It is embedded in the structure which you cannot lose independently, 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
[Bug] git-log prints wrong unixtime with --date=format:%s
Hello, the command git log --pretty=format:%ad --date=format:%s displays wrong unixtime values; apparently how much the printed value differs from the expected value depends on the system's time zone and whether daylight savings time is enabled or not. Here is a reproducible recipe compiled by Yuri Khan who helped me localizing the problem and whom I CC'd: 0. Versions $ git --version git version 2.8.2 $ dpkg -l tzdata | tail -1 ii tzdata 2016d-0ubuntu0.16.04 all time zone and daylight-saving time data 1. Initialize an empty Git repository: $ git init test $ cd test 2. Make a commit, using the Europe/Berlin time zone: $ TZ=Europe/Berlin git commit -m 'test' --allow-empty 3. Examine the timestamp recorded in the commit object: $ git cat-file -p HEAD | grep author author Yuri Khan 1463260938 +0200 4. Check that it corresponds to the current time: $ date +%s 1463260977 5. Try to get the commit date in the unixtime format: $ TZ=Europe/Berlin git log --pretty=format:%ad --date=format:%s -1 Expected result: 1463260938 (same as recorded in the commit object). Observed result: 1463264538 (3600s = one hour ahead). For lulz, use another time zone: $ TZ=Asia/Novosibirsk git log --pretty=format:%ad --date=format:%s -1 Expected result: 1463260938 (unixtime is always UTC and should not depend on TZ). Observed result: 1463246538 (-14400s = 4 hours behind). Not even specifying the UTC time zone helps: $ TZ=UTC git log --pretty=format:%ad --date=format:%s -1 Expected result: still 1463260938. Observed result: 1463268138 (7200s = 2 hours ahead). FWIW, personally I have not bound the TZ environment variable, my time zone is constantly that of Berlin, currently CEST. Many thanks in advance, Michael. -- 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 01/12] commit.c: use strchrnul() to scan for one line
On Mon, May 16, 2016 at 4:19 PM, Stefan Beller wrote:
>> if (*p) {
>> p += 2;
>> - for (eol = p; *eol && *eol != '\n'; eol++)
>> - ; /* do nothing */
>> + eol = strchrnul(p, '\n');
>
> Nit:
> You could include the +2 into the arg of strchrnul,
> such that you can drop the braces.
You're right. With or without braces,
eol = strchrnul(p + 2, '\n');
would be easier to understand (I do not know offhand if the value of p
matters after this part, as I am responding from GMail Web UI without
access to the wider context in the source, though).
--
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 02/12] attr.c: use strchrnul() to scan for one line
On Mon, May 16, 2016 at 4:28 PM, Stefan Beller wrote:
>> for (sp = buf; *sp; ) {
>> char *ep;
>> int more;
>> - for (ep = sp; *ep && *ep != '\n'; ep++)
>> - ;
>> +
>> + ep = strchrnul(sp, '\n');
>
> (Even lesser nit as in patch 1)
> You could directly assign ep and more when declaring them.
I'd prefer not to.
"A declaration block, blank and statement" pattern allows you
to have the declaration of variables to group related things
together and in an order that makes it easier to explain.
If you throw in initialization there, you cannot do that anymore
(e.g. ep must be initialized before you can compute more).
--
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 01/12] commit.c: use strchrnul() to scan for one line
On Mon, May 16, 2016 at 4:41 PM, Junio C Hamano wrote:
> On Mon, May 16, 2016 at 4:19 PM, Stefan Beller wrote:
>>> if (*p) {
>>> p += 2;
>>> - for (eol = p; *eol && *eol != '\n'; eol++)
>>> - ; /* do nothing */
>>> + eol = strchrnul(p, '\n');
>>
>> Nit:
>> You could include the +2 into the arg of strchrnul,
>> such that you can drop the braces.
>
> You're right. With or without braces,
>
> eol = strchrnul(p + 2, '\n');
>
> would be easier to understand (I do not know offhand if the value of p
> matters after this part, as I am responding from GMail Web UI without
> access to the wider context in the source, though).
Heh, good point, I did not think it through apparently. `p` matters.
--
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: git push --quiet option does not seem to work
>> Once I included the whole email in my reply, but otherwise I deleted it >> all. > Both are bad practice. If you are considerate with the reader's time, this > consideration is typically reprocicated. So it is a good idea to save the > reader time by giving them the precise context they need. This is among a few reasons I don't understand why in 2016 we use mail lists for this kind of stuff. First time I've had to deal with this since the 1990's so I forgot how it works. On Mon, May 16, 2016 at 11:17 AM, Jeff King wrote: > On Mon, May 16, 2016 at 05:04:34PM +0200, Johannes Schindelin wrote: > >> > Anyway, it is not a Powershell thing. I tested on another repo on >> > GitHub and it worked as expected. So I guess indeed the problem lies >> > with Microsoft's implementation. >> >> This is *really* unclear. >> >> What "Microsoft's implementation"??? Do you refer to VSTS, or do you refer >> to Git for Windows, or PowerShell? >> >> Please. To make it really simple for everybody involved, try to repeat as >> closely as possible the same push from PowerShell, Git CMD and Git Bash. >> We want to compare oranges to oranges. As I was mentioning GitHub I assumed "Microsoft implementation" would indicate their hosted Git service "Visual Studio Team Services". I really didn't think there was anything else to provide. The feedback lead me to test with Github and with that I verified that when the remote was for Github it worked, but when the remote was VSTS it was not.. and "not working" means not paying attention to "--quiet" setting only with git push, while it does work for other commands such as clone. (though I think I have to re-test with pull.) The problem is not with Powershell (though how it handles seeing output in STDERR is not anything I agree with). I was merely trying to point out that 'git push --quiet' was not working until we realized it was with VSTS. This "ticket" if it exists as such in a maillist can be closed. I think this accurately sums it up: > The "bug" is that the server is asking the client to write non-error > output to stderr, even though the client should have asked the server to > be quiet (though it would not hurt to check that it is doing so by > looking at the output of GIT_TRACE_PACKET). > > -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
[PATCH v6 0/9] connect: various cleanups
The main difference here is patch 2/9 now throwing an error in user's face when they have a core.gitProxy configuration with a port number. There might be some bikeshedding to do on the content of the error message. Mike Hommey (9): connect: call get_host_and_port() earlier connect: only match the host with core.gitProxy connect: fill the host header in the git protocol with the host and port variables connect: make parse_connect_url() return separated host and port connect: group CONNECT_DIAG_URL handling code connect: make parse_connect_url() return the user part of the url as a separate value connect: change the --diag-url output to separate user and host connect: actively reject git:// urls with a user part connect: move ssh command line preparation to a separate function connect.c | 243 + t/t5500-fetch-pack.sh | 42 ++--- t/t5532-fetch-proxy.sh | 8 +- 3 files changed, 183 insertions(+), 110 deletions(-) -- 2.8.2.411.ga570dec.dirty -- 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 v6 5/9] connect: group CONNECT_DIAG_URL handling code
Previous changes made both branches handling CONNECT_DIAG_URL identical.
We can now remove one of those branches and have CONNECT_DIAG_URL be
handled in one place.
Signed-off-by: Mike Hommey
---
connect.c | 16 +---
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/connect.c b/connect.c
index 4ce83f7..3a77b2f 100644
--- a/connect.c
+++ b/connect.c
@@ -708,7 +708,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, &host, &port, &path);
- if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
+ if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: userandhost=%s\n", host ? host : "NULL");
@@ -778,20 +778,6 @@ struct child_process *git_connect(int fd[2], const char
*url,
int putty = 0, tortoiseplink = 0;
transport_check_allowed("ssh");
- if (flags & CONNECT_DIAG_URL) {
- printf("Diag: url=%s\n", url ? url : "NULL");
- printf("Diag: protocol=%s\n",
prot_name(protocol));
- printf("Diag: userandhost=%s\n", host ? host :
"NULL");
- printf("Diag: port=%s\n", port ? port : "NONE");
- printf("Diag: path=%s\n", path ? path : "NULL");
-
- free(host);
- free(port);
- free(path);
- free(conn);
- return NULL;
- }
-
ssh = getenv("GIT_SSH_COMMAND");
if (!ssh) {
const char *base;
--
2.8.2.411.ga570dec.dirty
--
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 v6 8/9] connect: actively reject git:// urls with a user part
Currently, urls of the for git://user@host don't work because user@host
is not resolving at the DNS level, but we shouldn't be relying on it
being an invalid host name, and actively reject it for containing a
username in the first place.
Signed-off-by: Mike Hommey
---
connect.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/connect.c b/connect.c
index dcaf32f..7360d57 100644
--- a/connect.c
+++ b/connect.c
@@ -733,6 +733,9 @@ struct child_process *git_connect(int fd[2], const char
*url,
*/
struct strbuf target_host = STRBUF_INIT;
char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+ if (user)
+ die("user@host is not allowed in git:// urls");
+
if (override_vhost)
strbuf_addstr(&target_host, override_vhost);
else {
--
2.8.2.411.ga570dec.dirty
--
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 v6 2/9] connect: only match the host with core.gitProxy
Currently, core.gitProxy doesn't actually match purely on domain names
as documented: it also matches ports.
So a core.gitProxy value like "script for kernel.org" doesn't make the
script called for an url like git://kernel.org:port/path, while it is
called for git://kernel.org/path.
This per-port behavior seems like an oversight rather than a deliberate
choice, so, make git://kernel.org:port/path call the gitProxy script in
the case described above.
However, if people have been relying on this behavior, git now fails
with an error message inviting for a configuration change.
Signed-off-by: Mike Hommey
---
connect.c | 20 +---
t/t5532-fetch-proxy.sh | 8 +++-
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/connect.c b/connect.c
index d3448c2..0f48cde 100644
--- a/connect.c
+++ b/connect.c
@@ -490,6 +490,8 @@ static void git_tcp_connect(int fd[2], const char *host,
const char *port,
}
+static char *get_port(char *host);
+
static char *git_proxy_command;
static int git_proxy_command_options(const char *var, const char *value,
@@ -517,10 +519,14 @@ static int git_proxy_command_options(const char *var,
const char *value,
/* matches everybody */
matchlen = strlen(value);
else {
- hostlen = strlen(for_pos + 5);
+ struct strbuf host = STRBUF_INIT;
+ char *port;
+ strbuf_addstr(&host, for_pos + 5);
+ port = get_port(host.buf);
+ hostlen = strlen(host.buf);
if (rhost_len < hostlen)
matchlen = -1;
- else if (!strncmp(for_pos + 5,
+ else if (!strncmp(host.buf,
rhost_name + rhost_len - hostlen,
hostlen) &&
((rhost_len == hostlen) ||
@@ -528,6 +534,14 @@ static int git_proxy_command_options(const char *var,
const char *value,
matchlen = for_pos - value;
else
matchlen = -1;
+ if (matchlen > 0 && port)
+ die("core.gitProxy only supports \"for "
+ "host/domain\", not \"for host:port\".\n"
+ "Please change your configuration "
+ "accordingly.\nPlease note that the port "
+ "is given as second argument to the proxy "
+ "script.\n");
+ strbuf_release(&host);
}
if (0 <= matchlen) {
/* core.gitproxy = none for kernel.org */
@@ -706,7 +720,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
/* These underlying connection commands die() if they
* cannot connect.
*/
- if (git_use_proxy(hostandport))
+ if (git_use_proxy(host))
conn = git_proxy_connect(fd, host, port);
else
git_tcp_connect(fd, host, port, flags);
diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh
index 51c9669..efa80f8 100755
--- a/t/t5532-fetch-proxy.sh
+++ b/t/t5532-fetch-proxy.sh
@@ -33,7 +33,9 @@ test_expect_success 'setup proxy script' '
test_expect_success 'setup local repo' '
git remote add fake git://example.com/remote &&
- git config core.gitproxy ./proxy
+ git remote add fake_b git://example.org/remote &&
+ git config core.gitproxy "./proxy for example.com" &&
+ git config --add core.gitproxy "./proxy for example.org:9418"
'
test_expect_success 'fetch through proxy works' '
@@ -43,4 +45,8 @@ test_expect_success 'fetch through proxy works' '
test_cmp expect actual
'
+test_expect_success 'core.gitProxy with port fails' '
+ test_must_fail git fetch fake_b
+'
+
test_done
--
2.8.2.411.ga570dec.dirty
--
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 v6 1/9] connect: call get_host_and_port() earlier
Currently, get_host_and_port() is called in git_connect() for the ssh
protocol, and in git_tcp_connect_sock() for the git protocol. Instead
of doing this, just call it from a single place, right after
parse_connect_url(), and pass the host and port separately to
git_*_connect() functions.
We however preserve hostandport, at least for now.
Note that in git_tcp_connect_sock, the port was set to "" in a
case that never can happen, so that code path was removed.
Signed-off-by: Mike Hommey
---
connect.c | 47 +++
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/connect.c b/connect.c
index c53f3f1..d3448c2 100644
--- a/connect.c
+++ b/connect.c
@@ -343,18 +343,16 @@ static const char *ai_name(const struct addrinfo *ai)
/*
* Returns a connected socket() fd, or else die()s.
*/
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
{
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
- const char *port = STR(DEFAULT_GIT_PORT);
struct addrinfo hints, *ai0, *ai;
int gai;
int cnt = 0;
- get_host_and_port(&host, &port);
- if (!*port)
- port = "";
+ if (!port)
+ port = STR(DEFAULT_GIT_PORT);
memset(&hints, 0, sizeof(hints));
if (flags & CONNECT_IPV4)
@@ -411,11 +409,10 @@ static int git_tcp_connect_sock(char *host, int flags)
/*
* Returns a connected socket() fd, or else die()s.
*/
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
{
struct strbuf error_message = STRBUF_INIT;
int sockfd = -1;
- const char *port = STR(DEFAULT_GIT_PORT);
char *ep;
struct hostent *he;
struct sockaddr_in sa;
@@ -423,7 +420,8 @@ static int git_tcp_connect_sock(char *host, int flags)
unsigned int nport;
int cnt;
- get_host_and_port(&host, &port);
+ if (!port)
+ port = STR(DEFAULT_GIT_PORT);
if (flags & CONNECT_VERBOSE)
fprintf(stderr, "Looking up %s ... ", host);
@@ -482,9 +480,10 @@ static int git_tcp_connect_sock(char *host, int flags)
#endif /* NO_IPV6 */
-static void git_tcp_connect(int fd[2], char *host, int flags)
+static void git_tcp_connect(int fd[2], const char *host, const char *port,
+ int flags)
{
- int sockfd = git_tcp_connect_sock(host, flags);
+ int sockfd = git_tcp_connect_sock(host, port, flags);
fd[0] = sockfd;
fd[1] = dup(sockfd);
@@ -550,18 +549,16 @@ static int git_use_proxy(const char *host)
return (git_proxy_command && *git_proxy_command);
}
-static struct child_process *git_proxy_connect(int fd[2], char *host)
+static struct child_process *git_proxy_connect(int fd[2], const char *host,
+ const char *port)
{
- const char *port = STR(DEFAULT_GIT_PORT);
struct child_process *proxy;
- get_host_and_port(&host, &port);
-
proxy = xmalloc(sizeof(*proxy));
child_process_init(proxy);
argv_array_push(&proxy->args, git_proxy_command);
argv_array_push(&proxy->args, host);
- argv_array_push(&proxy->args, port);
+ argv_array_push(&proxy->args, port ? port : STR(DEFAULT_GIT_PORT));
proxy->in = -1;
proxy->out = -1;
if (start_command(proxy))
@@ -672,7 +669,8 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
struct child_process *git_connect(int fd[2], const char *url,
const char *prog, int flags)
{
- char *hostandport, *path;
+ char *hostandport, *path, *host;
+ const char *port = NULL;
struct child_process *conn = &no_fork;
enum protocol protocol;
struct strbuf cmd = STRBUF_INIT;
@@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, &hostandport, &path);
+ host = xstrdup(hostandport);
+ get_host_and_port(&host, &port);
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
@@ -707,9 +707,9 @@ struct child_process *git_connect(int fd[2], const char
*url,
* cannot connect.
*/
if (git_use_proxy(hostandport))
- conn = git_proxy_connect(fd, hostandport);
+ conn = git_proxy_connect(fd, host, port);
else
- git_tcp_connect(fd, hostandport, flags);
+ git_tcp_connect(fd, host, port, flags);
/*
* Separate original protocol components prog and
[PATCH v6 3/9] connect: fill the host header in the git protocol with the host and port variables
The last use of the hostandport variable, besides being strdup'ed before
being split into host and port, is to fill the host header in the git
protocol.
Instead of relying on parse_connect_url() to return a host:port string
that makes sense there, construct one from the host and port variables.
Signed-off-by: Mike Hommey
---
connect.c | 27 ---
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/connect.c b/connect.c
index 0f48cde..0676ee0 100644
--- a/connect.c
+++ b/connect.c
@@ -709,11 +709,24 @@ struct child_process *git_connect(int fd[2], const char
*url,
* connect, unless the user has overridden us in
* the environment.
*/
- char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
- if (target_host)
- target_host = xstrdup(target_host);
- else
- target_host = xstrdup(hostandport);
+ struct strbuf target_host = STRBUF_INIT;
+ char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+ if (override_vhost)
+ strbuf_addstr(&target_host, override_vhost);
+ else {
+ /* If the host contains a colon (ipv6 address), it
+* needs to be enclosed with square brackets. */
+ const char *colon = strchr(host, ':');
+ if (colon)
+ strbuf_addch(&target_host, '[');
+ strbuf_addstr(&target_host, host);
+ if (colon)
+ strbuf_addch(&target_host, ']');
+ if (port) {
+ strbuf_addch(&target_host, ':');
+ strbuf_addstr(&target_host, port);
+ }
+ }
transport_check_allowed("git");
@@ -734,8 +747,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
packet_write(fd[1],
"%s %s%chost=%s%c",
prog, path, 0,
-target_host, 0);
- free(target_host);
+target_host.buf, 0);
+ strbuf_release(&target_host);
} else {
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
--
2.8.2.411.ga570dec.dirty
--
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 v6 7/9] connect: change the --diag-url output to separate user and host
Signed-off-by: Mike Hommey
---
connect.c | 6 ++
t/t5500-fetch-pack.sh | 14 --
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 2659b40..dcaf32f 100644
--- a/connect.c
+++ b/connect.c
@@ -720,10 +720,8 @@ struct child_process *git_connect(int fd[2], const char
*url,
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
- if (user)
- printf("Diag: userandhost=%s@%s\n", user, host);
- else
- printf("Diag: userandhost=%s\n", host ? host : "NULL");
+ printf("Diag: user=%s\n", user ? user : "NULL");
+ printf("Diag: host=%s\n", host ? host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 739c6b1..2d9c4be 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -553,7 +553,7 @@ check_prot_path () {
Diag: protocol=$2
Diag: path=$3
EOF
- git fetch-pack --diag-url "$1" | egrep -v '(host|port)=' >actual &&
+ git fetch-pack --diag-url "$1" | egrep -v "(user|host|port)=" >actual &&
test_cmp expected actual
}
@@ -568,10 +568,20 @@ check_prot_host_port_path () {
;;
esac
ehost=$(echo $3 | tr -d "[]")
+ case "$ehost" in
+ *@*)
+ user=${ehost%@*}
+ ehost=${ehost#$user@}
+ ;;
+ *)
+ user=NULL
+ ;;
+ esac
cat >exp <<-EOF &&
Diag: url=$1
Diag: protocol=$pp
- Diag: userandhost=$ehost
+ Diag: user=$user
+ Diag: host=$ehost
Diag: port=$4
Diag: path=$5
EOF
--
2.8.2.411.ga570dec.dirty
--
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
