RE: [PATCH v3] coccicheck: process every source file at once

[Keller, Jacob E]

> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> Sent: Friday, October 05, 2018 9:25 AM
> To: SZEDER Gábor 
> Cc: Jacob Keller ; Keller, Jacob E
> ; Git mailing list 
> Subject: Re: [PATCH v3] coccicheck: process every source file at once
> 
> On Fri, Oct 05, 2018 at 02:40:48PM +0200, SZEDER Gábor wrote:
> 
> > On Thu, Oct 04, 2018 at 07:17:47PM -0700, Jacob Keller wrote:
> > > Junio, do you want me to update the commit message on my side with the
> > > memory concerns? Or could you update it to mention memory as a noted
> > > trade off.
> >
> > We have been running 'make -j2 coccicheck' in the static analysis
> > build job on Travis CI, which worked just fine so far.  The Travis CI
> > build environments have 3GB of memory available [1], but, as shown in
> > [2], with this patch the memory consumption jumps up to about
> > 1.3-1.8GB for each of those jobs.  So with two parallel jobs we will
> > very likely bump into this limit.
> >
> > So this patch should definitely change that build script to run only a
> > single job.
> 
> It should still be a net win, since the total CPU seems to drop by a
> factor of 3-4.
> 
> Are we OK with saying 1.3-1.8GB is necessary to run coccicheck? That
> doesn't feel like an exorbitant request for a developer-only tool these
> days, but I have noticed some people on the list tend to have lousier
> machines than I do. ;)
> 
> -Peff

It's probably not worth trying to make this more complicated and scale up how 
many files we do at once based on the amount of available memory on the 
system...

Thanks,
Jake

Re: [PATCH 0/2] add format specifiers to display trailers

[Keller, Jacob E]

On Mon, 2016-11-21 at 09:23 -0800, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > > We have %s and %b so that we can reconstruct the whole thing by
> > > using both.  It is unclear how %bT fits in this picture.  I
> > > wonder
> > > if we also need another placeholder that expands to the body of
> > > the
> > > message without the trailer---otherwise the whole set would
> > > become
> > > incoherent, no?
> > 
> > I'm not entirely sure what to do here. I just wanted a way to
> > easily
> > format "just the trailers" of a message. We could add something
> > that
> > formats just the non-trailers, that's not too difficult. Not really
> > sure what I'd call it though.
> 
> I was wondering if %(log:) was a better way to go.
> 
> %(log:title) and %(log:body) would be equivalents of traditional %s
> and %b, and %(log:body) in turn would be a shorter way to write
> %(log:description)%+(log:trailer), i.e. show the message body, and
> if there is a trailer block, add it after adding a blank line.
> 
> Or something like that?

That would work for me.

Thanks,
Jake

Re: [PATCH] rev-list: restore the NUL commit separator in --header mode

[Keller, Jacob E]

On Wed, 2016-10-19 at 15:39 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > 
> > Hi,
> > 
> > On Wed, Oct 19, 2016 at 2:04 PM, Dennis Kaarsemaker
> >  wrote:
> > > 
> > > Commit 660e113 (graph: add support for --line-prefix on all
> > > graph-aware
> > > output) changed the way commits were shown. Unfortunately this
> > > dropped
> > > the NUL between commits in --header mode. Restore the NUL and add
> > > a test
> > > for this feature.
> > > 
> > 
> > Oops! Thanks for the bug fix.
> > 
> > > 
> > > Signed-off-by: Dennis Kaarsemaker 
> > > ---
> > >  builtin/rev-list.c   | 4 
> > >  t/t6000-rev-list-misc.sh | 7 +++
> > >  2 files changed, 11 insertions(+)
> > > 
> > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > > index 8479f6e..cfa6a7d 100644
> > > --- a/builtin/rev-list.c
> > > +++ b/builtin/rev-list.c
> > > @@ -157,6 +157,10 @@ static void show_commit(struct commit
> > > *commit, void *data)
> > > if (revs->commit_format ==
> > > CMIT_FMT_ONELINE)
> > > putchar('\n');
> > > }
> > > +   if (revs->commit_format == CMIT_FMT_RAW) {
> > > +   putchar(info->hdr_termination);
> > > +   }
> > > +
> > 
> > This seems right to me. My one concern is that we make sure we
> > restore
> > it for every case (in case it needs to be there for other formats?)
> > I'm not entirely sure about whether other non-raw modes need this
> > or
> > not?
> 
> Right.  The original didn't do anything special for CMIT_FMT_RAW,
> and 660e113 did not remove anything special for CMIT_FMT_RAW, so it
> isn't immediately obvious why this patch is sufficient.  
> 
> Dennis, care to elaborate?

I believe all we need to do is change one of the places where we emit
"\n" with emiting info->hdr_termination instead.

I'm looking at the original code now.

Thanks,
Jake

Re: [PATCH v11 0/8] submodule inline diff format

[Keller, Jacob E]

On Fri, 2016-08-26 at 12:17 -0700, Stefan Beller wrote:
> On Thu, Aug 25, 2016 at 4:32 PM, Jacob Keller  om> wrote:
> 
> > 
> > @@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf
> > *buf, const char *path,
> > strbuf_addstr(buf, git_dir);
> > }
> > if (!is_git_directory(buf->buf)) {
> > +   gitmodules_config();
> 
> We determined via chat that calling gitmodules_config
> is not harmful w.r.t. correctness, but might require some
> improvements in the future for performance.
> (i.e. we may want to add in a later series a
> if (already called gitmodules_config)
>   set flag "already called gitmodules_config";
>   return;
> into gitmodules_config)
> 
> > 
> > 
> >  char *git_pathdup_submodule(const char *path, const char *fmt,
> > ...)
> >  {
> > +   int err;
> > va_list args;
> > struct strbuf buf = STRBUF_INIT;
> > va_start(args, fmt);
> > -   do_submodule_path(, path, fmt, args);
> > +   err = do_submodule_path(, path, fmt, args);
> > va_end(args);
> > +   if (err)
> 
> Here we need a strbuf_release() to avoid a memory leak?

No, cause we "strbuf_detach" after this to return the buffer? Or is
that not safe?

Thanks,
Jake

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v11 5/8] allow do_submodule_path to work even if submodule isn't checked out

[Keller, Jacob E]

On Fri, 2016-08-26 at 11:19 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > 
> > Currently, do_submodule_path will attempt locating the .git
> > directory by
> > using read_gitfile on /.git. If this fails it just assumes
> > the
> > /.git is actually a git directory.
> > 
> > This is good because it allows for handling submodules which were
> > cloned
> > in a regular manner first before being added to the parent project.
> 
> s/parent project/superproject/;
> 

Yep.

> > 
> > Unfortunately this fails if the  is not actually checked out
> > any
> > longer, such as by removing the directory.
> > 
> > Fix this by checking if the directory we found is actually a
> > gitdir. In
> > the case it is not, attempt to lookup the submodule configuration
> > and
> > find the name of where it is stored in the .git/modules/ folder of
> > the
> > parent project.
> 
> As you consistently say "directory" in the earlier part of the log
> message,
> 
> s/folder of the parent project/directory of the superproject/;
> 
> is desired.
> 

Yep.

> > 
> > 
> > If we can't locate the submodule configuration this might occur
> > because
> 
> I added s/configuration/&,/ to make it a bit easier to read.
> 

Makes sense.

> > 
> > for example a submodule gitlink was added but the corresponding
> > .gitmodules file was not properly updated. A die() here would not
> > be
> > pleasant to the users of submodule diff formats, so instead, modify
> > do_submodule_path to return an error code. For
> > git_pathdup_submodule,
> > just return NULL when we fail to find a path. For
> > strbuf_git_path_submodule
> > propagate the error code to the caller.
> 
> Somehow I had to read the latter half of this paragraph twice,
> before I realized that the last two sentence talks about how these
> two functions exactly do "to return an error code".  Tentatively
> what I queued has:
> 
> ... so instead, modify do_submodule_path() to return an error
> code:
> 
>  - git_pathdup_submodule() returns NULL when we fail to find a
> path.
>  - strbuf_git_path_submodule() propagates the error code to the
>    caller.
> 
> instead, hoping that would be easier to understand.
> 

That's much better and more clear. Thanks!

> > 
> > -static void do_submodule_path(struct strbuf *buf, const char
> > *path,
> > -     const char *fmt, va_list args)
> > +/* Returns 0 on success, non-zero on failure. */
> 
> s/non-zero/negative/;
> 


True.

> > 
> > +#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
> > +static int do_submodule_path(struct strbuf *buf, const char *path,
> > +    const char *fmt, va_list args)
> >  {
> 
> This 5/8 is the only changed one in the entire series from the
> previous round, I think.  I'll replace what was in 'pu' and wait for
> responses.
> 
> Thanks.

Yep, that's correct.

Regards,
Jake

Re: [PATCH v10 0/9] submodule inline diff format

[Keller, Jacob E]

On Fri, 2016-08-26 at 10:35 -0700, Stefan Beller wrote:
> > a) read_gitfile on /.git
> > b) if read_gitfile succeeds, use it's contents, otherwise use
> > /.git for next steps
> > c) check if the resulting file is a git directory, we're fine.. we
> > found a gitdir, so stop.
> > d) otherwise,  empty the buffer, then lookup submodules
> > e) when submodules lookup succeeds.. see if we found a name. If so,
> > use that.
> 
> When the submodules lookup succeeds, we can assert the name exists.
> There is currently only one way the lookup is populated, and that is
> lookup_or_create_by_name in submodule-config.c:182, which fills in
> the name all the time.

Yes, that was how I was trying to word it, and that's what I've done in
code.

> 
> > 
> > f) if we didn't just exit with an empty buffer.
> > 
> > That empty buffer *should* trigger  revision error codes since it
> > won't point to any valid path and it also triggers the regular
> > error
> > code in add_submodule_odb so it handles that with showing not
> > initizlied.
> > 
> > This method is less work then re-implementing a _gently() variant
> > for
> > all of these functions.
> > 
> > Stefan, does this make sense and seem reasonable?
> 
> Sounds reasonable to me.
> 
> Thanks for working on this!
> Stefan

Thanks for review!

Regards,
Jake

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Keller, Jacob E]

On Fri, 2016-04-29 at 15:44 -0700, Stefan Beller wrote:
> > 
> > Currently it's an "opt in" knob, so this doesn't make sense to me.
> +static int diff_compaction_heuristic = 1;
> 

Oops didn't know we'd made it default at some point. (all my versions
had it disabled by default)

> It's rather an opt-out knob going by the current
> origin/jk/diff-compact-heuristic
> 

Yea in that case, we could keep it.

> 
> > 
> > If
> > we remove the entire knob as is, we can always (fairly easily) add
> > it
> > back. I would keep the code inside xdiff as a knob, but set it to
> > enable default so that the user config has no knob at the top level
> > but
> > the xdiff machinery does (this making a "disable" be relatively
> > small
> > patch).
> When writing my reply, I thought about people using Git from a binary
> distribution with little to no admin rights. They want to have an
> emergency
> knob to disable this thing, but cannot patch/recompile Git.
> 
> If you can patch and compile your version of Git, then reverting is
> easy, so
> in that case Junios patch looks good to me.
> 
> Thanks,
> Stefan

True. I think the chances that it needs such a thing are quite minor,
and if an undocumented knob gets exposed it would have to become
documented and maintained, so I'd prefer to avoid it. Given that the
risk is pretty small I think that's ok.

Thanks,
Jake

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Keller, Jacob E]

On Fri, 2016-04-29 at 15:35 -0700, Stefan Beller wrote:
> On Fri, Apr 29, 2016 at 3:18 PM, Junio C Hamano 
> wrote:
> > 
> > Jacob Keller  writes:
> > 
> > > 
> > > On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano  > > m> wrote:
> > > > 
> > > > Jeff King  writes:
> > > > 
> > > > > 
> > > > > ... Having the two directly next to each other reads
> > > > > better to me. This is a pretty unusual diff, though, in that
> > > > > it did
> > > > > change the surrounding whitespace (and if you look further in
> > > > > the diff,
> > > > > the identical change is made elsewhere _without_ touching the
> > > > > whitespace). So this is kind of an anomaly. And IMHO the
> > > > > weirdness here
> > > > > is outweighed by the vast number of improvements elsewhere.
> > > > So... is everybody happy with the result and now we can drop
> > > > the
> > > > tweaking knob added to help experimentation before merging the
> > > > result to 'master'?
> > > > 
> > > > I am pretty happy with the end result myself.
> > > I am very happy with it. I haven't had any issues, and I think
> > > we'll
> > > find better traction by enabling it at this point and seeing
> > > when/if
> > > someone complains.
> > > 
> > > I think for most it won't be noticed and for those that do it
> > > will
> > > likely be positive.
> > I am doing this only to prepare in case we have a concensus,
> > i.e. this is not to declare that I do not care what other people
> > say.  Here is a patch to remove the experimentation knob.
> > 
> > Let's say we keep this patch out of tree for now and keep the topic
> > in 'next' so that people can further play with it for several more
> > weeks, and then apply this on top and merge the result to 'master'
> > early in the next cycle.
> > 
> > -- >8 --
> > diff: enable "compaction heuristics" and lose experimentation knob
> > 
> > It seems that the new "find a good hunk boundary by locating a
> > blank
> > line" heuristics gives much more pleasant result without much
> > noticeable downsides.  Let's make it the new algorithm for real,
> > without the opt-out knob we added while experimenting with it.
> I would remove the opt-out knob much later in the game, i.e.
> 
> 1) make a patch that removes the documentation only
>    before the next release (i.e. before 2.9)
> 2) make a patch to remove the actual (unlabeled) knobs,
> merge into master before 2.10 (i.e. just after the 2.9
> release)
> 
> Then we get the most of the community to test it with the 2.9 release
> and still have an emergency knob in case some major headaches
> show up. After one release cycle we'll be much more confident
> about its usage and its short comings and do not need the
> emergency turn off. If the community doesn't like it for some reason
> we can document it and debate the default setting?
> 
> I agree we want the knob gone eventually.
> Making it an undocumented feature is as good as that from
> a users point of view?
> 

