RE: [PATCH] git-submodule: fix expansion of depth for cmd_update

[Keller, Jacob E]

> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf 
> Of
> Junio C Hamano
> Sent: Thursday, August 22, 2019 2:01 PM
> To: Keller, Jacob E 
> Cc: git@vger.kernel.org; Jacob Keller 
> Subject: Re: [PATCH] git-submodule: fix expansion of depth for cmd_update
> 
> Jacob Keller  writes:
> 
> > From: Jacob Keller 
> >
> > The depth variable already contains "--depth=", so expanding it with an
> > additional --depth when invoking the update-clone git submodule--helper
> > is incorrect.
> >
> > Signed-off-by: Jacob Keller 
> > ---
> >
> > I'm *reasonably* sure this is correct, but I am not sure how to test it.
> > It's possible that it expands to "--depth --depth=N" and somehow this gets
> > handled properly?
> 
> I agree with your eyeballing of all the assignments to the variable,
> and other references to $depth take either one of these two forms:
> 
>   git submodule--helper ... ${depth:+"$depth"} ...
>   git submodule--helper ... $depth ...
> 
> As long as "git submodule ... --depth  ..." gets called with
>  that does not have $IFS, either would work fine, but the
> former is correct even when  has problematic characters in it
> and your patch uses that form, too).
> 
> However.
> 
> The command line parser for update_clone() stuffs --depth as a
> string to suc.depth, and then the machinery ends up calling
> prepare_to_clone_next_submodule() with such an instance of suc
> (struct submodule_update_clone).  Then that function just pushes the
> suc->depth to an argv array used to spawn a "submodule--helper clone".
> 
> So passing "--depth --depth=23" would be "correct", sadly, in that
> codepath (I am not saying other codepaths would not call the same
> prepare_to_clone_next_submodule() with "--depth 23", as I didn't
> check, and if there is such a codepath, it would break).
> 

Ok, so it's technically correct, but weird. I had trouble understanding it.

> We may need to clean the mess up X-<.
> 

Yea, I got confused looking at this code. Though if I recall, we were working 
towards rewriting it in C anyways (which is what the submodule--helper is 
doing).

Thanks,
Jake

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index c7f58c5756f7..4e7fc8bf3652 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -547,7 +547,7 @@ cmd_update()
> > ${update:+--update "$update"} \
> > ${reference:+"$reference"} \
> > ${dissociate:+"--dissociate"} \
> > -   ${depth:+--depth "$depth"} \
> > +   ${depth:+"$depth"} \
> > $recommend_shallow \
> > $jobs \
> > -- \

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(&buf, path, fmt, args);
> > +   err = do_submodule_path(&buf, path, fmt, args);
> > va_end(args);
> > +   if (err)
> 
> Here we need a strbuf_release(&buf) 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�&j:+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: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�&j:+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: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�&j:+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�&j:+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�&j:+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  writes:
> 
> > On Wed, Jul 22, 2015 at 5:05 PM, Jacob Keller <
> > jacob.e.kel...@intel.com> wrote:
> > > From: Jacob Keller 
> > > 
> > > 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 
cd 
git checkout master
git worktree add  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,   wrote:
> > From: Beat Bolli 
> > 
> > When referencing earlier commits in new commit messages or other 
> > text,
> > one of the established formats is
> > 
> > commit  ("", )
> 
> 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:  ("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 
> > Cc: Paul Mackerras 
> > ---
> >  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"  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  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 )
> > 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  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: [PATCH 1/9] git-compat-util.h: Add missing semicolon after struct itimerval

[Keller, Jacob E]

> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
> Behalf Of Jonas 'Sortie' Termansen
> Sent: Friday, August 29, 2014 12:44 PM
> To: Junio C Hamano
> Cc: git@vger.kernel.org; Keller, Jacob E; Johannes Sixt
> Subject: Re: [PATCH 1/9] git-compat-util.h: Add missing semicolon after
> struct itimerval
> 
> On 08/29/2014 09:07 PM, Junio C Hamano wrote:
> > Jonas 'Sortie' Termansen  writes:
> > That is easy to fix, isn't it?
> >
> > Where you said "If you have timer_settimer(), set this makefile
> > variable", you start the sentence with "If you have a working
> > timer_settimer()" instead.
> 
> That's mostly right. I care about cross-compilation, so that is also
> something to keep in mind. It would be best if this could be
> automatically and robustly determined (even when cross-compiling),
> rather than relying on the user setting magic make variables.
> 
> Jonas

Couldn't this be solved by making our configure script smart enough to tell 
when it's at least the "known" broken OpenBSD implementation?

Regards,
Jake

Re: [PATCH 9/9] Use timer_settime for new platforms

[Keller, Jacob E]

On Fri, 2014-08-29 at 11:02 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > From: Jonas 'Sortie' Termansen 
> >
> > setitimer() is an obsolescent XSI interface and may be removed in a
> > future standard. Applications should use the core POSIX timer_settime()
> > instead.
> >
> > It's important that code doesn't simply check if timer_settime is
> > available as it can give false positives. Some systems like contemporary
> > OpenBSD provides the function, but it unconditionally fails with ENOSYS
> > at runtime.
> 
> Doesn't this paragraph need tweaking?  I think you lost (which is a
> good thing) "notice that timer_settime() call failed with ENOSYS and
> switch to setitimer()", no?
> 
> > Clean up the progress reporting and change it to use timer_settime,
> > which will fall back to setitimer automatically if timer_settime is not
> > supported. (see git-compat-util.h for how it does this). If both
> > functions are not present, then git-compat-util.h provides replacements
> > which will always fail with ENOSYS.
> 
> While this paragraph may be true if patch 8b and 9 are taken
> together, isn't what it describes mostly what 8b did, not 9?
> 
> Here by 8b I mean the change to git-compat-util.h in 8; the patch
> might want to be split into two, 8a for the autoconf part whose log
> message may begin with "This function was not previously used by
> git." and 8b that adds an emulation of timer_settime() API in terms
> of setitimer() API, or the other way around.
> 
> What 9 did is only "we used to use the setitmer() API to implement
> the progress reporting; now we use timer_settime() API" (yes, it is
> thanks to the abstraction given by 8, but the "callers has to only
> know about one API, not worrying about the other API" is a merit
> attributable to 8b, not this one).
> 

