Re: [PATCH v3 0/3] Add "git rebase --show-current-patch"

[Tim Landscheidt]

Junio C Hamano  wrote:

>> Compared to v2:

>> - the potential loss of errno before it's printed out in builtin/am.c
>>   is fixed.
>> - keep update_ref() in sequencer.c non-fatal like this rest
>> - rename ORIG_COMMIT to REBASE_HEAD

>> Interdiff:

> This round hasn't seen any comments.  Is everybody happy with it?

> I personally do not have strong opinion for the feature but didn't
> spot anything against the execution, either, so...

Sorry for the late reply: I dislike REBASE_/HEAD/ because
ORIG_/HEAD/ refers to the tip of the original branch, and
/ORIG/_HEAD refers to the original branch, so
/REBASE/_/HEAD/ is doubly confusing IMHO.  I consider
ORIG_COMMIT easier to understand because ORIG_HEAD refers to
the tip of the original branch, and ORIG_COMMIT would refer
to one of the commits making up that original branch, but as
I suggested it myself I might not be very objective in that
regard :-).

(BTW, thanks, Duy, for doing the actual work!)

Tim

Re: [PATCH 0/2] Add "git rebase --show-patch"

[Tim Landscheidt]

Junio C Hamano  wrote:

>> The pseudo ref certainly has an appeal. For people very familiar with
>> Git's peculiarities such as FETCH_HEAD. Such as myself.

>> For users, it is probably substantially worse an experience than having a
>> cmdmode like --show-patch in the very command they were just running.

> I tend to agree with that assessment.  FETCH_HEAD was a required
> mechanism for commands in the toolchain to communicate and wasn't
> meant as a mechanism for end-users.  I do not think it is a good
> idea to add to the pile to these special files the users would need
> to look at, when we do not need to.

> Even if the internal implementation uses such a file, wrapping it
> with a documented command mode would make a better UI.

I disagree with that as I had:

| [alias]
| original-commit = !git show $(cat .git/rebase-apply/original-commit)

for a long time in my ~/.gitconfig :-) (until I realized
that Git accepted "rebase-apply/original-commit" everywhere;
for single-commit branches I always just used ORIG_HEAD).

This meant that whenever I wanted to look at the lay of the
land upon which the original commit was built, I had to do
"git original-commit" and copy & paste the SHA1 (without my
alias, I had to "git log ORIG_HEAD", pick hopefully the cor-
rect commit and copy & paste its SHA1).  Only with this SHA1
I could then run "git diff", "git show", "git grep", "git
blame", etc., etc., etc. because in my use cases looking at
the patch alone was usually not sufficient: I needed to know
why there was a conflict, i. e. how the file(s) the patch
changed looked before they had been altered upstream.

To me, this type of "algebra" is what makes Git so powerful:
I do not have to build a pipe that gets the SHA1 of a
branch's tip and xargs it to "git show", I can just say "git
show $branch".  Having a SHA1 with a special meaning that
has no easy way to access it by "git rev-parse" breaks this
UI IMHO.

Tim

Re: [PATCH 0/2] Add "git rebase --show-patch"

[Tim Landscheidt]

Nguyễn Thái Ngọc Duy  wrote:

> When a conflict happens during a rebase, you often need to look at the
> original patch to see what the changes are. This requires opening your
> favourite pager with some random path inside $GIT_DIR.

> This series makes that experience a bit better, by providing a command
> to read the patch. This is along the line of --edit-todo and --quit
> where you can just tell git what to do and not bother with details.

> My main focus is "git rebase", but because rebase uses "git am" behind
> the scene, "git am" gains --show-patch option too.

> There was something more I wanted to do, like coloring to the patch.
> But that probably will come later. I'll try to merge these two
> 21-months-old patches first.

> […]

I dislike the approach to use a separate command/option.
The nice thing about rebase-apply/original-commit is that
you can use it in /any/ git command, i. e. you can do "git
log $whatever..rebase-apply/original-commit".

What I would do instead is (besides documenting it :-)) to
provide an alias that is more in line with ORIG_HEAD,
FETCH_HEAD, etc.; i. e. something along the lines of (pseudo
code, will probably not work):

| --- a/builtin/am.c
| +++ b/builtin/am.c
| @@ -1110,6 +1110,7 @@ static void am_next(struct am_state *state)
|  
| oidclr(>orig_commit);
| unlink(am_path(state, "original-commit"));
| +   delete_ref(NULL, "ORIG_COMMIT", NULL, 0);
|  
| if (!get_oid("HEAD", ))
| write_state_text(state, "abort-safety", oid_to_hex());
| @@ -1441,6 +1442,7 @@ static int parse_mail_rebase(struct am_state *state, 
const char *mail)
|  
| oidcpy(>orig_commit, _oid);
| write_state_text(state, "original-commit", oid_to_hex(_oid));
| +   update_ref_oid("am", "ORIG_COMMIT", _oid, NULL, 0, 
UPDATE_REFS_DIE_ON_ERR);
|  
| return 0;
|  }

This (when working) would allow to use ORIG_COMMIT in place
of the mouthful rebase-apply/original-commit.

Tim