Currently it's an "opt in" knob, so this doesn't make sense to me. If
we remove the entire knob as is, we can always (fairly easily) add it
back. I would keep the code inside xdiff as a knob, but set it to
enable default so that the user config has no knob at the top level but
the xdiff machinery does (this making a "disable" be relatively small
patch).

Thanks,
Jake

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:56 -0500, Eric Sunshine wrote:
> On Mon, Nov 16, 2015 at 6:50 PM, Eric Sunshine  om> wrote:
> > Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> > exclusive with many of the other options? New tests would check
> > such
> > exclusivity as well.
> 
> In fact, while I agree with Szeder that it makes sense to re-use
> send-email's aliases parsing functionality (and was going to suggest
> the same, but he beat me to it), this new option is awfully
> orthogonal
> to the overall purpose of send-email, thus, doesn't really fit in
> well
> and almost cries out for a command of its own which would be used by
> send-email and bash completion (though I'm not convinced that it's
> worth going that route for this one minor use-case).

I don't think it's worth it at this point, because we'd have to extract
out the alias parsing logic from send-email, which is not easy.

The option is pretty orthogonal to git-send-email, but until/unless
git-send-email is re-implemented in C, i don't really see value in
trying to separate the logic out... That is a lot more effort for very
little gain.

Regards,
JakeN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v2 2/2] completion: add support for completing email aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:33 -0500, Eric Sunshine wrote:
> On Sun, Nov 15, 2015 at 3:22 PM, Jacob Keller  om> wrote:
> > Using the new --list-aliases option from git-send-email, add
> > completion
> > for --to, --cc, and --bcc with the available configured aliases.
> > 
> > Signed-off-by: Jacob Keller 
> > ---
> > diff --git a/contrib/completion/git-completion.bash
> > b/contrib/completion/git-completion.bash
> > @@ -1711,6 +1712,15 @@ __git_send_email_suppresscc_options="author
> > self cc bodycc sob cccmd body all"
> > 
> >  _git_send_email ()
> >  {
> > +   case "$prev" in
> > +   --to|--cc|--bcc)
> 
> What about --from, which also undergoes alias expansion?
> 

Makes sense, yep.

N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:50 -0500, Eric Sunshine wrote:
> It should be possible to extract the alias within the shell itself
> without a separate process. For instance:
> 
> read alias rest
> 
> will leave the first token in $alias and the remainder of the line in
> $rest, and it's all done within the shell process.
> 

I'll look into this :)

> > > New test(s) seem to be missing.
> > 
> > I had removed the tests from the old version because they weren't
> > necessary anymore. New ones wouldn't hurt here either, though..
> > I'll
> > work on that.
> 
> I'm not sure which tests you mean, but I was referring to tests to
> make sure that git-send-email recognizes --list-aliases (or
> --dump-aliases if you switch to that) and that it produces the
> expected output in the expected format.
> 

Yep, I added some in my respin.

> Also, shouldn't --list-aliases (or --dump-aliases) be mutually
> exclusive with many of the other options? New tests would check such
> exclusivity as well.

I am at a loss for how to do that correctly in the perl. Help would be
appreciated here.

Regards,
JakN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v2 1/2] sendemail: teach git-send-email to list aliases

[Keller, Jacob E]

On Mon, 2015-11-16 at 18:30 -0500, Eric Sunshine wrote:
> On Sun, Nov 15, 2015 at 3:22 PM, Jacob Keller  om> wrote:
> > Add an option "list-aliases" which changes the default behavior of
> > git-send-email. This mode will simply read the alias files
> > configured by
> > sendemail.aliasesfile and sendemail.aliasfiletype and print a list
> > of
> > all known aliases. The intended usecase for this option is the
> > bash-completion script which will use it to autocomplete aliases on
> > the
> > options which take addresses.
> 
> As this is primarily a plumbing option, I wonder if --dump-aliases
> might be a more suitable name.
> 

Sure that would be reasonable.

> Also, is it possible that some consumer down the road might want
> richer output which includes the expansion of each alias? For
> instance, it could emit the alias name as the first token on each
> line
> and the expansion as the remainder. Consumers interested in only the
> alias name would grab the first token on the line and ignore
> everything else.
> 

Maybe? The problem with printing the full address is that it may not be
quoted or similar, and it makes the bash completion require an extra
parameter.. I am not sure how valuable the alias expansion would be for
use? The main concern I have is we'd need to use another process on top
to extract only alias names.

> > Signed-off-by: Jacob Keller 
> > ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > @@ -101,6 +102,9 @@ git send-email [options]  > rev-list options >
> >   `git format-patch` ones.
> >  --force* Send even if safety checks
> > would prevent it.
> > 
> > +  Information:
> > +--list-aliases * read the aliases from
> > configured alias files
> 
> This description is odd. It seems to imply that aliases will be
> loaded
> (and used) only if this option is given, and says nothing about its
> actual purpose of dumping the aliases.
> 

I can re-word this.

> Also, with one exception, all the other option descriptions are
> capitalized. This probably ought to follow suit.
> 
> > +if ($list_aliases) {
> > +print $_,"\n" for (keys %aliases);
> > +exit(0);
> > +}
> 
> New test(s) seem to be missing.
> 

I had removed the tests from the old version because they weren't
necessary anymore. New ones wouldn't hurt here either, though.. I'll
work on that.

Regards,
Jake

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

[Keller, Jacob E]

On Tue, 2015-09-22 at 13:40 -0700, Junio C Hamano wrote:
> How about phrasing it totally differently?
> 
>   The ref specifies the full refname when it begins with
>   `refs/notes/`; otherwise `ref/notes/` is prefixed to form a
>   full name of the ref.
> 
> I think that would remove the need to illustrate with concrete
> examples like refs/heads/blah.
> 

Wait, what about the DWIM of notes/ goes to refs/notes/..
do we need to explain that here? it might seem that "notes/foo" ends up
as "refs/notes/notes/foo" which is not really what we mean.

Regards,
JakeN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH] notes: document behavior of --ref and --notes DWIMery

[Keller, Jacob E]

On Tue, 2015-09-22 at 13:40 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > From: Jacob Keller 
> > 
> > The --notes and --ref parameter for selecting which notes ref to
> > operate
> > on are based off of expand_notes_ref functionality. The
> > documentation
> > mentioned that an unqualified ref argument would be taken as under
> > `refs/notes/`. However, this does not clearly indicate that
> > `refs/heads/master` will expand to `refs/notes/refs/heads/master`,
> > so
> > document this behavior.
> > 
> > Add a further test for the expected behavior of git notes --ref
> > refs/heads/master get-ref as well, to ensure future patches do not
> > break
> > this assumption.
> > 
> > Signed-off-by: Jacob Keller 
> > ---
> 
> Looks OK to a cursory read, but I find "even if it is qualified
> under some other location" a bit tiring to read without adding much
> value.  To readers who consider "other" in that phrase to be clear
> enough (i.e. "location other than refs/notes"), it is totally
> redundant.  To other readers who feel "other" in that phrase to be
> under qualified (i.e. "location other than what???"), it is not
> informative enough.  Middle-ground readers who would not know if
> "refs/a" is inside or outside some "other" location, it is confusing.
> 
> After all, "a/b" is qualified under some location (i.e. a/) other
> than "refs/notes/", and it does mean "refs/notes/a/b".
> 
> How about phrasing it totally differently?
> 
>   The ref specifies the full refname when it begins with
>   `refs/notes/`; otherwise `ref/notes/` is prefixed to form a
>   full name of the ref.
> 
> I think that would remove the need to illustrate with concrete
> examples like refs/heads/blah.
> 

Yes, let's go with that.

Regards,
Jake

Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs

[Keller, Jacob E]

On Thu, 2015-07-23 at 15:12 -0700, Junio C Hamano wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:
 
  On Wed, Jul 22, 2015 at 5:05 PM, Jacob Keller 
  jacob.e.kel...@intel.com wrote:
   From: Jacob Keller jacob.kel...@gmail.com
   
   Modify logic of check_refname_component and add a new disposition
   regarding *. Allow refspecs that contain a single * if
   REFNAME_REFSPEC_PATTERN is set. Change the function to pass the 
   flags as
   a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * 
   so that
   only a single * is accepted.
   
   This loosens restrictions on refspecs by allowing patterns that 
   have
   a * within a component instead of only as the whole component. 
   Also
   remove the code that hangled refspec patterns in 
   check_refname_format
  
  s/hangled/handled/
  ...
 
 Thanks; here is what I queued yesterday.
 

Woah. I bow to your imperative commit message abilities. The new commit
message is very nicely worded. Thanks for taking the time to reword my
jumble correctly.

Everything looked great to me.

Regards,
JakeN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: Git tag: pre-receive hook issue

[Keller, Jacob E]

On Mon, 2015-07-20 at 13:13 +0530, Gaurav Chhabra wrote:
 Hi Jake,
 
 Thanks about the refs/tags check. I’m aware about this. Junio also
 explained it in one of his replies. I was actually confused why my
 current code was working in past for few of the annotated tags.
 Anyways, now that I have clarity about the mistake in the code, I
 guess, I’ll figure it out myself.
 
 @Junio: Thanks a lot for your detailed explanation about the ‘behind
 the scenes’ activity during a push process. That was definitely
 helpful and will help me in future too.
 
 @Jake: Thanks for your help and your patience in explaining things.
 

No problem. I'm also not certain why it would have worked in some
cases, so that is definitely confusing, but at least you can get it
sorted to do what you intend. Best of luck!

Regards,
Jake

Re: Problem with architecture git.

[Keller, Jacob E]

On Fri, 2015-07-17 at 15:12 +, Alexander wrote:
 Hello,
 
 I have problem with architecure of my project, help me to resolve 
 problem 
 . I want to do in my remote repo two branches with two different 
 working 
 folders (master , dev ) to check it . How can I do it ?
 Thank you. 

Sounds like you want the git worktree functionality currently being
baked.

git clone remote
cd repo
git checkout master
git worktree add path/to/dev dev

This allows you to have both master checked out at the location
path/to/dev and the master checked out where repo is stored. Quite
valuable

I think this solves what you're looking for? I don't know what version
git-worktree is supported by..

Regards,
Jake

Re: [PATCH] gitk: Add a Copy commit summary command

[Keller, Jacob E]

On Tue, 2015-07-14 at 13:34 -0700, Stefan Beller wrote:
 On Tue, Jul 14, 2015 at 9:42 AM,  dev+...@drbeat.li wrote:
  From: Beat Bolli dev+...@drbeat.li
  
  When referencing earlier commits in new commit messages or other 
  text,
  one of the established formats is
  
  commit abbrev-sha (summary, author-date)
 
 That sounds like I would use it a lot! Thanks :)
 

Yep, quite useful. Also, the kernel suggests using it as a tag like so

Fixes: abbrev-sha (summary)

I really like this :)