You are correct. I can re-base these patches and split them apart a bit
better.

Regards,
Jake

> > The approach used here enables us to use a single API (timer_settime)
> > without having to worry about checking for #ifdefs or if blocks which
> > make it an unreadable nightmare. The major downside is for systems
> > without timer_settime support, they may fall back on a wrapped
> > implementation which could have subtle differences. This should be a
> > minor issue as almost all modern systems provide timer_settime support.
> 
> As this paragraph.
> 
> > Note that this change means that git should never use setitimer on its
> > own now, as the fallback implementation of timer_settime assumes that it
> > is the sole user of ITIMER_REAL, and timer_delete will reset the
> > ITIMER_REAL.
> 
> And this one.
> 


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

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

[Keller, Jacob E]

On Fri, 2014-08-29 at 19:26 +0200, Johannes Sixt wrote:
> Am 29.08.2014 18:42, schrieb Jacob Keller:
> > From: Jonas 'Sortie' Termansen 
> > 
> > This function will be used in a following commit.
> > 
> > The timer_settime function is provided in librt on some systems. We
> > already use this library sometimes to get clock_gettime, so rework the
> > logic so we don't link with it twice.
> > 
> > This function was not previously used by git. This can cause trouble for
> > people on systems without timer_settime if they only rely on
> > config.mak.uname. They will need to set NO_TIMER_SETTIME manually.
> > 
> > Add proper replacement function macros for setitimer and timer_settime
> > that implement timer_settime as a wrapper for setitimer. In this way, if
> > the system has setitimer but not timer_settime then we will be able to
> > call timer_create, timer_settime, and timer_delete correctly and it will
> > wrap to setitimer under the hood. This will be used in the following
> > commit.
> > 
> > Signed-off-by: Jonas 'Sortie' Termansen 
> > Signed-off-by: Jacob Keller 
> > ---
> >  Makefile  | 21 +
> >  config.mak.uname  |  3 +++
> >  configure.ac  |  8 
> >  git-compat-util.h | 20 +++-
> >  4 files changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 66329e4b372b..5337ef0b7cd6 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -182,16 +182,22 @@ all::
> >  #
> >  # Define NO_SETITIMER if you don't have setitimer()
> >  #
> > +# Define NO_TIMER_SETTIME if you don't have timer_settime()
> > +#
> >  # Define NO_TIMER_T if you don't have timer_t.
> > +# This also implies NO_TIMER_SETTIME
> >  #
> >  # Define NO_STRUCT_TIMESPEC if you don't have struct timespec
> > +# This also implies NO_TIMER_SETTIME
> >  #
> >  # Define NO_STRUCT_SIGEVENT if you don't have struct sigevent
> > +# This also implies NO_TIMER_SETTIME
> >  #
> >  # Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
> >  # This also implies NO_SETITIMER
> >  #
> >  # Define NO_STRUCT_ITIMERSPEC if you don't have struct itimerspec
> > +# This also implies NO_TIMER_SETTIME
> >  #
> >  # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
> >  # generally faster on your platform than accessing the working directory.
> > @@ -1348,12 +1354,15 @@ ifdef OBJECT_CREATION_USES_RENAMES
> >  endif
> >  ifdef NO_TIMER_T
> > COMPAT_CFLAGS += -DNO_TIMER_T
> > +   NO_TIMER_SETTIME = YesPlease
> >  endif
> >  ifdef NO_STRUCT_TIMESPEC
> > COMPAT_CFLAGS += -DNO_STRUCT_TIMESPEC
> > +   NO_TIMER_SETTIME = YesPlease
> >  endif
> >  ifdef NO_STRUCT_SIGEVENT
> > COMPAT_CFLAGS += -DNO_STRUCT_SIGEVENT
> > +   NO_TIMER_SETTIME = YesPlease
> >  endif
> >  ifdef NO_STRUCT_ITIMERVAL
> > COMPAT_CFLAGS += -DNO_STRUCT_ITIMERVAL
> > @@ -1361,10 +1370,14 @@ ifdef NO_STRUCT_ITIMERVAL
> >  endif
> >  ifdef NO_STRUCT_ITIMERSPEC
> > COMPAT_CFLAGS += -DNO_STRUCT_ITIMERSPEC
> > +   NO_TIMER_SETTIME = YesPlease
> >  endif
> >  ifdef NO_SETITIMER
> > COMPAT_CFLAGS += -DNO_SETITIMER
> >  endif
> > +ifdef NO_TIMER_SETTIME
> > +   COMPAT_CFLAGS += -DNO_TIMER_SETTIME
> > +endif
> >  ifdef NO_PREAD
> > COMPAT_CFLAGS += -DNO_PREAD
> > COMPAT_OBJS += compat/pread.o
> > @@ -1524,6 +1537,14 @@ endif
> >  
> >  ifdef HAVE_CLOCK_GETTIME
> > BASIC_CFLAGS += -DHAVE_CLOCK_GETTIME
> > +   LINK_WITH_LIBRT = YesPlease
> > +endif
> > +
> > +ifndef NO_TIMER_SETTIME
> > +   LINK_WITH_LIBRT = YesPlease
> > +endif
> > +
> > +ifdef LINK_WITH_LIBRT
> > EXTLIBS += -lrt
> >  endif
> >  
> > diff --git a/config.mak.uname b/config.mak.uname
> > index f0d93ef868a7..d04deab2dfa8 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -99,6 +99,7 @@ ifeq ($(uname_S),Darwin)
> > USE_ST_TIMESPEC = YesPlease
> > HAVE_DEV_TTY = YesPlease
> > NO_STRUCT_ITIMERSPEC = UnfortunatelyYes
> > +   NO_TIMER_SETTIME = UnfortunatelyYes
> > COMPAT_OBJS += compat/precompose_utf8.o
> > BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
> >  endif
> > @@ -360,6 +361,7 @@ ifeq ($(uname_S),Windows)
> > NO_STRUCT_TIMESPEC = UnfortunatelyYes
> > NO_STRUCT_SIGEVENT = UnfortunatelyYes
> > NO_STRUCT_ITIMERSPEC = UnfortunatelyYes
> > +   NO_TIMER_SETTIME = UnfortunatelyYes
> >  
> > CC = compat/vcbuild/scripts/clink.pl
> > AR = compat/vcbuild/scripts/lib.pl
> > @@ -513,6 +515,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
> > NO_STRUCT_TIMESPEC = UnfortunatelyYes
> > NO_STRUCT_SIGEVENT = UnfortunatelyYes
> > NO_STRUCT_ITIMERSPEC = UnfortunatelyYes
> > +   NO_TIMER_SETTIME = UnfortunatelyYes
> > COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI 
> > -Icompat -Icompat/win32
> > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
> > COMPAT_OBJS += compat/mingw.o compat/winansi.o \
> > diff --git a/configure.ac b/configure.ac
> > index 954f9ddb03c2..9d6ec41acc82 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @

Re: [PATCH 1/9] git-compat-util.h: Add missing semicolon after struct itimerval

[Keller, Jacob E]

On Fri, 2014-08-29 at 09:42 -0700, Jacob Keller wrote:
> From: Jonas 'Sortie' Termansen 
> 
> This hasn't been a problem in practice as almost all systems have the
> setitimer() API (or it is provided by git in the case of mingw). This code
> wasn't used in any default circumstances, as the build system never sets
> NO_STRUCT_ITIMERVAL - this breakage only occured if the user asked for it.
> 
> We repair this case so we can rely on it in the following commits.
> 
> Signed-off-by: Jonas 'Sortie' Termansen 
> ---

I fixed up Jonas' series and tried to resolve the issues I found as well
as re-ordered the patches so that they fit what Junio requested. I also
modified the last two patches to make timer_settime fallback to
setitimer. I am not sure this is the best approach, but it should be
easy to take at least the obvious bug fixes from the first three
patches, and this gives an outline for how we could wrap the setitimer
stuff.

One could also write another layer of wrapper in some compat/itimer.c
code, but I don't think this is necessary.

Regards,
Jake

Re: [PATCH 9/9] Use timer_settime for new platforms

[Keller, Jacob E]

On Thu, 2014-08-28 at 12:43 -0700, Junio C Hamano wrote:
> Jonas 'Sortie' Termansen  writes:
> 
> > setitimer() is an obsolescent XSI interface and may be removed in a
> > future standard. Applications should use the core POSIX timer_settime()
> > instead.
> >
> > This patch cleans up the progress reporting and changes it to try using
> > timer_settime, or if that fails, setitimer. If either function is not
> > provided by the system, then git-compat-util.h provides replacements
> > that always fail with ENOSYS.
> >
> > It's important that code doesn't simply check if timer_settime is
> > available as it can give false positives. Some systems like contemporary
> > OpenBSD provides the function, but it unconditionally fails with ENOSYS
> > at runtime.
> >
> > This approach allows the code using timer_settime() and setitimer() to
> > be simple and readable. My first attempt used #ifdef around each use of
> > timer_settime(), this quickly turned a into unmaintainable maze of
> > preprocessor conditionals.
> >
> > Signed-off-by: Jonas 'Sortie' Termansen 
> > ---
> >  builtin/log.c | 47 ---
> >  progress.c| 34 +++---
> >  2 files changed, 67 insertions(+), 14 deletions(-)
> 
> Yuck.  I didn't look at the change very carefully, but are the two
> interface so vastly different that you cannot emulate one in terms
> of the other, and use a single API at the callsites, isolating the
> knowledge of which kind of API is used to interact with the system
> timer in one place (perhaps in compat/itimer.c)?
> 
> Having to sprinkle "if (is_using_timer_settime)" around means we
> need to support two APIs at each and every callsite that wants timer
> interrupt actions.
> 
> --
> 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