Regards,
Jake

  
  Add a Copy commit summary command to the context menu that puts 
  this
  text for the currently selected commit on the clipboard. This makes 
  it
  easy for our users to create well-formatted commit references.
  
  Signed-off-by: Beat Bolli dev+...@drbeat.li
  Cc: Paul Mackerras pau...@samba.org
  ---
   gitk-git/gitk | 14 ++
   1 file changed, 14 insertions(+)
  
  diff --git a/gitk-git/gitk b/gitk-git/gitk
  index 9a2daf3..0612331 100755
  --- a/gitk-git/gitk
  +++ b/gitk-git/gitk
  @@ -2617,6 +2617,7 @@ proc makewindow {} {
  {mc Diff selected - this command {diffvssel 1}}
  {mc Make patch command mkpatch}
  {mc Create tag command mktag}
  +   {mc Copy commit summary command copysummary}
  {mc Write commit to file command writecommit}
  {mc Create new branch command mkbranch}
  {mc Cherry-pick this commit command cherrypick}
  @@ -9341,6 +9342,19 @@ proc mktaggo {} {
   mktagcan
   }
  
  +proc copysummary {} {
  +global rowmenuid commitinfo
  +
  +set id [string range $rowmenuid 0 7]
  +set info $commitinfo($rowmenuid)
  +set commit [lindex $info 0]
  +set date [formatdate [lindex $info 2]]
  +set summary [mc commit] $id (\$commit\, $date)
  +
  +clipboard clear
  +clipboard append $summary
  +}
  +
   proc writecommit {} {
   global rowmenuid wrcomtop commitinfo wrcomcmd NS
  
  --
  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.htmlN�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

RE: git notes from incoming patch

[Keller, Jacob E]

 -Original Message-
 From: Junio C Hamano [mailto:gits...@pobox.com]
 Sent: Tuesday, March 03, 2015 12:14 PM
 To: Keller, Jacob E
 Cc: git@vger.kernel.org
 Subject: Re: git notes from incoming patch
 
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  I am wondering whether it is possible to read from a format-patch input
  and add notes when we generate the applied patch.
 
 I would think post-applypatch hook is the right place to do this.
 The hook has access to the incoming message in $GIT_DIR/rebase-apply
 directory ('next' records the message number in the series, and then
 you have individual pieces of e-mails separated out into 0001, 0002,
 etc. you can read from), and HEAD already points at the result of
 applying the patch.
 
 Peek 'post-applypatch' on my 'todo' branch for inspirations.

Excellent, I will investigate this.

Regards,
Jake
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

git notes from incoming patch

[Keller, Jacob E]

Hi,

I am wondering whether it is possible to read from a format-patch input
and add notes when we generate the applied patch.

The use case is to be able to send patches that had notes appended via

$git format-patch --notes ...

And have notes objects created on the remote repository to store this
information.

Is there any way to do this? and/or is there a way to get the same
results that maybe doesn't use notes?

The problem we are trying to solve is a way to track some information
about a patch that we need internally without submitting it upstream
when we submit the patches later. We use email to handle internal patch
queues, so essentially we want to be able to add note objects to the
format-patch and send them via email.

Regards,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

git fetch specific ref fails when run against a remote where HEAD points to missing master branch

[Keller, Jacob E]

Hi,

I am assuming this is simply a configuration issue on my end for this
particular remote, because I never pushed to the master branch, and so
the HEAD of this remote doesn't exist.

However, I am also using it to fetch specific refs I know the name of,
so it confuses me when I try to fetch and get back a failure, because
the remote HEAD ref doesn't exist...

Command git fetch --tags --progress 
git://gitlad.jf.intel.com/git/jekeller/testing/ixgbe 
+refs/heads/*:refs/remotes/origin/* returned status code 1:
stdout: Fetching submodule src/COMPAT

stderr: remote: Counting objects: 9, done.
remote: Compressing objects:  33% (1/3)   
remote: Compressing objects:  66% (2/3)   
remote: Compressing objects: 100% (3/3)   
remote: Compressing objects: 100% (3/3), done.
remote: Total 3 (delta 1), reused 0 (delta 0)
From git://gitlad.jf.intel.com/git/jekeller/testing/ixgbe
 * [new branch]  2014-09-17-b6c6ff2c76ae - origin/2014-09-17-b6c6ff2c76ae
fatal: Couldn't find remote ref HEAD

So in order to understand what I am doing:

I have a continuous integration build, which I can pre-test a series of
commits before pushing it to the real remote, by pushing to a ref in
this repository, and then indicating to my continuous build server to
test against that ref.

As it turns out, my remote didn't have a master branch, (as I never
pushed anything there!), and so even though it was a bare repository,
HEAD was pointing still to refs/heads/master

I fixed this issue by adding a master branch that was empty. I also
assume I could have updated HEAD to point to a different location if I
wanted also.. but I'm not sure this should be an error in the case that
I fetch a specific ref by command line parameter? Especially as it
manages to fetch all the refs I asked for correctly, but still fails
with a non-zero exit value.

Regards,
Jake

Re: [PATCH 8/9] autoconf: Check for timer_settime

[Keller, Jacob E]

On Wed, 2014-09-10 at 14:08 -0700, Junio C Hamano wrote:
 Karsten Blees karsten.bl...@gmail.com writes:
 
  While the timer extension (timer_settime) has graduated to mandatory in
  the current POSIX spec, the monotonic clock extension is still optional
  today (i.e. not necessarily supported even on newer Unices). In contrast
  to this, the XSI extensions seem to be widely supported.
 
  IMO the 'obsolescent' marker in the current POSIX spec is no reason to
  spring into action (at least not yet). E.g. utimes (also in sys/time.h)
  has been marked LEGACY in the 2004 version and is no longer LEGACY today.
  Btw., we'd also have to find a replacement for gettimeofday and probably
  a lot of other stuff...
 
  Therefore I tend to agree with Hannes that we should stick with setitimer
  and emulate it on systems that don't have it (as we do on Windows).
 
 Sounds sensible to me.

Ya that makes sense. Was thinking too much about compatability code for
Linux kernel drivers, which is a different issue.

Let's go with re-implementing settimer in terms of timer_settime on the
one system we have information that doesn't support settimer.

I could spin that patch, but I think the original author should, since
he's the one who could actually test it.

Regards,
Jake

 --
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Sep 2014, #01; Tue, 2)

[Keller, Jacob E]

On Wed, 2014-09-03 at 12:18 -0700, Junio C Hamano wrote:
 Johannes Sixt j...@kdbg.org writes:
 
  But IMHO, this topic goes in a wrong direction. Avoid deprecated
  interfaces is way overrated. It would be preferable (IMHO) to implement
  setitimer() in compat/ for systems that don't have it.
 
 I think I agree.
 
 Adding compat/setitimer.c that implements git_setitimer() in terms
 of whatever is available on the platform and #define calls to
 setitimer() away to git_setitimer() would be a lot less intrusive
 change than the series posted.
 
 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

I suppose overall this makes more sense :) That shouldn't be difficult
to do.

Regards,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: revert top most commit

[Keller, Jacob E]

On Wed, 2014-08-27 at 17:22 -0700, Jonathan Nieder wrote:
 Keller, Jacob E wrote:
  On Wed, 2014-08-27 at 21:14 +, Keller, Jacob E wrote:
 
  I am having trouble using revert. If I attempt to
 
  $ git revert sha1id
 
  where sha1id is the same as the HEAD commit, I instead get the output of
  what looks like git status.
 [...]
  It's actually not my repository, I was helping a co-worker, and thought
  I'd ask if anyone here knew if it was expected behavior or not.
 
 More details about the output would help --- otherwise people have to
 guess[*].  I'm guessing your co-worker's working tree is not clean.
 Commiting or stashing their changes first might get things working.
 
 Hope that helps,
 Jonathan
 
 [*] Another nice thing about quoting output is that it becomes more
 obvious what about it wasn't helpful, which sometimes leads to patches
 from kind people on the list to fix 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

I will see if I can get the actual output.

Thanks,
Jake

Re: revert top most commit

[Keller, Jacob E]

On Wed, 2014-08-27 at 17:22 -0700, Jonathan Nieder wrote:
 Keller, Jacob E wrote:
  On Wed, 2014-08-27 at 21:14 +, Keller, Jacob E wrote:
 
  I am having trouble using revert. If I attempt to
 
  $ git revert sha1id
 
  where sha1id is the same as the HEAD commit, I instead get the output of
  what looks like git status.
 [...]
  It's actually not my repository, I was helping a co-worker, and thought
  I'd ask if anyone here knew if it was expected behavior or not.
 
 More details about the output would help --- otherwise people have to
 guess[*].  I'm guessing your co-worker's working tree is not clean.
 Commiting or stashing their changes first might get things working.
 
 Hope that helps,
 Jonathan
 
 [*] Another nice thing about quoting output is that it becomes more
 obvious what about it wasn't helpful, which sometimes leads to patches
 from kind people on the list to fix 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

# Command, where  
$git revert b718c4d508204b7f46b147c7c47c51fe7a2c7e31On

# Output
branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean

I also just got one key piece of information regarding this, the patch
itself was blank!

For backstory, the issue is that he pushed to a tree which does not
allow non-fastforward updates (so he can't rewind). He was using stacked
git, and accidentally pushed an empty patch, and wanted to revert it so
that at least the log showed that we had removed it (even though the
patch itself was empty).

At minimum, this output from git revert could be made more clear about
why it's failing on an empty patch.

Regards,
Jake

revert top most commit

[Keller, Jacob E]

Hi,

I am having trouble using revert. If I attempt to

$ git revert sha1id

where sha1id is the same as the HEAD commit, I instead get the output of
what looks like git status.

Is there anything specific about git revert that prevents it from
reverting the most recent commit?

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: Improving the git remote command

[Keller, Jacob E]

On Wed, 2014-08-27 at 13:35 -0700, Junio C Hamano wrote:
 David Aguilar dav...@gmail.com writes:
 
  We have some internal scripts at Disney Animation that rely on git remote
  output so I would vote for #3 personally as well.
 
 I take it that you mean you would vote _against_ #3 which will break
 the expectation.
 
  I know that git config is porcelain, and I can get remote.(.*).url,
  but that's not obvious and I highly doubt that anyone does that.
 
 Perhaps that is something worth fixing.
 
  What if we said that git remote list --porcelain == git remote
  and then just leave git remote output as-is so that we don't have to
  have a flag day when we break people's scripts?
 
 I suspect that it is not likely a workable solution.  The commands
 being Porcelain by definition means that people aimed to make their
 output consumable by humans, and the current git remote, which may
 be what your script happens to use, is not by design the best
 representation of the information for all the script writers to
 want to call _good_.
 
 If we were to do git remote list, I'd imagine it would be far more
 useful to have --format=format specifiers option so that you can
 do something like
 
   git remote list --format=%(name) %(url) (%(direction))
 
 Then scripts can explicitly ask for what they want and have less
 chance of getting broken (I say less because what %(specifier)
 stands for could be changed either to fix mistakes or by mistake).
 
   Having said that, my preference is 
   
   0. Do nothing, but document the default to listing better if
  needed.
   
   and then 2. above, and then 1.
  
  Yeah, I'd agree with that.
 

Personally, I have always disliked that git remote only shows remote
names, which is almost entirely useless to me as a human. Obviously it
is easiest way to actually get the remote names out.

I would much prefer changing the output so that git remote shows all the
output.. But yes, this does potentially break expected output from a git
command that might be used by scripts.

I end up typing git remote and forgetting the -v a lot of the time, so I
have to re-run the command. It has also confused many new people I've
had to teach git.

Regards,
Jake

Re: revert top most commit

[Keller, Jacob E]

On Wed, 2014-08-27 at 18:15 -0400, David Turner wrote:
 On Wed, 2014-08-27 at 21:14 +, Keller, Jacob E wrote:
  Hi,
  
  I am having trouble using revert. If I attempt to
  
  $ git revert sha1id
  
  where sha1id is the same as the HEAD commit, I instead get the output of
  what looks like git status.
  
  Is there anything specific about git revert that prevents it from
  reverting the most recent commit?
 
 Works for me.  What version of git?  Is there a public repo that someone
 could test this out on?
 

It was version 1.9.3 and it's not a public repo.

It's actually not my repository, I was helping a co-worker, and thought
I'd ask if anyone here knew if it was expected behavior or not.

Thanks,
Jake

Re: cherry picking and merge

[Keller, Jacob E]

On Fri, 2014-08-01 at 09:56 -0700, Mike Stump wrote:
 Since everything I do goes up and down into repositories and I don’t want my 
 friends and family to scorn me, rebase isn’t the command I want to use.

You completely mis-understand what published means. Published history
is history from which other people can pull right now.

That means it has to be in a publicly addressable repository (ie: just
like the remote that you are pulling from as upstream).

rebasing commits which are already in the upstream is bad. Rebasing
commits which you have created locally is NOT bad. These commits would
not be published until you do a push.

This is the fundamental issue with rebase, and it is infact easy to
avoid mis-using, especially if you don't publish changes. The key is
that a commit isn't published until it's something someone else can
depend on.

Doing git pull --rebase essentially doesn't ever get you into trouble.

Regards,
Jake

Re: cherry picking and merge

[Keller, Jacob E]

On Thu, 2014-08-21 at 17:36 +, Keller, Jacob E wrote:
 On Fri, 2014-08-01 at 09:56 -0700, Mike Stump wrote:
  Since everything I do goes up and down into repositories and I don’t want 
  my friends and family to scorn me, rebase isn’t the command I want to use.
 
 You completely mis-understand what published means. Published history
 is history from which other people can pull right now.
 
 That means it has to be in a publicly addressable repository (ie: just
 like the remote that you are pulling from as upstream).
 
 rebasing commits which are already in the upstream is bad. Rebasing
 commits which you have created locally is NOT bad. These commits would
 not be published until you do a push.
 
 This is the fundamental issue with rebase, and it is infact easy to
 avoid mis-using, especially if you don't publish changes. The key is
 that a commit isn't published until it's something someone else can
 depend on.
 
 Doing git pull --rebase essentially doesn't ever get you into trouble.
 
 Regards,
 Jake
 �{.n�+���+%��lzwm��b�맲��r��z��{ay�ʇڙ�,j��f���h���z��w���
 ���j:+v���w�j�mzZ+�ݢj��!�i

Pardon me. You can actually ignore this post. I read through more of the
thread, and actually realize I completely misunderstood what your issue
was, and why rebase might not work.

Regards,
Jake

Re: [PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff

[Keller, Jacob E]

On Wed, 2014-07-30 at 20:09 -0400, Jeff King wrote:
 On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:
 
   git log --cc is one of the things I wanted for a long time to fix.
   When the user explicitly asks --cc, we currently ignore it, but
   because we know the user wants to view combined diff, we should turn
   -p on automatically.  And the change this patch introduces will be
   broken when we fix log --cc (stash list will end up always
   showing the patch, without a way to disable it).
  
   Can you make this conditional?  Do this only when options are
   given to git stash list command and that includes -p or
   something?
 
 I'm definitely sympathetic. Using --cc with the diff family _does_
 imply -p already, so it would be nice to do the same for the log
 family.
 
  Perhaps something along this line?
  
   git-stash.sh | 11 +--
   1 file changed, 9 insertions(+), 2 deletions(-)
  
  diff --git a/git-stash.sh b/git-stash.sh
  index ae73ba4..0db1b19 100755
  --- a/git-stash.sh
  +++ b/git-stash.sh
  @@ -297,8 +297,15 @@ have_stash () {
   
   list_stash () {
  have_stash || return 0
  -   git log --format=%gd: %gs -g --cc --simplify-combined-diff \
  -   $@ $ref_stash --
  +   case  $*  in
  +   *' --cc '*)
  +   ;; # the user knows what she is doing
  +   *' -p '* | *' -U'*)
  +   set x --cc --simplify-combined-diff $@
  +   shift
  +   ;;
  +   esac
  +   git log --format=%gd: %gs -g $@ $ref_stash --
 
 Ugh. I'd generally like to avoid this sort of ad-hoc command line
 parsing, as it is easy to get confused by option arguments or
 even non-options. At least we do not have to worry about pathspecs here,
 as we already do an explicit -- (so we might be confused by them, but
 they are broken anyway).
 
 Still, I wonder if a cleaner solution is to provide alternate default
 to this options for the revision.c option parser. We already have
 --default to pick the default starting point. Could we do something
 like --default-merge-handling=cc or something?
 
 There's a similar ad-hoc parsing case in stash show, where we add
 --stat if no arguments are given, but we have no clue if a diff format
 was given (so git stash show --color accidentally turns on patch
 format!). Something like --default-diff-format=stat would be more
 robust.
 
 I dunno. Maybe it is over-engineering. I just hate to see solutions that
 work most of the time and crumble in weird corner cases, even if people
 don't hit those corner cases very often.
 
 -Peff

I agree with you on this one. Those corner cases tend to bite the worst.

Thanks,
Jake

 --
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Wed, 2014-07-16 at 14:48 -0700, Jacob Keller wrote:
 Add support for configuring default sort ordering for git tags. Command
 line option will override this configured value, using the exact same
 syntax.
 
 Cc: Jeff King p...@peff.net
 Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---

Just a ping on the status of this patch?

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

There was no way to get the current error routine now, and I figured
that a stack was a simple way of saving the old routine.

Essentially these two paths would be the same as a save/restore except
we manage it via a stack. I don't really see how that would end up any
different. I mean I don't mind either approach.

Thanks,
Jake

On Tue, 2014-07-15 at 17:49 -0700, Junio C Hamano wrote:
 I actually am not a big fan of stack for a thing like this, to be honest.
 Wouldn't it be sufficient for the callers who want specific behaviour from
 its callees to
 
  - save away the current error/warning routines;
  - set error/warning routines to its own custom versions;
  - call the callees;
  - set error/warning routines back to their original; and
  - return from it
 
 at least in the code paths under discussion?
 
 
 On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E
 jacob.e.kel...@intel.com wrote:
  On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
  Jacob Keller jacob.e.kel...@intel.com writes:
 
extern void set_error_routine(void (*routine)(const char *err, va_list 
   params));
   +extern void pop_error_routine(void);
 
  pop that undoes set smells somewhat weird.  Perhaps we should rename
  set to push?  That would allow us catch possible topics that add new
  calls to set_error_routine() as well by forcing the system not to
  link when they are merged without necessary fixes.
 
 
  Also curious what your thoughts on making every set_*_routine to be a
  stack? For now I was only planning on error but it maybe makes sense to
  change them all?
 
  Thanks,
  Jake
 

Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Wed, 2014-07-16 at 10:59 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:
  On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:
  
   +static void error_bad_sort_config(const char *err, va_list params)
   +{
   +vreportf(warning: tag.sort has , err, params);
   +}
  
  This feels a little like an abuse of the prefix field of vreportf, but
  as you probably saw in my for fun patch, doing it right means
  formatting into a buffer and then reformatting that (which we're
  already doing again in vreportf, but less flexibly). I dunno.
  
  At any rate, this should be marked for translation, no?
  
  -Peff
 
  Oh, yes it probably should. It's definitely a little bit abusive of the
  prefix field, but that seemed easier.
 
 And i18n would automatically mean this will not work, no?  There is
 no guarantee that the translation of warning:  will be followed
 directly by the translation of tag.sort has without any words from
 variable part in all languages.
 
 After all, it seems to me that the one in
 
 http://thread.gmane.org/gmane.comp.version-control.git/253346
 
 struck the right balance among various abuses; let's use the error
 reporter from that version, instead of going down this rabbit hole.
 
 Thanks.
 
 
 

This is fine with me :)

Thanks,
Jake

Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Wed, 2014-07-16 at 10:59 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:
  On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:
  
   +static void error_bad_sort_config(const char *err, va_list params)
   +{
   +vreportf(warning: tag.sort has , err, params);
   +}
  
  This feels a little like an abuse of the prefix field of vreportf, but
  as you probably saw in my for fun patch, doing it right means
  formatting into a buffer and then reformatting that (which we're
  already doing again in vreportf, but less flexibly). I dunno.
  
  At any rate, this should be marked for translation, no?
  
  -Peff
 
  Oh, yes it probably should. It's definitely a little bit abusive of the
  prefix field, but that seemed easier.
 
 And i18n would automatically mean this will not work, no?  There is
 no guarantee that the translation of warning:  will be followed
 directly by the translation of tag.sort has without any words from
 variable part in all languages.
 
 After all, it seems to me that the one in
 
 http://thread.gmane.org/gmane.comp.version-control.git/253346
 
 struck the right balance among various abuses; let's use the error
 reporter from that version, instead of going down this rabbit hole.
 
 Thanks.
 
 
 

So is that patch series version acceptable then? Or should I spin one
again that is in that vein?

Thanks,
Jake

Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Wed, 2014-07-16 at 14:40 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  After all, it seems to me that the one in
  
  http://thread.gmane.org/gmane.comp.version-control.git/253346
  
  struck the right balance among various abuses; let's use the error
  reporter from that version, instead of going down this rabbit hole.
  
  Thanks.
 
  So is that patch series version acceptable then? Or should I spin one
  again that is in that vein?
 
 I do not offhand know what other changes you had in v9 since
 $gmane/253346, so I'll leave it up to you.  If the only difference
 is the error reporting codepath, and you are happy with what I have
 in my tree
 
 $ git log -p --reverse -3 a93d886b9
 
 then we can proceed with that version.  Have there been any issues
 raised on that older version other than error reporting?
 
 
 
 

I'll double check.

Thanks,
Jake

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
 
  I realize that I am reinventing the error-reporting wheel on a sleepy
  Sunday afternoon without having thought about it much, so there is
  probably some gotcha or case that makes this ugly, or perhaps it just
  ends up verbose in practice. But one can dream.
 
  Just for fun...
 
 Yes, that is fun.
 
 I actually think your In 'version:pefname' and 'wersion:refname',
 we want be able to report 'pefname' and 'wersion' are misspelled,
 and returning -1 or enum would not cut it is a good argument.  The
 callee wants to have flexibility on _what_ to report, just as the
 caller wants to have flexibility on _how_.  In this particular code
 path, I think the former far outweighs the latter, and my suggestion
 I called silly might not be so silly but may have struck the right
 balance.  I dunno.
 
 If you absolutely need to have both, you would need something like
 your approach, of course, but I am not sure if it is worth it.
 
 I am not sure how well this meshes with i18n (I know the for fun
 does not even attempt to, but if we tried to, I suspect it may
 become even uglier).  We would also need to override both error and
 warning routines and have the reporter tag the errors in these two
 categories, no?
 

Do we want to go this way? Should I respin my patch (again)? I'm not
sure we really need to get that complex.. I do like parsing errors a bit
cleaner and indicating what part is bad.. Note that our current parsing
method does not make it really possible to indicate which part is wrong.

Thanks,
Jake

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
  Jeff King p...@peff.net writes:
  
   On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
  
   I realize that I am reinventing the error-reporting wheel on a sleepy
   Sunday afternoon without having thought about it much, so there is
   probably some gotcha or case that makes this ugly, or perhaps it just
   ends up verbose in practice. But one can dream.
  
   Just for fun...
  
  Yes, that is fun.
  
  I actually think your In 'version:pefname' and 'wersion:refname',
  we want be able to report 'pefname' and 'wersion' are misspelled,
  and returning -1 or enum would not cut it is a good argument.  The
  callee wants to have flexibility on _what_ to report, just as the
  caller wants to have flexibility on _how_.  In this particular code
  path, I think the former far outweighs the latter, and my suggestion
  I called silly might not be so silly but may have struck the right
  balance.  I dunno.
  
  If you absolutely need to have both, you would need something like
  your approach, of course, but I am not sure if it is worth it.
  
  I am not sure how well this meshes with i18n (I know the for fun
  does not even attempt to, but if we tried to, I suspect it may
  become even uglier).  We would also need to override both error and
  warning routines and have the reporter tag the errors in these two
  categories, no?
 
  Do we want to go this way?
 
 I do not speak for Peff, but I personally think this is just a fun
 demonstration, nothing more, and my gut feeling is that it would
 make things unnecessary complex without much real gain to pursue it
 further.

I agree. But what about going back to the older setup where the caller
can output correct error message? I'm ok with using an enum style
return, to be completely honest. I would prefer this, actually.

Thanks,
Jake

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Tue, 2014-07-15 at 11:17 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
  ...
   Yes, that is fun.
   
   I actually think your In 'version:pefname' and 'wersion:refname',
   we want be able to report 'pefname' and 'wersion' are misspelled,
   and returning -1 or enum would not cut it is a good argument.  The
   callee wants to have flexibility on _what_ to report, just as the
   caller wants to have flexibility on _how_.  In this particular code
   path, I think the former far outweighs the latter, and my suggestion
   I called silly might not be so silly but may have struck the right
   balance.  I dunno.
  ...
  I agree. But what about going back to the older setup where the caller
  can output correct error message? I'm ok with using an enum style
  return, to be completely honest. I would prefer this, actually.
 
 Depends on which older setup you mean, I think.  The one that does
 not let us easily give more context dependent diagnoses that lets us
 distinguish between version:pefname and version:refname by returning
 only -1 or an enum?
 

I am going to re-submit this with an enum-style return. I am also
changing how we parse so that we can correctly report whether the sort
function or sort atom is incorrect.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  I am going to re-submit this with an enum-style return. I am also
  changing how we parse so that we can correctly report whether the sort
  function or sort atom is incorrect.
 
 Oh, our mails crossed, I guess.  As long as it will leave the door
 open for later enhancements for more context sensitive error
 diagnosis, I do not particularly mind a solution around enum.

Hmm. I looked at coding it this way, and it actually makes it less
sensitive than I would like. I'm not a fan of the extra value
parameter, but I do like a more proper error display, and indeed one
that is more precise.

I'll try to have a new series posted soon which takes these into
account.

Regards,
Jake

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  I am going to re-submit this with an enum-style return. I am also
  changing how we parse so that we can correctly report whether the sort
  function or sort atom is incorrect.
 
 Oh, our mails crossed, I guess.  As long as it will leave the door
 open for later enhancements for more context sensitive error
 diagnosis, I do not particularly mind a solution around enum.

I just sent a v8 of the series. I think I mostly followed Peff's idea of
using a pop_error_routine function, but not as complex as his was. This
overall results in more accurate errors, and doesn't clutter the
original parse_sort_string with too much knowledge about what particular
value is being parsed. Hopefully we can finally converge on a good set
of patches.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
   extern void set_error_routine(void (*routine)(const char *err, va_list 
  params));
  +extern void pop_error_routine(void);
 
 pop that undoes set smells somewhat weird.  Perhaps we should rename
 set to push?  That would allow us catch possible topics that add new
 calls to set_error_routine() as well by forcing the system not to
 link when they are merged without necessary fixes.
 

I thought about changing set too, but wasn't sure that made sense..?
That does make more sense now though. There *are* valid use cases where
a set_error_routine is used without a pop, (the one current use, I
think).

I'll update this patch with that change.

  +/* push error routine onto the error function stack */
   void set_error_routine(void (*routine)(const char *err, va_list params))
   {
  -   error_routine = routine;
  +   struct error_func_list *efl = xmalloc(sizeof(*efl));
  +   efl-func = routine;
  +   efl-next = error_funcs;
  +   error_funcs = efl;
  +}
  +
  +/* pop a single error routine off of the error function stack, thus 
  reverting
  + * to previous error. Should always be paired with a set_error_routine */
  +void pop_error_routine(void)
  +{
  +   assert(error_funcs != default_error_func);
  +
  +   struct error_func_list *efl = error_funcs;
 
 decl-after-stmt.  Can be fixed easily by flipping the above two
 lines.

Oh, right yes. I'll fix that in a resend as well.

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
   extern void set_error_routine(void (*routine)(const char *err, va_list 
  params));
  +extern void pop_error_routine(void);
 
 pop that undoes set smells somewhat weird.  Perhaps we should rename
 set to push?  That would allow us catch possible topics that add new
 calls to set_error_routine() as well by forcing the system not to
 link when they are merged without necessary fixes.
 

Also curious what your thoughts on making every set_*_routine to be a
stack? For now I was only planning on error but it maybe makes sense to
change them all?

Thanks,
Jake

Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:
 On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:
 
  +static void error_bad_sort_config(const char *err, va_list params)
  +{
  +   vreportf(warning: tag.sort has , err, params);
  +}
 
 This feels a little like an abuse of the prefix field of vreportf, but
 as you probably saw in my for fun patch, doing it right means
 formatting into a buffer and then reformatting that (which we're
 already doing again in vreportf, but less flexibly). I dunno.
 
 At any rate, this should be marked for translation, no?
 
 -Peff

Oh, yes it probably should. It's definitely a little bit abusive of the
prefix field, but that seemed easier.

Thanks,
Jake

RE: [Bug] data loss with cyclic alternates

[Keller, Jacob E]

 -Original Message-
 From: Jeff King [mailto:p...@peff.net]
 Sent: Friday, July 11, 2014 10:57 PM
 To: Keller, Jacob E
 Cc: gits...@pobox.com; dr.kh...@gmail.com; git@vger.kernel.org
 Subject: Re: [Bug] data loss with cyclic alternates
 
 On Fri, Jul 11, 2014 at 06:01:46PM +, Keller, Jacob E wrote:
 
   Yeah, don't do that.  A thinks eh, the other guy must have it and
   B thinks the same.  In general, do not prune or gc a repository
   other repositories borrow from, even if there is no cycle, because
   the borrowee does not know anythning about objects that it itself no
   longer needs but are still needed by its borrowers.
  
 
  Doesn't gc get run automatically at some points? Is the automatic gc run
  setup to avoid this problem?
 
 No, the automatic gc doesn't avoid this. It can't in the general case,
 as the parent repository does not know how many or which children are
 pointed to it as an alternate. And the borrowing repository does not
 even need to have write permission to the parent, so it cannot write a
 backpointer.
 
 If people are using alternates, they should probably turn off gc.auto in
 the borrowee (it doesn't seem unreasonable to me to do so automatically
 via clone -s in cases where we can write to the alternates repo, and
 to issue a warning otherwise).
 
 -Peff

Yes, if this is a known problem and we can avoid it by disabling garbage 
collection, that makes sense to me to do so..

Thanks,
Jake

RE: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

 -Original Message-
 From: Junio C Hamano [mailto:gits...@pobox.com]
 Sent: Sunday, July 13, 2014 10:01 AM
 To: Jeff King
 Cc: Keller, Jacob E; git@vger.kernel.org
 Subject: Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
 
 Jeff King p...@peff.net writes:
 
  On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote:
 
   Yeah, we're quite inconsistent there. In some cases we silently
 ignore
   something unknown (e.g., a color.diff.* slot that we do not
 understand),
   but in most cases if it is a config key we understand but a value we
 do
   not, we complain and die.
 
  Hm, that's bad---we've become less and less careful over time,
  perhaps?
 
  I don't think so. I think we've always been not-very-lenient with
  parsing values. Two examples:
  ...
  So I do not think we have ever had a rule, but if we did, it is probably
  silently ignore unknown keys, complain on unknown values.
 
 Yeah, somehow I was mixing up these two cases fuzzily in my mind
 while responding.  Rejecting unknown keys is bad, but we cannot be
 perfectly forward compatible nor behave sensibly on unknown values
 while issuing errors against known-to-be-bad values, so your rule
 above sounds like the most sensible thing to do.

The only difference is whether we halt or ignore the unknown value we 
complained about. Personally I am ok with either, but prefer the complain and 
choose the default so that older gits don't completely stop. But in some 
cases, the change is not easily compatible so then stopping might be preferred 
as the old git will not behave as expected.

Thanks,
Jake
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  Add support for configuring default sort ordering for git tags. Command
  line option will override this configured value, using the exact same
  syntax.
 
  Cc: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
  - v4
  * base on top of suggested change by Jeff King to use skip_prefix instead
 
   Documentation/config.txt  |  6 ++
   Documentation/git-tag.txt |  1 +
   builtin/tag.c | 46 
  --
   t/t7004-tag.sh| 21 +
   4 files changed, 60 insertions(+), 14 deletions(-)
 
  diff --git a/Documentation/config.txt b/Documentation/config.txt
  index 1d718bdb9662..ad8e75fed988 100644
  --- a/Documentation/config.txt
  +++ b/Documentation/config.txt
  @@ -2354,6 +2354,12 @@ submodule.name.ignore::
  --ignore-submodules option. The 'git submodule' commands are not
  affected by this setting.
   
  +tag.sort::
  +   This variable is used to control the sort ordering of tags. It is
  +   interpreted precisely the same way as --sort=value. If --sort is
  +   given on the command line it will override the selection chosen in the
  +   configuration. See linkgit:git-tag[1] for more details.
  +
 
 This is not technically incorrect per-se, but the third sentence
 talks about --sort on the command line while this applies only
 to the command line of the 'git tag' command and nobody else's
 --sort option.
 
 Perhaps rephrasing it like this may be easier to understand?
 
   When git tag command is used to list existing tags,
 without --sort=value option on the command line,
   the value of this variable is used as the default.
 
 This way, it would be clear, without explicitly saying anything,
 that the value will be spelled exactly the same way as you would
 spell the value for the --sort option on the command line.
 

Makes sense.

  diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
  index b424a1bc48bb..2d246725aeb5 100644
  --- a/Documentation/git-tag.txt
  +++ b/Documentation/git-tag.txt
  @@ -317,6 +317,7 @@ include::date-formats.txt[]
   SEE ALSO
   
   linkgit:git-check-ref-format[1].
  +linkgit:git-config[1].
 
 It is not particularly friendly to readers to refer to
 git-config[1] from any other page, if done without spelling out
 which variable the reader is expected to look up.  Some addition
 to the description of the --sort option is needed if this SEE ALSO
 were to be any useful, I guess?
 
   --sort=type::
 ... (existing description) ...
 When this option is not given, the sort order
 defaults to the value configured for the `tag.sort`
 variable, if exists, or lexicographic otherwise.
 
 or something like that, perhaps?
 

Alright, this sounds good too.

  diff --git a/builtin/tag.c b/builtin/tag.c
  index 7ccb6f3c581b..a53e27d4e7e4 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -18,6 +18,8 @@
   #include sha1-array.h
   #include column.h
   
  +static int tag_sort = 0;
 
 Please do not initialize variables in bss segment to 0 by hand.
 
 If this variable is meant to take one of these *CMP_SORT values
 defined as macro later in this file, it is better to define this
 variable somewhere after and close to the definitions of the macros.
 Perhaps immediately after the struct tag_filter is declared?
 

I put it just above the struct tag_filter now, as this puts it right
below the #defines regarding it's value.

  @@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
  Lines starting with '%c' will be kept; you may remove them
   yourself if you want to.\n);
   
  +static int parse_sort_string(const char *arg)
  +{
  +   int sort = 0;
  +   int flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   sort = VERCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   die(_(unsupported sort specification %s), arg);
 
 Hmm.  I _thought_ we try to catch unsupported option value coming
 from the command line and die but at the same time we try *not* to
 die but warn and whatever is sensible when the value comes from the
 configuration, so that .git/config or $HOME/.gitconfig can be shared
 by those who use different versions of Git.
 
 Do we already have many precedences where we see configuration value
 that our version of Git do not yet understand?
 
 Not a very strong objection; just something that worries me.
 

I like this change, and I think I have a way to handle it. I will
re-work this so that the die is handled by the normal block.

  +   sort |= flags;
  +
  +   return sort;
  +}
  +
   static int git_tag_config(const char *var, const char *value, void *cb)
   {
  -   int status = git_gpg_config(var, value, 

Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  Add support for configuring default sort ordering for git tags. Command
  line option will override this configured value, using the exact same
  syntax.
 
  Cc: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
  - v4
  * base on top of suggested change by Jeff King to use skip_prefix instead
 
   Documentation/config.txt  |  6 ++
   Documentation/git-tag.txt |  1 +
   builtin/tag.c | 46 
  --
   t/t7004-tag.sh| 21 +
   4 files changed, 60 insertions(+), 14 deletions(-)
 
  diff --git a/Documentation/config.txt b/Documentation/config.txt
  index 1d718bdb9662..ad8e75fed988 100644
  --- a/Documentation/config.txt
  +++ b/Documentation/config.txt
  @@ -2354,6 +2354,12 @@ submodule.name.ignore::
  --ignore-submodules option. The 'git submodule' commands are not
  affected by this setting.
   
  +tag.sort::
  +   This variable is used to control the sort ordering of tags. It is
  +   interpreted precisely the same way as --sort=value. If --sort is
  +   given on the command line it will override the selection chosen in the
  +   configuration. See linkgit:git-tag[1] for more details.
  +
 
 This is not technically incorrect per-se, but the third sentence
 talks about --sort on the command line while this applies only
 to the command line of the 'git tag' command and nobody else's
 --sort option.
 
 Perhaps rephrasing it like this may be easier to understand?
 
   When git tag command is used to list existing tags,
 without --sort=value option on the command line,
   the value of this variable is used as the default.
 
 This way, it would be clear, without explicitly saying anything,
 that the value will be spelled exactly the same way as you would
 spell the value for the --sort option on the command line.
 
  diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
  index b424a1bc48bb..2d246725aeb5 100644
  --- a/Documentation/git-tag.txt
  +++ b/Documentation/git-tag.txt
  @@ -317,6 +317,7 @@ include::date-formats.txt[]
   SEE ALSO
   
   linkgit:git-check-ref-format[1].
  +linkgit:git-config[1].
 
 It is not particularly friendly to readers to refer to
 git-config[1] from any other page, if done without spelling out
 which variable the reader is expected to look up.  Some addition
 to the description of the --sort option is needed if this SEE ALSO
 were to be any useful, I guess?
 
   --sort=type::
 ... (existing description) ...
 When this option is not given, the sort order
 defaults to the value configured for the `tag.sort`
 variable, if exists, or lexicographic otherwise.
 
 or something like that, perhaps?
 
  diff --git a/builtin/tag.c b/builtin/tag.c
  index 7ccb6f3c581b..a53e27d4e7e4 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -18,6 +18,8 @@
   #include sha1-array.h
   #include column.h
   
  +static int tag_sort = 0;
 
 Please do not initialize variables in bss segment to 0 by hand.
 
 If this variable is meant to take one of these *CMP_SORT values
 defined as macro later in this file, it is better to define this
 variable somewhere after and close to the definitions of the macros.
 Perhaps immediately after the struct tag_filter is declared?
 
  @@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
  Lines starting with '%c' will be kept; you may remove them
   yourself if you want to.\n);
   
  +static int parse_sort_string(const char *arg)
  +{
  +   int sort = 0;
  +   int flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   sort = VERCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   die(_(unsupported sort specification %s), arg);
 
 Hmm.  I _thought_ we try to catch unsupported option value coming
 from the command line and die but at the same time we try *not* to
 die but warn and whatever is sensible when the value comes from the
 configuration, so that .git/config or $HOME/.gitconfig can be shared
 by those who use different versions of Git.
 
 Do we already have many precedences where we see configuration value
 that our version of Git do not yet understand?
 
 Not a very strong objection; just something that worries me.
 
  +   sort |= flags;
  +
  +   return sort;
  +}
  +
   static int git_tag_config(const char *var, const char *value, void *cb)
   {
  -   int status = git_gpg_config(var, value, cb);
  +   int status;
  +
  +   if (!strcmp(var, tag.sort)) {
  +   tag_sort = parse_sort_string(value);
  +   }
  +
 
 Why doesn't this return success after noticing that the variable is
 to be interpreted by this block and nobody else?
 
 When there is no reason to have things in 

Re: pitfall with empty commits during git rebase

[Keller, Jacob E]

On Fri, 2014-07-11 at 12:15 +0200, Olaf Hering wrote:
 There is an incorrect message when doing git rebase -i remote/branch.
 I have it only in german, see below. what happend is:
 
 #01 make changes on another host
 #02 copy patchfile to localhost
 #03 apply patchfile
 #04 git commit -avs # create commit#1
 #05 sleep 123456
 #06 make different changes on another host
 #07 copy patchfile to localhost
 #08 git show | patch -Rp1
 #09 git commit -avs # create commit#2
 #10 apply patchfile
 #11 git commit -avs # create commit#3
 #12 git rebase -i remote/branch
  pick commit#1 msg
  fcommit#2 msg
  fcommit#3 msg
 
 
 .git/rebase-merge/git-rebase-todo 21L, 707C geschrieben
 Sie fragten den jüngsten Commit nachzubessern, aber das würde diesen leer
 machen. Sie können Ihr Kommando mit --allow-empty wiederholen, oder diesen
 Commit mit git reset HEAD^ vollständig entfernen.
 Rebase im Gange; auf c105589
 Sie sind gerade beim Rebase von Branch 'mybranch' auf 'c105589'.
 
 Keine Änderungen
 
 Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert
 
 
 Its not clear what '--allow-empty' refers to, git rebase does not seem to
 understand this option.
 

You should be able to fix this with

git commit --allow-empty
git rebase --continue

But yes the message could possibly be made a little clearer.

Thanks,
Jake

 I should have skipped step #09 to avoid the trouble.
 git version is 2.0.1. Please adjust the error msg above.
 
 
 Olaf
 --
 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


N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [Bug] data loss with cyclic alternates

[Keller, Jacob E]

On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote:
 Ephrim Khong dr.kh...@gmail.com writes:
 
  git seems to have issues with alternates when cycles are present (repo
  A has B/objects as alternates, B has A/objects as alternates).
 
 Yeah, don't do that.  A thinks eh, the other guy must have it and
 B thinks the same.  In general, do not prune or gc a repository
 other repositories borrow from, even if there is no cycle, because
 the borrowee does not know anythning about objects that it itself no
 longer needs but are still needed by its borrowers.
 

Doesn't gc get run automatically at some points? Is the automatic gc run
setup to avoid this problem? 

Thanks,
Jake

 --
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Keller, Jacob E]

On Fri, 2014-07-11 at 13:50 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:
 
  Make the parsing of the --sort parameter more readable by having
  skip_prefix keep our pointer up to date.
  
  Authored-by: Jeff King p...@peff.net
 
 I suspect Junio may just apply this on the version of the commit he has
 upstream, so you may not need this as part of your series.
 
 However, for reference, the right way to handle authorship is with a
 
   From: Jeff King p...@peff.net
 
 at the top of your message body. Git-am will pick that up and turn it
 into the author field of the commit.
 

Alright, thanks.

Regards,
Jake

 -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 3/3 v5] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
 
  Updated to include changes due to Junio's feedback. This has not resolved
  whether we should fail on a configuration error or simply warn. It appears 
  that
  we actually seem to error out more than warn, so I am unsure what the 
  correct
  action is here.
 
 Yeah, we're quite inconsistent there. In some cases we silently ignore
 something unknown (e.g., a color.diff.* slot that we do not understand),
 but in most cases if it is a config key we understand but a value we do
 not, we complain and die.
 
 It's probably user-unfriendly to be silent for those cases, though. The
 user gets no feedback on why their config value is doing nothing.
 
 I tend to think that warning is not much better than erroring out. It is
 helpful if you are running a single-shot of an old version (which is
 something that I do a lot when testing old versions), but would quickly
 become irritating if you were actually using an old version of git
 day-to-day.
 
 I dunno. Maybe it is worth making life easier for people in the former
 category.
 
  +static int parse_sort_string(const char *arg, int *sort)
  +{
  +   int type = 0, flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   type = VERCMP_SORT;
  +   else
  +   type = STRCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   return error(_(unsupported sort specification %s), arg);
  +
  +   *sort = (type | flags);
  +
  +   return 0;
  +}
 
 Regardless of how we handle the error, I think this version that
 assembles the final bitfield at the end is easier to read than the
 original.
 

Yes, I figured setting it up all at the end makes more sense, and is
less prone to subtle bugs, since we don't directly modify sort using |=
or relying on particular values of sort initially.

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal bad config
message. Any thoughts?

Thanks,
Jake

 -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 3/3 v5] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 14:22 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote:
 
  I personally prefer error out on options, even though it can make it a
  bit more difficult, though as far as I know unknown fields simply warn
  or are ignored. (ie: old versions of git just ignore unknown fields in
  configuration).
 
 Right, we _have_ to ignore unknown config options, because we
 specifically allow other programs built on git to store their config
 with us (and anyway, our callback style of parsing means that no single
 callback knows about all of the keys).
 
 In the past we have staked out particular areas of the namespace,
 though. E.g., the diff code said I own all of color.diff.*, and if you
 put in something I don't understand, I'll complain. That ended up being
 annoying, and now we ignore slots we don't understand there.
 
 So old gits will always silently ignore tag.sort if they don't know
 about it, and we can't change that. The only thing we can change is:
 
  It's possible we should warn instead though, so that older gits work
  with new sorts that they don't understand.
 
 Right. I think other config variables in similar situations will barf.
 This is backwards-compatible as long as the new variables are a superset
 (i.e., we only add new understood values, never remove or change the
 meaning of existing values). It's just not forwards-compatible.
 

So should I respin this so that config option doesn't error out?

  I am ok with warning but I don't know the best practice for how to warn
  here instead of failing. Returning error causes a fatal bad config
  message. Any thoughts?
 
 The simplest thing is ignoring the return from parse_sort_string and
 just calling return 0. That will still say error:, but continue on.
 If you really want it to say warning:, I think you'll have to pass a
 flag into parse_sort_string. I'm not sure if it's worth the effort.
 
 -Peff

Ok this makes sense, I am fine leaving it as error. Should I respin to
make it not die though?

Thanks,
Jake

Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 11:29 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  This is not how the rest of the current tests work. I will submit a
  patch which fixes up the current --sort tests (but not every test, for
  now) as well.
 
 I do not want to pile more work that is unrelated to the task at
 hand on your plate, i.e. clean-up work, so I would assign a very low
 priority to fix up the current tests.  At the same time, existing
 mistakes are not excuses to introduce new similar ones, hence my
 suggestions to the added code in the patch I saw.
 

It was trivial to fix at least the --sort tests, so I submitted a patch
that goes before this one to fix those as well.

Thanks,
Jake

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 13:51 -0700, Jacob Keller wrote:
 Add support for configuring default sort ordering for git tags. Command
 line option will override this configured value, using the exact same
 syntax.
 
 Cc: Jeff King p...@peff.net
 Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
 ---
 Updated based on Junio's suggestions, as well as making sure that we don't 
 bail
 if we can't understand config's option. We still print error message followed
 by a warning about using default order. In addition, added a few more tests to
 ensure that these work as expected.
 
  Documentation/config.txt  |  5 
  Documentation/git-tag.txt |  5 +++-
  builtin/tag.c | 61 
 ++-
  t/t7004-tag.sh| 36 
  4 files changed, 90 insertions(+), 17 deletions(-)
 
 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 1d718bdb9662..c55c22ab7be9 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -2354,6 +2354,11 @@ submodule.name.ignore::
   --ignore-submodules option. The 'git submodule' commands are not
   affected by this setting.
  
 +tag.sort::
 + This variable controls the sort ordering of tags when displayed by
 + linkgit:git-tag[1]. Without the --sort=value option provided, the
 + value of this variable will be used as the default.
 +
  tar.umask::
   This variable can be used to restrict the permission bits of
   tar archive entries.  The default is 0002, which turns off the
 diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
 index b424a1bc48bb..320908369f06 100644
 --- a/Documentation/git-tag.txt
 +++ b/Documentation/git-tag.txt
 @@ -99,7 +99,9 @@ OPTIONS
   Sort in a specific order. Supported type is refname
   (lexicographic order), version:refname or v:refname (tag
   names are treated as versions). Prepend - to reverse sort
 - order.
 + order. When this option is not given, the sort order defaults to the
 + value configured for the 'tag.sort' variable if it exists, or
 + lexicographic order otherwise. See linkgit:git-config[1].
  
  --column[=options]::
  --no-column::
 @@ -317,6 +319,7 @@ include::date-formats.txt[]
  SEE ALSO
  
  linkgit:git-check-ref-format[1].
 +linkgit:git-config[1].
  
  GIT
  ---
 diff --git a/builtin/tag.c b/builtin/tag.c
 index 7ccb6f3c581b..cb37b26159a6 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
  #define SORT_MASK   0x7fff
  #define REVERSE_SORT0x8000
  
 +static int tag_sort;
 +
  struct tag_filter {
   const char **patterns;
   int lines;
 @@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
   Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
  
 +/*
 + * Parse a sort string, and return 0 if parsed successfully. Will return
 + * non-zero when the sort string does not parse into a known type.
 + */
 +static int parse_sort_string(const char *arg, int *sort)
 +{
 + int type = 0, flags = 0;
 +
 + if (skip_prefix(arg, -, arg))
 + flags |= REVERSE_SORT;
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 + type = VERCMP_SORT;
 + else
 + type = STRCMP_SORT;
 +
 + if (strcmp(arg, refname))
 + return error(_(unsupported sort specification %s), arg);
 +
 + *sort = (type | flags);
 +
 + return 0;
 +}
 +
  static int git_tag_config(const char *var, const char *value, void *cb)
  {
 - int status = git_gpg_config(var, value, cb);
 + int status;
 +
 + if (!strcmp(var, tag.sort)) {
 + if (!value)
 + return config_error_nonbool(var);
 + status = parse_sort_string(value, tag_sort);
 + if (status) {
 + warning(using default lexicographic sort order);

Oops, I should also use warning(_()) here as well I believe...

Sorry for thrash.

Thanks,
Jake

 + tag_sort = STRCMP_SORT;
 + }
 + return 0;
 + }
 +
 + status = git_gpg_config(var, value, cb);
   if (status)
   return status;
   if (starts_with(var, column.))
 @@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt 
 __attribute__((unused)),
  static int parse_opt_sort(const struct option *opt, const char *arg, int 
 unset)
  {
   int *sort = opt-value;
 - int flags = 0;
  
 - if (skip_prefix(arg, -, arg))
 - flags |= REVERSE_SORT;
 -
 - if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 - *sort = VERCMP_SORT;
 -
 - if (strcmp(arg, refname))
 - die(_(unsupported sort specification %s), arg);
 - *sort |= flags;
 - return 0;
 + return parse_sort_string(arg, sort);
  }
  
  int cmd_tag(int argc, const char **argv, const char *prefix)

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
 
  +   if (!strcmp(var, tag.sort)) {
  +   if (!value)
  +   return config_error_nonbool(var);
  +   status = parse_sort_string(value, tag_sort);
  +   if (status) {
  +   warning(using default lexicographic sort order);
  +   tag_sort = STRCMP_SORT;
  +   }
 
 I think this is a good compromise of the issues we discussed earlier. As
 you noted, it should probably be marked for translation. I'm also not
 sure the message content is clear in all situations. If I have a broken
 tag.sort, I get:
 
   $ git config tag.sort bogus
   $ git tag /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order
 
 Not too bad, though reminding me that the value bogus came from
 tag.sort would be nice. But what if I override it:
 
   $ git tag --sort=v:refname /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order
 
 That's actively wrong, because we are using v:refname from the
 command-line. Perhaps we could phrase it like:
 
   warning: ignoring invalid config option tag.sort
 

ok that makes more sense. I can't really avoid printing the warning at
all since config parsing happens before the option parsing.

I like this wording. I will respin again :D

Thanks,
Jake

 or similar, which helps both cases.
 
 As an aside, I also think the error line could more clearly mark the
 literal contents of the variable. E.g., one of:
 
   error: unsupported sort specification: bogus
 
 or
 
   error: unsupported sort specification 'bogus'
 
 -Peff

Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 14:54 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
 
  Updated to include changes due to Junio's feedback. This has not resolved
  whether we should fail on a configuration error or simply warn. It appears 
  that
  we actually seem to error out more than warn, so I am unsure what the 
  correct
  action is here.
 
  Yeah, we're quite inconsistent there. In some cases we silently ignore
  something unknown (e.g., a color.diff.* slot that we do not understand),
  but in most cases if it is a config key we understand but a value we do
  not, we complain and die.
 
 Hm, that's bad---we've become less and less careful over time,
 perhaps?
 
 As we want to be able to enhance semantics of existing configuration
 variables without having to introduce new but similar ones, we would
 really want to make sure that those who share the same .git/config
 or $HOME/.gitconfig across different versions of Git would not have
 to suffer too much (i.e. forcing them to config --unset when using
 their older Git is not nice).
 
  It's probably user-unfriendly to be silent for those cases, though. The
  user gets no feedback on why their config value is doing nothing.
 
  I tend to think that warning is not much better than erroring out. It is
  helpful if you are running a single-shot of an old version (which is
  something that I do a lot when testing old versions), but would quickly
  become irritating if you were actually using an old version of git
  day-to-day.
 
  I dunno. Maybe it is worth making life easier for people in the former
  category.
 
 ... former cat meaning less irritating for single-shot use?  I
 dunno...
 
  +static int parse_sort_string(const char *arg, int *sort)
  +{
  +  int type = 0, flags = 0;
  +
  +  if (skip_prefix(arg, -, arg))
  +  flags |= REVERSE_SORT;
  +
  +  if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +  type = VERCMP_SORT;
  +  else
  +  type = STRCMP_SORT;
  +
  +  if (strcmp(arg, refname))
  +  return error(_(unsupported sort specification %s), arg);
  +
  +  *sort = (type | flags);
  +
  +  return 0;
  +}
 
  Regardless of how we handle the error, I think this version that
  assembles the final bitfield at the end is easier to read than the
  original.
 
 Yes, this part really is nicely done, I agree.

The current revision of the patch prints an error and warning about not
using the configured tag value. It does still run. I added a test to
ensure this as well.

The easiest way to change overall behavior is to change the git-config's
die_on_error to be false, so that we no longer die on configuration
errors.

It appears this would resolve the issue for many configuration options
(not all, as I think a few are still hard coded to die) but it would be
a change that is well outside the scope of this patch.

I think that for now behavior I added is good, as we *know* that
tag.sort will add new parameters in the near future, so it makes no
sense to have it die on a config that is only slightly newer than the
git version.

Glad I could help. Junio if you could review the v7 I sent a bit ago for
any possible mistakes that I forgot to clean up that would be great.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Fri, 2014-07-11 at 15:17 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
 
  +  if (!strcmp(var, tag.sort)) {
  +  if (!value)
  +  return config_error_nonbool(var);
  +  status = parse_sort_string(value, tag_sort);
  +  if (status) {
  +  warning(using default lexicographic sort order);
  +  tag_sort = STRCMP_SORT;
  +  }
 
  I think this is a good compromise of the issues we discussed earlier. As
  you noted, it should probably be marked for translation. I'm also not
  sure the message content is clear in all situations. If I have a broken
  tag.sort, I get:
 
$ git config tag.sort bogus
$ git tag /dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
 
  Not too bad, though reminding me that the value bogus came from
  tag.sort would be nice. But what if I override it:
 
$ git tag --sort=v:refname /dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
 
  That's actively wrong, because we are using v:refname from the
  command-line.  Perhaps we could phrase it like:
 
warning: ignoring invalid config option tag.sort
 
  or similar, which helps both cases.
 
 Hmph.  Looks like a mild-enough middle ground, I guess.  As
 parse_sort_string() is private for git tag implementation, I
 actually would not mind if we taught a bit more context to it and
 phrase its messages differently.  Perhaps something like this, where
 the config parser will tell what variable it came from with var
 and the command line parser will pass NULL there.
 
 static int parse_sort_string(const char *var, const char *value, int *sort)
 {
   ...
   if (strcmp(value, refname)) {
   if (!var)
   return error(_(unsupported sort specification '%s'), 
 value);
   else {
   warning(_(unsupported sort specification '%s' in 
 variable '%s'),
   var, value);
   return -1;
   }
   }
 
   *sort = (type | flags);
 
   return 0;
 }
 

Ok..  I suppose that could be done.

Thanks,
Jake

  As an aside, I also think the error line could more clearly mark the
  literal contents of the variable. E.g., one of:
 
error: unsupported sort specification: bogus
 
  or
 
error: unsupported sort specification 'bogus'
 
 Yup.

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

[Keller, Jacob E]

On Fri, 2014-07-11 at 15:44 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  From: Jeff King p...@peff.net
 
  Make the parsing of the --sort parameter more readable by having
  skip_prefix keep our pointer up to date.
 
  Signed-off-by: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
   builtin/tag.c | 14 --
   1 file changed, 4 insertions(+), 10 deletions(-)
 
  diff --git a/builtin/tag.c b/builtin/tag.c
  index ef765563388c..7ccb6f3c581b 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, 
  const char *arg, int unset)
  int *sort = opt-value;
  int flags = 0;
   
  -   if (*arg == '-') {
  +   if (skip_prefix(arg, -, arg))
  flags |= REVERSE_SORT;
  -   arg++;
  -   }
  -   if (starts_with(arg, version:)) {
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  *sort = VERCMP_SORT;
  -   arg += 8;
  -   } else if (starts_with(arg, v:)) {
  -   *sort = VERCMP_SORT;
  -   arg += 2;
  -   } else
  -   *sort = STRCMP_SORT;
  +
 
 By losing *sort = STRCMP_SORT, the version after this patch would
 stop clearing what is pointed by opt-value, so
 
   git tag --sort=v:refname --sort=refname
 
 would no longer implement the last one wins semantics, no?
 
 Am I misreading the patch?  I somehow thought Peff's original was
 clearing the variable...
 
  if (strcmp(arg, refname))
  die(_(unsupported sort specification %s), arg);
  *sort |= flags;

Indeed. My patch fixes this up but I will re-work this so we don't
introduce an inbetween bug :)

Thanks,
Jake

Re: [PATCH v2] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Thu, 2014-07-10 at 00:07 -0400, Jeff King wrote:
 On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
 
  +static int parse_sort_string(const char *arg)
  +{
  +   int sort = 0;
  +   int flags = 0;
  +
  +   if (*arg == '-') {
  +   flags |= REVERSE_SORT;
  +   arg++;
  +   }
  +   if (starts_with(arg, version:)) {
  +   sort = VERCMP_SORT;
  +   arg += 8;
  +   } else if (starts_with(arg, v:)) {
  +   sort = VERCMP_SORT;
  +   arg += 2;
  +   } else
  +   sort = STRCMP_SORT;
  +   if (strcmp(arg, refname))
  +   die(_(unsupported sort specification %s), arg);
  +   sort |= flags;
  +
  +   return sort;
  +}
 
 I know this is existing code you are moving, but I noticed it looks ripe
 for using skip_prefix. Perhaps while we are in the area we should do the
 following on top of your patch (I'd also be happy for it be squashed
 in, but that may be too much in one patch).
 

I am fine with this being another patch or squashed in. I didn't even
know that was available :) I also like putting the two equivalent
conditionals into the same if block.

Thanks,
Jake

 -- 8 --
 Subject: [PATCH] tag: use skip_prefix instead of magic numbers
 
 We can make the parsing of the --sort parameter a bit more
 readable by having skip_prefix keep our pointer up to date.
 
 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/tag.c | 14 +-
  1 file changed, 5 insertions(+), 9 deletions(-)
 
 diff --git a/builtin/tag.c b/builtin/tag.c
 index a679e32..016df98 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -353,18 +353,14 @@ static int parse_sort_string(const char *arg)
   int sort = 0;
   int flags = 0;
  
 - if (*arg == '-') {
 + if (skip_prefix(arg, -, arg))
   flags |= REVERSE_SORT;
 - arg++;
 - }
 - if (starts_with(arg, version:)) {
 - sort = VERCMP_SORT;
 - arg += 8;
 - } else if (starts_with(arg, v:)) {
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
   sort = VERCMP_SORT;
 - arg += 2;
 - } else
 + else
   sort = STRCMP_SORT;
 +
   if (strcmp(arg, refname))
   die(_(unsupported sort specification %s), arg);
   sort |= flags;

Re: [PATCH] makefile: add ability to run specific test files

[Keller, Jacob E]

On Wed, 2014-07-09 at 21:14 -0700, Junio C Hamano wrote:
 On Wed, Jul 9, 2014 at 4:49 PM, Keller, Jacob E
 jacob.e.kel...@intel.com wrote:
  On Wed, 2014-07-09 at 15:59 -0700, Junio C Hamano wrote:
 
  What kind of things are missing, exactly?  Perhaps that is something
  you need to fix, instead of mucking with the top-level Makefile.
 
  It uses the git from my environment instead of the git I have built,
  which is bad since I don't really want to run make install.
 
 Are you sure about that?  Try adding something like
 
   die(I am broken);
 
 at the very beginning of main() in git.c, rebuild your git (i.e.
 make, not make install)
 and then
 
   $ cd t
   $ sh ./t1234-test.sh -v
 
 for any of the test scripts. You should see any test piece that runs git 
 sees
 git dying with that message.
 
 Otherwise, there is something wrong with git you are building. Unless you have
 a patch or two to t/test-lib.sh or something that breaks the test framework, 
 you
 should be able to test what you just have built without getting affected by 
 what
 is installed in your $PATH. After all, that is how we bootstrap git
 from a tarball
 without any installed version, and friends do not force friends install 
 without
 testing first ;-)

Ok, I'll give it a shot. All I know for sure right now is running the
test directly passed and running from make test it failed.

I'll see if that makes any difference.

Thanks,
Jake

Re: [PATCH v2] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Thu, 2014-07-10 at 15:34 -0400, Jeff King wrote:
 On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:
 
  Jeff King p...@peff.net writes:
  
   I know this is existing code you are moving, but I noticed it looks ripe
   for using skip_prefix. Perhaps while we are in the area we should do the
   following on top of your patch (I'd also be happy for it be squashed
   in, but that may be too much in one patch).
  
  I am tempted to suggest going the other way around, i.e. queue (an
  equivalent of) this on jk/skip-prefix and merge it to 'next' and
  then 'master' quickly.
  
  I can go either way, but I tend to prefer building new things on top
  of obviously correct clean-up, not the other way around.
 
 Me too. I just didn't want to make more work for Jacob (in having to
 rebase on top of mine) or for you (in having to do a non-obvious merge a
 few days from now).
 
 -Peff

I'm perfectly fine rebasing. :)

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH] makefile: add ability to run specific test files

[Keller, Jacob E]

On Thu, 2014-07-10 at 04:14 +, Junio C Hamano wrote:
 On Wed, Jul 9, 2014 at 4:49 PM, Keller, Jacob E
 jacob.e.kel...@intel.com wrote:
  On Wed, 2014-07-09 at 15:59 -0700, Junio C Hamano wrote:
 
  What kind of things are missing, exactly?  Perhaps that is something
  you need to fix, instead of mucking with the top-level Makefile.
 
  It uses the git from my environment instead of the git I have built,
  which is bad since I don't really want to run make install.
 
 Are you sure about that?  Try adding something like
 
   die(I am broken);
 
 at the very beginning of main() in git.c, rebuild your git (i.e.
 make, not make install)
 and then
 
   $ cd t
   $ sh ./t1234-test.sh -v
 
 for any of the test scripts. You should see any test piece that runs git 
 sees
 git dying with that message.
 
 Otherwise, there is something wrong with git you are building. Unless you have
 a patch or two to t/test-lib.sh or something that breaks the test framework, 
 you
 should be able to test what you just have built without getting affected by 
 what
 is installed in your $PATH. After all, that is how we bootstrap git
 from a tarball
 without any installed version, and friends do not force friends install 
 without
 testing first ;-)