Agreed! The biggest difference is that timer_settime() uses nanoseconds
value, which is very easy to convert microseconds into nanoseconds.

In fact, you could probably easily provide a wrapper for timer_settime
that works for settimer by simply converting it. This is a much better
approach. (though settimer "could" lose some precision since you ahve to
divide by 1000)...

The other big difference is abstracting away the timer_create aspect
which is ok, since we would always call timer create, and use the
correct timer, but we just fall back to setitimer to use the default
timer.

I'll submit a patch series which does what I think should work, and will
result in less if checks.

Regards,
Jake

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

[Keller, Jacob E]

On Thu, 2014-08-28 at 03:04 +0200, Jonas 'Sortie' Termansen wrote:
> This function will be used in a following commit.
> 
> The timer_settime function is provided in librt on some systems. We
> already use this library sometimes to get clock_gettime, so rework the
> logic so we don't link with it twice.
> 
> This function was not previously used by git. This can cause trouble for
> people on systems without timer_settime if they only rely on
> config.mak.uname. They will need to set NO_TIMER_SETTIME manually.
> 
> Add proper replacement function macros for setitimer and timer_settime
> that evaluates the arguments and fails with ENOSYS to simulate stub
> implementations. This will be useful in a following commit.
> 
> Signed-off-by: Jonas 'Sortie' Termansen 
> 
> ---
> 
> This patch can be improved by finding out which systems doesn't have
> timer_settime and adding entries for them to config.mak.uname.
> 
>  Makefile  | 21 +
>  config.mak.uname  |  3 +++
>  configure.ac  |  8 
>  git-compat-util.h |  8 +++-
>  4 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 66329e4..5609e44 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -182,16 +182,22 @@ all::
>  #
>  # Define NO_SETITIMER if you don't have setitimer()
>  #
> +# Define NO_TIMER_SETTIME if you don't have timer_settime()
> +#
>  # Define NO_TIMER_T if you don't have timer_t.
> +# This also implies NO_SETITIMER

Don't you mean it implies NO_TIMER_SETTIME?

It seems to me that these were all added for TIMER_SETTIME, and not
NO_SETTIMER? Or am I just thoroughly confused?

Regards,
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 +0000, Keller, Jacob E wrote:
> 
> >>> I am having trouble using revert. If I attempt to
> >>>
> >>> $ git revert 
> >>>
> >>> 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

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 +0000, Keller, Jacob E wrote:
> 
> >>> I am having trouble using revert. If I attempt to
> >>>
> >>> $ git revert 
> >>>
> >>> 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 18:15 -0400, David Turner wrote:
> On Wed, 2014-08-27 at 21:14 +0000, Keller, Jacob E wrote:
> > Hi,
> > 
> > I am having trouble using revert. If I attempt to
> > 
> > $ git revert 
> > 
> > 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: Improving the git remote command

[Keller, Jacob E]

On Wed, 2014-08-27 at 13:35 -0700, Junio C Hamano wrote:
> David Aguilar  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="" 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

revert top most commit

[Keller, Jacob E]

Hi,

I am having trouble using revert. If I attempt to

$ git revert 

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: 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: 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: [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  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 
> Signed-off-by: Jacob Keller 
> Signed-off-by: Junio C Hamano 
> ---

Just a ping on the status of this patch?

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"  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 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"  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 10:59 -0700, Junio C Hamano wrote:
> "Keller, Jacob E"  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 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
>  wrote:
> > On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
> >> Jacob Keller  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: [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  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 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  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 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"  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 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"  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 11:17 -0700, Junio C Hamano wrote:
> "Keller, Jacob E"  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 09:03 -0700, Junio C Hamano wrote:
> "Keller, Jacob E"  writes:
> 
> > On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
> >> Jeff King  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 Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote:
> Jeff King  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 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  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: [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 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  writes:
> 
> > From: Jeff King 
> >
> > Make the parsing of the --sort parameter more readable by having
> > skip_prefix keep our pointer up to date.
> >
> > Signed-off-by: Jeff King 
> > Signed-off-by: Jacob Keller 
> > ---
> >  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 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  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 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  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 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] 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 
> Signed-off-by: Jacob Keller 
> ---
> 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..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=" 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[=]::
>  --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 |= fl

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"  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 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 +0000, 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 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 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 
> 
> 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 
> 
> 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: [Bug] data loss with cyclic alternates

[Keller, Jacob E]

On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote:
> Ephrim Khong  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: 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: [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  writes:
> 

> > +
> > +   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.

Based on a cursory glance at how the config is parsed, we actually
die_on_error for most options. Does it makes sense to warn or not? I am
not really sure here.. At any rate I updated this to be a bit cleaner
and use error( instead of die, so that it will work within how config is
supposed to.. Plus, this also makes it so that --sort shows the usage if
it's invalid.

Please comment more on the new set I am sending so we can really
determine what the correct course of action is here.

Regards,
Jake

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  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 
> > Signed-off-by: Jacob Keller 
> > ---
> > - 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..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=". 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=" 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=::
> ... (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);
> >

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  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 
> > Signed-off-by: Jacob Keller 
> > ---
> > - 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..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=". 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=" 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=::
> ... (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;
> > +
> > 

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

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  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] 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
>  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 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  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 Wed, 2014-07-09 at 21:14 -0700, Junio C Hamano wrote:
> On Wed, Jul 9, 2014 at 4:49 PM, Keller, Jacob E
>  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 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 
> ---
>  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 15:59 -0700, Junio C Hamano wrote:
> Jacob Keller  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: [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= 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] 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 
> Signed-off-by: Jeff King 
> ---
> 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: 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 +0000, 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: 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 
 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 
 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 
 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)" =

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: [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 
> ---
>  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