This is even more interesting. I tried your die check, and it definitely
runs the correct version of git.

However, if I run the test directly:

cd t ; sh t3200-branch.sh -v

it passes.

if I run:

make test

that particular test fails. If I have this patch applied, and I run

make t/t3200-branch.sh

it also fails.

I have done this directly on current master branch. So something is
differing between the two test runs.

Also, if I run:

make -C t t3200-branch.sh

that passes, so it really *is* something setup by the main makefile.

Any more suggestions?

Thanks,
Jake

Re: [PATCH v4] linuxptp: add phc_ctl program to help debug PHC devices

[Keller, Jacob E]

On Thu, 2014-07-10 at 16:20 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  This is an updated version of a script I wrote a couple years ago for
 
 I suspect that this is not for us ;-)
 --
 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


Indeed. This was intended for the LinuxPTP project, and I accidentally
sent here.

Please just ignore these.

Thanks,
Jake

Re: [PATCH] gitignore: add .version as this is generated during a make

[Keller, Jacob E]

On Thu, 2014-07-10 at 16:13 -0700, Jonathan Nieder wrote:
 Hi,
 
 Jacob Keller wrote:
 
  Subject: gitignore: add .version as this is generated during a make
 
 What program generates that file?  When I build on a Debian machine, I
 get
 
   $ make
   [...]
   SUBDIR templates
   $ ls -la .version
   ls: cannot access .version: No such file or directory
 
 Curious,
 Jonathan

Sorry I accidentally set these two to the wrong mailing list. These were
meant for the LinuxPTP project. OOPS

Thanks,
Jake

t3200-branch.sh number 102 fails when run under make test

[Keller, Jacob E]

Hello,

I recently cloned the master branch of the git repo, and when I ran make
test, it fails on test 102 of the t3200-branch.sh test cases.

not ok 102 - tracking with unexpected .fetch refspec
#
#   rm -rf a b c d 
#   git init a 
#   (
#   cd a 
#   test_commit a
#   ) 
#   git init b 
#   (
#   cd b 
#   test_commit b
#   ) 
#   git init c 
#   (
#   cd c 
#   test_commit c 
#   git remote add a ../a 
#   git remote add b ../b 
#   git fetch --all
#   ) 
#   git init d 
#   (
#   cd d 
#   git remote add c ../c 
#   git config remote.c.fetch 
+refs/remotes/*:refs/remotes/* 
#   git fetch c 
#   git branch --track local/a/master remotes/a/master 
#   test $(git config branch.local/a/master.remote) = c 

#   test $(git config branch.local/a/master.merge) = 
refs/remotes/a/master 
#   git rev-parse --verify a expect 
#   git rev-parse --verify local/a/master actual 
#   test_cmp expect actual
#   )
#
# failed 1 among 102 test(s)
1..102

However, when I run the test file manually it passes. I am currently
running through a verbose output test run to see if I can find more
useful output..

For reference, the commit I am testing against is:

72c779457cd7 (line-log: use commit_list_append() instead of duplicating its 
code)

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: t3200-branch.sh number 102 fails when run under make test

[Keller, Jacob E]

On Wed, 2014-07-09 at 13:37 -0700, Jacob E Keller wrote:
 Hello,
 
 I recently cloned the master branch of the git repo, and when I ran make
 test, it fails on test 102 of the t3200-branch.sh test cases.
 
 not ok 102 - tracking with unexpected .fetch refspec
 #
 #   rm -rf a b c d 
 #   git init a 
 #   (
 #   cd a 
 #   test_commit a
 #   ) 
 #   git init b 
 #   (
 #   cd b 
 #   test_commit b
 #   ) 
 #   git init c 
 #   (
 #   cd c 
 #   test_commit c 
 #   git remote add a ../a 
 #   git remote add b ../b 
 #   git fetch --all
 #   ) 
 #   git init d 
 #   (
 #   cd d 
 #   git remote add c ../c 
 #   git config remote.c.fetch 
 +refs/remotes/*:refs/remotes/* 
 #   git fetch c 
 #   git branch --track local/a/master remotes/a/master 
 #   test $(git config branch.local/a/master.remote) = 
 c 
 #   test $(git config branch.local/a/master.merge) = 
 refs/remotes/a/master 
 #   git rev-parse --verify a expect 
 #   git rev-parse --verify local/a/master actual 
 #   test_cmp expect actual
 #   )
 #
 # failed 1 among 102 test(s)
 1..102
 
 However, when I run the test file manually it passes. I am currently
 running through a verbose output test run to see if I can find more
 useful output..
 
 For reference, the commit I am testing against is:
 
 72c779457cd7 (line-log: use commit_list_append() instead of duplicating its 
 code)
 
 Thanks,
 Jake

I ran the test wit the GIT_TEST_OPS set to --verbose, and the output I got is:
expecting success: 
rm -rf a b c d 
git init a 
(
cd a 
test_commit a
) 
git init b 
(
cd b 
test_commit b
) 
git init c 
(
cd c 
test_commit c 
git remote add a ../a 
git remote add b ../b 
git fetch --all
) 
git init d 
(
cd d 
git remote add c ../c 
git config remote.c.fetch +refs/remotes/*:refs/remotes/* 
git fetch c 
git branch --track local/a/master remotes/a/master 
test $(git config branch.local/a/master.remote) = c 
test $(git config branch.local/a/master.merge) = 
refs/remotes/a/master 
git rev-parse --verify a expect 
git rev-parse --verify local/a/master actual 
test_cmp expect actual
)

Initialized empty Git repository in /home/jekeller/git/git/t/trash 
directory.t3200-branch/a/.git/
[master (root-commit) ce450c7] a
 Author: A U Thor aut...@example.com
 1 file changed, 1 insertion(+)
 create mode 100644 a.t
Initialized empty Git repository in /home/jekeller/git/git/t/trash 
directory.t3200-branch/b/.git/
[master (root-commit) 19acec0] b
 Author: A U Thor aut...@example.com
 1 file changed, 1 insertion(+)
 create mode 100644 b.t
Initialized empty Git repository in /home/jekeller/git/git/t/trash 
directory.t3200-branch/c/.git/
[master (root-commit) ea1ac38] c
 Author: A U Thor aut...@example.com
 1 file changed, 1 insertion(+)
 create mode 100644 c.t
fatal: Invalid refspec '+refs/heads/*:refs/remotes/b/*'
not ok 102 - tracking with unexpected .fetch refspec
#   
#   rm -rf a b c d 
#   git init a 
#   (
#   cd a 
#   test_commit a
#   ) 
#   git init b 
#   (
#   cd b 
#   test_commit b
#   ) 
#   git init c 
#   (
#   cd c 
#   test_commit c 
#   git remote add a ../a 
#   git remote add b ../b 
#   git fetch --all
#   ) 
#   git init d 
#   (
#   cd d 
#   git remote add c ../c 
#   git config remote.c.fetch 
+refs/remotes/*:refs/remotes/* 
#   git fetch c 
#   git branch --track local/a/master remotes/a/master 
#   test $(git config branch.local/a/master.remote) = c 

#   test $(git config branch.local/a/master.merge) = 
refs/remotes/a/master 
#   git rev-parse --verify a expect 
#   git rev-parse --verify local/a/master actual 
#

Re: t3200-branch.sh number 102 fails when run under make test

[Keller, Jacob E]

On Wed, 2014-07-09 at 16:54 -0400, Jeff King wrote:
 On Wed, Jul 09, 2014 at 08:37:51PM +, Keller, Jacob E wrote:
 
  I recently cloned the master branch of the git repo, and when I ran make
  test, it fails on test 102 of the t3200-branch.sh test cases.
 
 Just a guess, but try reverting 745224e (refs.c: SSE2 optimizations for
 check_refname_component, 2014-06-18).
 
 That commit causes some weird memory accesses that only show up under
 certain conditions[1]. There's a possible fix that is not yet applied,
 but reverting should be an easy way to test.
 
 -Peff
 
 [1] http://thread.gmane.org/gmane.comp.version-control.git/252881
 --
 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

Yes, performing the revert appears to fix the issue. Hopefully the
proposed fix also works.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH] remote-curl: do not complain on EOF from parent git

[Keller, Jacob E]

On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
 The parent git process is supposed to send us an empty line
 to indicate that the conversation is over. However, the
 parent process may die() if there is a problem with the
 operaiton (e.g., we try to fetch a ref that does not exist). 

Nitpick, but you probably meant operation here.

Thanks,
Jake

 In this case, it produces a useful message, but then
 remote-curl _also_ produces an unhelpful message:
 
   $ git pull origin matser
   fatal: couldn't find remote ref matser
   Unexpected end of command stream
 
 The right way to fix this is to teach the parent git to
 always cleanly close the connection to the helper, letting
 it know that we are done. Implementing that is rather
 clunky, though, as it would involve either replacing die()
 operations with returning errors up the stack (until we
 disconnect the transport), or adding an atexit handler to
 clean up any transport helpers left open.
 
 It's much simpler to just suppress the EOF message in
 remote-curl. It was not added to address any real-world
 situation in the first place, but rather a we should
 probably report unexpected things suggestion[1].
 
 It is the parent git which drives the operation, and whose
 exit value actually matters. If the parent dies, then the
 helper has no need to complain (except as a debugging aid).
 In the off chance that the pipe is closed without the parent
 dying, the parent can still notice the non-zero exit code.
 
 [1] http://article.gmane.org/gmane.comp.version-control.git/176036
 
 Reported-by: Dmitry wiped...@yandex.ru
 Signed-off-by: Jeff King p...@peff.net
 ---
 The original discussion that led to this code being implemented was due
 to us checking the helper's exit code in the first place. However, we
 seem to be inconsistent about doing so. I'm not inclined to pursue it
 further, though, as these subtle details of the transport helper code
 usually turn into a can of worms, and more importantly, I don't think it
 hurts anything in the real world. Either the parent git gets the
 expected protocol output from the helper or it doesn't, and we report
 errors on that. An error from a helper after the operation completes is
 not really important to the parent git either way.
 
  remote-curl.c | 2 --
  1 file changed, 2 deletions(-)
 
 diff --git a/remote-curl.c b/remote-curl.c
 index 4493b38..0454ffc 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -971,8 +971,6 @@ int main(int argc, const char **argv)
   if (strbuf_getline(buf, stdin, '\n') == EOF) {
   if (ferror(stdin))
   fprintf(stderr, Error reading command 
 stream\n);
 - else
 - fprintf(stderr, Unexpected end of command 
 stream\n);
   return 1;
   }
   if (buf.len == 0)

Re: [PATCH] tag: support configuring --sort via .gitconfig

[Keller, Jacob E]

On Wed, 2014-07-09 at 17:58 -0400, Jeff King wrote:
 On Wed, Jul 09, 2014 at 02:21:00PM -0700, Jacob Keller wrote:
 
  Add support for configuring default sort ordering for git tags. Command
  line option will override this configured value, using the exact same
  syntax.
 
 This makes sense, and was something I was expecting to add once tag and
 branch both learned for-each-ref's sorting code. I don't think there
 will be any compatibility problems in adding it now, though; the
 ultimate interface will be the same (it will just know about more types
 of sorting).
 

I only noticed the sort recently, and I first tried to add an alias like
tag = tag --sort=v:refname but git didn't pick up the alias of the
name already. I use a lot of version-style tags so I wanted this to be
default. I did notice that the format of the sort parameter was actually
pretty complex, so it seemed that more was intended to be added to it.

The only main issue would be the location of the sort-configure code,
but that is actually easy to move so I don't think it's a big deal. The
configuration interface should remain similar.

  +tag.sort::
  +   This variable is used to control the sort ordering of tags when
  +   displayed via linkgit:git-tag[5]. The current supported types are
  +   refname for lexicographic order (default) or version:refname or
  +   v:refname for tag names treated as versions. You may prepend a - to
  +   reverse the sort ordering.
 
 Your link to git-tag[5] should be to git-tag[1], I think. The final
 number is the manpage section.
 

Which I thought was 5 for the commands? Or is it always [1]?

 I was going to complain that this is duplicating the sort documentation
 from git-tag, but I see you actually moved it from the --sort option to
 here, and refer back from there to here.
 

I actually didn't remove anything from git-tag, I only added the
reference to git-config. But I can keep from duplicating the
documentation in there. I like the idea of using an included file
though.

 I think I'd prefer it the other way around (and explain this as behave
 as if --sort=value had been given). As the sort options grow, it
 seems likely that we will grow a new section in the git-tag manpage (and
 eventually probably feed that content from an include that works for all
 of for-each-ref, branch, and tag).

yes, I agree this makes more sense.

 
  +static char *configured_tag_sort;
 
 When dealing with a config option that also has a command-line version,
 we usually forgo this separate storage for the configured value.
 Instead, we just set sort directly from the config callback (which may
 require making it a global so the callback can access it), and then let
 the command-line parser overwrite it.
 

Alright that makes sense. I will send a v2 soon. Thanks for the
comments.

 -Peff

Regards,
Jake

Re: [PATCH] makefile: add ability to run specific test files

[Keller, Jacob E]

On Wed, 2014-07-09 at 15:59 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  Running a specific test file manually does not obtain the exact
  environment setup by the Makefile.
 
 What kind of things are missing, exactly?  Perhaps that is something
 you need to fix, instead of mucking with the top-level Makefile.
 
 I recall last time when I did a patch like this I was told to look
 into make -C t ;-)  What is different this round?

It uses the git from my environment instead of the git I have built,
which is bad since I don't really want to run make install.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

RE: [StGit PATCH] Fix dirty index errors when resolving conflicts

[Keller, Jacob E]

 -Original Message-
 From: Zane Bitter [mailto:zbit...@redhat.com]
 Sent: Wednesday, July 17, 2013 6:57 AM
 To: git@vger.kernel.org
 Cc: Keller, Jacob E; Waskiewicz Jr, Peter P; catalin.mari...@gmail.com
 Subject: [StGit PATCH] Fix dirty index errors when resolving conflicts
 
 The patch 6e8fdc58c786a45d7a63c5edf9c702f1874a7a19 causes StGit
 to raise
 warnings (actually: errors) in the event that there are changes staged in
 the index and a refresh is performed without specifying either --index or
 --force. This is great for preventing an entire class of common mistakes,
 but is also a giant pain when resolving conflicts after a pull/rebase.
 Depending on the workflow in use, this may occur with a frequency
 anywhere
 between never and mulitple times on every pull.
 
 This patch removes the pain by:
  - Reporting unresolved conflicts *before* complaining about staged
changes, since it goes without saying that, when present, these are the
main problem.
  - Not complaining about staged changes if there are no unstaged changes
 in
the working directory, since the presence of --index is immaterial in
this case.
 
 Signed-off-by: Zane Bitter zbit...@redhat.com
 ---
  stgit/commands/refresh.py |   13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)
 
 diff --git a/stgit/commands/refresh.py b/stgit/commands/refresh.py
 index a2bab42..331c18d 100644
 --- a/stgit/commands/refresh.py
 +++ b/stgit/commands/refresh.py
 @@ -247,18 +247,19 @@ def func(parser, options, args):
  patch_name = get_patch(stack, options.patch)
  paths = list_files(stack, patch_name, args, options.index,
 options.update)
 
 -# Make sure the index is clean before performing a full refresh
 -if not options.index and not options.force:
 -if not stack.repository.default_index.is_clean(stack.head):
 -raise common.CmdException(
 -'The index is dirty. Did you mean --index? To force a full 
 refresh
 use --force.')
 -
  # Make sure there are no conflicts in the files we want to
  # refresh.
  if stack.repository.default_index.conflicts()  paths:
  raise common.CmdException(
  'Cannot refresh -- resolve conflicts first')
 
 +# Make sure the index is clean before performing a full refresh
 +if not options.index and not options.force:
 +if not (stack.repository.default_index.is_clean(stack.head) or
 +stack.repository.default_iw.worktree_clean()):
 +raise common.CmdException(
 +'The index is dirty. Did you mean --index? To force a full 
 refresh
 use --force.')
 +
  # Commit index to temp patch, and absorb it into the target patch.
  retval, temp_name = make_temp_patch(
  stack, patch_name, paths, temp_index = path_limiting)

ACK. This looks great. Thanks for noticing!

- Jake